From cfa08c3b7740275ef3994ff07a2c7f4639c88ccb Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Thu, 17 Nov 2016 13:21:27 -0800 Subject: [PATCH] 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 --- ssl/internal.h | 4 +++ ssl/t1_lib.c | 26 +++++++++++++++++++- ssl/test/runner/runner.go | 52 ++++++++++++++++++++++++++++++++++++--- ssl/tls13_both.c | 3 ++- 4 files changed, 79 insertions(+), 6 deletions(-) diff --git a/ssl/internal.h b/ssl/internal.h index 7e505db8..1d78ec17 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -1073,6 +1073,10 @@ int ssl_ext_pre_shared_key_parse_clienthello(SSL *ssl, 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 3679f8d7..421232f1 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -1357,6 +1357,7 @@ static int ext_sct_parse_serverhello(SSL *ssl, uint8_t *out_alert, /* 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 @@ static int ext_sct_parse_serverhello(SSL *ssl, uint8_t *out_alert, * 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 @@ int ssl_do_channel_id_callback(SSL *ssl) { 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 3877a8b7..6c0cd576 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -169,10 +169,10 @@ var channelIDKey *ecdsa.PrivateKey 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 @@ func addExtensionTests() { 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 @@ func addExtensionTests() { config: Config{ MaxVersion: ver.version, Bugs: ProtocolBugs{ - SendSCTListOnResume: []byte("bogus"), + SendSCTListOnResume: differentSCTList, }, }, flags: []string{ @@ -5201,6 +5205,42 @@ func addExtensionTests() { 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 @@ func addExtensionTests() { 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 @@ func addExtensionTests() { // 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 9e4d4501..9bf169d2 100644 --- a/ssl/tls13_both.c +++ b/ssl/tls13_both.c @@ -269,7 +269,8 @@ int tls13_process_certificate(SSL *ssl, int allow_anonymous) { 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; }