Don't reauthenticate on renegotiation.

We currently forbid the server certificate from changing on
renegotiation. This means re-verifying the certificate is pointless and
indeed the callback being called again seems to surprise consumers more
than anything else.

Carry over the initial handshake's SCT lists and OCSP responses (don't
enforce they don't change since the server may have, say, picked up new
OCSP responses in the meantime), ignore new ones received on
renegotiation, and don't bother redoing verification.

For our purposes, TLS 1.2 renegotiation is an overcomplicated TLS 1.3
KeyUpdate + post-handshake auth. The server is not allowed to change
identity.

Bug: 126
Change-Id: I0dae85bcf243943b1a5a97fa4f30f100c9e6e41e
Reviewed-on: https://boringssl-review.googlesource.com/19665
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

PORTING.md

@@ -134,6 +134,13 @@ Things which do not work:
   not offer a session on renegotiation or resume any session established by a
   renegotiation handshake.
 
+* The server may not change its certificate in the renegotiation. This mitigates
+  the [triple handshake attack](https://mitls.org/pages/attacks/3SHAKE). Any new
+  stapled OCSP response and SCT list will be ignored. As no authentication state
+  may change, BoringSSL will not re-verify the certificate on a renegotiation.
+  Callbacks such as `SSL_CTX_set_custom_verify` will only run on the initial
+  handshake.
+
 ### Lowercase hexadecimal
 
 BoringSSL's `BN_bn2hex` function uses lowercase hexadecimal digits instead of

ssl/handshake_client.cc

@@ -1100,33 +1100,6 @@ static int ssl3_get_server_certificate(SSL_HANDSHAKE *hs) {
     return -1;
   }
 
-  /* Disallow the server certificate from changing during a renegotiation. See
-   * https://mitls.org/pages/attacks/3SHAKE. We never resume on renegotiation,
-   * so this check is sufficient. */
-  if (ssl->s3->established_session != NULL) {
-    if (sk_CRYPTO_BUFFER_num(ssl->s3->established_session->certs) !=
-        sk_CRYPTO_BUFFER_num(hs->new_session->certs)) {
-      OPENSSL_PUT_ERROR(SSL, SSL_R_SERVER_CERT_CHANGED);
-      ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
-      return -1;
-    }
-
-    for (size_t i = 0; i < sk_CRYPTO_BUFFER_num(hs->new_session->certs); i++) {
-      const CRYPTO_BUFFER *old_cert =
-          sk_CRYPTO_BUFFER_value(ssl->s3->established_session->certs, i);
-      const CRYPTO_BUFFER *new_cert =
-          sk_CRYPTO_BUFFER_value(hs->new_session->certs, i);
-      if (CRYPTO_BUFFER_len(old_cert) != CRYPTO_BUFFER_len(new_cert) ||
-          OPENSSL_memcmp(CRYPTO_BUFFER_data(old_cert),
-                         CRYPTO_BUFFER_data(new_cert),
-                         CRYPTO_BUFFER_len(old_cert)) != 0) {
-        OPENSSL_PUT_ERROR(SSL, SSL_R_SERVER_CERT_CHANGED);
-        ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
-        return -1;
-      }
-    }
-  }
-
   ssl->method->next_message(ssl);
   return 1;
 }

ssl/s3_both.cc

@@ -852,8 +852,59 @@ int ssl_parse_extensions(const CBS *cbs, uint8_t *out_alert,
   return 1;
 }
 
