Description: yaSSL was only handling the cases of zero or

vsmurthys · vsmurthys · commit 8c65e082f397 · 2016-01-04T15:31:45.000+05:30
one leading zeros for the key agreement instead of
potentially any number.
There is about 1 in 50,000 connections to fail
when using DHE cipher suites.  The second problem was the
case where a server would send a public value shorter than
the prime value, causing about 1 in 128 client connections
to fail, and also caused the yaSSL client to read off the
end of memory.
All client side DHE cipher suite users should update.
Note: The patch is received from YaSSL people
diff --git a/extra/yassl/README b/extra/yassl/README
@@ -12,6 +12,17 @@ before calling SSL_new();
 
 *** end Note ***
 
+yaSSL Release notes, version 2.3.9 (12/01/2015)
+    This release of yaSSL fixes two client side Diffie-Hellman problems.
+    yaSSL was only handling the cases of zero or one leading zeros for the key
+    agreement instead of potentially any number.  This caused about 1 in 50,000
+    connections to fail when using DHE cipher suites.  The second problem was
+    the case where a server would send a public value shorter than the prime
+    value, causing about 1 in 128 client connections to fail, and also
+    caused the yaSSL client to read off the end of memory.  All client side
+    DHE cipher suite users should update.
+    Thanks to Adam Langely (agl@imperialviolet.org) for the detailed report!
+
 yaSSL Release notes, version 2.3.8 (9/17/2015)
     This release of yaSSL fixes a high security vulnerability.  All users
     SHOULD update.  If using yaSSL for TLS on the server side with private
