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>

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

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(&copy, &sct_list) ||
+      CBS_len(&copy) != 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;
+}

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{

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;
       }