+static void set_crypto_buffer(CRYPTO_BUFFER **dest, CRYPTO_BUFFER *src) {
+  /* TODO(davidben): Remove this helper once |SSL_SESSION| can use |UniquePtr|
+   * and |UniquePtr| has up_ref helpers. */
+  CRYPTO_BUFFER_free(*dest);
+  *dest = src;
+  if (src != nullptr) {
+    CRYPTO_BUFFER_up_ref(src);
+  }
+}
+
 enum ssl_verify_result_t ssl_verify_peer_cert(SSL_HANDSHAKE *hs) {
   SSL *const ssl = hs->ssl;
+  const SSL_SESSION *prev_session = ssl->s3->established_session;
+  if (prev_session != NULL) {
+    /* If renegotiating, the server must not change the server certificate. See
+     * https://mitls.org/pages/attacks/3SHAKE. We never resume on renegotiation,
+     * so this check is sufficient to ensure the reported peer certificate never
+     * changes on renegotiation. */
+    assert(!ssl->server);
+    if (sk_CRYPTO_BUFFER_num(prev_session->certs) !=
+        sk_CRYPTO_BUFFER_num(hs->new_session->certs)) {
+      OPENSSL_PUT_ERROR(SSL, SSL_R_SERVER_CERT_CHANGED);
+      ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
+      return ssl_verify_invalid;
+    }
+
+    for (size_t i = 0; i < sk_CRYPTO_BUFFER_num(hs->new_session->certs); i++) {
+      const CRYPTO_BUFFER *old_cert =
+          sk_CRYPTO_BUFFER_value(prev_session->certs, i);
+      const CRYPTO_BUFFER *new_cert =
+          sk_CRYPTO_BUFFER_value(hs->new_session->certs, i);
+      if (CRYPTO_BUFFER_len(old_cert) != CRYPTO_BUFFER_len(new_cert) ||
+          OPENSSL_memcmp(CRYPTO_BUFFER_data(old_cert),
+                         CRYPTO_BUFFER_data(new_cert),
+                         CRYPTO_BUFFER_len(old_cert)) != 0) {
+        OPENSSL_PUT_ERROR(SSL, SSL_R_SERVER_CERT_CHANGED);
+        ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
+        return ssl_verify_invalid;
+      }
+    }
+
+    /* The certificate is identical, so we may skip re-verifying the
+     * certificate. Since we only authenticated the previous one, copy other
+     * authentication from the established session and ignore what was newly
+     * received. */
+    set_crypto_buffer(&hs->new_session->ocsp_response,
+                      prev_session->ocsp_response);
+    set_crypto_buffer(&hs->new_session->signed_cert_timestamp_list,
+                      prev_session->signed_cert_timestamp_list);
+    hs->new_session->verify_result = prev_session->verify_result;
+    return ssl_verify_ok;
+  }
+
   uint8_t alert = SSL_AD_CERTIFICATE_UNKNOWN;
   enum ssl_verify_result_t ret;
   if (ssl->custom_verify_callback != nullptr) {

ssl/test/bssl_shim.cc

@@ -115,6 +115,7 @@ struct TestState {
   bool custom_verify_ready = false;
   std::string msg_callback_text;
   bool msg_callback_ok = true;
+  bool cert_verified = false;
 };
 
 static void TestStateExFree(void *parent, void *ptr, CRYPTO_EX_DATA *ad,
@@ -699,6 +700,11 @@ static bool CheckVerifyCallback(SSL *ssl) {
     }
   }
 
+  if (GetTestState(ssl)->cert_verified) {
+    fprintf(stderr, "Certificate verified twice.\n");
+    return false;
+  }
+
   return true;
 }
 
@@ -715,6 +721,7 @@ static int CertVerifyCallback(X509_STORE_CTX *store_ctx, void *arg) {
     return 0;
   }
 
+  GetTestState(ssl)->cert_verified = true;
   return 1;
 }
 
@@ -732,6 +739,7 @@ static ssl_verify_result_t CustomVerifyCallback(SSL *ssl, uint8_t *out_alert) {
     return ssl_verify_invalid;
   }
 
+  GetTestState(ssl)->cert_verified = true;
   return ssl_verify_ok;
 }
 
@@ -1483,11 +1491,115 @@ static uint16_t GetProtocolVersion(const SSL *ssl) {
   return 0x0201 + ~version;
 }
 
