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;
