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 <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

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

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

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;

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",
 				})
 			}

ssl/test/test_config.cc

@@ -73,6 +73,7 @@ const Flag<bool> 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<SSL_CTX> 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);
   }
 

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<std::pair<std::string, std::string>> application_settings;