From 7472f782230e3d8779968f7c6b72aab38c663a57 Mon Sep 17 00:00:00 2001 From: csoler Date: Wed, 15 Nov 2017 23:24:43 +0100 Subject: [PATCH 01/11] added sha256 calculation functions, and non backward compatible SSL Id computation code to active later (0.7) --- ...ON_BACKWARD_COMPATIBLE_CHANGES_FOR_V07.txt | 14 ++++++++++ libretroshare/src/pqi/sslfns.cc | 28 +++++++++++++++---- libretroshare/src/retroshare/rsids.h | 3 ++ libretroshare/src/util/rsdir.cc | 21 ++++++++++++++ libretroshare/src/util/rsdir.h | 3 +- 5 files changed, 63 insertions(+), 6 deletions(-) create mode 100644 LIST_OF_NON_BACKWARD_COMPATIBLE_CHANGES_FOR_V07.txt diff --git a/LIST_OF_NON_BACKWARD_COMPATIBLE_CHANGES_FOR_V07.txt b/LIST_OF_NON_BACKWARD_COMPATIBLE_CHANGES_FOR_V07.txt new file mode 100644 index 000000000..83a9efa1b --- /dev/null +++ b/LIST_OF_NON_BACKWARD_COMPATIBLE_CHANGES_FOR_V07.txt @@ -0,0 +1,14 @@ +V07_NON_BACKWARD_COMPATIBLE_CHANGE_001: + + What: Computes the node id by performing a sha256 hash of the certificate's PGP signature, instead of simply picking up the last 20 bytes of it. + + Why: There is no real risk in forging a certificate with the same ID as the authentication is performed over the PGP signature of the certificate + which hashes the full SSL certificate (i.e. the full serialized CERT_INFO structure). However the possibility to + create two certificates with the same IDs is a problem, as it can be used to cause disturbance in the software. + + Backward compat: makes connexions impossible with non patched peers, probably because the SSL id that is computed is not the same on both side, + and in particular unpatched peers see a cerficate with ID different (because computed with the old method) than the ID that was + submitted when making friends. + + Note: the advantage of basing the ID on the signature rather than the public key is not very clear, given that the signature is based on a hash + of the public key (and the rest of the certificate info). diff --git a/libretroshare/src/pqi/sslfns.cc b/libretroshare/src/pqi/sslfns.cc index ab79d9825..16ba1744d 100644 --- a/libretroshare/src/pqi/sslfns.cc +++ b/libretroshare/src/pqi/sslfns.cc @@ -610,6 +610,22 @@ bool getX509id(X509 *x509, RsPeerId& xid) X509_get0_signature(&signature,&algor,x509); #endif + +#ifndef V07_NON_BACKWARD_COMPATIBLE_CHANGE_001 + + // What: Computes the node id by performing a sha256 hash of the certificate's PGP signature, instead of simply picking up the last 20 bytes of it. + // + // Why: There is no real risk in forging a certificate with the same ID as the authentication is performed over the PGP signature of the certificate + // which hashes the full SSL certificate (i.e. the full serialized CERT_INFO structure). However the possibility to + // create two certificates with the same IDs is a problem, as it can be used to cause disturbance in the software. + // + // Backward compat: makes connexions impossible with non patched peers, probably because the SSL id that is computed is not the same on both side, + // and in particular unpatched peers see a cerficate with ID different (because computed with the old method) than the ID that was + // submitted when making friends. + // + // Note: the advantage of basing the ID on the signature rather than the public key is not very clear, given that the signature is based on a hash + // of the public key (and the rest of the certificate info). + // int signlen = ASN1_STRING_length(signature); if (signlen < CERTSIGNLEN) { @@ -627,14 +643,16 @@ bool getX509id(X509 *x509, RsPeerId& xid) * more randomness */ -#warning csoler 2017-02-19: This is cryptographically horrible. We should do a hash of the public key here!!! +#warning csoler 2017-02-19: This is cryptographically horrible. We should hash the entire signature here! xid = RsPeerId(&signdata[signlen - CERTSIGNLEN]) ; +#else - //for(int i = signlen - CERTSIGNLEN; i < signlen; i++) - //{ - // rs_sprintf_append(xid, "%02x", (uint16_t) (((uint8_t *) (signdata))[i])); - //} + if(RsPeerId::SIZE_IN_BYTES > Sha256CheckSum::SIZE_IN_BYTES) + return false ; + + xid = RsPeerId(RsDirUtil::sha256sum(ASN1_STRING_data(const_cast(signature)),ASN1_STRING_length(signature)).toByteArray()) ; +#endif return true; } diff --git a/libretroshare/src/retroshare/rsids.h b/libretroshare/src/retroshare/rsids.h index addd08a85..21a8fd754 100644 --- a/libretroshare/src/retroshare/rsids.h +++ b/libretroshare/src/retroshare/rsids.h @@ -221,6 +221,7 @@ static const int CERT_SIGN_LEN = 16 ; // = CERTSIGNLEN static const int PGP_KEY_ID_SIZE = 8 ; static const int PGP_KEY_FINGERPRINT_SIZE = 20 ; static const int SHA1_SIZE = 20 ; +static const int SHA256_SIZE = 32 ; // These constants are random, but should be different, in order to make the various IDs incompatible with each other. // @@ -236,10 +237,12 @@ static const uint32_t RS_GENERIC_ID_GROUTER_ID_TYPE = 0x0009 ; static const uint32_t RS_GENERIC_ID_GXS_TUNNEL_ID_TYPE = 0x0010 ; static const uint32_t RS_GENERIC_ID_GXS_DISTANT_CHAT_ID_TYPE = 0x0011 ; static const uint32_t RS_GENERIC_ID_NODE_GROUP_ID_TYPE = 0x0012 ; +static const uint32_t RS_GENERIC_ID_SHA256_ID_TYPE = 0x0013 ; typedef t_RsGenericIdType< SSL_ID_SIZE , false, RS_GENERIC_ID_SSL_ID_TYPE> SSLIdType ; typedef t_RsGenericIdType< PGP_KEY_ID_SIZE , true, RS_GENERIC_ID_PGP_ID_TYPE> PGPIdType ; typedef t_RsGenericIdType< SHA1_SIZE , false, RS_GENERIC_ID_SHA1_ID_TYPE> Sha1CheckSum ; +typedef t_RsGenericIdType< SHA256_SIZE , false, RS_GENERIC_ID_SHA256_ID_TYPE> Sha256CheckSum ; typedef t_RsGenericIdType< PGP_KEY_FINGERPRINT_SIZE, true, RS_GENERIC_ID_PGP_FINGERPRINT_TYPE> PGPFingerprintType ; typedef t_RsGenericIdType< CERT_SIGN_LEN , false, RS_GENERIC_ID_GXS_GROUP_ID_TYPE > GXSGroupId ; diff --git a/libretroshare/src/util/rsdir.cc b/libretroshare/src/util/rsdir.cc index 42e075cdf..8be50f277 100644 --- a/libretroshare/src/util/rsdir.cc +++ b/libretroshare/src/util/rsdir.cc @@ -610,6 +610,27 @@ Sha1CheckSum RsDirUtil::sha1sum(const unsigned char *data, uint32_t size) return Sha1CheckSum(sha_buf) ; } +Sha256CheckSum RsDirUtil::sha256sum(const unsigned char *data, uint32_t size) +{ + SHA256_CTX sha_ctx ; + + if(SHA256_DIGEST_LENGTH != 32) + throw std::runtime_error("Warning: can't compute Sha2561Sum with sum size != 32") ; + + SHA256_Init(&sha_ctx); + while(size > 512) + { + SHA256_Update(&sha_ctx, data, 512); + data = &data[512] ; + size -= 512 ; + } + SHA256_Update(&sha_ctx, data, size); + + unsigned char sha_buf[SHA256_DIGEST_LENGTH]; + SHA256_Final(&sha_buf[0], &sha_ctx); + + return Sha256CheckSum(sha_buf) ; +} bool RsDirUtil::saveStringToFile(const std::string &file, const std::string &str) { std::ofstream out(file.c_str(), std::ios_base::out | std::ios_base::binary); diff --git a/libretroshare/src/util/rsdir.h b/libretroshare/src/util/rsdir.h index 8f52b009f..3fd3b9f1d 100644 --- a/libretroshare/src/util/rsdir.h +++ b/libretroshare/src/util/rsdir.h @@ -101,7 +101,8 @@ bool cleanupDirectoryFaster(const std::string& dir, const std::set Date: Sun, 19 Nov 2017 18:21:56 +0100 Subject: [PATCH 02/11] added two additional non packward compatible changes for future version 0.7, and improvements of verifications of certificate signatures --- ...ON_BACKWARD_COMPATIBLE_CHANGES_FOR_V07.txt | 37 ++-- libretroshare/src/pgp/pgphandler.cc | 14 +- libretroshare/src/pgp/pgpkeyutil.cc | 12 +- libretroshare/src/pgp/pgpkeyutil.h | 44 ++++- libretroshare/src/pqi/authssl.cc | 167 ++++++++++++++---- openpgpsdk/src/openpgpsdk/packet.h | 3 + openpgpsdk/src/openpgpsdk/signature.c | 64 +++++-- openpgpsdk/src/openpgpsdk/signature.h | 2 +- 8 files changed, 274 insertions(+), 69 deletions(-) diff --git a/LIST_OF_NON_BACKWARD_COMPATIBLE_CHANGES_FOR_V07.txt b/LIST_OF_NON_BACKWARD_COMPATIBLE_CHANGES_FOR_V07.txt index 83a9efa1b..17d637807 100644 --- a/LIST_OF_NON_BACKWARD_COMPATIBLE_CHANGES_FOR_V07.txt +++ b/LIST_OF_NON_BACKWARD_COMPATIBLE_CHANGES_FOR_V07.txt @@ -1,14 +1,31 @@ V07_NON_BACKWARD_COMPATIBLE_CHANGE_001: - What: Computes the node id by performing a sha256 hash of the certificate's PGP signature, instead of simply picking up the last 20 bytes of it. - - Why: There is no real risk in forging a certificate with the same ID as the authentication is performed over the PGP signature of the certificate - which hashes the full SSL certificate (i.e. the full serialized CERT_INFO structure). However the possibility to - create two certificates with the same IDs is a problem, as it can be used to cause disturbance in the software. + What: Computes the node id by performing a sha256 hash of the certificate's PGP signature, instead of simply picking up the last 20 bytes of it. + + Why: There is no real risk in forging a certificate with the same ID as the authentication is performed over the PGP signature of the certificate + which hashes the full SSL certificate (i.e. the full serialized CERT_INFO structure). However the possibility to + create two certificates with the same IDs is a problem, as it can be used to cause disturbance in the software. - Backward compat: makes connexions impossible with non patched peers, probably because the SSL id that is computed is not the same on both side, - and in particular unpatched peers see a cerficate with ID different (because computed with the old method) than the ID that was - submitted when making friends. + Backward compat: makes connexions impossible with non patched peers, probably because the SSL id that is computed is not the same on both side, + and in particular unpatched peers see a cerficate with ID different (because computed with the old method) than the ID that was + submitted when making friends. + + Note: the advantage of basing the ID on the signature rather than the public key is not very clear, given that the signature is based on a hash + of the public key (and the rest of the certificate info). + +V07_NON_BACKWARD_COMPATIBLE_CHANGE_002: + + What: Use RSA+SHA256 instead of RSA+SHA1 for PGP certificate signatures + + Why: Sha1 is likely to be prone to primary collisions anytime soon, so it is urgent to turn to a more secure solution. + + Backward compat: unpatched peers are able to verify signatures since openpgp-sdk already handle it. + +V07_NON_BACKWARD_COMPATIBLE_CHANGE_003: + + What: Do not hash PGP certificate twice when signing + + Why: hasing twice is not per se a security issue, but it makes it harder to change the settings for hashing. + + Backward compat: patched peers cannot connect to non patched peers. - Note: the advantage of basing the ID on the signature rather than the public key is not very clear, given that the signature is based on a hash - of the public key (and the rest of the certificate info). diff --git a/libretroshare/src/pgp/pgphandler.cc b/libretroshare/src/pgp/pgphandler.cc index cb6d4e999..df95fc7f9 100644 --- a/libretroshare/src/pgp/pgphandler.cc +++ b/libretroshare/src/pgp/pgphandler.cc @@ -1382,7 +1382,11 @@ ops_secret_key_t *secret_key = NULL ; // then do the signature. ops_boolean_t not_raw = !use_raw_signature ; - ops_memory_t *memres = ops_sign_buf(data,len,(ops_sig_type_t)0x00,secret_key,ops_false,ops_false,not_raw,not_raw) ; +#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 + ops_memory_t *memres = ops_sign_buf(data,len,OPS_SIG_BINARY,OPS_HASH_SHA256,secret_key,ops_false,ops_false,not_raw,not_raw) ; +#else + ops_memory_t *memres = ops_sign_buf(data,len,OPS_SIG_BINARY,OPS_HASH_SHA1,secret_key,ops_false,ops_false,not_raw,not_raw) ; +#endif if(!memres) return false ; @@ -1695,16 +1699,16 @@ bool PGPHandler::mergeKeySignatures(ops_keydata_t *dst,const ops_keydata_t *src) bool PGPHandler::parseSignature(unsigned char *sign, unsigned int signlen,RsPgpId& issuer_id) { - uint64_t issuer ; + PGPSignatureInfo info ; - if(!PGPKeyManagement::parseSignature(sign,signlen,issuer)) + if(!PGPKeyManagement::parseSignature(sign,signlen,info)) return false ; unsigned char bytes[8] ; for(int i=0;i<8;++i) { - bytes[7-i] = issuer & 0xff ; - issuer >>= 8 ; + bytes[7-i] = info.issuer & 0xff ; + info.issuer >>= 8 ; } issuer_id = RsPgpId(bytes) ; diff --git a/libretroshare/src/pgp/pgpkeyutil.cc b/libretroshare/src/pgp/pgpkeyutil.cc index c324ab37c..ebd2ccf5b 100644 --- a/libretroshare/src/pgp/pgpkeyutil.cc +++ b/libretroshare/src/pgp/pgpkeyutil.cc @@ -160,7 +160,7 @@ uint32_t PGPKeyManagement::compute24bitsCRC(unsigned char *octets, size_t len) return crc & 0xFFFFFFL; } -bool PGPKeyManagement::parseSignature(const unsigned char *signature, size_t sign_len, uint64_t& issuer) +bool PGPKeyManagement::parseSignature(const unsigned char *signature, size_t sign_len, PGPSignatureInfo& info) { unsigned char *data = (unsigned char *)signature ; @@ -189,10 +189,10 @@ bool PGPKeyManagement::parseSignature(const unsigned char *signature, size_t sig if(signature_type != 4) return false ; - data += 1 ; // skip version number - data += 1 ; // skip signature type - data += 1 ; // skip public key algorithm - data += 1 ; // skip hash algorithm + info.signature_version = data[0] ; data += 1 ; // skip version number + info.signature_type = data[0] ; data += 1 ; // skip signature type + info.public_key_algorithm = data[0] ; data += 1 ; // skip public key algorithm + info.hash_algorithm = data[0] ; data += 1 ; // skip hash algorithm uint32_t hashed_size = 256u*data[0] + data[1] ; data += 2 ; @@ -214,7 +214,7 @@ bool PGPKeyManagement::parseSignature(const unsigned char *signature, size_t sig if(subpacket_type == PGPKeyParser::PGP_PACKET_TAG_ISSUER && subpacket_size == 9) { issuer_found = true ; - issuer = PGPKeyParser::read_KeyID(data) ; + info.issuer = PGPKeyParser::read_KeyID(data) ; } else data += subpacket_size-1 ; // we remove the size of subpacket type diff --git a/libretroshare/src/pgp/pgpkeyutil.h b/libretroshare/src/pgp/pgpkeyutil.h index 9041485f8..672550b1b 100644 --- a/libretroshare/src/pgp/pgpkeyutil.h +++ b/libretroshare/src/pgp/pgpkeyutil.h @@ -41,6 +41,46 @@ #include #include +static const uint8_t PGP_PACKET_TAG_HASH_ALGORITHM_UNKNOWN = 0 ; +static const uint8_t PGP_PACKET_TAG_HASH_ALGORITHM_MD5 = 1 ; +static const uint8_t PGP_PACKET_TAG_HASH_ALGORITHM_SHA1 = 2 ; +static const uint8_t PGP_PACKET_TAG_HASH_ALGORITHM_SHA256 = 8 ; +static const uint8_t PGP_PACKET_TAG_HASH_ALGORITHM_SHA512 = 10 ; + +static const uint8_t PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_UNKNOWN = 0 ; +static const uint8_t PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_RSA_ES = 1 ; +static const uint8_t PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_RSA_E = 2 ; +static const uint8_t PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_RSA_S = 3 ; +static const uint8_t PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_DSA = 17 ; + +static const uint8_t PGP_PACKET_TAG_SIGNATURE_VERSION_UNKNOWN = 0 ; +static const uint8_t PGP_PACKET_TAG_SIGNATURE_VERSION_V3 = 3 ; +static const uint8_t PGP_PACKET_TAG_SIGNATURE_VERSION_V4 = 4 ; + +static const uint8_t PGP_PACKET_TAG_SIGNATURE_TYPE_UNKNOWN = 0xff ; +static const uint8_t PGP_PACKET_TAG_SIGNATURE_TYPE_BINARY_DOCUMENT = 0x00 ; +static const uint8_t PGP_PACKET_TAG_SIGNATURE_TYPE_CANONICAL_TEXT = 0x01 ; +static const uint8_t PGP_PACKET_TAG_SIGNATURE_TYPE_STANDALONE_SIG = 0x02 ; +// All other consts for signature types not used, so not defines. + +class PGPSignatureInfo +{ +public: + PGPSignatureInfo() : + signature_version (PGP_PACKET_TAG_SIGNATURE_VERSION_UNKNOWN), + signature_type (PGP_PACKET_TAG_SIGNATURE_TYPE_UNKNOWN), + issuer (0), + public_key_algorithm(PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_UNKNOWN), + hash_algorithm (PGP_PACKET_TAG_HASH_ALGORITHM_UNKNOWN) + {} + + uint8_t signature_version ; + uint8_t signature_type ; + uint64_t issuer ; + uint8_t public_key_algorithm ; + uint8_t hash_algorithm ; +}; + // This class handles GPG keys. For now we only clean them from signatures, but // in the future, we might cache them to avoid unnecessary calls to gpgme. // @@ -66,7 +106,7 @@ class PGPKeyManagement // static uint32_t compute24bitsCRC(unsigned char *data,size_t len) ; - static bool parseSignature(const unsigned char *signature, size_t sign_len, uint64_t &issuer) ; + static bool parseSignature(const unsigned char *signature, size_t sign_len, PGPSignatureInfo& info) ; }; // This class handles the parsing of PGP packet headers under various (old and new) formats. @@ -74,6 +114,8 @@ class PGPKeyManagement class PGPKeyParser { public: + // These constants correspond to packet tags from RFC4880 + static const uint8_t PGP_PACKET_TAG_PUBLIC_KEY = 6 ; static const uint8_t PGP_PACKET_TAG_USER_ID = 13 ; static const uint8_t PGP_PACKET_TAG_SIGNATURE = 2 ; diff --git a/libretroshare/src/pqi/authssl.cc b/libretroshare/src/pqi/authssl.cc index 4d57b1f24..b697321ff 100644 --- a/libretroshare/src/pqi/authssl.cc +++ b/libretroshare/src/pqi/authssl.cc @@ -40,6 +40,7 @@ #include "rsitems/rsconfigitems.h" #include "util/rsdir.h" #include "util/rsstring.h" +#include "pgp/pgpkeyutil.h" #include "retroshare/rspeers.h" // for RsPeerDetails structure #include "retroshare/rsids.h" // for RsPeerDetails structure @@ -54,13 +55,17 @@ /* SSL connection diagnostic */ -const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_UNKNOWN = 0x00 ; -const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_OK = 0x01 ; -const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_CERTIFICATE_NOT_VALID = 0x02 ; -const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_ISSUER_UNKNOWN = 0x03 ; -const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_MALLOC_ERROR = 0x04 ; -const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE = 0x05 ; -const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_CERTIFICATE_MISSING = 0x06 ; +const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_UNKNOWN = 0x00 ; +const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_OK = 0x01 ; +const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_CERTIFICATE_NOT_VALID = 0x02 ; +const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_ISSUER_UNKNOWN = 0x03 ; +const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_MALLOC_ERROR = 0x04 ; +const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE = 0x05 ; +const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_CERTIFICATE_MISSING = 0x06 ; +const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_HASH_ALGORITHM_NOT_ACCEPTED = 0x07 ; +const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_KEY_ALGORITHM_NOT_ACCEPTED = 0x08 ; +const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE_TYPE = 0x09 ; +const uint32_t RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE_VERSION = 0x0a ; /**** * #define AUTHSSL_DEBUG 1 @@ -815,7 +820,11 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) ASN1_BIT_STRING *signature = const_cast(tmp_signature); #endif //EVP_PKEY *pkey = NULL; +#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 + const EVP_MD *type = EVP_sha256(); +#else const EVP_MD *type = EVP_sha1(); +#endif EVP_MD_CTX *ctx = EVP_MD_CTX_create(); int inl=0,hashoutl=0; @@ -855,44 +864,57 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) #if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER) inl=i2d(data,NULL); unsigned char *buf_in=(unsigned char *)OPENSSL_malloc((unsigned int)inl); - unsigned char *p=NULL; + i2d(data,&buf_in); #else unsigned char *buf_in=NULL; inl=i2d_re_X509_tbs(x509,&buf_in) ; // this does the i2d over x509->cert_info #endif +#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 + sigoutl=2048; // hashoutl; //EVP_PKEY_size(pkey); + unsigned char *buf_sigout=(unsigned char *)OPENSSL_malloc((unsigned int)sigoutl); + + if ((buf_in == NULL) || (buf_sigout == NULL)) + { + sigoutl=0; + fprintf(stderr, "AuthSSLimpl::SignX509Req: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_MALLOC_FAILURE)\n"); + goto err; + } + std::cerr << "Buffers Allocated" << std::endl; + + /* NOW Sign via GPG Functions */ + if (!AuthGPG::getAuthGPG()->SignDataBin(buf_in, inl, buf_sigout, (unsigned int *) &sigoutl,"AuthSSLimpl::SignX509ReqWithGPG()")) + { + sigoutl = 0; + goto err; + } +#else hashoutl=EVP_MD_size(type); unsigned char *buf_hashout=(unsigned char *)OPENSSL_malloc((unsigned int)hashoutl); sigoutl=2048; // hashoutl; //EVP_PKEY_size(pkey); unsigned char *buf_sigout=(unsigned char *)OPENSSL_malloc((unsigned int)sigoutl); - if ((buf_in == NULL) || (buf_hashout == NULL) || (buf_sigout == NULL)) - { - hashoutl=0; - sigoutl=0; - fprintf(stderr, "AuthSSLimpl::SignX509Req: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_MALLOC_FAILURE)\n"); - goto err; - } + if((buf_hashout == NULL) || (buf_sigout == NULL)) + { + hashoutl=0; + sigoutl=0; + fprintf(stderr, "AuthSSLimpl::SignX509Req: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_MALLOC_FAILURE)\n"); + goto err; + } std::cerr << "Buffers Allocated" << std::endl; -#if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER) - p=buf_in; - i2d(data,&p); -#endif - /* data in buf_in, ready to be hashed */ EVP_DigestInit_ex(ctx,type, NULL); EVP_DigestUpdate(ctx,(unsigned char *)buf_in,inl); - if (!EVP_DigestFinal(ctx,(unsigned char *)buf_hashout, - (unsigned int *)&hashoutl)) - { - hashoutl=0; - fprintf(stderr, "AuthSSLimpl::SignX509Req: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_EVP_LIB)\n"); - goto err; - } + if (!EVP_DigestFinal(ctx,(unsigned char *)buf_hashout, (unsigned int *)&hashoutl)) + { + hashoutl=0; + fprintf(stderr, "AuthSSLimpl::SignX509Req: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_EVP_LIB)\n"); + goto err; + } - std::cerr << "Digest Applied: len: " << hashoutl << std::endl; + std::cerr << "Digest Applied: len: " << hashoutl << std::endl; /* NOW Sign via GPG Functions */ if (!AuthGPG::getAuthGPG()->SignDataBin(buf_hashout, hashoutl, buf_sigout, (unsigned int *) &sigoutl,"AuthSSLimpl::SignX509ReqWithGPG()")) @@ -900,6 +922,7 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) sigoutl = 0; goto err; } +#endif std::cerr << "Buffer Sizes: in: " << inl; std::cerr << " HashOut: " << hashoutl; @@ -926,6 +949,14 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) EVP_MD_CTX_destroy(ctx) ; + // debug + { + int pkey_nid = OBJ_obj2nid(x509->cert_info->key->algor->algorithm); + const char* sslbuf = OBJ_nid2ln(pkey_nid); + + std::cerr << "Signature hash algorithm: " << sslbuf << std::endl; + } + return x509; /* XXX CLEANUP */ @@ -1038,11 +1069,13 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) p=buf_in; i2d(data,&p); #endif + +#ifndef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 /* data in buf_in, ready to be hashed */ EVP_DigestInit_ex(ctx,type, NULL); EVP_DigestUpdate(ctx,(unsigned char *)buf_in,inl); - if (!EVP_DigestFinal(ctx,(unsigned char *)buf_hashout, - (unsigned int *)&hashoutl)) + + if (!EVP_DigestFinal(ctx,(unsigned char *)buf_hashout, (unsigned int *)&hashoutl)) { hashoutl=0; fprintf(stderr, "AuthSSLimpl::AuthX509: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_EVP_LIB)\n"); @@ -1052,14 +1085,15 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) #ifdef AUTHSSL_DEBUG std::cerr << "Digest Applied: len: " << hashoutl << std::endl; +#endif #endif /* copy data into signature */ - if(sigoutl < signature->length) - { - diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_MALLOC_ERROR ; - goto err; - } + if(sigoutl < signature->length) + { + diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_MALLOC_ERROR ; + goto err; + } sigoutl = signature->length; memmove(buf_sigout, signature->data, sigoutl); @@ -1069,10 +1103,73 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) std::cerr << "AuthSSLimpl::AuthX509() verifying the gpg sig with keyprint : " << pd.fpr << std::endl; std::cerr << "Sigoutl = " << sigoutl << std::endl ; std::cerr << "pd.fpr = " << pd.fpr << std::endl ; +#ifndef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 std::cerr << "hashoutl = " << hashoutl << std::endl ; +#endif #endif + // Take a early look at signature parameters. In particular we dont accept signatures with unsecure hash algorithms. + { + PGPSignatureInfo signature_info ; + PGPKeyManagement::parseSignature(buf_sigout,sigoutl,signature_info) ; + + if(signature_info.signature_version != PGP_PACKET_TAG_SIGNATURE_VERSION_V4) + { + diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE_VERSION ; + return false ; + } + + switch(signature_info.signature_type) + { + case PGP_PACKET_TAG_SIGNATURE_TYPE_STANDALONE_SIG : + break ; + + case PGP_PACKET_TAG_SIGNATURE_TYPE_CANONICAL_TEXT : + case PGP_PACKET_TAG_SIGNATURE_TYPE_BINARY_DOCUMENT : + case PGP_PACKET_TAG_SIGNATURE_TYPE_UNKNOWN : + default: + diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE_TYPE ; + return false ; + } + + switch(signature_info.hash_algorithm) + { + case PGP_PACKET_TAG_HASH_ALGORITHM_SHA1 : + case PGP_PACKET_TAG_HASH_ALGORITHM_SHA256: + case PGP_PACKET_TAG_HASH_ALGORITHM_SHA512: + break ; + + // We dont accept signatures with unknown or week hash algorithms. + + case PGP_PACKET_TAG_HASH_ALGORITHM_MD5: + case PGP_PACKET_TAG_HASH_ALGORITHM_UNKNOWN: + default: + diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_HASH_ALGORITHM_NOT_ACCEPTED ; + return false ; + } + + switch(signature_info.public_key_algorithm) + { + case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_RSA_ES : + case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_RSA_S : + case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_DSA : + break ; + + case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_RSA_E : + case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_UNKNOWN: + default: + diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_HASH_ALGORITHM_NOT_ACCEPTED ; + return false ; + } + } + + // passed, verify the signature itself + +#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 + if (!AuthGPG::getAuthGPG()->VerifySignBin(buf_in, inl, buf_sigout, (unsigned int) sigoutl, pd.fpr)) { +#else if (!AuthGPG::getAuthGPG()->VerifySignBin(buf_hashout, hashoutl, buf_sigout, (unsigned int) sigoutl, pd.fpr)) { +#endif sigoutl = 0; diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE ; goto err; diff --git a/openpgpsdk/src/openpgpsdk/packet.h b/openpgpsdk/src/openpgpsdk/packet.h index cbbd3499e..49e59a556 100644 --- a/openpgpsdk/src/openpgpsdk/packet.h +++ b/openpgpsdk/src/openpgpsdk/packet.h @@ -348,7 +348,10 @@ typedef enum // SHA1 Hash Size \todo is this the same as OPS_CHECKHASH_SIZE?? #define OPS_SHA1_HASH_SIZE SHA_DIGEST_LENGTH +#define OPS_SHA224_HASH_SIZE SHA224_DIGEST_LENGTH #define OPS_SHA256_HASH_SIZE SHA256_DIGEST_LENGTH +#define OPS_SHA384_HASH_SIZE SHA384_DIGEST_LENGTH +#define OPS_SHA512_HASH_SIZE SHA512_DIGEST_LENGTH // Max hash size #define OPS_MAX_HASH_SIZE 64 diff --git a/openpgpsdk/src/openpgpsdk/signature.c b/openpgpsdk/src/openpgpsdk/signature.c index 2da7bf087..b62ebdb9f 100644 --- a/openpgpsdk/src/openpgpsdk/signature.c +++ b/openpgpsdk/src/openpgpsdk/signature.c @@ -216,8 +216,49 @@ static ops_boolean_t rsa_sign(ops_hash_t *hash, const ops_rsa_public_key_t *rsa, unsigned t; BIGNUM *bn; + int plen ; + unsigned int hash_length ; + unsigned char *prefix ; + + switch(hash->algorithm) + { + case OPS_HASH_SHA1: hashsize=OPS_SHA1_HASH_SIZE+sizeof prefix_sha1; + hash_length=OPS_SHA1_HASH_SIZE ; + prefix = prefix_sha1; + plen = sizeof prefix_sha1 ; + break ; + + case OPS_HASH_SHA224: hashsize=OPS_SHA224_HASH_SIZE+sizeof prefix_sha224; + hash_length=OPS_SHA224_HASH_SIZE ; + prefix = prefix_sha224; + plen = sizeof prefix_sha224 ; + break ; + + case OPS_HASH_SHA256: hashsize=OPS_SHA256_HASH_SIZE+sizeof prefix_sha256; + hash_length=OPS_SHA256_HASH_SIZE ; + prefix = prefix_sha256; + plen = sizeof prefix_sha256 ; + break ; + + case OPS_HASH_SHA384: hashsize=OPS_SHA384_HASH_SIZE+sizeof prefix_sha384; + hash_length=OPS_SHA384_HASH_SIZE ; + prefix = prefix_sha384; + plen = sizeof prefix_sha384 ; + break ; + + case OPS_HASH_SHA512: hashsize=OPS_SHA512_HASH_SIZE+sizeof prefix_sha512; + hash_length=OPS_SHA512_HASH_SIZE ; + prefix = prefix_sha512; + plen = sizeof prefix_sha512 ; + break ; + + case OPS_HASH_MD5: fprintf(stderr,"(insecure) MD5+RSA signatures not supported in RSA sign") ; + return ops_false ; + + default: fprintf(stderr,"Hash algorithm %d not supported in RSA sign",hash->algorithm) ; + return ops_false ; + } // XXX: we assume hash is sha-1 for now - hashsize=20+sizeof prefix_sha1; keysize=BN_num_bytes(rsa->n); @@ -240,12 +281,12 @@ static ops_boolean_t rsa_sign(ops_hash_t *hash, const ops_rsa_public_key_t *rsa, hashbuf[n]=0xff; hashbuf[n++]=0; - memcpy(&hashbuf[n], prefix_sha1, sizeof prefix_sha1); - n+=sizeof prefix_sha1; + memcpy(&hashbuf[n], prefix, plen); + n+=plen; t=hash->finish(hash, &hashbuf[n]); - if(t != 20) + if(t != hash_length) { fprintf(stderr,"Wrong hash size. Should be 20! can't sign.") ; return ops_false ; @@ -1376,12 +1417,13 @@ void example(const ops_secret_key_t *skey) \endcode */ ops_memory_t* ops_sign_buf(const void* input, const size_t input_len, - const ops_sig_type_t sig_type, - const ops_secret_key_t *skey, - const ops_boolean_t use_armour, - ops_boolean_t include_data, - ops_boolean_t include_creation_time, - ops_boolean_t include_key_id) + const ops_sig_type_t sig_type, + const ops_hash_algorithm_t hash_algorithm, + const ops_secret_key_t *skey, + const ops_boolean_t use_armour, + ops_boolean_t include_data, + ops_boolean_t include_creation_time, + ops_boolean_t include_key_id) { // \todo allow choice of hash algorithams // enforce use of SHA1 for now @@ -1392,7 +1434,7 @@ ops_memory_t* ops_sign_buf(const void* input, const size_t input_len, ops_create_info_t *cinfo=NULL; ops_memory_t *mem=ops_memory_new(); - ops_hash_algorithm_t hash_alg=OPS_HASH_SHA1; + ops_hash_algorithm_t hash_alg=hash_algorithm ; ops_literal_data_type_t ld_type; ops_hash_t* hash=NULL; diff --git a/openpgpsdk/src/openpgpsdk/signature.h b/openpgpsdk/src/openpgpsdk/signature.h index a40c073ff..eb87d0ee5 100644 --- a/openpgpsdk/src/openpgpsdk/signature.h +++ b/openpgpsdk/src/openpgpsdk/signature.h @@ -90,7 +90,7 @@ void ops_signature_add_primary_user_id(ops_create_signature_t *sig, ops_boolean_t ops_sign_file_as_cleartext(const char* input_filename, const char* output_filename, const ops_secret_key_t *skey, const ops_boolean_t overwrite); ops_boolean_t ops_sign_buf_as_cleartext(const char* input, const size_t len, ops_memory_t** output, const ops_secret_key_t *skey); ops_boolean_t ops_sign_file(const char* input_filename, const char* output_filename, const ops_secret_key_t *skey, const ops_boolean_t use_armour, const ops_boolean_t overwrite); -ops_memory_t * ops_sign_buf(const void* input, const size_t input_len, const ops_sig_type_t sig_type, const ops_secret_key_t *skey, const ops_boolean_t use_armour,ops_boolean_t include_data,ops_boolean_t include_creation_time,ops_boolean_t include_key_id); +ops_memory_t * ops_sign_buf(const void* input, const size_t input_len, const ops_sig_type_t sig_type, const ops_hash_algorithm_t hash_algorithm, const ops_secret_key_t *skey, const ops_boolean_t use_armour, ops_boolean_t include_data, ops_boolean_t include_creation_time, ops_boolean_t include_key_id); ops_boolean_t ops_writer_push_signed(ops_create_info_t *cinfo, const ops_sig_type_t sig_type, const ops_secret_key_t *skey); #endif From e72bd9ff4fdeef44d02df38e5cf70485bccb3e7d Mon Sep 17 00:00:00 2001 From: csoler Date: Sun, 19 Nov 2017 18:38:46 +0100 Subject: [PATCH 03/11] fixed bug causing certificate rejection --- libretroshare/src/pqi/authssl.cc | 51 +++++++++++++++++++------------- 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/libretroshare/src/pqi/authssl.cc b/libretroshare/src/pqi/authssl.cc index b697321ff..6c224f2f7 100644 --- a/libretroshare/src/pqi/authssl.cc +++ b/libretroshare/src/pqi/authssl.cc @@ -533,6 +533,7 @@ bool AuthSSLimpl::validateOwnCertificate(X509 *x509, EVP_PKEY *pkey) /* standard authentication */ if (!AuthX509WithGPG(x509,diagnostic)) { + std::cerr << "Validate Own certificate ERROR: diagnostic = " << diagnostic << std::endl; return false; } return true; @@ -1024,7 +1025,11 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) #endif +#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 + const EVP_MD *type = EVP_sha256(); +#else const EVP_MD *type = EVP_sha1(); +#endif EVP_MD_CTX *ctx = EVP_MD_CTX_create(); int inl=0,hashoutl=0; @@ -1119,25 +1124,42 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) return false ; } + std::string sigtypestring ; + switch(signature_info.signature_type) { - case PGP_PACKET_TAG_SIGNATURE_TYPE_STANDALONE_SIG : + case PGP_PACKET_TAG_SIGNATURE_TYPE_BINARY_DOCUMENT : break ; + case PGP_PACKET_TAG_SIGNATURE_TYPE_STANDALONE_SIG : case PGP_PACKET_TAG_SIGNATURE_TYPE_CANONICAL_TEXT : - case PGP_PACKET_TAG_SIGNATURE_TYPE_BINARY_DOCUMENT : case PGP_PACKET_TAG_SIGNATURE_TYPE_UNKNOWN : default: diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE_TYPE ; return false ; } + switch(signature_info.public_key_algorithm) + { + case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_RSA_ES : + case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_RSA_S : sigtypestring = "RSA" ; + break ; + + case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_DSA : sigtypestring = "DSA" ; + break ; + + case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_RSA_E : + case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_UNKNOWN: + default: + diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_HASH_ALGORITHM_NOT_ACCEPTED ; + return false ; + } + switch(signature_info.hash_algorithm) { - case PGP_PACKET_TAG_HASH_ALGORITHM_SHA1 : - case PGP_PACKET_TAG_HASH_ALGORITHM_SHA256: - case PGP_PACKET_TAG_HASH_ALGORITHM_SHA512: - break ; + case PGP_PACKET_TAG_HASH_ALGORITHM_SHA1 : sigtypestring += "+SHA1" ; break; + case PGP_PACKET_TAG_HASH_ALGORITHM_SHA256: sigtypestring += "+SHA256" ; break; + case PGP_PACKET_TAG_HASH_ALGORITHM_SHA512: sigtypestring += "+SHA512" ; break; // We dont accept signatures with unknown or week hash algorithms. @@ -1148,20 +1170,6 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) return false ; } - switch(signature_info.public_key_algorithm) - { - case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_RSA_ES : - case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_RSA_S : - case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_DSA : - break ; - - case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_RSA_E : - case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_UNKNOWN: - default: - diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_HASH_ALGORITHM_NOT_ACCEPTED ; - return false ; - } - } // passed, verify the signature itself @@ -1175,6 +1183,9 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) goto err; } + std::cerr << "Verified signature of type " << sigtypestring << " on certificate using PGP key with fingerprint " << pd.fpr.toStdString() << std::endl; + } + #ifdef AUTHSSL_DEBUG std::cerr << "AuthSSLimpl::AuthX509() X509 authenticated" << std::endl; #endif From 863e6256c3e16774f207bd3b1ef3c241232a03da Mon Sep 17 00:00:00 2001 From: csoler Date: Sun, 19 Nov 2017 19:34:54 +0100 Subject: [PATCH 04/11] centralized the defines into a single file --- ...ON_BACKWARD_COMPATIBLE_CHANGES_FOR_V07.txt | 31 ---------------- libretroshare/src/pgp/pgphandler.cc | 2 +- libretroshare/src/pqi/authssl.cc | 17 +++++---- libretroshare/src/pqi/sslfns.cc | 15 ++++---- libretroshare/src/retroshare/rsdefines.h | 37 +++++++++++++++++++ 5 files changed, 55 insertions(+), 47 deletions(-) delete mode 100644 LIST_OF_NON_BACKWARD_COMPATIBLE_CHANGES_FOR_V07.txt create mode 100644 libretroshare/src/retroshare/rsdefines.h diff --git a/LIST_OF_NON_BACKWARD_COMPATIBLE_CHANGES_FOR_V07.txt b/LIST_OF_NON_BACKWARD_COMPATIBLE_CHANGES_FOR_V07.txt deleted file mode 100644 index 17d637807..000000000 --- a/LIST_OF_NON_BACKWARD_COMPATIBLE_CHANGES_FOR_V07.txt +++ /dev/null @@ -1,31 +0,0 @@ -V07_NON_BACKWARD_COMPATIBLE_CHANGE_001: - - What: Computes the node id by performing a sha256 hash of the certificate's PGP signature, instead of simply picking up the last 20 bytes of it. - - Why: There is no real risk in forging a certificate with the same ID as the authentication is performed over the PGP signature of the certificate - which hashes the full SSL certificate (i.e. the full serialized CERT_INFO structure). However the possibility to - create two certificates with the same IDs is a problem, as it can be used to cause disturbance in the software. - - Backward compat: makes connexions impossible with non patched peers, probably because the SSL id that is computed is not the same on both side, - and in particular unpatched peers see a cerficate with ID different (because computed with the old method) than the ID that was - submitted when making friends. - - Note: the advantage of basing the ID on the signature rather than the public key is not very clear, given that the signature is based on a hash - of the public key (and the rest of the certificate info). - -V07_NON_BACKWARD_COMPATIBLE_CHANGE_002: - - What: Use RSA+SHA256 instead of RSA+SHA1 for PGP certificate signatures - - Why: Sha1 is likely to be prone to primary collisions anytime soon, so it is urgent to turn to a more secure solution. - - Backward compat: unpatched peers are able to verify signatures since openpgp-sdk already handle it. - -V07_NON_BACKWARD_COMPATIBLE_CHANGE_003: - - What: Do not hash PGP certificate twice when signing - - Why: hasing twice is not per se a security issue, but it makes it harder to change the settings for hashing. - - Backward compat: patched peers cannot connect to non patched peers. - diff --git a/libretroshare/src/pgp/pgphandler.cc b/libretroshare/src/pgp/pgphandler.cc index df95fc7f9..195746bb5 100644 --- a/libretroshare/src/pgp/pgphandler.cc +++ b/libretroshare/src/pgp/pgphandler.cc @@ -1382,7 +1382,7 @@ ops_secret_key_t *secret_key = NULL ; // then do the signature. ops_boolean_t not_raw = !use_raw_signature ; -#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 +#if V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 ops_memory_t *memres = ops_sign_buf(data,len,OPS_SIG_BINARY,OPS_HASH_SHA256,secret_key,ops_false,ops_false,not_raw,not_raw) ; #else ops_memory_t *memres = ops_sign_buf(data,len,OPS_SIG_BINARY,OPS_HASH_SHA1,secret_key,ops_false,ops_false,not_raw,not_raw) ; diff --git a/libretroshare/src/pqi/authssl.cc b/libretroshare/src/pqi/authssl.cc index 6c224f2f7..fa0309945 100644 --- a/libretroshare/src/pqi/authssl.cc +++ b/libretroshare/src/pqi/authssl.cc @@ -43,7 +43,8 @@ #include "pgp/pgpkeyutil.h" #include "retroshare/rspeers.h" // for RsPeerDetails structure -#include "retroshare/rsids.h" // for RsPeerDetails structure +#include "retroshare/rsdefines.h" +#include "retroshare/rsids.h" // for RsPeerDetails structure #include "rsserver/p3face.h" /******************** notify of new Cert **************************/ @@ -821,7 +822,7 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) ASN1_BIT_STRING *signature = const_cast(tmp_signature); #endif //EVP_PKEY *pkey = NULL; -#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 +#if V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 const EVP_MD *type = EVP_sha256(); #else const EVP_MD *type = EVP_sha1(); @@ -871,7 +872,7 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) inl=i2d_re_X509_tbs(x509,&buf_in) ; // this does the i2d over x509->cert_info #endif -#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 +#if V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 sigoutl=2048; // hashoutl; //EVP_PKEY_size(pkey); unsigned char *buf_sigout=(unsigned char *)OPENSSL_malloc((unsigned int)sigoutl); @@ -965,8 +966,10 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) /* cleanup */ if(buf_in != NULL) OPENSSL_free(buf_in) ; +#if !V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 if(buf_hashout != NULL) OPENSSL_free(buf_hashout) ; +#endif if(buf_sigout != NULL) OPENSSL_free(buf_sigout) ; std::cerr << "GPGAuthMgr::SignX509Req() err: FAIL" << std::endl; @@ -1025,7 +1028,7 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) #endif -#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 +#if V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 const EVP_MD *type = EVP_sha256(); #else const EVP_MD *type = EVP_sha1(); @@ -1075,7 +1078,7 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) i2d(data,&p); #endif -#ifndef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 +#if !V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 /* data in buf_in, ready to be hashed */ EVP_DigestInit_ex(ctx,type, NULL); EVP_DigestUpdate(ctx,(unsigned char *)buf_in,inl); @@ -1108,7 +1111,7 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) std::cerr << "AuthSSLimpl::AuthX509() verifying the gpg sig with keyprint : " << pd.fpr << std::endl; std::cerr << "Sigoutl = " << sigoutl << std::endl ; std::cerr << "pd.fpr = " << pd.fpr << std::endl ; -#ifndef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 +#if !V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 std::cerr << "hashoutl = " << hashoutl << std::endl ; #endif #endif @@ -1173,7 +1176,7 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) // passed, verify the signature itself -#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 +#if V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 if (!AuthGPG::getAuthGPG()->VerifySignBin(buf_in, inl, buf_sigout, (unsigned int) sigoutl, pd.fpr)) { #else if (!AuthGPG::getAuthGPG()->VerifySignBin(buf_hashout, hashoutl, buf_sigout, (unsigned int) sigoutl, pd.fpr)) { diff --git a/libretroshare/src/pqi/sslfns.cc b/libretroshare/src/pqi/sslfns.cc index 16ba1744d..56a775b15 100644 --- a/libretroshare/src/pqi/sslfns.cc +++ b/libretroshare/src/pqi/sslfns.cc @@ -611,8 +611,7 @@ bool getX509id(X509 *x509, RsPeerId& xid) X509_get0_signature(&signature,&algor,x509); #endif -#ifndef V07_NON_BACKWARD_COMPATIBLE_CHANGE_001 - +#if V07_NON_BACKWARD_COMPATIBLE_CHANGE_001 // What: Computes the node id by performing a sha256 hash of the certificate's PGP signature, instead of simply picking up the last 20 bytes of it. // // Why: There is no real risk in forging a certificate with the same ID as the authentication is performed over the PGP signature of the certificate @@ -626,6 +625,12 @@ bool getX509id(X509 *x509, RsPeerId& xid) // Note: the advantage of basing the ID on the signature rather than the public key is not very clear, given that the signature is based on a hash // of the public key (and the rest of the certificate info). // + + if(RsPeerId::SIZE_IN_BYTES > Sha256CheckSum::SIZE_IN_BYTES) + return false ; + + xid = RsPeerId(RsDirUtil::sha256sum(ASN1_STRING_data(const_cast(signature)),ASN1_STRING_length(signature)).toByteArray()) ; +#else int signlen = ASN1_STRING_length(signature); if (signlen < CERTSIGNLEN) { @@ -646,12 +651,6 @@ bool getX509id(X509 *x509, RsPeerId& xid) #warning csoler 2017-02-19: This is cryptographically horrible. We should hash the entire signature here! xid = RsPeerId(&signdata[signlen - CERTSIGNLEN]) ; -#else - - if(RsPeerId::SIZE_IN_BYTES > Sha256CheckSum::SIZE_IN_BYTES) - return false ; - - xid = RsPeerId(RsDirUtil::sha256sum(ASN1_STRING_data(const_cast(signature)),ASN1_STRING_length(signature)).toByteArray()) ; #endif return true; diff --git a/libretroshare/src/retroshare/rsdefines.h b/libretroshare/src/retroshare/rsdefines.h new file mode 100644 index 000000000..0a47307e9 --- /dev/null +++ b/libretroshare/src/retroshare/rsdefines.h @@ -0,0 +1,37 @@ +/************************************************************************************************************************************************** + * + * V07_NON_BACKWARD_COMPATIBLE_CHANGE_001: + * + * What: Computes the node id by performing a sha256 hash of the certificate's PGP signature, instead of simply picking up the last 20 bytes of it. + * + * Why: There is no real risk in forging a certificate with the same ID as the authentication is performed over the PGP signature of the certificate + * which hashes the full SSL certificate (i.e. the full serialized CERT_INFO structure). However the possibility to + * create two certificates with the same IDs is a problem, as it can be used to cause disturbance in the software. + * + * Backward compat: makes connexions impossible with non patched peers, probably because the SSL id that is computed is not the same on both side, + * and in particular unpatched peers see a cerficate with ID different (because computed with the old method) than the ID that was + * submitted when making friends. + * + * Note: the advantage of basing the ID on the signature rather than the public key is not very clear, given that the signature is based on a hash + * of the public key (and the rest of the certificate info). + * + * V07_NON_BACKWARD_COMPATIBLE_CHANGE_002: + * + * What: Use RSA+SHA256 instead of RSA+SHA1 for PGP certificate signatures + * + * Why: Sha1 is likely to be prone to primary collisions anytime soon, so it is urgent to turn to a more secure solution. + * + * Backward compat: unpatched peers are able to verify signatures since openpgp-sdk already handle it. + * + * V07_NON_BACKWARD_COMPATIBLE_CHANGE_003: + * + * What: Do not hash PGP certificate twice when signing + * + * Why: hasing twice is not per se a security issue, but it makes it harder to change the settings for hashing. + * + * Backward compat: patched peers cannot connect to non patched peers. + ***************************************************************************************************************************************************/ + +#define V07_NON_BACKWARD_COMPATIBLE_CHANGE_001 False +#define V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 False +#define V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 False From ef1a61374cec8a39732937e43fdb5e1c1d42ab30 Mon Sep 17 00:00:00 2001 From: csoler Date: Sun, 19 Nov 2017 19:57:38 +0100 Subject: [PATCH 05/11] added proper debug output for signature verification --- libretroshare/src/pqi/authssl.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libretroshare/src/pqi/authssl.cc b/libretroshare/src/pqi/authssl.cc index fa0309945..4325ed46b 100644 --- a/libretroshare/src/pqi/authssl.cc +++ b/libretroshare/src/pqi/authssl.cc @@ -1186,7 +1186,10 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) goto err; } - std::cerr << "Verified signature of type " << sigtypestring << " on certificate using PGP key with fingerprint " << pd.fpr.toStdString() << std::endl; + RsPeerId peerIdstr ; + getX509id(x509, peerIdstr) ; + + std::cerr << "Verified signature of type " << sigtypestring << " on certificate " << peerIdstr << " using PGP key with fingerprint " << pd.fpr.toStdString() << std::endl; } #ifdef AUTHSSL_DEBUG From 9367aa0d84a1ea727d765cf0140320c29bcce5c8 Mon Sep 17 00:00:00 2001 From: csoler Date: Sun, 19 Nov 2017 20:15:36 +0100 Subject: [PATCH 06/11] changed #ifs into #ifdefs because it us more robust, and moved the definition of variables to retroshare.pri --- libretroshare/src/pgp/pgphandler.cc | 2 +- libretroshare/src/pqi/authssl.cc | 15 +++++---- libretroshare/src/pqi/sslfns.cc | 2 +- libretroshare/src/retroshare/rsdefines.h | 37 ---------------------- retroshare.pri | 40 ++++++++++++++++++++++++ 5 files changed, 49 insertions(+), 47 deletions(-) delete mode 100644 libretroshare/src/retroshare/rsdefines.h diff --git a/libretroshare/src/pgp/pgphandler.cc b/libretroshare/src/pgp/pgphandler.cc index 195746bb5..df95fc7f9 100644 --- a/libretroshare/src/pgp/pgphandler.cc +++ b/libretroshare/src/pgp/pgphandler.cc @@ -1382,7 +1382,7 @@ ops_secret_key_t *secret_key = NULL ; // then do the signature. ops_boolean_t not_raw = !use_raw_signature ; -#if V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 +#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 ops_memory_t *memres = ops_sign_buf(data,len,OPS_SIG_BINARY,OPS_HASH_SHA256,secret_key,ops_false,ops_false,not_raw,not_raw) ; #else ops_memory_t *memres = ops_sign_buf(data,len,OPS_SIG_BINARY,OPS_HASH_SHA1,secret_key,ops_false,ops_false,not_raw,not_raw) ; diff --git a/libretroshare/src/pqi/authssl.cc b/libretroshare/src/pqi/authssl.cc index 4325ed46b..524930220 100644 --- a/libretroshare/src/pqi/authssl.cc +++ b/libretroshare/src/pqi/authssl.cc @@ -43,7 +43,6 @@ #include "pgp/pgpkeyutil.h" #include "retroshare/rspeers.h" // for RsPeerDetails structure -#include "retroshare/rsdefines.h" #include "retroshare/rsids.h" // for RsPeerDetails structure #include "rsserver/p3face.h" @@ -822,7 +821,7 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) ASN1_BIT_STRING *signature = const_cast(tmp_signature); #endif //EVP_PKEY *pkey = NULL; -#if V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 +#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 const EVP_MD *type = EVP_sha256(); #else const EVP_MD *type = EVP_sha1(); @@ -872,7 +871,7 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) inl=i2d_re_X509_tbs(x509,&buf_in) ; // this does the i2d over x509->cert_info #endif -#if V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 +#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 sigoutl=2048; // hashoutl; //EVP_PKEY_size(pkey); unsigned char *buf_sigout=(unsigned char *)OPENSSL_malloc((unsigned int)sigoutl); @@ -966,7 +965,7 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) /* cleanup */ if(buf_in != NULL) OPENSSL_free(buf_in) ; -#if !V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 +#ifndef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 if(buf_hashout != NULL) OPENSSL_free(buf_hashout) ; #endif @@ -1028,7 +1027,7 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) #endif -#if V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 +#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 const EVP_MD *type = EVP_sha256(); #else const EVP_MD *type = EVP_sha1(); @@ -1078,7 +1077,7 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) i2d(data,&p); #endif -#if !V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 +#ifndef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 /* data in buf_in, ready to be hashed */ EVP_DigestInit_ex(ctx,type, NULL); EVP_DigestUpdate(ctx,(unsigned char *)buf_in,inl); @@ -1111,7 +1110,7 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) std::cerr << "AuthSSLimpl::AuthX509() verifying the gpg sig with keyprint : " << pd.fpr << std::endl; std::cerr << "Sigoutl = " << sigoutl << std::endl ; std::cerr << "pd.fpr = " << pd.fpr << std::endl ; -#if !V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 +#ifndef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 std::cerr << "hashoutl = " << hashoutl << std::endl ; #endif #endif @@ -1176,7 +1175,7 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) // passed, verify the signature itself -#if V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 +#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 if (!AuthGPG::getAuthGPG()->VerifySignBin(buf_in, inl, buf_sigout, (unsigned int) sigoutl, pd.fpr)) { #else if (!AuthGPG::getAuthGPG()->VerifySignBin(buf_hashout, hashoutl, buf_sigout, (unsigned int) sigoutl, pd.fpr)) { diff --git a/libretroshare/src/pqi/sslfns.cc b/libretroshare/src/pqi/sslfns.cc index 56a775b15..d9fbf61d2 100644 --- a/libretroshare/src/pqi/sslfns.cc +++ b/libretroshare/src/pqi/sslfns.cc @@ -611,7 +611,7 @@ bool getX509id(X509 *x509, RsPeerId& xid) X509_get0_signature(&signature,&algor,x509); #endif -#if V07_NON_BACKWARD_COMPATIBLE_CHANGE_001 +#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_001 // What: Computes the node id by performing a sha256 hash of the certificate's PGP signature, instead of simply picking up the last 20 bytes of it. // // Why: There is no real risk in forging a certificate with the same ID as the authentication is performed over the PGP signature of the certificate diff --git a/libretroshare/src/retroshare/rsdefines.h b/libretroshare/src/retroshare/rsdefines.h deleted file mode 100644 index 0a47307e9..000000000 --- a/libretroshare/src/retroshare/rsdefines.h +++ /dev/null @@ -1,37 +0,0 @@ -/************************************************************************************************************************************************** - * - * V07_NON_BACKWARD_COMPATIBLE_CHANGE_001: - * - * What: Computes the node id by performing a sha256 hash of the certificate's PGP signature, instead of simply picking up the last 20 bytes of it. - * - * Why: There is no real risk in forging a certificate with the same ID as the authentication is performed over the PGP signature of the certificate - * which hashes the full SSL certificate (i.e. the full serialized CERT_INFO structure). However the possibility to - * create two certificates with the same IDs is a problem, as it can be used to cause disturbance in the software. - * - * Backward compat: makes connexions impossible with non patched peers, probably because the SSL id that is computed is not the same on both side, - * and in particular unpatched peers see a cerficate with ID different (because computed with the old method) than the ID that was - * submitted when making friends. - * - * Note: the advantage of basing the ID on the signature rather than the public key is not very clear, given that the signature is based on a hash - * of the public key (and the rest of the certificate info). - * - * V07_NON_BACKWARD_COMPATIBLE_CHANGE_002: - * - * What: Use RSA+SHA256 instead of RSA+SHA1 for PGP certificate signatures - * - * Why: Sha1 is likely to be prone to primary collisions anytime soon, so it is urgent to turn to a more secure solution. - * - * Backward compat: unpatched peers are able to verify signatures since openpgp-sdk already handle it. - * - * V07_NON_BACKWARD_COMPATIBLE_CHANGE_003: - * - * What: Do not hash PGP certificate twice when signing - * - * Why: hasing twice is not per se a security issue, but it makes it harder to change the settings for hashing. - * - * Backward compat: patched peers cannot connect to non patched peers. - ***************************************************************************************************************************************************/ - -#define V07_NON_BACKWARD_COMPATIBLE_CHANGE_001 False -#define V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 False -#define V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 False diff --git a/retroshare.pri b/retroshare.pri index 0b61671f6..3b2ef6ec6 100644 --- a/retroshare.pri +++ b/retroshare.pri @@ -262,3 +262,43 @@ rs_async_chat { rs_chatserver { DEFINES *= RS_CHATSERVER } + +########################################################################################################################################################### +# +# V07_NON_BACKWARD_COMPATIBLE_CHANGE_001: +# +# What: Computes the node id by performing a sha256 hash of the certificate's PGP signature, instead of simply picking up the last 20 bytes of it. +# +# Why: There is no real risk in forging a certificate with the same ID as the authentication is performed over the PGP signature of the certificate +# which hashes the full SSL certificate (i.e. the full serialized CERT_INFO structure). However the possibility to +# create two certificates with the same IDs is a problem, as it can be used to cause disturbance in the software. +# +# Backward compat: makes connexions impossible with non patched peers, probably because the SSL id that is computed is not the same on both side, +# and in particular unpatched peers see a cerficate with ID different (because computed with the old method) than the ID that was +# submitted when making friends. +# +# Note: the advantage of basing the ID on the signature rather than the public key is not very clear, given that the signature is based on a hash +# of the public key (and the rest of the certificate info). +# +# V07_NON_BACKWARD_COMPATIBLE_CHANGE_002: +# +# What: Use RSA+SHA256 instead of RSA+SHA1 for PGP certificate signatures +# +# Why: Sha1 is likely to be prone to primary collisions anytime soon, so it is urgent to turn to a more secure solution. +# +# Backward compat: unpatched peers are able to verify signatures since openpgp-sdk already handle it. +# +# V07_NON_BACKWARD_COMPATIBLE_CHANGE_003: +# +# What: Do not hash PGP certificate twice when signing +# +# Why: hasing twice is not per se a security issue, but it makes it harder to change the settings for hashing. +# +# Backward compat: patched peers cannot connect to non patched peers. +########################################################################################################################################################### + +rs_v07_changes { + DEFINES += V07_NON_BACKWARD_COMPATIBLE_CHANGE_001 + DEFINES += V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 + DEFINES += V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 +} From cd51afbc7033cd845325ff4d02abef0f5775759a Mon Sep 17 00:00:00 2001 From: csoler Date: Mon, 20 Nov 2017 22:26:14 +0100 Subject: [PATCH 07/11] fixed small bug in signature and compilation --- libretroshare/src/pqi/authssl.cc | 37 +++++++++++++++++--------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/libretroshare/src/pqi/authssl.cc b/libretroshare/src/pqi/authssl.cc index 524930220..11cbe8bf9 100644 --- a/libretroshare/src/pqi/authssl.cc +++ b/libretroshare/src/pqi/authssl.cc @@ -828,8 +828,7 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) #endif EVP_MD_CTX *ctx = EVP_MD_CTX_create(); - int inl=0,hashoutl=0; - int sigoutl=0; + int inl=0; X509_ALGOR *a; /* FIX ALGORITHMS */ @@ -861,21 +860,29 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) std::cerr << "Algorithms Fixed" << std::endl; + unsigned int sigoutl=2048; // hashoutl; //EVP_PKEY_size(pkey); + unsigned char *buf_sigout=(unsigned char *)OPENSSL_malloc((unsigned int)sigoutl); + /* input buffer */ #if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER) inl=i2d(data,NULL); unsigned char *buf_in=(unsigned char *)OPENSSL_malloc((unsigned int)inl); - i2d(data,&buf_in); + + if(buf_in == NULL) + { + sigoutl=0; + fprintf(stderr, "AuthSSLimpl::SignX509Req: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_MALLOC_FAILURE)\n"); + return NULL ; + } + unsigned char *p=buf_in; // This because i2d modifies the pointer after writing to it. + i2d(data,&p); #else unsigned char *buf_in=NULL; inl=i2d_re_X509_tbs(x509,&buf_in) ; // this does the i2d over x509->cert_info #endif #ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 - sigoutl=2048; // hashoutl; //EVP_PKEY_size(pkey); - unsigned char *buf_sigout=(unsigned char *)OPENSSL_malloc((unsigned int)sigoutl); - - if ((buf_in == NULL) || (buf_sigout == NULL)) + if((buf_in == NULL) || (buf_sigout == NULL)) { sigoutl=0; fprintf(stderr, "AuthSSLimpl::SignX509Req: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_MALLOC_FAILURE)\n"); @@ -890,12 +897,9 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) goto err; } #else - hashoutl=EVP_MD_size(type); + unsigned int hashoutl=EVP_MD_size(type); unsigned char *buf_hashout=(unsigned char *)OPENSSL_malloc((unsigned int)hashoutl); - sigoutl=2048; // hashoutl; //EVP_PKEY_size(pkey); - unsigned char *buf_sigout=(unsigned char *)OPENSSL_malloc((unsigned int)sigoutl); - if((buf_hashout == NULL) || (buf_sigout == NULL)) { hashoutl=0; @@ -951,12 +955,11 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) EVP_MD_CTX_destroy(ctx) ; // debug - { - int pkey_nid = OBJ_obj2nid(x509->cert_info->key->algor->algorithm); - const char* sslbuf = OBJ_nid2ln(pkey_nid); - - std::cerr << "Signature hash algorithm: " << sslbuf << std::endl; - } + // { + // int pkey_nid = OBJ_obj2nid(x509->sig_alg->algorithm); + // const char* sslbuf = OBJ_nid2ln(pkey_nid); + // std::cerr << "Signature hash algorithm: " << sslbuf << std::endl; + // } return x509; From e2c1661c49c3044f1ac5943c6779ab8f1e08fb34 Mon Sep 17 00:00:00 2001 From: csoler Date: Mon, 20 Nov 2017 22:44:34 +0100 Subject: [PATCH 08/11] fixed compilation with v0.7 defines --- libretroshare/src/pqi/authssl.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libretroshare/src/pqi/authssl.cc b/libretroshare/src/pqi/authssl.cc index 11cbe8bf9..01f4fac68 100644 --- a/libretroshare/src/pqi/authssl.cc +++ b/libretroshare/src/pqi/authssl.cc @@ -930,7 +930,9 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) #endif std::cerr << "Buffer Sizes: in: " << inl; +#ifndef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 std::cerr << " HashOut: " << hashoutl; +#endif std::cerr << " SigOut: " << sigoutl; std::cerr << std::endl; From f6d69e09d59721b52a4ca7bf6484145b3e53ad0d Mon Sep 17 00:00:00 2001 From: csoler Date: Wed, 22 Nov 2017 00:02:11 +0100 Subject: [PATCH 09/11] ensured maximum backward compatibility for crypto changes that will occur in future v0.7 --- libretroshare/src/pqi/authssl.cc | 94 ++++++++++++++++++------------ libretroshare/src/pqi/sslfns.cc | 99 +++++++++++++++++++++----------- libretroshare/src/pqi/sslfns.h | 11 ++++ retroshare.pri | 6 +- 4 files changed, 137 insertions(+), 73 deletions(-) diff --git a/libretroshare/src/pqi/authssl.cc b/libretroshare/src/pqi/authssl.cc index 01f4fac68..f8f10db3c 100644 --- a/libretroshare/src/pqi/authssl.cc +++ b/libretroshare/src/pqi/authssl.cc @@ -708,7 +708,6 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) #define SERIAL_RAND_BITS 64 //const EVP_MD *digest = EVP_sha1(); - ASN1_INTEGER *serial = ASN1_INTEGER_new(); EVP_PKEY *tmppkey; X509 *x509 = X509_new(); if (x509 == NULL) @@ -733,12 +732,28 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) std::cerr << "AuthSSLimpl::SignX509Req() Issuer name: " << AuthGPG::getAuthGPG()->getGPGOwnId().toStdString() << std::endl; +#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 + static const uint64_t CERTIFICATE_SERIAL_NUMBER = RS_CERTIFICATE_VERSION_NUMBER_07_0001 ; +#else +#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_001 + static const uint64_t CERTIFICATE_SERIAL_NUMBER = RS_CERTIFICATE_VERSION_NUMBER_06_0001 ; +#else + static const uint64_t CERTIFICATE_SERIAL_NUMBER = RS_CERTIFICATE_VERSION_NUMBER_06_0000 ; +#endif +#endif + BIGNUM *btmp = BN_new(); + BN_set_word(btmp,CERTIFICATE_SERIAL_NUMBER) ; + +#ifdef OLD_CODE if (!BN_pseudo_rand(btmp, SERIAL_RAND_BITS, 0, 0)) { std::cerr << "AuthSSLimpl::SignX509Req() rand FAIL" << std::endl; return NULL; } +#endif + ASN1_INTEGER *serial = ASN1_INTEGER_new(); + if (!BN_to_ASN1_INTEGER(btmp, serial)) { std::cerr << "AuthSSLimpl::SignX509Req() asn1 FAIL" << std::endl; @@ -769,21 +784,6 @@ X509 *AuthSSLimpl::SignX509ReqWithGPG(X509_REQ *req, long /*days*/) ASN1_TIME_set(X509_get_notBefore(x509), 0); ASN1_TIME_set(X509_get_notAfter(x509), 0); - // OLD code, sets validity time of cert to be between now and some days in the future - /* - if (!X509_gmtime_adj(X509_get_notBefore(x509),0)) - { - std::cerr << "AuthSSLimpl::SignX509Req() notbefore FAIL" << std::endl; - return NULL; - } - - if (!X509_gmtime_adj(X509_get_notAfter(x509), (long)60*60*24*days)) - { - std::cerr << "AuthSSLimpl::SignX509Req() notafter FAIL" << std::endl; - return NULL; - } - */ - if (!X509_set_subject_name(x509, X509_REQ_get_subject_name(req))) { std::cerr << "AuthSSLimpl::SignX509Req() sub FAIL" << std::endl; @@ -1053,11 +1053,12 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) #endif hashoutl=EVP_MD_size(type); - unsigned char *buf_hashout=(unsigned char *)OPENSSL_malloc((unsigned int)hashoutl); + unsigned char *buf_hashout=NULL ; sigoutl=2048; //hashoutl; //EVP_PKEY_size(pkey); unsigned char *buf_sigout=(unsigned char *)OPENSSL_malloc((unsigned int)sigoutl); + uint32_t certificate_version = getX509RetroshareCertificateVersion(x509) ; #ifdef AUTHSSL_DEBUG std::cerr << "Buffer Sizes: in: " << inl; std::cerr << " HashOut: " << hashoutl; @@ -1065,7 +1066,8 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) std::cerr << std::endl; #endif - if ((buf_in == NULL) || (buf_hashout == NULL) || (buf_sigout == NULL)) { + if ((buf_in == NULL) || (buf_sigout == NULL)) + { hashoutl=0; sigoutl=0; fprintf(stderr, "AuthSSLimpl::AuthX509: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_MALLOC_FAILURE)\n"); @@ -1082,23 +1084,34 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) i2d(data,&p); #endif -#ifndef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 - /* data in buf_in, ready to be hashed */ - EVP_DigestInit_ex(ctx,type, NULL); - EVP_DigestUpdate(ctx,(unsigned char *)buf_in,inl); - - if (!EVP_DigestFinal(ctx,(unsigned char *)buf_hashout, (unsigned int *)&hashoutl)) + if(certificate_version < RS_CERTIFICATE_VERSION_NUMBER_07_0001) { - hashoutl=0; - fprintf(stderr, "AuthSSLimpl::AuthX509: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_EVP_LIB)\n"); - diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_MALLOC_ERROR ; - goto err; - } + buf_hashout=(unsigned char *)OPENSSL_malloc((unsigned int)hashoutl); + + if(buf_hashout == NULL) + { + hashoutl=0; + sigoutl=0; + fprintf(stderr, "AuthSSLimpl::AuthX509: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_MALLOC_FAILURE)\n"); + diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_MALLOC_ERROR ; + goto err; + } + /* data in buf_in, ready to be hashed */ + EVP_DigestInit_ex(ctx,type, NULL); + EVP_DigestUpdate(ctx,(unsigned char *)buf_in,inl); + + if (!EVP_DigestFinal(ctx,(unsigned char *)buf_hashout, (unsigned int *)&hashoutl)) + { + hashoutl=0; + fprintf(stderr, "AuthSSLimpl::AuthX509: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_EVP_LIB)\n"); + diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_MALLOC_ERROR ; + goto err; + } #ifdef AUTHSSL_DEBUG - std::cerr << "Digest Applied: len: " << hashoutl << std::endl; -#endif + std::cerr << "Digest Applied: len: " << hashoutl << std::endl; #endif + } /* copy data into signature */ if(sigoutl < signature->length) @@ -1180,11 +1193,17 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) // passed, verify the signature itself -#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 - if (!AuthGPG::getAuthGPG()->VerifySignBin(buf_in, inl, buf_sigout, (unsigned int) sigoutl, pd.fpr)) { -#else - if (!AuthGPG::getAuthGPG()->VerifySignBin(buf_hashout, hashoutl, buf_sigout, (unsigned int) sigoutl, pd.fpr)) { -#endif + if(certificate_version < RS_CERTIFICATE_VERSION_NUMBER_07_0001) + { + if (!AuthGPG::getAuthGPG()->VerifySignBin(buf_hashout, hashoutl, buf_sigout, (unsigned int) sigoutl, pd.fpr)) + { + sigoutl = 0; + diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE ; + goto err; + } + } + else if (!AuthGPG::getAuthGPG()->VerifySignBin(buf_in, inl, buf_sigout, (unsigned int) sigoutl, pd.fpr)) + { sigoutl = 0; diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE ; goto err; @@ -1193,7 +1212,8 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) RsPeerId peerIdstr ; getX509id(x509, peerIdstr) ; - std::cerr << "Verified signature of type " << sigtypestring << " on certificate " << peerIdstr << " using PGP key with fingerprint " << pd.fpr.toStdString() << std::endl; + std::cerr << "Verified signature of type " << sigtypestring << " on certificate " << peerIdstr << " Version " << std::hex << certificate_version + << std::dec << " using PGP key with fingerprint " << pd.fpr.toStdString() << std::endl; } #ifdef AUTHSSL_DEBUG diff --git a/libretroshare/src/pqi/sslfns.cc b/libretroshare/src/pqi/sslfns.cc index d9fbf61d2..ff964c45c 100644 --- a/libretroshare/src/pqi/sslfns.cc +++ b/libretroshare/src/pqi/sslfns.cc @@ -611,47 +611,52 @@ bool getX509id(X509 *x509, RsPeerId& xid) X509_get0_signature(&signature,&algor,x509); #endif -#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_001 - // What: Computes the node id by performing a sha256 hash of the certificate's PGP signature, instead of simply picking up the last 20 bytes of it. - // - // Why: There is no real risk in forging a certificate with the same ID as the authentication is performed over the PGP signature of the certificate - // which hashes the full SSL certificate (i.e. the full serialized CERT_INFO structure). However the possibility to - // create two certificates with the same IDs is a problem, as it can be used to cause disturbance in the software. - // - // Backward compat: makes connexions impossible with non patched peers, probably because the SSL id that is computed is not the same on both side, - // and in particular unpatched peers see a cerficate with ID different (because computed with the old method) than the ID that was - // submitted when making friends. - // - // Note: the advantage of basing the ID on the signature rather than the public key is not very clear, given that the signature is based on a hash - // of the public key (and the rest of the certificate info). - // + uint32_t version_number = getX509RetroshareCertificateVersion(x509) ; - if(RsPeerId::SIZE_IN_BYTES > Sha256CheckSum::SIZE_IN_BYTES) - return false ; - - xid = RsPeerId(RsDirUtil::sha256sum(ASN1_STRING_data(const_cast(signature)),ASN1_STRING_length(signature)).toByteArray()) ; -#else - int signlen = ASN1_STRING_length(signature); - if (signlen < CERTSIGNLEN) + if(version_number >= RS_CERTIFICATE_VERSION_NUMBER_06_0001) { -#ifdef AUTHSSL_DEBUG - std::cerr << "AuthSSL::getX509id() ERROR: Short Signature"; - std::cerr << std::endl; -#endif - return false; + // What: Computes the node id by performing a sha256 hash of the certificate's PGP signature, instead of simply picking up the last 20 bytes of it. + // + // Why: There is no real risk in forging a certificate with the same ID as the authentication is performed over the PGP signature of the certificate + // which hashes the full SSL certificate (i.e. the full serialized CERT_INFO structure). However the possibility to + // create two certificates with the same IDs is a problem, as it can be used to cause disturbance in the software. + // + // Backward compat: makes connexions impossible with non patched peers, probably because the SSL id that is computed is not the same on both side, + // and in particular unpatched peers see a cerficate with ID different (because computed with the old method) than the ID that was + // submitted when making friends. + // + // Note: the advantage of basing the ID on the signature rather than the public key is not very clear, given that the signature is based on a hash + // of the public key (and the rest of the certificate info). + // + + if(RsPeerId::SIZE_IN_BYTES > Sha256CheckSum::SIZE_IN_BYTES) + return false ; + + xid = RsPeerId(RsDirUtil::sha256sum(ASN1_STRING_data(const_cast(signature)),ASN1_STRING_length(signature)).toByteArray()) ; } + else + { + int signlen = ASN1_STRING_length(signature); + if (signlen < CERTSIGNLEN) + { +#ifdef AUTHSSL_DEBUG + std::cerr << "AuthSSL::getX509id() ERROR: Short Signature"; + std::cerr << std::endl; +#endif + return false; + } - // else copy in the first CERTSIGNLEN. - unsigned char *signdata = ASN1_STRING_data(const_cast(signature)); + // else copy in the first CERTSIGNLEN. + unsigned char *signdata = ASN1_STRING_data(const_cast(signature)); - /* switched to the other end of the signature. for - * more randomness - */ + /* switched to the other end of the signature. for + * more randomness + */ #warning csoler 2017-02-19: This is cryptographically horrible. We should hash the entire signature here! - xid = RsPeerId(&signdata[signlen - CERTSIGNLEN]) ; -#endif + xid = RsPeerId(&signdata[signlen - CERTSIGNLEN]) ; + } return true; } @@ -680,6 +685,34 @@ bool CheckX509Certificate(X509 */*x509*/) return true; } +uint64_t getX509SerialNumber(X509 *cert) +{ + ASN1_INTEGER *serial = X509_get_serialNumber(cert); + + BIGNUM *btmp = ASN1_INTEGER_to_BN(serial, NULL); + + uint64_t res = BN_get_word(btmp) ; + BN_free(btmp); + + return res ; +} + +uint32_t getX509RetroshareCertificateVersion(X509 *cert) +{ + // Because the serial number was totally random before being used to identity the handshake protocol, we check if we see known version strings. If not, + // we assume v0.6-0000 + // + // We compare the uint32_t into a uint64_t on purpose,to make sure that the highest bits are 0 and not random. + + switch(getX509SerialNumber(cert)) + { + case uint64_t(RS_CERTIFICATE_VERSION_NUMBER_06_0000): return RS_CERTIFICATE_VERSION_NUMBER_06_0000 ; + case uint64_t(RS_CERTIFICATE_VERSION_NUMBER_06_0001): return RS_CERTIFICATE_VERSION_NUMBER_06_0001 ; + case uint64_t(RS_CERTIFICATE_VERSION_NUMBER_07_0001): return RS_CERTIFICATE_VERSION_NUMBER_07_0001 ; + default: + return RS_CERTIFICATE_VERSION_NUMBER_06_0000; + } +} // Not dependent on sslroot. load, and detroys the X509 memory. int LoadCheckX509(const char *cert_file, RsPgpId& issuerName, std::string &location, RsPeerId &userId) diff --git a/libretroshare/src/pqi/sslfns.h b/libretroshare/src/pqi/sslfns.h index 3821c4872..4682810b5 100644 --- a/libretroshare/src/pqi/sslfns.h +++ b/libretroshare/src/pqi/sslfns.h @@ -61,7 +61,15 @@ int EVP_CIPHER_CTX_rand_key(EVP_CIPHER_CTX *ctx, unsigned char *key); #endif +// Certificates serial number is used to store the protocol version for the handshake. (*) means current version. +// +// 06_0000: < Nov.2017. +// * 06_0001: > Nov 2017. SSL id is computed by hashing the entire signature of the cert instead of simply picking up the last bytes. +// 07_0001: Signatures are performed using SHA256+RSA instead of SHA1+RSA +static const uint32_t RS_CERTIFICATE_VERSION_NUMBER_06_0000 = 0x00060000 ; // means version RS-0.6, certificate version 0. Default version before patch. +static const uint32_t RS_CERTIFICATE_VERSION_NUMBER_06_0001 = 0x00060001 ; // means version RS-0.6, certificate version 1. +static const uint32_t RS_CERTIFICATE_VERSION_NUMBER_07_0001 = 0x00070001 ; // means version RS-0.7, certificate version 1. X509_REQ *GenerateX509Req( std::string pkey_file, std::string passwd, @@ -122,6 +130,9 @@ std::string getX509OrgString(X509_NAME *name); std::string getX509CountryString(X509_NAME *name); std::string getX509Info(X509 *cert); +uint64_t getX509SerialNumber(X509 *cert); +uint32_t getX509RetroshareCertificateVersion(X509 *cert) ; + /********** SSL ERROR STUFF ******************************************/ int printSSLError(SSL *ssl, int retval, int err, unsigned long err2, std::string &out); diff --git a/retroshare.pri b/retroshare.pri index 3b2ef6ec6..1a11c526f 100644 --- a/retroshare.pri +++ b/retroshare.pri @@ -273,7 +273,7 @@ rs_chatserver { # which hashes the full SSL certificate (i.e. the full serialized CERT_INFO structure). However the possibility to # create two certificates with the same IDs is a problem, as it can be used to cause disturbance in the software. # -# Backward compat: makes connexions impossible with non patched peers, probably because the SSL id that is computed is not the same on both side, +# Backward compat: connexions impossible with non patched peers older than Nov 2017, probably because the SSL id that is computed is not the same on both side, # and in particular unpatched peers see a cerficate with ID different (because computed with the old method) than the ID that was # submitted when making friends. # @@ -286,7 +286,7 @@ rs_chatserver { # # Why: Sha1 is likely to be prone to primary collisions anytime soon, so it is urgent to turn to a more secure solution. # -# Backward compat: unpatched peers are able to verify signatures since openpgp-sdk already handle it. +# Backward compat: unpatched peers after Nov 2017 are able to verify signatures since openpgp-sdk already handle it. # # V07_NON_BACKWARD_COMPATIBLE_CHANGE_003: # @@ -294,7 +294,7 @@ rs_chatserver { # # Why: hasing twice is not per se a security issue, but it makes it harder to change the settings for hashing. # -# Backward compat: patched peers cannot connect to non patched peers. +# Backward compat: patched peers cannot connect to non patched peers older than Nov 2017. ########################################################################################################################################################### rs_v07_changes { From 669f2ba7ba2b0a017eb0e48d19a74bca85d18aea Mon Sep 17 00:00:00 2001 From: csoler Date: Wed, 22 Nov 2017 22:56:40 +0100 Subject: [PATCH 10/11] fixed a few bugs in signature verification code accross versions --- libretroshare/src/pqi/authssl.cc | 189 ++++++++++++++----------------- retroshare.pri | 1 + 2 files changed, 89 insertions(+), 101 deletions(-) diff --git a/libretroshare/src/pqi/authssl.cc b/libretroshare/src/pqi/authssl.cc index f8f10db3c..bb5d49590 100644 --- a/libretroshare/src/pqi/authssl.cc +++ b/libretroshare/src/pqi/authssl.cc @@ -1031,17 +1031,15 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) X509_get0_signature(&signature,&algor2,x509); #endif - -#ifdef V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 - const EVP_MD *type = EVP_sha256(); -#else - const EVP_MD *type = EVP_sha1(); -#endif + uint32_t certificate_version = getX509RetroshareCertificateVersion(x509) ; EVP_MD_CTX *ctx = EVP_MD_CTX_create(); - int inl=0,hashoutl=0; + int inl=0; int sigoutl=0; + const unsigned char *signed_data = NULL ; + uint32_t signed_data_length =0; + /* input buffer */ #if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER) inl=i2d(data,NULL); @@ -1052,13 +1050,9 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) inl=i2d_re_X509_tbs(x509,&buf_in) ; // this does the i2d over x509->cert_info #endif - hashoutl=EVP_MD_size(type); - unsigned char *buf_hashout=NULL ; - sigoutl=2048; //hashoutl; //EVP_PKEY_size(pkey); unsigned char *buf_sigout=(unsigned char *)OPENSSL_malloc((unsigned int)sigoutl); - uint32_t certificate_version = getX509RetroshareCertificateVersion(x509) ; #ifdef AUTHSSL_DEBUG std::cerr << "Buffer Sizes: in: " << inl; std::cerr << " HashOut: " << hashoutl; @@ -1068,7 +1062,6 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) if ((buf_in == NULL) || (buf_sigout == NULL)) { - hashoutl=0; sigoutl=0; fprintf(stderr, "AuthSSLimpl::AuthX509: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_MALLOC_FAILURE)\n"); diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_MALLOC_ERROR ; @@ -1083,137 +1076,134 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) p=buf_in; i2d(data,&p); #endif + { // this is to avoid cross-initialization jumps to err. - if(certificate_version < RS_CERTIFICATE_VERSION_NUMBER_07_0001) - { - buf_hashout=(unsigned char *)OPENSSL_malloc((unsigned int)hashoutl); + const Sha1CheckSum sha1 = RsDirUtil::sha1sum(buf_in,inl) ; // olds the memory until destruction - if(buf_hashout == NULL) + if(certificate_version < RS_CERTIFICATE_VERSION_NUMBER_07_0001) + { + // const EVP_MD *type = EVP_sha1(); + // + // int hashoutl=EVP_MD_size(type); + // unsigned char *buf_hashout = (unsigned char *)OPENSSL_malloc((unsigned int)hashoutl); + // + // if(buf_hashout == NULL) + // { + // hashoutl=0; + // sigoutl=0; + // fprintf(stderr, "AuthSSLimpl::AuthX509: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_MALLOC_FAILURE)\n"); + // diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_MALLOC_ERROR ; + // goto err; + // } + // /* data in buf_in, ready to be hashed */ + // EVP_DigestInit_ex(ctx,type, NULL); + // EVP_DigestUpdate(ctx,(unsigned char *)buf_in,inl); + // + // if (!EVP_DigestFinal(ctx,(unsigned char *)buf_hashout, (unsigned int *)&hashoutl)) + // { + // hashoutl=0; + // fprintf(stderr, "AuthSSLimpl::AuthX509: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_EVP_LIB)\n"); + // diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_MALLOC_ERROR ; + // goto err; + // } + + signed_data = sha1.toByteArray() ; + signed_data_length = sha1.SIZE_IN_BYTES; + } + else + { + signed_data = buf_in ; + signed_data_length = inl ; + } + + /* copy data into signature */ + if(sigoutl < signature->length) { - hashoutl=0; - sigoutl=0; - fprintf(stderr, "AuthSSLimpl::AuthX509: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_MALLOC_FAILURE)\n"); diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_MALLOC_ERROR ; goto err; } - /* data in buf_in, ready to be hashed */ - EVP_DigestInit_ex(ctx,type, NULL); - EVP_DigestUpdate(ctx,(unsigned char *)buf_in,inl); + sigoutl = signature->length; + memmove(buf_sigout, signature->data, sigoutl); - if (!EVP_DigestFinal(ctx,(unsigned char *)buf_hashout, (unsigned int *)&hashoutl)) + /* NOW check sign via GPG Functions */ + //get the fingerprint of the key that is supposed to sign +#ifdef AUTHSSL_DEBUG + std::cerr << "AuthSSLimpl::AuthX509() verifying the gpg sig with keyprint : " << pd.fpr << std::endl; + std::cerr << "Sigoutl = " << sigoutl << std::endl ; + std::cerr << "pd.fpr = " << pd.fpr << std::endl ; +#endif + + // Take a early look at signature parameters. In particular we dont accept signatures with unsecure hash algorithms. + + PGPSignatureInfo signature_info ; + PGPKeyManagement::parseSignature(buf_sigout,sigoutl,signature_info) ; + + if(signature_info.signature_version != PGP_PACKET_TAG_SIGNATURE_VERSION_V4) { - hashoutl=0; - fprintf(stderr, "AuthSSLimpl::AuthX509: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_EVP_LIB)\n"); - diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_MALLOC_ERROR ; - goto err; + diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE_VERSION ; + return false ; } -#ifdef AUTHSSL_DEBUG - std::cerr << "Digest Applied: len: " << hashoutl << std::endl; -#endif - } + std::string sigtypestring ; - /* copy data into signature */ - if(sigoutl < signature->length) - { - diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_MALLOC_ERROR ; - goto err; - } - sigoutl = signature->length; - memmove(buf_sigout, signature->data, sigoutl); - - /* NOW check sign via GPG Functions */ - //get the fingerprint of the key that is supposed to sign -#ifdef AUTHSSL_DEBUG - std::cerr << "AuthSSLimpl::AuthX509() verifying the gpg sig with keyprint : " << pd.fpr << std::endl; - std::cerr << "Sigoutl = " << sigoutl << std::endl ; - std::cerr << "pd.fpr = " << pd.fpr << std::endl ; -#ifndef V07_NON_BACKWARD_COMPATIBLE_CHANGE_003 - std::cerr << "hashoutl = " << hashoutl << std::endl ; -#endif -#endif - - // Take a early look at signature parameters. In particular we dont accept signatures with unsecure hash algorithms. - { - PGPSignatureInfo signature_info ; - PGPKeyManagement::parseSignature(buf_sigout,sigoutl,signature_info) ; - - if(signature_info.signature_version != PGP_PACKET_TAG_SIGNATURE_VERSION_V4) - { - diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE_VERSION ; - return false ; - } - - std::string sigtypestring ; - - switch(signature_info.signature_type) - { + switch(signature_info.signature_type) + { case PGP_PACKET_TAG_SIGNATURE_TYPE_BINARY_DOCUMENT : - break ; + break ; case PGP_PACKET_TAG_SIGNATURE_TYPE_STANDALONE_SIG : case PGP_PACKET_TAG_SIGNATURE_TYPE_CANONICAL_TEXT : case PGP_PACKET_TAG_SIGNATURE_TYPE_UNKNOWN : default: - diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE_TYPE ; - return false ; - } + diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE_TYPE ; + return false ; + } - switch(signature_info.public_key_algorithm) - { + switch(signature_info.public_key_algorithm) + { case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_RSA_ES : case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_RSA_S : sigtypestring = "RSA" ; - break ; + break ; case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_DSA : sigtypestring = "DSA" ; - break ; + break ; case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_RSA_E : case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_UNKNOWN: - default: - diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_HASH_ALGORITHM_NOT_ACCEPTED ; - return false ; - } + default: + diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_HASH_ALGORITHM_NOT_ACCEPTED ; + return false ; + } - switch(signature_info.hash_algorithm) - { + switch(signature_info.hash_algorithm) + { case PGP_PACKET_TAG_HASH_ALGORITHM_SHA1 : sigtypestring += "+SHA1" ; break; case PGP_PACKET_TAG_HASH_ALGORITHM_SHA256: sigtypestring += "+SHA256" ; break; case PGP_PACKET_TAG_HASH_ALGORITHM_SHA512: sigtypestring += "+SHA512" ; break; - // We dont accept signatures with unknown or week hash algorithms. + // We dont accept signatures with unknown or week hash algorithms. case PGP_PACKET_TAG_HASH_ALGORITHM_MD5: case PGP_PACKET_TAG_HASH_ALGORITHM_UNKNOWN: default: diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_HASH_ALGORITHM_NOT_ACCEPTED ; return false ; - } + } + // passed, verify the signature itself - // passed, verify the signature itself - - if(certificate_version < RS_CERTIFICATE_VERSION_NUMBER_07_0001) - { - if (!AuthGPG::getAuthGPG()->VerifySignBin(buf_hashout, hashoutl, buf_sigout, (unsigned int) sigoutl, pd.fpr)) + if (!AuthGPG::getAuthGPG()->VerifySignBin(signed_data, signed_data_length, buf_sigout, (unsigned int) sigoutl, pd.fpr)) { sigoutl = 0; diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE ; goto err; } - } - else if (!AuthGPG::getAuthGPG()->VerifySignBin(buf_in, inl, buf_sigout, (unsigned int) sigoutl, pd.fpr)) - { - sigoutl = 0; - diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE ; - goto err; - } - RsPeerId peerIdstr ; - getX509id(x509, peerIdstr) ; + RsPeerId peerIdstr ; + getX509id(x509, peerIdstr) ; - std::cerr << "Verified signature of type " << sigtypestring << " on certificate " << peerIdstr << " Version " << std::hex << certificate_version - << std::dec << " using PGP key with fingerprint " << pd.fpr.toStdString() << std::endl; + std::cerr << "Verified signature of type " << sigtypestring << " on certificate " << peerIdstr << " Version " << std::hex << certificate_version + << std::dec << " using PGP key with fingerprint " << pd.fpr.toStdString() << std::endl; } #ifdef AUTHSSL_DEBUG @@ -1222,7 +1212,6 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) EVP_MD_CTX_destroy(ctx) ; OPENSSL_free(buf_in) ; - OPENSSL_free(buf_hashout) ; OPENSSL_free(buf_sigout) ; diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_OK ; @@ -1234,8 +1223,6 @@ err: if(buf_in != NULL) OPENSSL_free(buf_in) ; - if(buf_hashout != NULL) - OPENSSL_free(buf_hashout) ; if(buf_sigout != NULL) OPENSSL_free(buf_sigout) ; return false; diff --git a/retroshare.pri b/retroshare.pri index 1a11c526f..c4441563f 100644 --- a/retroshare.pri +++ b/retroshare.pri @@ -297,6 +297,7 @@ rs_chatserver { # Backward compat: patched peers cannot connect to non patched peers older than Nov 2017. ########################################################################################################################################################### +#CONFIG += rs_v07_changes rs_v07_changes { DEFINES += V07_NON_BACKWARD_COMPATIBLE_CHANGE_001 DEFINES += V07_NON_BACKWARD_COMPATIBLE_CHANGE_002 From 1faa274e07f63df0f5a9fa550a7c9db10191e9f9 Mon Sep 17 00:00:00 2001 From: csoler Date: Wed, 22 Nov 2017 23:46:57 +0100 Subject: [PATCH 11/11] simplified memory management in certificate signature verification code --- libretroshare/src/pqi/authssl.cc | 79 ++++++++------------------------ 1 file changed, 19 insertions(+), 60 deletions(-) diff --git a/libretroshare/src/pqi/authssl.cc b/libretroshare/src/pqi/authssl.cc index bb5d49590..9956459d3 100644 --- a/libretroshare/src/pqi/authssl.cc +++ b/libretroshare/src/pqi/authssl.cc @@ -1035,7 +1035,6 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) EVP_MD_CTX *ctx = EVP_MD_CTX_create(); int inl=0; - int sigoutl=0; const unsigned char *signed_data = NULL ; uint32_t signed_data_length =0; @@ -1050,22 +1049,12 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) inl=i2d_re_X509_tbs(x509,&buf_in) ; // this does the i2d over x509->cert_info #endif - sigoutl=2048; //hashoutl; //EVP_PKEY_size(pkey); - unsigned char *buf_sigout=(unsigned char *)OPENSSL_malloc((unsigned int)sigoutl); - -#ifdef AUTHSSL_DEBUG - std::cerr << "Buffer Sizes: in: " << inl; - std::cerr << " HashOut: " << hashoutl; - std::cerr << " SigOut: " << sigoutl; - std::cerr << std::endl; -#endif - - if ((buf_in == NULL) || (buf_sigout == NULL)) + if(buf_in == NULL) { - sigoutl=0; fprintf(stderr, "AuthSSLimpl::AuthX509: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_MALLOC_FAILURE)\n"); diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_MALLOC_ERROR ; - goto err; + + return false ; } #ifdef AUTHSSL_DEBUG @@ -1076,36 +1065,14 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) p=buf_in; i2d(data,&p); #endif - { // this is to avoid cross-initialization jumps to err. + + { // this scope is to avoid cross-initialization jumps to err. const Sha1CheckSum sha1 = RsDirUtil::sha1sum(buf_in,inl) ; // olds the memory until destruction if(certificate_version < RS_CERTIFICATE_VERSION_NUMBER_07_0001) { - // const EVP_MD *type = EVP_sha1(); - // - // int hashoutl=EVP_MD_size(type); - // unsigned char *buf_hashout = (unsigned char *)OPENSSL_malloc((unsigned int)hashoutl); - // - // if(buf_hashout == NULL) - // { - // hashoutl=0; - // sigoutl=0; - // fprintf(stderr, "AuthSSLimpl::AuthX509: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_MALLOC_FAILURE)\n"); - // diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_MALLOC_ERROR ; - // goto err; - // } - // /* data in buf_in, ready to be hashed */ - // EVP_DigestInit_ex(ctx,type, NULL); - // EVP_DigestUpdate(ctx,(unsigned char *)buf_in,inl); - // - // if (!EVP_DigestFinal(ctx,(unsigned char *)buf_hashout, (unsigned int *)&hashoutl)) - // { - // hashoutl=0; - // fprintf(stderr, "AuthSSLimpl::AuthX509: ASN1err(ASN1_F_ASN1_SIGN,ERR_R_EVP_LIB)\n"); - // diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_MALLOC_ERROR ; - // goto err; - // } + // If the certificate belongs to 0.6 version, we hash it here, and then re-hash the hash it in the PGP signature. signed_data = sha1.toByteArray() ; signed_data_length = sha1.SIZE_IN_BYTES; @@ -1116,15 +1083,6 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) signed_data_length = inl ; } - /* copy data into signature */ - if(sigoutl < signature->length) - { - diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_MALLOC_ERROR ; - goto err; - } - sigoutl = signature->length; - memmove(buf_sigout, signature->data, sigoutl); - /* NOW check sign via GPG Functions */ //get the fingerprint of the key that is supposed to sign #ifdef AUTHSSL_DEBUG @@ -1136,12 +1094,12 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) // Take a early look at signature parameters. In particular we dont accept signatures with unsecure hash algorithms. PGPSignatureInfo signature_info ; - PGPKeyManagement::parseSignature(buf_sigout,sigoutl,signature_info) ; + PGPKeyManagement::parseSignature(signature->data,signature->length,signature_info) ; if(signature_info.signature_version != PGP_PACKET_TAG_SIGNATURE_VERSION_V4) { diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE_VERSION ; - return false ; + goto err ; } std::string sigtypestring ; @@ -1156,7 +1114,7 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) case PGP_PACKET_TAG_SIGNATURE_TYPE_UNKNOWN : default: diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE_TYPE ; - return false ; + goto err ; } switch(signature_info.public_key_algorithm) @@ -1172,7 +1130,7 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) case PGP_PACKET_TAG_PUBLIC_KEY_ALGORITHM_UNKNOWN: default: diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_HASH_ALGORITHM_NOT_ACCEPTED ; - return false ; + goto err ; } switch(signature_info.hash_algorithm) @@ -1187,14 +1145,13 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) case PGP_PACKET_TAG_HASH_ALGORITHM_UNKNOWN: default: diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_HASH_ALGORITHM_NOT_ACCEPTED ; - return false ; + goto err ; } // passed, verify the signature itself - if (!AuthGPG::getAuthGPG()->VerifySignBin(signed_data, signed_data_length, buf_sigout, (unsigned int) sigoutl, pd.fpr)) + if (!AuthGPG::getAuthGPG()->VerifySignBin(signed_data, signed_data_length, signature->data, signature->length, pd.fpr)) { - sigoutl = 0; diagnostic = RS_SSL_HANDSHAKE_DIAGNOSTIC_WRONG_SIGNATURE ; goto err; } @@ -1202,8 +1159,12 @@ bool AuthSSLimpl::AuthX509WithGPG(X509 *x509,uint32_t& diagnostic) RsPeerId peerIdstr ; getX509id(x509, peerIdstr) ; - std::cerr << "Verified signature of type " << sigtypestring << " on certificate " << peerIdstr << " Version " << std::hex << certificate_version - << std::dec << " using PGP key with fingerprint " << pd.fpr.toStdString() << std::endl; + std::string fpr = pd.fpr.toStdString(); + std::cerr << "Verified " << sigtypestring << " signature of certificate " << peerIdstr << ", Version " << std::hex << certificate_version + << std::dec << " using PGP key " ; + for(uint32_t i=0;i