Reject if the ALPN callback returned an empty protocol.
If the callback returns an empty ALPN, we forget we negotiated ALPN at
all (bssl::Array does not distinguish null and empty). Empty ALPN
protocols are forbidden anyway, so reject these ahead of time.
Change-Id: I42f1fc4c843bc865e23fb2a2e5d57424b569ee99
Reviewed-on: https://boringssl-review.googlesource.com/28546
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc
index 7fae0da..40b7ff0 100644
--- a/ssl/t1_lib.cc
+++ b/ssl/t1_lib.cc
@@ -1532,6 +1532,11 @@
ssl, &selected, &selected_len, CBS_data(&protocol_name_list),
CBS_len(&protocol_name_list),
ssl->ctx->alpn_select_cb_arg) == SSL_TLSEXT_ERR_OK) {
+ if (selected_len == 0) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_ALPN_PROTOCOL);
+ *out_alert = SSL_AD_INTERNAL_ERROR;
+ return false;
+ }
if (!ssl->s3->alpn_selected.CopyFrom(
MakeConstSpan(selected, selected_len))) {
*out_alert = SSL_AD_INTERNAL_ERROR;
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index 1d15771..87da27a 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -766,6 +766,7 @@
exit(1);
}
+ assert(config->select_alpn.empty() || !config->select_empty_alpn);
*out = (const uint8_t*)config->select_alpn.data();
*outlen = config->select_alpn.size();
return SSL_TLSEXT_ERR_OK;
@@ -1228,7 +1229,8 @@
NULL);
}
- if (!config->select_alpn.empty() || config->decline_alpn) {
+ if (!config->select_alpn.empty() || config->decline_alpn ||
+ config->select_empty_alpn) {
SSL_CTX_set_alpn_select_cb(ssl_ctx.get(), AlpnSelectCallback, NULL);
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 3d8a6b0..ba52706 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -6193,6 +6193,24 @@
expectNoNextProto: true,
resumeSession: true,
})
+ // Test that the server implementation catches itself if the
+ // callback tries to return an invalid empty ALPN protocol.
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "ALPNServer-SelectEmpty-" + ver.name,
+ config: Config{
+ MaxVersion: ver.version,
+ NextProtos: []string{"foo", "bar", "baz"},
+ },
+ flags: []string{
+ "-expect-advertised-alpn", "\x03foo\x03bar\x03baz",
+ "-select-empty-alpn",
+ },
+ tls13Variant: ver.tls13Variant,
+ shouldFail: true,
+ expectedLocalError: "remote error: internal error",
+ expectedError: ":INVALID_ALPN_PROTOCOL:",
+ })
// Test ALPN in async mode as well to ensure that extensions callbacks are only
// called once.
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc
index c5ce610..fdcb0a9 100644
--- a/ssl/test/test_config.cc
+++ b/ssl/test/test_config.cc
@@ -64,6 +64,7 @@
{ "-shim-writes-first", &TestConfig::shim_writes_first },
{ "-expect-session-miss", &TestConfig::expect_session_miss },
{ "-decline-alpn", &TestConfig::decline_alpn },
+ { "-select-empty-alpn", &TestConfig::select_empty_alpn },
{ "-expect-extended-master-secret",
&TestConfig::expect_extended_master_secret },
{ "-enable-ocsp-stapling", &TestConfig::enable_ocsp_stapling },
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h
index 57c4ced..6488a26 100644
--- a/ssl/test/test_config.h
+++ b/ssl/test/test_config.h
@@ -59,6 +59,7 @@
std::string expected_advertised_alpn;
std::string select_alpn;
bool decline_alpn = false;
+ bool select_empty_alpn = false;
std::string quic_transport_params;
std::string expected_quic_transport_params;
bool expect_session_miss = false;
