Work around yaSSL bug.

yaSSL has a couple of bugs in their DH client implementation. This
change works around the worst of the two.

Firstly, they expect the the DH public value to be the same length as
the prime. This change pads the public value as needed to ensure this.

Secondly, although they handle the first byte of the shared key being
zero, they don't handle the case of the second, third, etc bytes being
zero. So whenever that happens the handshake fails. I don't think that
there's anything that we can do about that one.

Change-Id: I789c9e5739f19449473305d59fe5c3fb9b4a6167
Reviewed-on: https://boringssl-review.googlesource.com/6578
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index cd5d2a1..b5ec1b5 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -1221,6 +1221,9 @@
   int n;
   CERT *cert;
   BIGNUM *r[4];
+  /* r_pad_bytes[i] contains the number of zero padding bytes that need to
+   * precede |r[i]| when serialising it. */
+  unsigned r_pad_bytes[4] = {0};
   int nr[4];
   BUF_MEM *buf;
   EVP_MD_CTX md_ctx;
@@ -1292,6 +1295,10 @@
       r[0] = dh->p;
       r[1] = dh->g;
       r[2] = dh->pub_key;
+      /* Due to a bug in yaSSL, the public key must be zero padded to the size
+       * of the prime. */
+      assert(BN_num_bytes(dh->pub_key) <= BN_num_bytes(dh->p));
+      r_pad_bytes[2] = BN_num_bytes(dh->p) - BN_num_bytes(dh->pub_key);
     } else if (alg_k & SSL_kECDHE) {
       /* Determine the curve to use. */
       int nid = NID_undef;
@@ -1378,7 +1385,7 @@
     }
 
     for (i = 0; i < 4 && r[i] != NULL; i++) {
-      nr[i] = BN_num_bytes(r[i]);
+      nr[i] = BN_num_bytes(r[i]) + r_pad_bytes[i];
       n += 2 + nr[i];
     }
 
@@ -1390,7 +1397,10 @@
 
     for (i = 0; i < 4 && r[i] != NULL; i++) {
       s2n(nr[i], p);
-      BN_bn2bin(r[i], p);
+      if (!BN_bn2bin_padded(p, nr[i], r[i])) {
+        OPENSSL_PUT_ERROR(SSL, ERR_LIB_BN);
+        goto err;
+      }
       p += nr[i];
     }
 
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index 1321b2a..67b74b4 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -727,6 +727,24 @@
   }
 
   ScopedDH dh(DH_get_2048_256(NULL));
+
+  if (config->use_sparse_dh_prime) {
+    // This prime number is 2^1024 + 643 – a value just above a power of two.
+    // Because of its form, values modulo it are essentially certain to be one
+    // byte shorter. This is used to test padding of these values.
+    if (BN_hex2bn(
+            &dh->p,
+            "1000000000000000000000000000000000000000000000000000000000000000"
+            "0000000000000000000000000000000000000000000000000000000000000000"
+            "0000000000000000000000000000000000000000000000000000000000000000"
+            "0000000000000000000000000000000000000000000000000000000000000028"
+            "3") == 0 ||
+        !BN_set_word(dh->g, 2)) {
+      return nullptr;
+    }
+    dh->priv_length = 0;
+  }
+
   if (!dh || !SSL_CTX_set_tmp_dh(ssl_ctx.get(), dh.get())) {
     return nullptr;
   }
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 078c227..fd6ca71 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -787,6 +787,11 @@
 	// HelloRequest handshake message to be sent before each application
 	// data record. This only makes sense for a server.
 	SendHelloRequestBeforeEveryAppDataRecord bool
+
+	// RequireDHPublicValueLen causes a fatal error if the length (in
+	// bytes) of the server's Diffie-Hellman public value is not equal to
+	// this.
+	RequireDHPublicValueLen int
 }
 
 func (c *Config) serverInit() {
diff --git a/ssl/test/runner/key_agreement.go b/ssl/test/runner/key_agreement.go
index 3a9b899..4a07ed7 100644
--- a/ssl/test/runner/key_agreement.go
+++ b/ssl/test/runner/key_agreement.go
@@ -15,6 +15,7 @@
 	"crypto/x509"
 	"encoding/asn1"
 	"errors"
+	"fmt"
 	"io"
 	"math/big"
 )
@@ -652,6 +653,10 @@
 		return errServerKeyExchange
 	}
 
+	if l := config.Bugs.RequireDHPublicValueLen; l != 0 && l != yLen {
+		return fmt.Errorf("RequireDHPublicValueLen set to %d, but server's public value was %d bytes on the wire and %d bytes if minimal", l, yLen, (ka.yTheirs.BitLen()+7)/8)
+	}
+
 	sig := k
 	serverDHParams := skx.key[:len(skx.key)-len(sig)]
 
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index a1801c8..befde86 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2185,6 +2185,20 @@
 		expectedError: ":DH_P_TOO_LONG:",
 	})
 
+	// This test ensures that Diffie-Hellman public values are padded with
+	// zeros so that they're the same length as the prime. This is to avoid
+	// hitting a bug in yaSSL.
+	testCases = append(testCases, testCase{
+		testType: serverTest,
+		name:     "DHPublicValuePadded",
+		config: Config{
+			CipherSuites: []uint16{TLS_DHE_RSA_WITH_AES_128_GCM_SHA256},
+			Bugs: ProtocolBugs{
+				RequireDHPublicValueLen: (1025 + 7) / 8,
+			},
+		},
+		flags: []string{"-use-sparse-dh-prime"},
+	})
 
 	// versionSpecificCiphersTest specifies a test for the TLS 1.0 and TLS
 	// 1.1 specific cipher suite settings. A server is setup with the given
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc
index 23b0879..ba44b4d 100644
--- a/ssl/test/test_config.cc
+++ b/ssl/test/test_config.cc
@@ -99,6 +99,7 @@
   { "-renegotiate-ignore", &TestConfig::renegotiate_ignore },
   { "-disable-npn", &TestConfig::disable_npn },
   { "-p384-only", &TestConfig::p384_only },
+  { "-use-sparse-dh-prime", &TestConfig::use_sparse_dh_prime },
 };
 
 const Flag<std::string> kStringFlags[] = {
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h
index 733e0a1..2fabcfc 100644
--- a/ssl/test/test_config.h
+++ b/ssl/test/test_config.h
@@ -102,6 +102,7 @@
   bool disable_npn = false;
   int expect_server_key_exchange_hash = 0;
   bool p384_only = false;
+  bool use_sparse_dh_prime = false;
 };
 
 bool ParseConfig(int argc, char **argv, TestConfig *out_config);