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>
diff --git a/PORTING.md b/PORTING.md
index eca7194..e2fdb3a 100644
--- a/PORTING.md
+++ b/PORTING.md
@@ -134,6 +134,13 @@
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
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 6afd00b..f018c0e 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -1100,33 +1100,6 @@
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;
}
diff --git a/ssl/s3_both.cc b/ssl/s3_both.cc
index 80e6f7b..f51af69 100644
--- a/ssl/s3_both.cc
+++ b/ssl/s3_both.cc
@@ -852,8 +852,59 @@
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) {
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index d7d2efa..ce09060 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -115,6 +115,7 @@
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 @@
}
}
+ if (GetTestState(ssl)->cert_verified) {
+ fprintf(stderr, "Certificate verified twice.\n");
+ return false;
+ }
+
return true;
}
@@ -715,6 +721,7 @@
return 0;
}
+ GetTestState(ssl)->cert_verified = true;
return 1;
}
@@ -732,6 +739,7 @@
return ssl_verify_invalid;
}
+ GetTestState(ssl)->cert_verified = true;
return ssl_verify_ok;
}
@@ -1483,11 +1491,115 @@
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 @@
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 @@
}
}
- 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 @@
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) {
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index bf56ca2..b2f5277 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -1085,11 +1085,19 @@
// 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
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index 194244d..5dee5fb 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -1442,6 +1442,10 @@
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 @@
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())
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index a0f5a9c..9f548fc 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -199,7 +199,9 @@
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 @@
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 @@
// 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 @@
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() {
