diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index d8c472d8..38a96c57 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -817,7 +817,10 @@ OPENSSL_EXPORT int SSL_clear_chain_certs(SSL *ssl); * * On the client, the callback may call |SSL_get0_certificate_types| and * |SSL_get_client_CA_list| for information on the server's certificate - * request. */ + * request. + * + * On the server, the callback will be called on non-resumption handshakes, + * after extensions have been processed. */ OPENSSL_EXPORT void SSL_CTX_set_cert_cb(SSL_CTX *ctx, int (*cb)(SSL *ssl, void *arg), void *arg); diff --git a/include/openssl/ssl3.h b/include/openssl/ssl3.h index 4439a385..4a991968 100644 --- a/include/openssl/ssl3.h +++ b/include/openssl/ssl3.h @@ -351,6 +351,7 @@ OPENSSL_COMPILE_ASSERT( #define SSL3_ST_SR_CLNT_HELLO_A (0x110 | SSL_ST_ACCEPT) #define SSL3_ST_SR_CLNT_HELLO_B (0x111 | SSL_ST_ACCEPT) #define SSL3_ST_SR_CLNT_HELLO_C (0x112 | SSL_ST_ACCEPT) +#define SSL3_ST_SR_CLNT_HELLO_D (0x113 | SSL_ST_ACCEPT) /* write to client */ #define SSL3_ST_SW_HELLO_REQ_A (0x120 | SSL_ST_ACCEPT) #define SSL3_ST_SW_HELLO_REQ_B (0x121 | SSL_ST_ACCEPT) diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c index aad28f90..ec812bf6 100644 --- a/ssl/handshake_server.c +++ b/ssl/handshake_server.c @@ -226,6 +226,7 @@ int ssl3_accept(SSL *ssl) { case SSL3_ST_SR_CLNT_HELLO_A: case SSL3_ST_SR_CLNT_HELLO_B: case SSL3_ST_SR_CLNT_HELLO_C: + case SSL3_ST_SR_CLNT_HELLO_D: ret = ssl3_get_client_hello(ssl); if (ssl->state == SSL_ST_TLS13) { break; @@ -550,9 +551,53 @@ int ssl_client_cipher_list_contains_cipher( return 0; } +static int negotiate_version( + SSL *ssl, int *out_alert, + const struct ssl_early_callback_ctx *client_hello) { + uint16_t min_version, max_version; + if (!ssl_get_version_range(ssl, &min_version, &max_version)) { + *out_alert = SSL_AD_PROTOCOL_VERSION; + return 0; + } + + uint16_t client_version = + ssl->method->version_from_wire(client_hello->version); + ssl->client_version = client_hello->version; + + /* Select the version to use. */ + uint16_t version = client_version; + if (version > max_version) { + version = max_version; + } + + if (version < min_version) { + OPENSSL_PUT_ERROR(SSL, SSL_R_UNSUPPORTED_PROTOCOL); + *out_alert = SSL_AD_PROTOCOL_VERSION; + return 0; + } + + /* Handle FALLBACK_SCSV. */ + if (ssl_client_cipher_list_contains_cipher(client_hello, + SSL3_CK_FALLBACK_SCSV & 0xffff) && + version < max_version) { + OPENSSL_PUT_ERROR(SSL, SSL_R_INAPPROPRIATE_FALLBACK); + *out_alert = SSL3_AD_INAPPROPRIATE_FALLBACK; + return 0; + } + + ssl->version = ssl->method->version_to_wire(version); + ssl->s3->enc_method = ssl3_get_enc_method(version); + assert(ssl->s3->enc_method != NULL); + + /* At this point, the connection's version is known and |ssl->version| is + * fixed. Begin enforcing the record-layer version. */ + ssl->s3->have_version = 1; + + return 1; +} + static int ssl3_get_client_hello(SSL *ssl) { int al = SSL_AD_INTERNAL_ERROR, ret = -1; - const SSL_CIPHER *c; SSL_SESSION *session = NULL; if (ssl->state == SSL3_ST_SR_CLNT_HELLO_A) { @@ -598,169 +643,138 @@ static int ssl3_get_client_hello(SSL *ssl) { } } - uint16_t client_version = - ssl->method->version_from_wire(client_hello.version); - ssl->client_version = client_hello.version; - - uint16_t min_version, max_version; - if (!ssl_get_version_range(ssl, &min_version, &max_version)) { - al = SSL_AD_PROTOCOL_VERSION; - goto f_err; - } - - /* Note: This codepath may run twice if |ssl_get_prev_session| completes - * asynchronously. - * - * TODO(davidben): Clean up the order of events around ClientHello - * processing. */ + /* Negotiate the protocol version if we have not done so yet. */ if (!ssl->s3->have_version) { - /* Select the version to use. */ - uint16_t version = client_version; - if (version > max_version) { - version = max_version; - } - - if (version < min_version) { - OPENSSL_PUT_ERROR(SSL, SSL_R_UNSUPPORTED_PROTOCOL); - al = SSL_AD_PROTOCOL_VERSION; + if (!negotiate_version(ssl, &al, &client_hello)) { goto f_err; } - /* Handle FALLBACK_SCSV. */ - if (ssl_client_cipher_list_contains_cipher( - &client_hello, SSL3_CK_FALLBACK_SCSV & 0xffff) && - version < max_version) { - al = SSL3_AD_INAPPROPRIATE_FALLBACK; - OPENSSL_PUT_ERROR(SSL, SSL_R_INAPPROPRIATE_FALLBACK); + if (ssl3_protocol_version(ssl) >= TLS1_3_VERSION) { + ssl->state = SSL_ST_TLS13; + return 1; + } + } + + if (ssl->state == SSL3_ST_SR_CLNT_HELLO_C) { + /* Load the client random. */ + if (client_hello.random_len != SSL3_RANDOM_SIZE) { + OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR); + return -1; + } + memcpy(ssl->s3->client_random, client_hello.random, + client_hello.random_len); + + /* Determine whether we are doing session resumption. */ + int send_new_ticket = 0; + switch ( + ssl_get_prev_session(ssl, &session, &send_new_ticket, &client_hello)) { + case ssl_session_success: + break; + case ssl_session_error: + goto err; + case ssl_session_retry: + ssl->rwstate = SSL_PENDING_SESSION; + goto err; + } + ssl->tlsext_ticket_expected = send_new_ticket; + + /* The EMS state is needed when making the resumption decision, but + * extensions are not normally parsed until later. This detects the EMS + * extension for the resumption decision and it's checked against the result + * of the normal parse later in this function. */ + CBS ems; + int have_extended_master_secret = + ssl->version != SSL3_VERSION && + ssl_early_callback_get_extension(&client_hello, &ems, + TLSEXT_TYPE_extended_master_secret) && + CBS_len(&ems) == 0; + + int has_session = 0; + if (session != NULL) { + if (session->extended_master_secret && + !have_extended_master_secret) { + /* A ClientHello without EMS that attempts to resume a session with EMS + * is fatal to the connection. */ + al = SSL_AD_HANDSHAKE_FAILURE; + OPENSSL_PUT_ERROR(SSL, SSL_R_RESUMED_EMS_SESSION_WITHOUT_EMS_EXTENSION); + goto f_err; + } + + has_session = + /* Only resume if the session's version matches the negotiated + * version: most clients do not accept a mismatch. */ + ssl->version == session->ssl_version && + /* If the client offers the EMS extension, but the previous session + * didn't use it, then negotiate a new session. */ + have_extended_master_secret == session->extended_master_secret; + } + + if (has_session) { + /* Use the old session. */ + ssl->session = session; + session = NULL; + ssl->verify_result = ssl->session->verify_result; + } else { + SSL_set_session(ssl, NULL); + if (!ssl_get_new_session(ssl, 1 /* server */)) { + goto err; + } + + /* Clear the session ID if we want the session to be single-use. */ + if (!(ssl->ctx->session_cache_mode & SSL_SESS_CACHE_SERVER)) { + ssl->s3->new_session->session_id_length = 0; + } + } + + if (ssl->ctx->dos_protection_cb != NULL && + ssl->ctx->dos_protection_cb(&client_hello) == 0) { + /* Connection rejected for DOS reasons. */ + al = SSL_AD_ACCESS_DENIED; + OPENSSL_PUT_ERROR(SSL, SSL_R_CONNECTION_REJECTED); goto f_err; } - ssl->version = ssl->method->version_to_wire(version); - ssl->s3->enc_method = ssl3_get_enc_method(version); - assert(ssl->s3->enc_method != NULL); - /* At this point, the connection's version is known and |ssl->version| is - * fixed. Begin enforcing the record-layer version. */ - ssl->s3->have_version = 1; - } else if (client_version < ssl3_protocol_version(ssl)) { - OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_VERSION_NUMBER); - al = SSL_AD_PROTOCOL_VERSION; - goto f_err; - } + /* Only null compression is supported. */ + if (memchr(client_hello.compression_methods, 0, + client_hello.compression_methods_len) == NULL) { + al = SSL_AD_ILLEGAL_PARAMETER; + OPENSSL_PUT_ERROR(SSL, SSL_R_NO_COMPRESSION_SPECIFIED); + goto f_err; + } - if (ssl3_protocol_version(ssl) >= TLS1_3_VERSION) { - ssl->state = SSL_ST_TLS13; - return 1; - } - - /* Load the client random. */ - if (client_hello.random_len != SSL3_RANDOM_SIZE) { - OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR); - return -1; - } - memcpy(ssl->s3->client_random, client_hello.random, client_hello.random_len); - - int send_new_ticket = 0; - switch ( - ssl_get_prev_session(ssl, &session, &send_new_ticket, &client_hello)) { - case ssl_session_success: - break; - case ssl_session_error: + /* TLS extensions. */ + if (!ssl_parse_clienthello_tlsext(ssl, &client_hello)) { + OPENSSL_PUT_ERROR(SSL, SSL_R_PARSE_TLSEXT); goto err; - case ssl_session_retry: - ssl->rwstate = SSL_PENDING_SESSION; - goto err; - } - ssl->tlsext_ticket_expected = send_new_ticket; + } - /* The EMS state is needed when making the resumption decision, but - * extensions are not normally parsed until later. This detects the EMS - * extension for the resumption decision and it's checked against the result - * of the normal parse later in this function. */ - CBS ems; - int have_extended_master_secret = - ssl->version != SSL3_VERSION && - ssl_early_callback_get_extension(&client_hello, &ems, - TLSEXT_TYPE_extended_master_secret) && - CBS_len(&ems) == 0; - - int has_session = 0; - if (session != NULL) { - if (session->extended_master_secret && - !have_extended_master_secret) { - /* A ClientHello without EMS that attempts to resume a session with EMS - * is fatal to the connection. */ - al = SSL_AD_HANDSHAKE_FAILURE; - OPENSSL_PUT_ERROR(SSL, SSL_R_RESUMED_EMS_SESSION_WITHOUT_EMS_EXTENSION); + if (have_extended_master_secret != ssl->s3->tmp.extended_master_secret) { + al = SSL_AD_INTERNAL_ERROR; + OPENSSL_PUT_ERROR(SSL, SSL_R_EMS_STATE_INCONSISTENT); goto f_err; } - has_session = - /* Only resume if the session's version matches the negotiated version: - * most clients do not accept a mismatch. */ - ssl->version == session->ssl_version && - /* If the client offers the EMS extension, but the previous session - * didn't use it, then negotiate a new session. */ - have_extended_master_secret == session->extended_master_secret; + ssl->state = SSL3_ST_SR_CLNT_HELLO_D; } - if (has_session) { - /* Use the old session. */ - ssl->session = session; - session = NULL; - ssl->verify_result = ssl->session->verify_result; + /* Determine the remaining connection parameters. This is a separate state so + * |cert_cb| does not cause earlier logic to run multiple times. */ + assert(ssl->state == SSL3_ST_SR_CLNT_HELLO_D); + + if (ssl->session != NULL) { + /* Check that the cipher is in the list. */ + if (!ssl_client_cipher_list_contains_cipher( + &client_hello, (uint16_t)ssl->session->cipher->id)) { + al = SSL_AD_ILLEGAL_PARAMETER; + OPENSSL_PUT_ERROR(SSL, SSL_R_REQUIRED_CIPHER_MISSING); + goto f_err; + } + + ssl->s3->tmp.new_cipher = ssl->session->cipher; + ssl->s3->tmp.cert_request = 0; } else { - SSL_set_session(ssl, NULL); - if (!ssl_get_new_session(ssl, 1 /* server */)) { - goto err; - } - - /* Clear the session ID if we want the session to be single-use. */ - if (!(ssl->ctx->session_cache_mode & SSL_SESS_CACHE_SERVER)) { - ssl->s3->new_session->session_id_length = 0; - } - } - - if (ssl->ctx->dos_protection_cb != NULL && - ssl->ctx->dos_protection_cb(&client_hello) == 0) { - /* Connection rejected for DOS reasons. */ - al = SSL_AD_ACCESS_DENIED; - OPENSSL_PUT_ERROR(SSL, SSL_R_CONNECTION_REJECTED); - goto f_err; - } - - /* If it is a hit, check that the cipher is in the list. */ - if (ssl->session != NULL && - !ssl_client_cipher_list_contains_cipher( - &client_hello, (uint16_t)ssl->session->cipher->id)) { - al = SSL_AD_ILLEGAL_PARAMETER; - OPENSSL_PUT_ERROR(SSL, SSL_R_REQUIRED_CIPHER_MISSING); - goto f_err; - } - - /* Only null compression is supported. */ - if (memchr(client_hello.compression_methods, 0, - client_hello.compression_methods_len) == NULL) { - al = SSL_AD_ILLEGAL_PARAMETER; - OPENSSL_PUT_ERROR(SSL, SSL_R_NO_COMPRESSION_SPECIFIED); - goto f_err; - } - - /* TLS extensions. */ - if (!ssl_parse_clienthello_tlsext(ssl, &client_hello)) { - OPENSSL_PUT_ERROR(SSL, SSL_R_PARSE_TLSEXT); - goto err; - } - - if (have_extended_master_secret != ssl->s3->tmp.extended_master_secret) { - al = SSL_AD_INTERNAL_ERROR; - OPENSSL_PUT_ERROR(SSL, SSL_R_EMS_STATE_INCONSISTENT); - goto f_err; - } - - /* Given ciphers and SSL_get_ciphers, we must pick a cipher */ - if (ssl->session == NULL) { - /* Let cert callback update server certificates if required */ - if (ssl->cert->cert_cb) { + /* Call |cert_cb| to update server certificates if required. */ + if (ssl->cert->cert_cb != NULL) { int rv = ssl->cert->cert_cb(ssl, ssl->cert->cert_cb_arg); if (rv == 0) { al = SSL_AD_INTERNAL_ERROR; @@ -773,7 +787,8 @@ static int ssl3_get_client_hello(SSL *ssl) { } } - c = ssl3_choose_cipher(ssl, &client_hello, ssl_get_cipher_preferences(ssl)); + const SSL_CIPHER *c = + ssl3_choose_cipher(ssl, &client_hello, ssl_get_cipher_preferences(ssl)); if (c == NULL) { al = SSL_AD_HANDSHAKE_FAILURE; OPENSSL_PUT_ERROR(SSL, SSL_R_NO_SHARED_CIPHER); @@ -794,10 +809,6 @@ static int ssl3_get_client_hello(SSL *ssl) { if (!ssl_cipher_uses_certificate_auth(ssl->s3->tmp.new_cipher)) { ssl->s3->tmp.cert_request = 0; } - } else { - /* Session-id reuse */ - ssl->s3->tmp.new_cipher = ssl->session->cipher; - ssl->s3->tmp.cert_request = 0; } /* Now that the cipher is known, initialize the handshake hash. */ diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index aa7f3982..2a4db6be 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc @@ -101,6 +101,8 @@ struct TestState { // operation has been retried. unsigned private_key_retries = 0; bool got_new_session = false; + bool ticket_decrypt_done = false; + bool alpn_select_done = false; }; static void TestStateExFree(void *parent, void *ptr, CRYPTO_EX_DATA *ad, @@ -515,6 +517,13 @@ static int NextProtoSelectCallback(SSL* ssl, uint8_t** out, uint8_t* outlen, static int AlpnSelectCallback(SSL* ssl, const uint8_t** out, uint8_t* outlen, const uint8_t* in, unsigned inlen, void* arg) { + if (GetTestState(ssl)->alpn_select_done) { + fprintf(stderr, "AlpnSelectCallback called after completion.\n"); + exit(1); + } + + GetTestState(ssl)->alpn_select_done = true; + const TestConfig *config = GetTestConfig(ssl); if (config->decline_alpn) { return SSL_TLSEXT_ERR_NOACK; @@ -627,6 +636,10 @@ static void InfoCallback(const SSL *ssl, int type, int val) { abort(); } GetTestState(ssl)->handshake_done = true; + + // Callbacks may be called again on a new handshake. + GetTestState(ssl)->ticket_decrypt_done = false; + GetTestState(ssl)->alpn_select_done = false; } } @@ -640,6 +653,15 @@ static int NewSessionCallback(SSL *ssl, SSL_SESSION *session) { static int TicketKeyCallback(SSL *ssl, uint8_t *key_name, uint8_t *iv, EVP_CIPHER_CTX *ctx, HMAC_CTX *hmac_ctx, int encrypt) { + if (!encrypt) { + if (GetTestState(ssl)->ticket_decrypt_done) { + fprintf(stderr, "TicketKeyCallback called after completion.\n"); + return -1; + } + + GetTestState(ssl)->ticket_decrypt_done = true; + } + // This is just test code, so use the all-zeros key. static const uint8_t kZeros[16] = {0}; diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 2a76580c..09a6fccb 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -4338,6 +4338,25 @@ func addExtensionTests() { resumeSession: resumeSession, }) + // Test ALPN in async mode as well to ensure that extensions callbacks are only + // called once. + testCases = append(testCases, testCase{ + testType: serverTest, + name: "ALPNServer-Async-" + ver.name, + config: Config{ + MaxVersion: ver.version, + NextProtos: []string{"foo", "bar", "baz"}, + }, + flags: []string{ + "-expect-advertised-alpn", "\x03foo\x03bar\x03baz", + "-select-alpn", "foo", + "-async", + }, + expectedNextProto: "foo", + expectedNextProtoType: alpn, + resumeSession: resumeSession, + }) + var emptyString string testCases = append(testCases, testCase{ testType: clientTest, @@ -4502,6 +4521,26 @@ func addExtensionTests() { resumeSession: true, }) + // Test that the ticket callback is only called once when everything before + // it in the ClientHello is asynchronous. This corrupts the ticket so + // certificate selection callbacks run. + testCases = append(testCases, testCase{ + testType: serverTest, + name: "TicketCallback-SingleCall-" + ver.name, + config: Config{ + MaxVersion: ver.version, + Bugs: ProtocolBugs{ + CorruptTicket: true, + }, + }, + resumeSession: true, + expectResumeRejected: true, + flags: []string{ + "-use-ticket-callback", + "-async", + }, + }) + // Resume with an oversized session id. testCases = append(testCases, testCase{ testType: serverTest, diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c index ec444364..a1aeeea9 100644 --- a/ssl/tls13_server.c +++ b/ssl/tls13_server.c @@ -29,6 +29,7 @@ enum server_hs_state_t { state_process_client_hello = 0, + state_select_parameters, state_send_hello_retry_request, state_flush_hello_retry_request, state_process_second_client_hello, @@ -150,9 +151,12 @@ static enum ssl_hs_wait_t do_process_client_hello(SSL *ssl, SSL_HANDSHAKE *hs) { return ssl_hs_error; } - /* Let cert callback update server certificates if required. - * - * TODO(davidben): Can this get run earlier? */ + hs->state = state_select_parameters; + return ssl_hs_ok; +} + +static enum ssl_hs_wait_t do_select_parameters(SSL *ssl, SSL_HANDSHAKE *hs) { + /* Call |cert_cb| to update server certificates if required. */ if (ssl->cert->cert_cb != NULL) { int rv = ssl->cert->cert_cb(ssl, ssl->cert->cert_cb_arg); if (rv == 0) { @@ -161,11 +165,19 @@ static enum ssl_hs_wait_t do_process_client_hello(SSL *ssl, SSL_HANDSHAKE *hs) { return ssl_hs_error; } if (rv < 0) { - hs->state = state_process_client_hello; + hs->state = state_select_parameters; return ssl_hs_x509_lookup; } } + struct ssl_early_callback_ctx client_hello; + if (!ssl_early_callback_init(ssl, &client_hello, ssl->init_msg, + ssl->init_num)) { + OPENSSL_PUT_ERROR(SSL, SSL_R_CLIENTHELLO_PARSE_FAILED); + ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); + return ssl_hs_error; + } + const SSL_CIPHER *cipher = ssl3_choose_cipher(ssl, &client_hello, ssl_get_cipher_preferences(ssl)); if (cipher == NULL) { @@ -529,6 +541,9 @@ enum ssl_hs_wait_t tls13_server_handshake(SSL *ssl) { case state_process_client_hello: ret = do_process_client_hello(ssl, hs); break; + case state_select_parameters: + ret = do_select_parameters(ssl, hs); + break; case state_send_hello_retry_request: ret = do_send_hello_retry_request(ssl, hs); break;