From c02c19e0d842f54d903a9b62316476f4b9c4e3f0 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 10 Feb 2021 17:49:20 -0500 Subject: [PATCH] Honor SSL_TLSEXT_ERR_ALERT_FATAL in the ALPN callback. This aligns with OpenSSL's behavior. RFC7301 says servers should return no_application_protocol if the client supported ALPN but no common protocol was found. We currently interpret all values as SSL_TLSEXT_ERR_NOACK. Instead, implement both modes and give guidance on whne to use each. (NOACK is still useful because the callback may be shared across multiple configurations, some of which don't support ALPN at all. Those would want to return NOACK to ignore the list.) To match upstream, I've also switched SSL_R_MISSING_ALPN, added for QUIC, to SSL_R_NO_APPLICATION_PROTOCOL. Update-Note: Callers that return SSL_TLSEXT_ERR_ALERT_FATAL from the ALPN callback will change behavior. The old behavior may be restored by returning SSL_TLSEXT_ERR_NOACK, though see the documentation for new recommendations on return values. Change-Id: Ib7917b5f8a098571bed764c79aa7a4ce0f728297 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/45504 Reviewed-by: Adam Langley Commit-Queue: David Benjamin --- crypto/err/ssl.errordata | 2 +- include/openssl/ssl.h | 34 ++++++++++++++++++------- ssl/t1_lib.cc | 52 +++++++++++++++++++++++++-------------- ssl/test/runner/runner.go | 25 +++++++++++++++---- ssl/test/test_config.cc | 7 +++++- ssl/test/test_config.h | 1 + 6 files changed, 86 insertions(+), 35 deletions(-) diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata index cd6f87a4a..44c3904f3 100644 --- a/crypto/err/ssl.errordata +++ b/crypto/err/ssl.errordata @@ -87,7 +87,6 @@ SSL,160,INVALID_SSL_SESSION SSL,161,INVALID_TICKET_KEYS_LENGTH SSL,302,KEY_USAGE_BIT_INCORRECT SSL,162,LENGTH_MISMATCH -SSL,307,MISSING_ALPN SSL,164,MISSING_EXTENSION SSL,258,MISSING_KEY_SHARE SSL,165,MISSING_RSA_CERTIFICATE @@ -99,6 +98,7 @@ SSL,308,NEGOTIATED_ALPS_WITHOUT_ALPN SSL,170,NEGOTIATED_BOTH_NPN_AND_ALPN SSL,285,NEGOTIATED_TB_WITHOUT_EMS_OR_RI SSL,171,NESTED_GROUP +SSL,307,NO_APPLICATION_PROTOCOL SSL,172,NO_CERTIFICATES_RETURNED SSL,173,NO_CERTIFICATE_ASSIGNED SSL,174,NO_CERTIFICATE_SET diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 7ff7e72c8..b932d2463 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2743,18 +2743,34 @@ OPENSSL_EXPORT int SSL_set_alpn_protos(SSL *ssl, const uint8_t *protos, // SSL_CTX_set_alpn_select_cb sets a callback function on |ctx| that is called // during ClientHello processing in order to select an ALPN protocol from the -// client's list of offered protocols. Configuring this callback enables ALPN on -// a server. +// client's list of offered protocols. // // The callback is passed a wire-format (i.e. a series of non-empty, 8-bit -// length-prefixed strings) ALPN protocol list in |in|. It should set |*out| and -// |*out_len| to the selected protocol and return |SSL_TLSEXT_ERR_OK| on -// success. It does not pass ownership of the buffer. Otherwise, it should -// return |SSL_TLSEXT_ERR_NOACK|. Other |SSL_TLSEXT_ERR_*| values are -// unimplemented and will be treated as |SSL_TLSEXT_ERR_NOACK|. +// length-prefixed strings) ALPN protocol list in |in|. To select a protocol, +// the callback should set |*out| and |*out_len| to the selected protocol and +// return |SSL_TLSEXT_ERR_OK| on success. It does not pass ownership of the +// buffer, so |*out| should point to a static string, a buffer that outlives the +// callback call, or the corresponding entry in |in|. +// +// If the server supports ALPN, but there are no protocols in common, the +// callback should return |SSL_TLSEXT_ERR_ALERT_FATAL| to abort the connection +// with a no_application_protocol alert. +// +// If the server does not support ALPN, it can return |SSL_TLSEXT_ERR_NOACK| to +// continue the handshake without negotiating a protocol. This may be useful if +// multiple server configurations share an |SSL_CTX|, only some of which have +// ALPN protocols configured. +// +// |SSL_TLSEXT_ERR_ALERT_WARNING| is ignored and will be treated as +// |SSL_TLSEXT_ERR_NOACK|. +// +// The callback will only be called if the client supports ALPN. Callers that +// wish to require ALPN for all clients must check |SSL_get0_alpn_selected| +// after the handshake. In QUIC connections, this is done automatically. // // The cipher suite is selected before negotiating ALPN. The callback may use -// |SSL_get_pending_cipher| to query the cipher suite. +// |SSL_get_pending_cipher| to query the cipher suite. This may be used to +// implement HTTP/2's cipher suite constraints. OPENSSL_EXPORT void SSL_CTX_set_alpn_select_cb( SSL_CTX *ctx, int (*cb)(SSL *ssl, const uint8_t **out, uint8_t *out_len, const uint8_t *in, unsigned in_len, void *arg), @@ -5286,7 +5302,7 @@ BSSL_NAMESPACE_END #define SSL_R_CIPHER_MISMATCH_ON_EARLY_DATA 304 #define SSL_R_QUIC_TRANSPORT_PARAMETERS_MISCONFIGURED 305 #define SSL_R_UNEXPECTED_COMPATIBILITY_MODE 306 -#define SSL_R_MISSING_ALPN 307 +#define SSL_R_NO_APPLICATION_PROTOCOL 307 #define SSL_R_NEGOTIATED_ALPS_WITHOUT_ALPN 308 #define SSL_R_ALPS_MISMATCH_ON_EARLY_DATA 309 #define SSL_R_SSLV3_ALERT_CLOSE_NOTIFY 1000 diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc index 342c17021..484eee9b0 100644 --- a/ssl/t1_lib.cc +++ b/ssl/t1_lib.cc @@ -1428,7 +1428,7 @@ static bool ext_alpn_add_clienthello(SSL_HANDSHAKE *hs, CBB *out) { SSL *const ssl = hs->ssl; if (hs->config->alpn_client_proto_list.empty() && ssl->quic_method) { // ALPN MUST be used with QUIC. - OPENSSL_PUT_ERROR(SSL, SSL_R_MISSING_ALPN); + OPENSSL_PUT_ERROR(SSL, SSL_R_NO_APPLICATION_PROTOCOL); return false; } @@ -1456,7 +1456,7 @@ static bool ext_alpn_parse_serverhello(SSL_HANDSHAKE *hs, uint8_t *out_alert, if (contents == NULL) { if (ssl->quic_method) { // ALPN is required when QUIC is used. - OPENSSL_PUT_ERROR(SSL, SSL_R_MISSING_ALPN); + OPENSSL_PUT_ERROR(SSL, SSL_R_NO_APPLICATION_PROTOCOL); *out_alert = SSL_AD_NO_APPLICATION_PROTOCOL; return false; } @@ -1537,7 +1537,7 @@ bool ssl_negotiate_alpn(SSL_HANDSHAKE *hs, uint8_t *out_alert, TLSEXT_TYPE_application_layer_protocol_negotiation)) { if (ssl->quic_method) { // ALPN is required when QUIC is used. - OPENSSL_PUT_ERROR(SSL, SSL_R_MISSING_ALPN); + OPENSSL_PUT_ERROR(SSL, SSL_R_NO_APPLICATION_PROTOCOL); *out_alert = SSL_AD_NO_APPLICATION_PROTOCOL; return false; } @@ -1572,25 +1572,39 @@ bool ssl_negotiate_alpn(SSL_HANDSHAKE *hs, uint8_t *out_alert, const uint8_t *selected; uint8_t selected_len; - if (ssl->ctx->alpn_select_cb( - 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; + int ret = ssl->ctx->alpn_select_cb( + ssl, &selected, &selected_len, CBS_data(&protocol_name_list), + CBS_len(&protocol_name_list), ssl->ctx->alpn_select_cb_arg); + // ALPN is required when QUIC is used. + if (ssl->quic_method && + (ret == SSL_TLSEXT_ERR_NOACK || ret == SSL_TLSEXT_ERR_ALERT_WARNING)) { + ret = SSL_TLSEXT_ERR_ALERT_FATAL; + } + switch (ret) { + case 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; + return false; + } + break; + case SSL_TLSEXT_ERR_NOACK: + case SSL_TLSEXT_ERR_ALERT_WARNING: + break; + case SSL_TLSEXT_ERR_ALERT_FATAL: + *out_alert = SSL_AD_NO_APPLICATION_PROTOCOL; + OPENSSL_PUT_ERROR(SSL, SSL_R_NO_APPLICATION_PROTOCOL); return false; - } - if (!ssl->s3->alpn_selected.CopyFrom( - MakeConstSpan(selected, selected_len))) { + default: + // Invalid return value. *out_alert = SSL_AD_INTERNAL_ERROR; + OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR); return false; - } - } else if (ssl->quic_method) { - // ALPN is required when QUIC is used. - OPENSSL_PUT_ERROR(SSL, SSL_R_MISSING_ALPN); - *out_alert = SSL_AD_NO_APPLICATION_PROTOCOL; - return false; } return true; diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 07c0aa634..9af820ece 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -6756,7 +6756,7 @@ func addExtensionTests() { if protocol == quic { // ALPN is mandatory in QUIC. shouldDeclineALPNFail = true - declineALPNError = ":MISSING_ALPN:" + declineALPNError = ":NO_APPLICATION_PROTOCOL:" declineALPNLocalError = "remote error: no application protocol" } testCases = append(testCases, testCase{ @@ -6778,6 +6778,21 @@ func addExtensionTests() { expectedLocalError: declineALPNLocalError, }) + testCases = append(testCases, testCase{ + protocol: protocol, + testType: serverTest, + skipQUICALPNConfig: true, + name: "ALPNServer-Reject-" + suffix, + config: Config{ + MaxVersion: ver.version, + NextProtos: []string{"foo", "bar", "baz"}, + }, + flags: []string{"-reject-alpn"}, + shouldFail: true, + expectedError: ":NO_APPLICATION_PROTOCOL:", + expectedLocalError: "remote error: no application protocol", + }) + // Test that the server implementation catches itself if the // callback tries to return an invalid empty ALPN protocol. testCases = append(testCases, testCase{ @@ -6957,7 +6972,7 @@ func addExtensionTests() { }, skipQUICALPNConfig: true, shouldFail: true, - expectedError: ":MISSING_ALPN:", + expectedError: ":NO_APPLICATION_PROTOCOL:", }) testCases = append(testCases, testCase{ testType: clientTest, @@ -6972,7 +6987,7 @@ func addExtensionTests() { }, skipQUICALPNConfig: true, shouldFail: true, - expectedError: ":MISSING_ALPN:", + expectedError: ":NO_APPLICATION_PROTOCOL:", expectedLocalError: "remote error: no application protocol", }) testCases = append(testCases, testCase{ @@ -6985,7 +7000,7 @@ func addExtensionTests() { }, skipQUICALPNConfig: true, shouldFail: true, - expectedError: ":MISSING_ALPN:", + expectedError: ":NO_APPLICATION_PROTOCOL:", expectedLocalError: "remote error: no application protocol", }) testCases = append(testCases, testCase{ @@ -7002,7 +7017,7 @@ func addExtensionTests() { }, skipQUICALPNConfig: true, shouldFail: true, - expectedError: ":MISSING_ALPN:", + expectedError: ":NO_APPLICATION_PROTOCOL:", expectedLocalError: "remote error: no application protocol", }) } diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index c1d215bbc..59c5e399d 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc @@ -73,6 +73,7 @@ const Flag kBoolFlags[] = { {"-shim-writes-first", &TestConfig::shim_writes_first}, {"-expect-session-miss", &TestConfig::expect_session_miss}, {"-decline-alpn", &TestConfig::decline_alpn}, + {"-reject-alpn", &TestConfig::reject_alpn}, {"-select-empty-alpn", &TestConfig::select_empty_alpn}, {"-defer-alps", &TestConfig::defer_alps}, {"-expect-extended-master-secret", @@ -669,6 +670,9 @@ static int AlpnSelectCallback(SSL *ssl, const uint8_t **out, uint8_t *outlen, if (config->decline_alpn) { return SSL_TLSEXT_ERR_NOACK; } + if (config->reject_alpn) { + return SSL_TLSEXT_ERR_ALERT_FATAL; + } if (!config->expect_advertised_alpn.empty() && (config->expect_advertised_alpn.size() != inlen || @@ -1274,7 +1278,8 @@ bssl::UniquePtr TestConfig::SetupCtx(SSL_CTX *old_ctx) const { NULL); } - if (!select_alpn.empty() || decline_alpn || select_empty_alpn) { + if (!select_alpn.empty() || decline_alpn || reject_alpn || + select_empty_alpn) { SSL_CTX_set_alpn_select_cb(ssl_ctx.get(), AlpnSelectCallback, NULL); } diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index 7d7799431..c24c3769b 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h @@ -68,6 +68,7 @@ struct TestConfig { std::string expect_advertised_alpn; std::string select_alpn; bool decline_alpn = false; + bool reject_alpn = false; bool select_empty_alpn = false; bool defer_alps = false; std::vector> application_settings;