Insert a state before cert_cb.
If cert_cb runs asynchronously, we end up repeating a large part of very
stateful ClientHello processing. This seems to be mostly fine and there
are few users of server-side cert_cb (it's a new API in 1.0.2), but it's
a little scary.
This is also visible to external consumers because some callbacks get
called multiple times. We especially should try to avoid that as there
is no guarantee that these callbacks are idempotent and give the same
answer each time.
Change-Id: I212b2325eae2cfca0fb423dace101e466c5e5d4e
Reviewed-on: https://boringssl-review.googlesource.com/10224
Commit-Queue: David Benjamin <davidben@google.com>
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/include/openssl/ssl.h b/include/openssl/ssl.h
index d8c472d..38a96c5 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -817,7 +817,10 @@
*
* 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 4439a38..4a99196 100644
--- a/include/openssl/ssl3.h
+++ b/include/openssl/ssl3.h
@@ -351,6 +351,7 @@
#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 aad28f9..ec812bf 100644
--- a/ssl/handshake_server.c
+++ b/ssl/handshake_server.c
@@ -226,6 +226,7 @@
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 @@
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 @@
}
}
- 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 @@
}
}
- 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 @@
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 aa7f398..2a4db6b 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -101,6 +101,8 @@
// 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 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 @@
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 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 2a76580..09a6fcc 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -4338,6 +4338,25 @@
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 @@
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 ec44436..a1aeeea 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 @@
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 @@
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 @@
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;