diff --git a/ChangeLog b/ChangeLog index c6721f72..0e7e482e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2008-02-24 Tatsuhiro Tsujikawa + + Fixed the bug that DH key exchange sometimes fails due to bad handling + of the number of bytes required for storing public key and shared + secret. + * src/LibgcryptDHKeyExchange.h + * src/LibsslDHKeyExchange.h: Also added function name to handleError. + * src/MSEHandshake.cc + * test/DHKeyExchangeTest.cc + 2008-02-24 Tatsuhiro Tsujikawa Removed since they are not used. diff --git a/src/LibgcryptDHKeyExchange.h b/src/LibgcryptDHKeyExchange.h index f1eb9cb1..d15375b0 100644 --- a/src/LibgcryptDHKeyExchange.h +++ b/src/LibgcryptDHKeyExchange.h @@ -43,6 +43,7 @@ namespace aria2 { class DHKeyExchange { private: + size_t _keyLength; gcry_mpi_t _prime; @@ -57,12 +58,13 @@ private: throw new DlAbortEx("Exception in libgcrypt routine(DHKeyExchange class): %s", gcry_strerror(errno)); } - public: - DHKeyExchange():_prime(0), - _generator(0), - _privateKey(0), - _publicKey(0) {} + DHKeyExchange(): + _keyLength(0), + _prime(0), + _generator(0), + _privateKey(0), + _publicKey(0) {} ~DHKeyExchange() { @@ -72,7 +74,8 @@ public: gcry_mpi_release(_publicKey); } - void init(const unsigned char* prime, const unsigned char* generator, + void init(const unsigned char* prime, size_t primeBits, + const unsigned char* generator, size_t privateKeyBits) { gcry_mpi_release(_prime); @@ -94,6 +97,7 @@ public: } _privateKey = gcry_mpi_new(0); gcry_mpi_randomize(_privateKey, privateKeyBits, GCRY_STRONG_RANDOM); + _keyLength = (primeBits+7)/8; } void generatePublicKey() @@ -103,19 +107,18 @@ public: gcry_mpi_powm(_publicKey, _generator, _privateKey, _prime); } - size_t publicKeyLength() const - { - return (gcry_mpi_get_nbits(_publicKey)+7)/8; - } - size_t getPublicKey(unsigned char* out, size_t outLength) const { - if(outLength < publicKeyLength()) { + if(outLength < _keyLength) { throw new DlAbortEx("Insufficient buffer for public key. expect:%u, actual:%u", - publicKeyLength(), outLength); + _keyLength, outLength); } + memset(out, 0, outLength); + size_t publicKeyBytes = (gcry_mpi_get_nbits(_publicKey)+7)/8; + size_t offset = _keyLength-publicKeyBytes; size_t nwritten; - gcry_error_t r = gcry_mpi_print(GCRYMPI_FMT_USG, out, outLength, &nwritten, _publicKey); + gcry_error_t r = gcry_mpi_print(GCRYMPI_FMT_USG, out+offset, + outLength-offset, &nwritten, _publicKey); if(r) { handleError(r); } @@ -131,9 +134,9 @@ public: const unsigned char* peerPublicKeyData, size_t peerPublicKeyLength) const { - if(outLength < publicKeyLength()) { + if(outLength < _keyLength) { throw new DlAbortEx("Insufficient buffer for secret. expect:%u, actual:%u", - publicKeyLength(), outLength); + _keyLength, outLength); } gcry_mpi_t peerPublicKey; { @@ -147,9 +150,13 @@ public: gcry_mpi_powm(secret, peerPublicKey, _privateKey, _prime); gcry_mpi_release(peerPublicKey); + memset(out, 0, outLength); + size_t secretBytes = (gcry_mpi_get_nbits(secret)+7/8); + size_t offset = _keyLength-secretBytes; size_t nwritten; { - gcry_error_t r = gcry_mpi_print(GCRYMPI_FMT_USG, out, outLength, &nwritten, secret); + gcry_error_t r = gcry_mpi_print(GCRYMPI_FMT_USG, out+offset, + outLength-offset, &nwritten, secret); gcry_mpi_release(secret); if(r) { handleError(r); diff --git a/src/LibsslDHKeyExchange.h b/src/LibsslDHKeyExchange.h index 1f18849d..76f20ec3 100644 --- a/src/LibsslDHKeyExchange.h +++ b/src/LibsslDHKeyExchange.h @@ -48,6 +48,8 @@ private: BN_CTX* _bnCtx; + size_t _keyLength; + BIGNUM* _prime; BIGNUM* _generator; @@ -56,14 +58,14 @@ private: BIGNUM* _publicKey; - void handleError() const + void handleError(const std::string& funName) const { - throw new DlAbortEx("Exception in libssl routine(DHKeyExchange class): %s", - ERR_error_string(ERR_get_error(), 0)); + throw new DlAbortEx("Exception in libssl routine %s(DHKeyExchange class): %s", + funName.c_str(), ERR_error_string(ERR_get_error(), 0)); } - public: DHKeyExchange():_bnCtx(0), + _keyLength(0), _prime(0), _generator(0), _privateKey(0), @@ -78,13 +80,14 @@ public: BN_free(_publicKey); } - void init(const unsigned char* prime, const unsigned char* generator, + void init(const unsigned char* prime, size_t primeBits, + const unsigned char* generator, size_t privateKeyBits) { BN_CTX_free(_bnCtx); _bnCtx = BN_CTX_new(); if(!_bnCtx) { - handleError(); + handleError("BN_CTX_new in init"); } BN_free(_prime); @@ -95,15 +98,16 @@ public: _privateKey = 0; if(BN_hex2bn(&_prime, reinterpret_cast(prime)) == 0) { - handleError(); + handleError("BN_hex2bn in init"); } if(BN_hex2bn(&_generator, reinterpret_cast(generator)) == 0) { - handleError(); + handleError("BN_hex2bn in init"); } _privateKey = BN_new(); if(BN_rand(_privateKey, privateKeyBits, -1, false) == 0) { - handleError(); + handleError("BN_new in init"); } + _keyLength = (primeBits+7)/8; } void generatePublicKey() @@ -113,21 +117,18 @@ public: BN_mod_exp(_publicKey, _generator, _privateKey, _prime, _bnCtx); } - size_t publicKeyLength() const - { - return BN_num_bytes(_publicKey); - } - size_t getPublicKey(unsigned char* out, size_t outLength) const { - size_t pubKeyLen = publicKeyLength(); - if(outLength < pubKeyLen) { + if(outLength < _keyLength) { throw new DlAbortEx("Insufficient buffer for public key. expect:%u, actual:%u", - publicKeyLength(), outLength); + _keyLength, outLength); } - size_t nwritten = BN_bn2bin(_publicKey, out); - if(nwritten != pubKeyLen) { - handleError(); + memset(out, 0, outLength); + size_t publicKeyBytes = BN_num_bytes(_publicKey); + size_t offset = _keyLength-publicKeyBytes; + size_t nwritten = BN_bn2bin(_publicKey, out+offset); + if(nwritten != publicKeyBytes) { + throw new DlAbortEx("BN_bn2bin in DHKeyExchange::getPublicKey, %u bytes written, but %u bytes expected.", nwritten, publicKeyBytes); } return nwritten; } @@ -135,7 +136,7 @@ public: void generateNonce(unsigned char* out, size_t outLength) const { if(RAND_bytes(out, outLength) != 1) { - handleError(); + handleError("RAND_bytes in generateNonce"); } } @@ -143,26 +144,28 @@ public: const unsigned char* peerPublicKeyData, size_t peerPublicKeyLength) const { - size_t pubKeyLen = publicKeyLength(); - if(outLength < pubKeyLen) { + if(outLength < _keyLength) { throw new DlAbortEx("Insufficient buffer for secret. expect:%u, actual:%u", - publicKeyLength(), outLength); + _keyLength, outLength); } BIGNUM* peerPublicKey = BN_bin2bn(peerPublicKeyData, peerPublicKeyLength, 0); if(!peerPublicKey) { - handleError(); + handleError("BN_bin2bn in computeSecret"); } BIGNUM* secret = BN_new(); BN_mod_exp(secret, peerPublicKey, _privateKey, _prime, _bnCtx); BN_free(peerPublicKey); - size_t nwritten = BN_bn2bin(secret, out); + memset(out, 0, outLength); + size_t secretBytes = BN_num_bytes(secret); + size_t offset = _keyLength-secretBytes; + size_t nwritten = BN_bn2bin(secret, out+offset); BN_free(secret); - if(nwritten != pubKeyLen) { - handleError(); + if(nwritten != secretBytes) { + throw new DlAbortEx("BN_bn2bin in DHKeyExchange::getPublicKey, %u bytes written, but %u bytes expected.", nwritten, secretBytes); } return nwritten; } diff --git a/src/MSEHandshake.cc b/src/MSEHandshake.cc index 49facca2..1b390e95 100644 --- a/src/MSEHandshake.cc +++ b/src/MSEHandshake.cc @@ -110,7 +110,7 @@ void MSEHandshake::initEncryptionFacility(bool initiator) { delete _dh; _dh = new DHKeyExchange(); - _dh->init(PRIME, GENERATOR, 160); + _dh->init(PRIME, PRIME_BITS, GENERATOR, 160); _dh->generatePublicKey(); _logger->debug("CUID#%d - DH initialized.", _cuid); diff --git a/test/DHKeyExchangeTest.cc b/test/DHKeyExchangeTest.cc index 02479cd6..457cbd98 100644 --- a/test/DHKeyExchangeTest.cc +++ b/test/DHKeyExchangeTest.cc @@ -28,10 +28,12 @@ void DHKeyExchangeTest::testHandshake() const unsigned char* PRIME = reinterpret_cast("FFFFFFFFFFFFFFFFC90FDAA22168C234C4C6628B80DC1CD129024E088A67CC74020BBEA63B139B22514A08798E3404DDEF9519B3CD3A431B302B0A6DF25F14374FE1356D6D51C245E485B576625E7EC6F44C42E9A63A36210000000000090563"); + const size_t PRIME_BITS = 768; + const unsigned char* GENERATOR = reinterpret_cast("2"); - dhA.init(PRIME, GENERATOR, 160); - dhB.init(PRIME, GENERATOR, 160); + dhA.init(PRIME, PRIME_BITS, GENERATOR, 160); + dhB.init(PRIME, PRIME_BITS, GENERATOR, 160); dhA.generatePublicKey(); dhB.generatePublicKey(); @@ -46,7 +48,8 @@ void DHKeyExchangeTest::testHandshake() dhA.computeSecret(secretA, sizeof(secretA), publicKeyB, sizeof(publicKeyB)); dhB.computeSecret(secretB, sizeof(secretB), publicKeyA, sizeof(publicKeyA)); - CPPUNIT_ASSERT(memcmp(secretA, secretB, sizeof(secretA)) == 0); + CPPUNIT_ASSERT_EQUAL(Util::toHex(secretA, sizeof(secretA)), + Util::toHex(secretB, sizeof(secretB))); } } // namespace aria2