diff --git a/extra/yassl/include/crypto_wrapper.hpp b/extra/yassl/include/crypto_wrapper.hpp
@@ -378,6 +378,7 @@ class DiffieHellman  {
 
     uint        get_agreedKeyLength() const;
     const byte* get_agreedKey()       const;
+    uint        get_publicKeyLength() const;
     const byte* get_publicKey()       const;
     void        makeAgreement(const byte*, unsigned int);
 
diff --git a/extra/yassl/include/openssl/ssl.h b/extra/yassl/include/openssl/ssl.h
@@ -35,7 +35,7 @@
 #include "rsa.h"
 
 
-#define YASSL_VERSION "2.3.8"
+#define YASSL_VERSION "2.3.9"
 
 
 #if defined(__cplusplus)
diff --git a/extra/yassl/src/crypto_wrapper.cpp b/extra/yassl/src/crypto_wrapper.cpp
@@ -751,9 +751,10 @@ struct DiffieHellman::DHImpl {
     byte* publicKey_;
     byte* privateKey_;
     byte* agreedKey_;
+    uint  pubKeyLength_;
 
     DHImpl(TaoCrypt::RandomNumberGenerator& r) : ranPool_(r), publicKey_(0),
-                                               privateKey_(0), agreedKey_(0) {}
+                              privateKey_(0), agreedKey_(0), pubKeyLength_(0) {}
     ~DHImpl() 
     {   
         ysArrayDelete(agreedKey_); 
@@ -762,7 +763,7 @@ struct DiffieHellman::DHImpl {
     }
 
     DHImpl(const DHImpl& that) : dh_(that.dh_), ranPool_(that.ranPool_),
-                                 publicKey_(0), privateKey_(0), agreedKey_(0)
+                  publicKey_(0), privateKey_(0), agreedKey_(0), pubKeyLength_(0)
     {
         uint length = dh_.GetByteLength();
         AllocKeys(length, length, length);
@@ -810,7 +811,7 @@ DiffieHellman::DiffieHellman(const byte* p, unsigned int pSz, const byte* g,
     using TaoCrypt::Integer;
 
     pimpl_->dh_.Initialize(Integer(p, pSz).Ref(), Integer(g, gSz).Ref());
-    pimpl_->publicKey_ = NEW_YS opaque[pubSz];
+    pimpl_->publicKey_ = NEW_YS opaque[pimpl_->pubKeyLength_ = pubSz];
     memcpy(pimpl_->publicKey_, pub, pubSz);
 }
 
@@ -869,6 +870,10 @@ const byte* DiffieHellman::get_agreedKey() const
     return pimpl_->agreedKey_;
 }
 
+uint DiffieHellman::get_publicKeyLength() const
+{
+    return pimpl_->pubKeyLength_;
+}
 
 const byte* DiffieHellman::get_publicKey() const
 {
diff --git a/extra/yassl/src/yassl_imp.cpp b/extra/yassl/src/yassl_imp.cpp
@@ -109,15 +109,12 @@ void ClientDiffieHellmanPublic::build(SSL& ssl)
     uint keyLength = dhClient.get_agreedKeyLength(); // pub and agree same
 
     alloc(keyLength, true);
-    dhClient.makeAgreement(dhServer.get_publicKey(), keyLength);
+    dhClient.makeAgreement(dhServer.get_publicKey(),
+                           dhServer.get_publicKeyLength());
     c16toa(keyLength, Yc_);
     memcpy(Yc_ + KEY_OFFSET, dhClient.get_publicKey(), keyLength);
 
-    // because of encoding first byte might be zero, don't use it for preMaster
-    if (*dhClient.get_agreedKey() == 0) 
-        ssl.set_preMaster(dhClient.get_agreedKey() + 1, keyLength - 1);
-    else
-        ssl.set_preMaster(dhClient.get_agreedKey(), keyLength);
+    ssl.set_preMaster(dhClient.get_agreedKey(), keyLength);
 }
 
 
@@ -321,11 +318,7 @@ void ClientDiffieHellmanPublic::read(SSL& ssl, input_buffer& input)
     }
     dh.makeAgreement(Yc_, keyLength); 
 
-    // because of encoding, first byte might be 0, don't use for preMaster 
-    if (*dh.get_agreedKey() == 0) 
-        ssl.set_preMaster(dh.get_agreedKey() + 1, dh.get_agreedKeyLength() - 1);
-    else
-        ssl.set_preMaster(dh.get_agreedKey(), dh.get_agreedKeyLength());
+    ssl.set_preMaster(dh.get_agreedKey(), dh.get_agreedKeyLength());
     ssl.makeMasterSecret();
 }
 
diff --git a/extra/yassl/src/yassl_int.cpp b/extra/yassl/src/yassl_int.cpp
@@ -807,6 +807,19 @@ void SSL::set_random(const opaque* random, ConnectionEnd sender)
 // store client pre master secret
 void SSL::set_preMaster(const opaque* pre, uint sz)
 {
+    uint i(0);  // trim leading zeros
+    uint fullSz(sz);
+
+    while (i++ < fullSz && *pre == 0) {
+        sz--;
+        pre++;
+    }
+
+    if (sz == 0) {
+        SetError(bad_input);
+        return;
+    }
+
     secure_.use_connection().AllocPreSecret(sz);
     memcpy(secure_.use_connection().pre_master_secret_, pre, sz);
 }
@@ -924,6 +937,8 @@ void SSL::order_error()
 // Create and store the master secret see page 32, 6.1
 void SSL::makeMasterSecret()
 {
+    if (GetError()) return;
+
     if (isTLS())
         makeTLSMasterSecret();
     else {

Original file line number	Diff line number	Diff line change
`@@ -751,9 +751,10 @@ struct DiffieHellman::DHImpl {`
`751`	`751`	`byte* publicKey_;`
`752`	`752`	`byte* privateKey_;`
`753`	`753`	`byte* agreedKey_;`
	`754`	`+ uint pubKeyLength_;`
`754`	`755`
`755`	`756`	`DHImpl(TaoCrypt::RandomNumberGenerator& r) : ranPool_(r), publicKey_(0),`
`756`		`- privateKey_(0), agreedKey_(0) {}`
	`757`	`+ privateKey_(0), agreedKey_(0), pubKeyLength_(0) {}`
`757`	`758`	`~DHImpl()`
`758`	`759`	`{`
`759`	`760`	`ysArrayDelete(agreedKey_);`
`@@ -762,7 +763,7 @@ struct DiffieHellman::DHImpl {`
`762`	`763`	`}`
`763`	`764`
`764`	`765`	`DHImpl(const DHImpl& that) : dh_(that.dh_), ranPool_(that.ranPool_),`
`765`		`- publicKey_(0), privateKey_(0), agreedKey_(0)`
	`766`	`+ publicKey_(0), privateKey_(0), agreedKey_(0), pubKeyLength_(0)`
`766`	`767`	`{`
`767`	`768`	`uint length = dh_.GetByteLength();`
`768`	`769`	`AllocKeys(length, length, length);`
`@@ -810,7 +811,7 @@ DiffieHellman::DiffieHellman(const byte* p, unsigned int pSz, const byte* g,`
`810`	`811`	`using TaoCrypt::Integer;`
`811`	`812`
`812`	`813`	`pimpl_->dh_.Initialize(Integer(p, pSz).Ref(), Integer(g, gSz).Ref());`
`813`		`- pimpl_->publicKey_ = NEW_YS opaque[pubSz];`
	`814`	`+ pimpl_->publicKey_ = NEW_YS opaque[pimpl_->pubKeyLength_ = pubSz];`
`814`	`815`	`memcpy(pimpl_->publicKey_, pub, pubSz);`
`815`	`816`	`}`
`816`	`817`
`@@ -869,6 +870,10 @@ const byte* DiffieHellman::get_agreedKey() const`
`869`	`870`	`return pimpl_->agreedKey_;`
`870`	`871`	`}`
`871`	`872`
	`873`	`+uint DiffieHellman::get_publicKeyLength() const`
	`874`	`+{`
	`875`	`+ return pimpl_->pubKeyLength_;`
	`876`	`+}`
`872`	`877`
`873`	`878`	`const byte* DiffieHellman::get_publicKey() const`
`874`	`879`	`{`