Enforce basic sanity of SCT lists.
According to the RFC[1], SCT lists may not be empty and nor may any SCT
itself be empty.
[1] https://tools.ietf.org/html/rfc6962#section-3.3
Change-Id: Ia1f855907588b36a4fea60872f87e25dc20782b4
Reviewed-on: https://boringssl-review.googlesource.com/12362
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/internal.h b/ssl/internal.h
index 7e505db..1d78ec1 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1073,6 +1073,10 @@
uint8_t *out_alert, CBS *contents);
int ssl_ext_pre_shared_key_add_serverhello(SSL *ssl, CBB *out);
+/* ssl_is_sct_list_valid does a shallow parse of the SCT list in |contents| and
+ * returns one iff it's valid. */
+int ssl_is_sct_list_valid(const CBS *contents);
+
int ssl_write_client_hello(SSL *ssl);
/* ssl_clear_tls13_state releases client state only needed for TLS 1.3. It
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 3679f8d..421232f 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -1357,6 +1357,7 @@
/* TLS 1.3 SCTs are included in the Certificate extensions. */
if (ssl3_protocol_version(ssl) >= TLS1_3_VERSION) {
+ *out_alert = SSL_AD_DECODE_ERROR;
return 0;
}
@@ -1364,7 +1365,7 @@
* ClientHello and thus this function should never have been called. */
assert(ssl->signed_cert_timestamps_enabled);
- if (CBS_len(contents) == 0) {
+ if (!ssl_is_sct_list_valid(contents)) {
*out_alert = SSL_AD_DECODE_ERROR;
return 0;
}
@@ -3469,3 +3470,26 @@
EVP_PKEY_free(key);
return ret;
}
+
+int ssl_is_sct_list_valid(const CBS *contents) {
+ /* Shallow parse the SCT list for sanity. By the RFC
+ * (https://tools.ietf.org/html/rfc6962#section-3.3) neither the list nor any
+ * of the SCTs may be empty. */
+ CBS copy = *contents;
+ CBS sct_list;
+ if (!CBS_get_u16_length_prefixed(©, &sct_list) ||
+ CBS_len(©) != 0 ||
+ CBS_len(&sct_list) == 0) {
+ return 0;
+ }
+
+ while (CBS_len(&sct_list) > 0) {
+ CBS sct;
+ if (!CBS_get_u16_length_prefixed(&sct_list, &sct) ||
+ CBS_len(&sct) == 0) {
+ return 0;
+ }
+ }
+
+ return 1;
+}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 3877a8b..6c0cd57 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -169,10 +169,10 @@
var channelIDBytes []byte
var testOCSPResponse = []byte{1, 2, 3, 4}
-var testSCTList = []byte{5, 6, 7, 8}
+var testSCTList = []byte{0, 6, 0, 4, 5, 6, 7, 8}
var testOCSPExtension = append([]byte{byte(extensionStatusRequest) >> 8, byte(extensionStatusRequest), 0, 8, statusTypeOCSP, 0, 0, 4}, testOCSPResponse...)
-var testSCTExtension = append([]byte{byte(extensionSignedCertificateTimestamp) >> 8, byte(extensionSignedCertificateTimestamp), 0, 4}, testSCTList...)
+var testSCTExtension = append([]byte{byte(extensionSignedCertificateTimestamp) >> 8, byte(extensionSignedCertificateTimestamp), 0, byte(len(testSCTList))}, testSCTList...)
func initCertificates() {
for i := range testCerts {
@@ -5169,6 +5169,10 @@
resumeSession: true,
})
+ var differentSCTList []byte
+ differentSCTList = append(differentSCTList, testSCTList...)
+ differentSCTList[len(differentSCTList)-1] ^= 1
+
// The SCT extension did not specify that it must only be sent on resumption as it
// should have, so test that we tolerate but ignore it.
testCases = append(testCases, testCase{
@@ -5176,7 +5180,7 @@
config: Config{
MaxVersion: ver.version,
Bugs: ProtocolBugs{
- SendSCTListOnResume: []byte("bogus"),
+ SendSCTListOnResume: differentSCTList,
},
},
flags: []string{
@@ -5201,6 +5205,42 @@
resumeSession: true,
})
+ emptySCTListCert := *testCerts[0].cert
+ emptySCTListCert.SignedCertificateTimestampList = []byte{0, 0}
+
+ // Test empty SCT list.
+ testCases = append(testCases, testCase{
+ name: "SignedCertificateTimestampListEmpty-Client-" + ver.name,
+ testType: clientTest,
+ config: Config{
+ MaxVersion: ver.version,
+ Certificates: []Certificate{emptySCTListCert},
+ },
+ flags: []string{
+ "-enable-signed-cert-timestamps",
+ },
+ shouldFail: true,
+ expectedError: ":ERROR_PARSING_EXTENSION:",
+ })
+
+ emptySCTCert := *testCerts[0].cert
+ emptySCTCert.SignedCertificateTimestampList = []byte{0, 6, 0, 2, 1, 2, 0, 0}
+
+ // Test empty SCT in non-empty list.
+ testCases = append(testCases, testCase{
+ name: "SignedCertificateTimestampListEmptySCT-Client-" + ver.name,
+ testType: clientTest,
+ config: Config{
+ MaxVersion: ver.version,
+ Certificates: []Certificate{emptySCTCert},
+ },
+ flags: []string{
+ "-enable-signed-cert-timestamps",
+ },
+ shouldFail: true,
+ expectedError: ":ERROR_PARSING_EXTENSION:",
+ })
+
// Test that certificate-related extensions are not sent unsolicited.
testCases = append(testCases, testCase{
testType: serverTest,
@@ -5437,6 +5477,10 @@
expectedError: ":UNEXPECTED_EXTENSION:",
})
+ var differentSCTList []byte
+ differentSCTList = append(differentSCTList, testSCTList...)
+ differentSCTList[len(differentSCTList)-1] ^= 1
+
// Test that extensions on intermediates are allowed but ignored.
testCases = append(testCases, testCase{
name: "IgnoreExtensionsOnIntermediates-TLS13",
@@ -5448,7 +5492,7 @@
// the intermediate's extensions do not override the
// leaf's.
SendOCSPOnIntermediates: []byte{1, 3, 3, 7},
- SendSCTOnIntermediates: []byte{1, 3, 3, 7},
+ SendSCTOnIntermediates: differentSCTList,
},
},
flags: []string{
diff --git a/ssl/tls13_both.c b/ssl/tls13_both.c
index 9e4d450..9bf169d 100644
--- a/ssl/tls13_both.c
+++ b/ssl/tls13_both.c
@@ -269,7 +269,8 @@
goto err;
}
- if (CBS_len(&sct) == 0) {
+ if (!ssl_is_sct_list_valid(&sct)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_ERROR_PARSING_EXTENSION);
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
goto err;
}
