Check early ALPN before offering 0-RTT. We enforce that servers don't send bogus ALPN values, so consumers may assume that SSL_get0_alpn_selected won't have anything terribly weird. To maintain that invariant in the face of folks whose ALPN preferences change (consider a persisted session cache), we should decline to offer 0-RTT if early_alpn would have been rejected by the check anyway. Change-Id: Ic3a9ba4041d5d4618742eb05e27033525d96ade1 Reviewed-on: https://boringssl-review.googlesource.com/22067 Commit-Queue: David Benjamin <davidben@google.com> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Reviewed-by: Steven Valdez <svaldez@google.com>
diff --git a/ssl/internal.h b/ssl/internal.h index 23bd8a1..3953fbd 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -1575,6 +1575,10 @@ SSL_HANDSHAKE *hs, Array<uint8_t> *out, enum ssl_cert_verify_context_t cert_verify_context); +// ssl_is_alpn_protocol_allowed returns whether |protocol| is a valid server +// selection for |ssl|'s client preferences. +bool ssl_is_alpn_protocol_allowed(const SSL *ssl, Span<const uint8_t> protocol); + // ssl_negotiate_alpn negotiates the ALPN extension, if applicable. It returns // true on successful negotiation or if nothing was negotiated. It returns false // and sets |*out_alert| to an alert on error.
diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc index 09110dd..40b0dc0 100644 --- a/ssl/t1_lib.cc +++ b/ssl/t1_lib.cc
@@ -1382,33 +1382,10 @@ return false; } - if (!ssl->ctx->allow_unknown_alpn_protos) { - // Check that the protocol name is one of the ones we advertised. - bool protocol_ok = false; - CBS client_protocol_name_list, client_protocol_name; - CBS_init(&client_protocol_name_list, ssl->alpn_client_proto_list, - ssl->alpn_client_proto_list_len); - while (CBS_len(&client_protocol_name_list) > 0) { - if (!CBS_get_u8_length_prefixed(&client_protocol_name_list, - &client_protocol_name)) { - *out_alert = SSL_AD_INTERNAL_ERROR; - return false; - } - - if (CBS_len(&client_protocol_name) == CBS_len(&protocol_name) && - OPENSSL_memcmp(CBS_data(&client_protocol_name), - CBS_data(&protocol_name), - CBS_len(&protocol_name)) == 0) { - protocol_ok = true; - break; - } - } - - if (!protocol_ok) { - OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_ALPN_PROTOCOL); - *out_alert = SSL_AD_ILLEGAL_PARAMETER; - return false; - } + if (!ssl_is_alpn_protocol_allowed(ssl, protocol_name)) { + OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_ALPN_PROTOCOL); + *out_alert = SSL_AD_ILLEGAL_PARAMETER; + return false; } if (!ssl->s3->alpn_selected.CopyFrom(protocol_name)) { @@ -1419,6 +1396,34 @@ return true; } +bool ssl_is_alpn_protocol_allowed(const SSL *ssl, + Span<const uint8_t> protocol) { + if (ssl->alpn_client_proto_list == nullptr) { + return false; + } + + if (ssl->ctx->allow_unknown_alpn_protos) { + return true; + } + + // Check that the protocol name is one of the ones we advertised. + CBS client_protocol_name_list, client_protocol_name; + CBS_init(&client_protocol_name_list, ssl->alpn_client_proto_list, + ssl->alpn_client_proto_list_len); + while (CBS_len(&client_protocol_name_list) > 0) { + if (!CBS_get_u8_length_prefixed(&client_protocol_name_list, + &client_protocol_name)) { + return false; + } + + if (client_protocol_name == protocol) { + return true; + } + } + + return false; +} + bool ssl_negotiate_alpn(SSL_HANDSHAKE *hs, uint8_t *out_alert, const SSL_CLIENT_HELLO *client_hello) { SSL *const ssl = hs->ssl; @@ -1989,11 +1994,19 @@ static bool ext_early_data_add_clienthello(SSL_HANDSHAKE *hs, CBB *out) { SSL *const ssl = hs->ssl; - if (ssl->session == NULL || + if (!ssl->cert->enable_early_data || + // Session must be 0-RTT capable. + ssl->session == NULL || ssl_session_protocol_version(ssl->session) < TLS1_3_VERSION || ssl->session->ticket_max_early_data == 0 || + // The second ClientHello never offers early data. hs->received_hello_retry_request || - !ssl->cert->enable_early_data) { + // In case ALPN preferences changed since this session was established, + // avoid reporting a confusing value in |SSL_get0_alpn_selected|. + (ssl->session->early_alpn_len != 0 && + !ssl_is_alpn_protocol_allowed( + ssl, MakeConstSpan(ssl->session->early_alpn, + ssl->session->early_alpn_len)))) { return true; }
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index 1d7b996..660cf21 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc
@@ -2201,6 +2201,12 @@ return false; } + if (is_resume && !is_retry && !config->is_server && + config->expect_no_offer_early_data && SSL_in_early_data(ssl)) { + fprintf(stderr, "Client unexpectedly offered early data.\n"); + return false; + } + if (config->handshake_twice) { do { ret = SSL_do_handshake(ssl);
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index e666404..fe9afee 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go
@@ -11726,6 +11726,28 @@ expectedError: ":ALPN_MISMATCH_ON_EARLY_DATA:", }) + // Test that the client does not offer early data if it is incompatible + // with ALPN preferences. + testCases = append(testCases, testCase{ + testType: clientTest, + name: "TLS13-EarlyData-ALPNPreferenceChanged", + config: Config{ + MaxVersion: VersionTLS13, + MaxEarlyDataSize: 16384, + NextProtos: []string{"foo", "bar"}, + }, + resumeSession: true, + flags: []string{ + "-enable-early-data", + "-expect-early-data-info", + "-expect-no-offer-early-data", + "-on-initial-advertise-alpn", "\x03foo", + "-on-resume-advertise-alpn", "\x03bar", + "-on-initial-expect-alpn", "foo", + "-on-resume-expect-alpn", "bar", + }, + }) + // Test that the server correctly rejects 0-RTT when the previous // session did not allow early data on resumption. testCases = append(testCases, testCase{
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 6df8d2a..3354851 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc
@@ -121,6 +121,7 @@ { "-expect-no-session-id", &TestConfig::expect_no_session_id }, { "-expect-accept-early-data", &TestConfig::expect_accept_early_data }, { "-expect-reject-early-data", &TestConfig::expect_reject_early_data }, + { "-expect-no-offer-early-data", &TestConfig::expect_no_offer_early_data }, { "-no-op-extra-handshake", &TestConfig::no_op_extra_handshake }, { "-handshake-twice", &TestConfig::handshake_twice }, { "-allow-unknown-alpn-protos", &TestConfig::allow_unknown_alpn_protos },
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index 9af64bc..b96d9e5 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h
@@ -90,6 +90,7 @@ bool expect_early_data_info = false; bool expect_accept_early_data = false; bool expect_reject_early_data = false; + bool expect_no_offer_early_data = false; bool use_ticket_callback = false; bool renew_ticket = false; bool enable_early_data = false;