+// CheckAuthProperties checks, after the initial handshake is completed or
+// after a renegotiation, that authentication-related properties match |config|.
+static bool CheckAuthProperties(SSL *ssl, bool is_resume,
+                                const TestConfig *config) {
+  if (!config->expected_ocsp_response.empty()) {
+    const uint8_t *data;
+    size_t len;
+    SSL_get0_ocsp_response(ssl, &data, &len);
+    if (config->expected_ocsp_response.size() != len ||
+        OPENSSL_memcmp(config->expected_ocsp_response.data(), data, len) != 0) {
+      fprintf(stderr, "OCSP response mismatch\n");
+      return false;
+    }
+  }
+
+  if (!config->expected_signed_cert_timestamps.empty()) {
+    const uint8_t *data;
+    size_t len;
+    SSL_get0_signed_cert_timestamp_list(ssl, &data, &len);
+    if (config->expected_signed_cert_timestamps.size() != len ||
+        OPENSSL_memcmp(config->expected_signed_cert_timestamps.data(), data,
+                       len) != 0) {
+      fprintf(stderr, "SCT list mismatch\n");
+      return false;
+    }
+  }
+
+  if (config->expect_verify_result) {
+    int expected_verify_result = config->verify_fail ?
+      X509_V_ERR_APPLICATION_VERIFICATION :
+      X509_V_OK;
+
+    if (SSL_get_verify_result(ssl) != expected_verify_result) {
+      fprintf(stderr, "Wrong certificate verification result\n");
+      return false;
+    }
+  }
+
+  if (!config->expect_peer_cert_file.empty()) {
+    bssl::UniquePtr<X509> expect_leaf;
+    bssl::UniquePtr<STACK_OF(X509)> expect_chain;
+    if (!LoadCertificate(&expect_leaf, &expect_chain,
+                         config->expect_peer_cert_file)) {
+      return false;
+    }
+
+    // For historical reasons, clients report a chain with a leaf and servers
+    // without.
+    if (!config->is_server) {
+      if (!sk_X509_insert(expect_chain.get(), expect_leaf.get(), 0)) {
+        return false;
+      }
+      X509_up_ref(expect_leaf.get());  // sk_X509_push takes ownership.
+    }
+
+    bssl::UniquePtr<X509> leaf(SSL_get_peer_certificate(ssl));
+    STACK_OF(X509) *chain = SSL_get_peer_cert_chain(ssl);
+    if (X509_cmp(leaf.get(), expect_leaf.get()) != 0) {
+      fprintf(stderr, "Received a different leaf certificate than expected.\n");
+      return false;
+    }
+
+    if (sk_X509_num(chain) != sk_X509_num(expect_chain.get())) {
+      fprintf(stderr, "Received a chain of length %zu instead of %zu.\n",
+              sk_X509_num(chain), sk_X509_num(expect_chain.get()));
+      return false;
+    }
+
+    for (size_t i = 0; i < sk_X509_num(chain); i++) {
+      if (X509_cmp(sk_X509_value(chain, i),
+                   sk_X509_value(expect_chain.get(), i)) != 0) {
+        fprintf(stderr, "Chain certificate %zu did not match.\n",
+                i + 1);
+        return false;
+      }
+    }
+  }
+
+  bool expected_sha256_client_cert = config->expect_sha256_client_cert_initial;
+  if (is_resume) {
+    expected_sha256_client_cert = config->expect_sha256_client_cert_resume;
+  }
+
+  if (SSL_get_session(ssl)->peer_sha256_valid != expected_sha256_client_cert) {
+    fprintf(stderr,
+            "Unexpected SHA-256 client cert state: expected:%d is_resume:%d.\n",
+            expected_sha256_client_cert, is_resume);
+    return false;
+  }
+
+  if (expected_sha256_client_cert &&
+      SSL_get_session(ssl)->certs != nullptr) {
+    fprintf(stderr, "Have both client cert and SHA-256 hash: is_resume:%d.\n",
+            is_resume);
+    return false;
+  }
+
+  return true;
+}
+
 // CheckHandshakeProperties checks, immediately after |ssl| completes its
 // initial handshake (or False Starts), whether all the properties are
 // consistent with the test configuration and invariants.
 static bool CheckHandshakeProperties(SSL *ssl, bool is_resume,
                                      const TestConfig *config) {
+  if (!CheckAuthProperties(ssl, is_resume, config)) {
+    return false;
+  }
+
   if (SSL_get_current_cipher(ssl) == nullptr) {
     fprintf(stderr, "null cipher after handshake\n");
     return false;
@@ -1613,40 +1725,6 @@ static bool CheckHandshakeProperties(SSL *ssl, bool is_resume,
     return false;
   }
 
-  if (!config->expected_ocsp_response.empty()) {
-    const uint8_t *data;
-    size_t len;
-    SSL_get0_ocsp_response(ssl, &data, &len);
-    if (config->expected_ocsp_response.size() != len ||
-        OPENSSL_memcmp(config->expected_ocsp_response.data(), data, len) != 0) {
-      fprintf(stderr, "OCSP response mismatch\n");
-      return false;
-    }
-  }
-
-  if (!config->expected_signed_cert_timestamps.empty()) {
-    const uint8_t *data;
-    size_t len;
-    SSL_get0_signed_cert_timestamp_list(ssl, &data, &len);
-    if (config->expected_signed_cert_timestamps.size() != len ||
-        OPENSSL_memcmp(config->expected_signed_cert_timestamps.data(), data,
-                       len) != 0) {
-      fprintf(stderr, "SCT list mismatch\n");
-      return false;
-    }
-  }
-
-  if (config->expect_verify_result) {
-    int expected_verify_result = config->verify_fail ?
-      X509_V_ERR_APPLICATION_VERIFICATION :
-      X509_V_OK;
-
-    if (SSL_get_verify_result(ssl) != expected_verify_result) {
-      fprintf(stderr, "Wrong certificate verification result\n");
-      return false;
-    }
-  }
-
   if (config->expect_peer_signature_algorithm != 0 &&
       config->expect_peer_signature_algorithm !=
           SSL_get_peer_signature_algorithm(ssl)) {
@@ -1705,65 +1783,6 @@ static bool CheckHandshakeProperties(SSL *ssl, bool is_resume,
     }
   }
 
-  if (!config->expect_peer_cert_file.empty()) {
-    bssl::UniquePtr<X509> expect_leaf;
-    bssl::UniquePtr<STACK_OF(X509)> expect_chain;
-    if (!LoadCertificate(&expect_leaf, &expect_chain,
-                         config->expect_peer_cert_file)) {
-      return false;
-    }
-
-    // For historical reasons, clients report a chain with a leaf and servers
-    // without.
-    if (!config->is_server) {
-      if (!sk_X509_insert(expect_chain.get(), expect_leaf.get(), 0)) {
-        return false;
-      }
-      X509_up_ref(expect_leaf.get());  // sk_X509_push takes ownership.
-    }
-
-    bssl::UniquePtr<X509> leaf(SSL_get_peer_certificate(ssl));
-    STACK_OF(X509) *chain = SSL_get_peer_cert_chain(ssl);
-    if (X509_cmp(leaf.get(), expect_leaf.get()) != 0) {
-      fprintf(stderr, "Received a different leaf certificate than expected.\n");
-      return false;
-    }
-
-    if (sk_X509_num(chain) != sk_X509_num(expect_chain.get())) {
-      fprintf(stderr, "Received a chain of length %zu instead of %zu.\n",
-              sk_X509_num(chain), sk_X509_num(expect_chain.get()));
-      return false;
-    }
-
-    for (size_t i = 0; i < sk_X509_num(chain); i++) {
-      if (X509_cmp(sk_X509_value(chain, i),
-                   sk_X509_value(expect_chain.get(), i)) != 0) {
-        fprintf(stderr, "Chain certificate %zu did not match.\n",
-                i + 1);
-        return false;
-      }
-    }
-  }
-
-  bool expected_sha256_client_cert = config->expect_sha256_client_cert_initial;
-  if (is_resume) {
-    expected_sha256_client_cert = config->expect_sha256_client_cert_resume;
-  }
-
-  if (SSL_get_session(ssl)->peer_sha256_valid != expected_sha256_client_cert) {
-    fprintf(stderr,
-            "Unexpected SHA-256 client cert state: expected:%d is_resume:%d.\n",
-            expected_sha256_client_cert, is_resume);
-    return false;
-  }
-
-  if (expected_sha256_client_cert &&
-      SSL_get_session(ssl)->certs != nullptr) {
-    fprintf(stderr, "Have both client cert and SHA-256 hash: is_resume:%d.\n",
-            is_resume);
-    return false;
-  }
-
   if (is_resume && config->expect_ticket_age_skew != 0 &&
       SSL_get_ticket_age_skew(ssl) != config->expect_ticket_age_skew) {
     fprintf(stderr, "Ticket age skew was %" PRId32 ", wanted %d\n",
@@ -2372,11 +2391,19 @@ static bool DoExchange(bssl::UniquePtr<SSL_SESSION> *out_session, SSL *ssl,
     return false;
   }
 
-  if (SSL_total_renegotiations(ssl) > 0 &&
-      !SSL_get_session(ssl)->not_resumable) {
-    fprintf(stderr,
-            "Renegotiations should never produce resumable sessions.\n");
-    return false;
+  if (SSL_total_renegotiations(ssl) > 0) {
+    if (!SSL_get_session(ssl)->not_resumable) {
+      fprintf(stderr,
+              "Renegotiations should never produce resumable sessions.\n");
+      return false;
+    }
+
+    // Re-check authentication properties after a renegotiation. The reported
+    // values should remain unchanged even if the server sent different SCT
+    // lists.
+    if (!CheckAuthProperties(ssl, is_resume, config)) {
+      return false;
+    }
   }
 
   if (SSL_total_renegotiations(ssl) != config->expect_total_renegotiations) {

ssl/test/runner/common.go

@@ -1085,11 +1085,19 @@ type ProtocolBugs struct {
 	// supplied SCT list in resumption handshakes.
 	SendSCTListOnResume []byte
 
+	// SendSCTListOnRenegotiation, if not nil, causes the server to send the
+	// supplied SCT list on renegotiation.
+	SendSCTListOnRenegotiation []byte
+
 	// SendOCSPResponseOnResume, if not nil, causes the server to advertise
 	// OCSP stapling in resumption handshakes and, if applicable, send the
 	// supplied stapled response.
 	SendOCSPResponseOnResume []byte
 
+	// SendOCSPResponseOnResume, if not nil, causes the server to send the
+	// supplied OCSP response on renegotiation.
+	SendOCSPResponseOnRenegotiation []byte
+
 	// SendExtensionOnCertificate, if not nil, causes the runner to send the
 	// supplied bytes in the extensions on the Certificate message.
 	SendExtensionOnCertificate []byte

ssl/test/runner/handshake_server.go

@@ -1442,6 +1442,10 @@ func (hs *serverHandshakeState) doFullHandshake() error {
 		hs.hello.extensions.sctList = hs.cert.SignedCertificateTimestampList
 	}
 
+	if len(c.clientVerify) > 0 && config.Bugs.SendSCTListOnRenegotiation != nil {
+		hs.hello.extensions.sctList = config.Bugs.SendSCTListOnRenegotiation
+	}
+
 	hs.hello.extensions.ticketSupported = hs.clientHello.ticketSupported && !config.SessionTicketsDisabled && c.vers > VersionSSL30
 	hs.hello.cipherSuite = hs.suite.id
 	if config.Bugs.SendCipherSuite != 0 {
@@ -1488,6 +1492,9 @@ func (hs *serverHandshakeState) doFullHandshake() error {
 		certStatus := new(certificateStatusMsg)
 		certStatus.statusType = statusTypeOCSP
 		certStatus.response = hs.cert.OCSPStaple
+		if len(c.clientVerify) > 0 && config.Bugs.SendOCSPResponseOnRenegotiation != nil {
+			certStatus.response = config.Bugs.SendOCSPResponseOnRenegotiation
+		}
 		hs.writeServerHash(certStatus.marshal())
 		c.writeRecord(recordTypeHandshake, certStatus.marshal())
 	}

ssl/test/runner/runner.go

@@ -199,7 +199,9 @@ var channelIDKey *ecdsa.PrivateKey
 var channelIDBytes []byte
 
 var testOCSPResponse = []byte{1, 2, 3, 4}
+var testOCSPResponse2 = []byte{5, 6, 7, 8}
 var testSCTList = []byte{0, 6, 0, 4, 5, 6, 7, 8}
+var testSCTList2 = []byte{0, 6, 0, 4, 1, 2, 3, 4}
 
 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, byte(len(testSCTList))}, testSCTList...)
@@ -6473,10 +6475,6 @@ 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",
@@ -6487,8 +6485,8 @@ func addExtensionTests() {
 				// Send different values on the intermediate. This tests
 				// the intermediate's extensions do not override the
 				// leaf's.
-				SendOCSPOnIntermediates: []byte{1, 3, 3, 7},
-				SendSCTOnIntermediates:  differentSCTList,
+				SendOCSPOnIntermediates: testOCSPResponse2,
+				SendSCTOnIntermediates:  testSCTList2,
 			},
 		},
 		flags: []string{
@@ -7543,6 +7541,34 @@ func addRenegotiationTests() {
 		shouldFail:    true,
 		expectedError: ":UNEXPECTED_EXTENSION:",
 	})
+
+	// The server may send different stapled OCSP responses or SCT lists on
+	// renegotiation, but BoringSSL ignores this and reports the old values.
+	// Also test that non-fatal verify results are preserved.
+	testCases = append(testCases, testCase{
+		testType: clientTest,
+		name:     "Renegotiation-ChangeAuthProperties",
+		config: Config{
+			MaxVersion: VersionTLS12,
+			Bugs: ProtocolBugs{
+				SendOCSPResponseOnRenegotiation: testOCSPResponse2,
+				SendSCTListOnRenegotiation:      testSCTList2,
+			},
+		},
+		renegotiate: 1,
+		flags: []string{
+			"-renegotiate-freely",
+			"-expect-total-renegotiations", "1",
+			"-enable-ocsp-stapling",
+			"-expect-ocsp-response",
+			base64.StdEncoding.EncodeToString(testOCSPResponse),
+			"-enable-signed-cert-timestamps",
+			"-expect-signed-cert-timestamps",
+			base64.StdEncoding.EncodeToString(testSCTList),
+			"-verify-fail",
+			"-expect-verify-result",
+		},
+	})
 }
 
 func addDTLSReplayTests() {