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>
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;
}
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index d2f29fa..cd6c8f9 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -1076,6 +1076,10 @@
// 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 d9d4451..c5c4495 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -113,6 +113,7 @@
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 8f87881..63290fb 100644
--- a/ssl/test/runner/handshake_messages.go
+++ b/ssl/test/runner/handshake_messages.go
@@ -142,6 +142,7 @@
supportedPoints []uint8
hasKeyShares bool
keyShares []keyShareEntry
+ trailingKeyShareData bool
pskIdentities [][]uint8
hasEarlyData bool
earlyDataContext []byte
@@ -181,6 +182,7 @@
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 @@
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 99d7fac..c350ac5 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -7978,6 +7978,8 @@
DuplicateKeyShares: true,
},
},
+ shouldFail: true,
+ expectedError: ":DUPLICATE_KEY_SHARE:",
})
testCases = append(testCases, testCase{
@@ -8175,6 +8177,19 @@
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 4cfd265..53e5363 100644
--- a/ssl/tls13_server.c
+++ b/ssl/tls13_server.c
@@ -88,7 +88,7 @@
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)) {
