commit 7e1f984a7c4eede37d6297cba141aef7de4509b0
author David Benjamin <davidben@google.com> Tue Sep 20 19:24:40 2016 -0400
committer David Benjamin <davidben@google.com> Wed Sep 21 20:40:10 2016 +0000
tree 0b699b02867fa1a341b9c8e0e89618f8034c8c13
parent e4706906336f8724a25b68f82967dcf82d2fb45e

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>

ssl/t1_lib.c

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index a89f5cf..32d9594 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -2196,41 +2196,66 @@
   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;
 }