diff --git a/PORTING.md b/PORTING.md index eca7194e..e2fdb3a5 100644 --- a/PORTING.md +++ b/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 diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index 6afd00b1..f018c0ec 100644 --- a/ssl/handshake_client.cc +++ b/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; } diff --git a/ssl/s3_both.cc b/ssl/s3_both.cc index 80e6f7b0..f51af697 100644 --- a/ssl/s3_both.cc +++ b/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) { diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index d7d2efac..ce09060a 100644 --- a/ssl/test/bssl_shim.cc +++ b/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 expect_leaf; + bssl::UniquePtr 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 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 expect_leaf; - bssl::UniquePtr 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 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 *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) { diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index bf56ca21..b2f5277e 100644 --- a/ssl/test/runner/common.go +++ b/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 diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index 194244d6..5dee5fbe 100644 --- a/ssl/test/runner/handshake_server.go +++ b/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()) } diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index a0f5a9c7..9f548fc5 100644 --- a/ssl/test/runner/runner.go +++ b/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() {