Fix some bugs in TLS 1.3 server key_share code.

Found by libFuzzer and then one more mistake caught by valgrind. Add a
test for this case.

Change-Id: I92773bc1231bafe5fc069e8568d93ac0df4c8acb
Reviewed-on: https://boringssl-review.googlesource.com/11129
Reviewed-by: David Benjamin <davidben@google.com>

8 files changed, 75 insertions, 22 deletions

diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata
index bca49316..f94156f7 100644
--- a/crypto/err/ssl.errordata
+++ b/crypto/err/ssl.errordata
@@ -46,6 +46,7 @@ SSL,142,DIGEST_CHECK_FAILED
 SSL,254,DOWNGRADE_DETECTED
 SSL,143,DTLS_MESSAGE_TOO_BIG
 SSL,257,DUPLICATE_EXTENSION
+SSL,264,DUPLICATE_KEY_SHARE
 SSL,144,ECC_CERT_NOT_FOR_SIGNING
 SSL,145,EMS_STATE_INCONSISTENT
 SSL,146,ENCRYPTED_LENGTH_TOO_LONG
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 8f097fe7..1e76df39 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -4776,6 +4776,7 @@ BORINGSSL_MAKE_DELETER(SSL_SESSION, SSL_SESSION_free)
 #define SSL_R_BLOCK_CIPHER_PAD_IS_WRONG 261
 #define SSL_R_NO_CIPHERS_SPECIFIED 262
 #define SSL_R_RENEGOTIATION_EMS_MISMATCH 263
+#define SSL_R_DUPLICATE_KEY_SHARE 264
 #define SSL_R_SSLV3_ALERT_CLOSE_NOTIFY 1000
 #define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010
 #define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index a89f5cfd..32d95940 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -2196,41 +2196,66 @@ int ssl_ext_key_share_parse_clienthello(SSL *ssl, int *out_found,
   if (!tls1_get_shared_group(ssl, &group_id) ||
       !CBS_get_u16_length_prefixed(contents, &key_shares) ||
       CBS_len(contents) != 0) {
+    OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
     return 0;
   }
 
