2008-02-24 Tatsuhiro Tsujikawa <tujikawa at rednoah dot com>

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

ChangeLog

@@ -1,3 +1,13 @@
+2008-02-24  Tatsuhiro Tsujikawa  <tujikawa at rednoah dot com>
+
+	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  <tujikawa at rednoah dot com>
 
 	Removed since they are not used.

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),
+  DHKeyExchange():
-		  _generator(0),
+    _keyLength(0),
-		  _privateKey(0),
+    _prime(0),
-		  _publicKey(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);

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",
+    throw new DlAbortEx("Exception in libssl routine %s(DHKeyExchange class): %s",
-			ERR_error_string(ERR_get_error(), 0));
+			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<const char*>(prime)) == 0) {
-      handleError();
+      handleError("BN_hex2bn in init");
     }
     if(BN_hex2bn(&_generator, reinterpret_cast<const char*>(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 < _keyLength) {
-    if(outLength < pubKeyLen) {
       throw new DlAbortEx("Insufficient buffer for public key. expect:%u, actual:%u",
-			  publicKeyLength(), outLength);
+			  _keyLength, outLength);
     }
-    size_t nwritten = BN_bn2bin(_publicKey, out);
+    memset(out, 0, outLength);
-    if(nwritten != pubKeyLen) {
+    size_t publicKeyBytes = BN_num_bytes(_publicKey);
-      handleError();
+    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 < _keyLength) {
-    if(outLength < pubKeyLen) {
       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) {
+    if(nwritten != secretBytes) {
-      handleError();
+      throw new DlAbortEx("BN_bn2bin in DHKeyExchange::getPublicKey, %u bytes written, but %u bytes expected.", nwritten, secretBytes);
     }
     return nwritten;
   }

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);
 

test/DHKeyExchangeTest.cc

@@ -28,10 +28,12 @@ void DHKeyExchangeTest::testHandshake()
 
   const unsigned char* PRIME = reinterpret_cast<const unsigned char*>("FFFFFFFFFFFFFFFFC90FDAA22168C234C4C6628B80DC1CD129024E088A67CC74020BBEA63B139B22514A08798E3404DDEF9519B3CD3A431B302B0A6DF25F14374FE1356D6D51C245E485B576625E7EC6F44C42E9A63A36210000000000090563");
 
+  const size_t PRIME_BITS = 768;
+
   const unsigned char* GENERATOR = reinterpret_cast<const unsigned char*>("2");
 
-  dhA.init(PRIME, GENERATOR, 160);
+  dhA.init(PRIME, PRIME_BITS, GENERATOR, 160);
-  dhB.init(PRIME, 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