Check for invalid ALPN inputs in SSL_(CTX_)set_alpn_protos.
See also 86a90dc749af91f8a7b8da6628c9ffca2bae3009 from upstream. This
differs from upstream's which treats {NULL, 2} as a valid way to spell
the empty list. (I think this is a mistake and have asked them about
it.)
Upstream's CL also, for them, newly makes the empty list disable ALPN,
when previously they'd disable it but misread it as a malloc failure.
For us, we'd already fixed the misreading due to our switch to
bssl::Array and bssl::Span, but the documentation was odd. This CL
preserves that behavior, but updates the documentation and writes a
test.
Update-Note: SSL_CTX_set_alpn_protos and SSL_set_alpn_protos will now
reject invalud inputs. Previously, they would accept them, but silently
send an invalid ALPN extension which the server would almost certainly
error on.
Change-Id: Id5830b2d8c3a5cee4712878fe92ee350c4914367
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46804
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata
index da85b1f..04c7d53 100644
--- a/crypto/err/ssl.errordata
+++ b/crypto/err/ssl.errordata
@@ -79,6 +79,7 @@
SSL,157,INAPPROPRIATE_FALLBACK
SSL,303,INCONSISTENT_CLIENT_HELLO
SSL,259,INVALID_ALPN_PROTOCOL
+SSL,315,INVALID_ALPN_PROTOCOL_LIST
SSL,314,INVALID_CLIENT_HELLO_INNER
SSL,158,INVALID_COMMAND
SSL,256,INVALID_COMPRESSION_LIST
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 16b64c1..8d8e091 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -2723,8 +2723,9 @@
// SSL_CTX_set_alpn_protos sets the client ALPN protocol list on |ctx| to
// |protos|. |protos| must be in wire-format (i.e. a series of non-empty, 8-bit
-// length-prefixed strings). It returns zero on success and one on failure.
-// Configuring this list enables ALPN on a client.
+// length-prefixed strings), or the empty string to disable ALPN. It returns
+// zero on success and one on failure. Configuring a non-empty string enables
+// ALPN on a client.
//
// WARNING: this function is dangerous because it breaks the usual return value
// convention.
@@ -2733,8 +2734,9 @@
// SSL_set_alpn_protos sets the client ALPN protocol list on |ssl| to |protos|.
// |protos| must be in wire-format (i.e. a series of non-empty, 8-bit
-// length-prefixed strings). It returns zero on success and one on failure.
-// Configuring this list enables ALPN on a client.
+// length-prefixed strings), or the empty string to disable ALPN. It returns
+// zero on success and one on failure. Configuring a non-empty string enables
+// ALPN on a client.
//
// WARNING: this function is dangerous because it breaks the usual return value
// convention.
@@ -5368,6 +5370,7 @@
#define SSL_R_UNSUPPORTED_ECH_SERVER_CONFIG 312
#define SSL_R_ECH_SERVER_WOULD_HAVE_NO_RETRY_CONFIGS 313
#define SSL_R_INVALID_CLIENT_HELLO_INNER 314
+#define SSL_R_INVALID_ALPN_PROTOCOL_LIST 315
#define SSL_R_SSLV3_ALERT_CLOSE_NOTIFY 1000
#define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010
#define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020
diff --git a/ssl/internal.h b/ssl/internal.h
index 7a3979b..e1585c3 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -2052,6 +2052,9 @@
SSL_HANDSHAKE *hs, Array<uint8_t> *out,
enum ssl_cert_verify_context_t cert_verify_context);
+// ssl_is_valid_alpn_list returns whether |in| is a valid ALPN protocol list.
+bool ssl_is_valid_alpn_list(Span<const uint8_t> in);
+
// ssl_is_alpn_protocol_allowed returns whether |protocol| is a valid server
// selection for |hs->ssl|'s client preferences.
bool ssl_is_alpn_protocol_allowed(const SSL_HANDSHAKE *hs,
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index 4e820f0..522c09e 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -2300,21 +2300,26 @@
int SSL_CTX_set_alpn_protos(SSL_CTX *ctx, const uint8_t *protos,
unsigned protos_len) {
- // Note this function's calling convention is backwards.
- return ctx->alpn_client_proto_list.CopyFrom(MakeConstSpan(protos, protos_len))
- ? 0
- : 1;
+ // Note this function's return value is backwards.
+ auto span = MakeConstSpan(protos, protos_len);
+ if (!span.empty() && !ssl_is_valid_alpn_list(span)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_ALPN_PROTOCOL_LIST);
+ return 1;
+ }
+ return ctx->alpn_client_proto_list.CopyFrom(span) ? 0 : 1;
}
int SSL_set_alpn_protos(SSL *ssl, const uint8_t *protos, unsigned protos_len) {
- // Note this function's calling convention is backwards.
+ // Note this function's return value is backwards.
if (!ssl->config) {
return 1;
}
- return ssl->config->alpn_client_proto_list.CopyFrom(
- MakeConstSpan(protos, protos_len))
- ? 0
- : 1;
+ auto span = MakeConstSpan(protos, protos_len);
+ if (!span.empty() && !ssl_is_valid_alpn_list(span)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_ALPN_PROTOCOL_LIST);
+ return 1;
+ }
+ return ssl->config->alpn_client_proto_list.CopyFrom(span) ? 0 : 1;
}
void SSL_CTX_set_alpn_select_cb(SSL_CTX *ctx,
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 0c6bf50..1f402db 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -6914,5 +6914,53 @@
}
}
+TEST(SSLTest, ALPNConfig) {
+ bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
+ ASSERT_TRUE(ctx);
+ bssl::UniquePtr<X509> cert = GetTestCertificate();
+ bssl::UniquePtr<EVP_PKEY> key = GetTestKey();
+ ASSERT_TRUE(cert);
+ ASSERT_TRUE(key);
+ ASSERT_TRUE(SSL_CTX_use_certificate(ctx.get(), cert.get()));
+ ASSERT_TRUE(SSL_CTX_use_PrivateKey(ctx.get(), key.get()));
+
+ // Set up some machinery to check the configured ALPN against what is actually
+ // sent over the wire. Note that the ALPN callback is only called when the
+ // client offers ALPN.
+ std::vector<uint8_t> observed_alpn;
+ SSL_CTX_set_alpn_select_cb(
+ ctx.get(),
+ [](SSL *ssl, const uint8_t **out, uint8_t *out_len, const uint8_t *in,
+ unsigned in_len, void *arg) -> int {
+ std::vector<uint8_t> *observed_alpn_ptr =
+ static_cast<std::vector<uint8_t> *>(arg);
+ observed_alpn_ptr->assign(in, in + in_len);
+ return SSL_TLSEXT_ERR_NOACK;
+ },
+ &observed_alpn);
+ auto check_alpn_proto = [&](Span<const uint8_t> expected) {
+ observed_alpn.clear();
+ bssl::UniquePtr<SSL> client, server;
+ EXPECT_TRUE(ConnectClientAndServer(&client, &server, ctx.get(), ctx.get()));
+ EXPECT_EQ(Bytes(expected), Bytes(observed_alpn));
+ };
+
+ // Note that |SSL_CTX_set_alpn_protos|'s return value is reversed.
+ static const uint8_t kValidList[] = {0x03, 'f', 'o', 'o',
+ 0x03, 'b', 'a', 'r'};
+ EXPECT_EQ(0,
+ SSL_CTX_set_alpn_protos(ctx.get(), kValidList, sizeof(kValidList)));
+ check_alpn_proto(kValidList);
+
+ // Invalid lists are rejected.
+ static const uint8_t kInvalidList[] = {0x04, 'f', 'o', 'o'};
+ EXPECT_EQ(1, SSL_CTX_set_alpn_protos(ctx.get(), kInvalidList,
+ sizeof(kInvalidList)));
+
+ // Empty lists are valid and are interpreted as disabling ALPN.
+ EXPECT_EQ(0, SSL_CTX_set_alpn_protos(ctx.get(), nullptr, 0));
+ check_alpn_proto({});
+}
+
} // namespace
BSSL_NAMESPACE_END
diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc
index 20517c4..3a88cd1 100644
--- a/ssl/t1_lib.cc
+++ b/ssl/t1_lib.cc
@@ -1528,6 +1528,22 @@
return true;
}
+bool ssl_is_valid_alpn_list(Span<const uint8_t> in) {
+ CBS protocol_name_list = in;
+ if (CBS_len(&protocol_name_list) == 0) {
+ return false;
+ }
+ while (CBS_len(&protocol_name_list) > 0) {
+ CBS protocol_name;
+ if (!CBS_get_u8_length_prefixed(&protocol_name_list, &protocol_name) ||
+ // Empty protocol names are forbidden.
+ CBS_len(&protocol_name) == 0) {
+ return false;
+ }
+ }
+ return true;
+}
+
bool ssl_is_alpn_protocol_allowed(const SSL_HANDSHAKE *hs,
Span<const uint8_t> protocol) {
if (hs->config->alpn_client_proto_list.empty()) {
@@ -1580,25 +1596,12 @@
CBS protocol_name_list;
if (!CBS_get_u16_length_prefixed(&contents, &protocol_name_list) ||
CBS_len(&contents) != 0 ||
- CBS_len(&protocol_name_list) < 2) {
+ !ssl_is_valid_alpn_list(protocol_name_list)) {
OPENSSL_PUT_ERROR(SSL, SSL_R_PARSE_TLSEXT);
*out_alert = SSL_AD_DECODE_ERROR;
return false;
}
- // Validate the protocol list.
- CBS protocol_name_list_copy = protocol_name_list;
- while (CBS_len(&protocol_name_list_copy) > 0) {
- CBS protocol_name;
- if (!CBS_get_u8_length_prefixed(&protocol_name_list_copy, &protocol_name) ||
- // Empty protocol names are forbidden.
- CBS_len(&protocol_name) == 0) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_PARSE_TLSEXT);
- *out_alert = SSL_AD_DECODE_ERROR;
- return false;
- }
- }
-
const uint8_t *selected;
uint8_t selected_len;
int ret = ssl->ctx->alpn_select_cb(