-  *out_found = 0;
+  /* Find the corresponding key share. */
+  int found = 0;
+  CBS peer_key;
   while (CBS_len(&key_shares) > 0) {
     uint16_t id;
-    CBS peer_key;
+    CBS peer_key_tmp;
     if (!CBS_get_u16(&key_shares, &id) ||
-        !CBS_get_u16_length_prefixed(&key_shares, &peer_key)) {
+        !CBS_get_u16_length_prefixed(&key_shares, &peer_key_tmp)) {
+      OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
       return 0;
     }
 
-    if (id != group_id || *out_found) {
-      continue;
-    }
+    if (id == group_id) {
+      if (found) {
+        OPENSSL_PUT_ERROR(SSL, SSL_R_DUPLICATE_KEY_SHARE);
+        *out_alert = SSL_AD_ILLEGAL_PARAMETER;
+        return 0;
+      }
 
-    SSL_ECDH_CTX group;
-    memset(&group, 0, sizeof(SSL_ECDH_CTX));
-    CBB public_key;
-    if (!CBB_init(&public_key, 0) ||
-        !SSL_ECDH_CTX_init(&group, group_id) ||
-        !SSL_ECDH_CTX_accept(&group, &public_key, out_secret, out_secret_len,
-                             out_alert, CBS_data(&peer_key),
-                             CBS_len(&peer_key)) ||
-        !CBB_finish(&public_key, &ssl->s3->hs->public_key,
-                    &ssl->s3->hs->public_key_len)) {
-      SSL_ECDH_CTX_cleanup(&group);
-      CBB_cleanup(&public_key);
-      return 0;
+      found = 1;
+      peer_key = peer_key_tmp;
+      /* Continue parsing the structure to keep peers honest. */
     }
-    SSL_ECDH_CTX_cleanup(&group);
+  }
 
-    *out_found = 1;
+  if (!found) {
+    *out_found = 0;
+    *out_secret = NULL;
+    *out_secret_len = 0;
+    return 1;
   }
 
+  /* Compute the DH secret. */
+  uint8_t *secret = NULL;
+  size_t secret_len;
+  SSL_ECDH_CTX group;
+  memset(&group, 0, sizeof(SSL_ECDH_CTX));
+  CBB public_key;
+  if (!CBB_init(&public_key, 32) ||
+      !SSL_ECDH_CTX_init(&group, group_id) ||
+      !SSL_ECDH_CTX_accept(&group, &public_key, &secret, &secret_len,
+                           out_alert, CBS_data(&peer_key),
+                           CBS_len(&peer_key)) ||
+      !CBB_finish(&public_key, &ssl->s3->hs->public_key,
+                  &ssl->s3->hs->public_key_len)) {
+    OPENSSL_free(secret);
+    SSL_ECDH_CTX_cleanup(&group);
+    CBB_cleanup(&public_key);
+    return 0;
+  }
+
+  SSL_ECDH_CTX_cleanup(&group);
+
+  *out_secret = secret;
+  *out_secret_len = secret_len;
+  *out_found = 1;
   return 1;
 }
 
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index d2f29fa4..cd6c8f93 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -1076,6 +1076,10 @@ type ProtocolBugs struct {
 	// always send a ServerKeyExchange for PSK ciphers, even if the identity
 	// hint is empty.
 	AlwaysSendPreSharedKeyIdentityHint bool
+
+	// TrailingKeyShareData, if true, causes the client key share list to
+	// include a trailing byte.
+	TrailingKeyShareData bool
 }
 
 func (c *Config) serverInit() {
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index d9d4451e..c5c4495c 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -113,6 +113,7 @@ func (c *Conn) clientHandshake() error {
 	if hello.vers >= VersionTLS13 {
 		keyShares = make(map[CurveID]ecdhCurve)
 		hello.hasKeyShares = true
+		hello.trailingKeyShareData = c.config.Bugs.TrailingKeyShareData
 		curvesToSend := c.config.defaultCurves()
 		for _, curveID := range hello.supportedCurves {
 			if !curvesToSend[curveID] {
diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go
index 8f87881e..63290fb4 100644
--- a/ssl/test/runner/handshake_messages.go
+++ b/ssl/test/runner/handshake_messages.go
@@ -142,6 +142,7 @@ type clientHelloMsg struct {
 	supportedPoints         []uint8
 	hasKeyShares            bool
 	keyShares               []keyShareEntry
+	trailingKeyShareData    bool
 	pskIdentities           [][]uint8
 	hasEarlyData            bool
 	earlyDataContext        []byte
@@ -181,6 +182,7 @@ func (m *clientHelloMsg) equal(i interface{}) bool {
 		bytes.Equal(m.supportedPoints, m1.supportedPoints) &&
 		m.hasKeyShares == m1.hasKeyShares &&
 		eqKeyShareEntryLists(m.keyShares, m1.keyShares) &&
+		m.trailingKeyShareData == m1.trailingKeyShareData &&
 		eqByteSlices(m.pskIdentities, m1.pskIdentities) &&
 		m.hasEarlyData == m1.hasEarlyData &&
 		bytes.Equal(m.earlyDataContext, m1.earlyDataContext) &&
@@ -300,6 +302,10 @@ func (m *clientHelloMsg) marshal() []byte {
 			keyExchange := keyShares.addU16LengthPrefixed()
 			keyExchange.addBytes(keyShare.keyExchange)
 		}
+
+		if m.trailingKeyShareData {
+			keyShares.addU8(0)
+		}
 	}
 	if len(m.pskIdentities) > 0 {
 		extensions.addU16(extensionPreSharedKey)
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 99d7fac5..c350ac5b 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -7978,6 +7978,8 @@ func addTLS13HandshakeTests() {
 				DuplicateKeyShares: true,
 			},
 		},
+		shouldFail:    true,
+		expectedError: ":DUPLICATE_KEY_SHARE:",
 	})
 
 	testCases = append(testCases, testCase{
@@ -8175,6 +8177,19 @@ func addTLS13HandshakeTests() {
 		shouldFail:    true,
 		expectedError: ":DECODE_ERROR:",
 	})
+
+	testCases = append(testCases, testCase{
+		testType: serverTest,
+		name:     "TLS13-TrailingKeyShareData",
+		config: Config{
+			MaxVersion: VersionTLS13,
+			Bugs: ProtocolBugs{
+				TrailingKeyShareData: true,
+			},
+		},
+		shouldFail:    true,
+		expectedError: ":DECODE_ERROR:",
+	})
 }
 
 func worker(statusChan chan statusMsg, c chan *testCase, shimPath string, wg *sync.WaitGroup) {
diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c
index 4cfd265b..53e53638 100644
--- a/ssl/tls13_server.c
+++ b/ssl/tls13_server.c
@@ -88,7 +88,7 @@ static int resolve_ecdhe_secret(SSL *ssl, int *out_need_retry,
   int found_key_share;
   uint8_t *dhe_secret;
   size_t dhe_secret_len;
-  uint8_t alert;
+  uint8_t alert = SSL_AD_DECODE_ERROR;
   if (!ssl_ext_key_share_parse_clienthello(ssl, &found_key_share, &dhe_secret,
                                            &dhe_secret_len, &alert,
                                            &key_share)) {