Bug 1683710 - Add a means to disable ALPN, r=bbeurdouche NSS_3_67_BETA1
authorMartin Thomson <mt@lowentropy.net>
Thu, 10 Jun 2021 08:39:07 +0000
changeset 15940 f703a3855749693c1b538c64850a93d119d476c7
parent 15939 948968caef8dc5a695b53af70e79b3c22e55d763
child 15941 9d016c89bbbb004482dd269323b686ac3f2750cf
child 15944 194e150b059e039d74aa64f3b678069b890f0f6a
push id3976
push userbbeurdouche@mozilla.com
push dateThu, 10 Jun 2021 08:52:17 +0000
reviewersbbeurdouche
bugs1683710
Bug 1683710 - Add a means to disable ALPN, r=bbeurdouche We've recently learned the value of ALPN and SNI when it comes to protecting against cross-protocol attacks. However, some protocols don't have ALPN yet. For servers that terminate connections for those connections, validating that the client has not offered ALPN provides a way to protect against cross-protocol attacks. If the cross-protocol attack uses a protocol that does include ALPN, being able to reject those connections safely reduces exposure. This modifies SSL_SetNextProtoNego() to accept a zero-length buffer as an argument. Previously, this would have crashed. Now it causes the server to reject a handshake if ALPN is offered by the client. It was always possible to implement this by passing a function that always returns SECFailure to SSL_SetNextProtoCallback(). This approach has the advantage that the server generates a no_application_protocol alert, which is not something that user-provided code can do. Differential Revision: https://phabricator.services.mozilla.com/D110887
gtests/ssl_gtest/ssl_extension_unittest.cc
lib/ssl/ssl.h
lib/ssl/ssl3exthandle.c
lib/ssl/sslsock.c
--- a/gtests/ssl_gtest/ssl_extension_unittest.cc
+++ b/gtests/ssl_gtest/ssl_extension_unittest.cc
@@ -321,16 +321,37 @@ TEST_P(TlsExtensionTestGeneric, AlpnMism
   const uint8_t client_alpn[] = {0x01, 0x61};
   client_->EnableAlpn(client_alpn, sizeof(client_alpn));
   const uint8_t server_alpn[] = {0x02, 0x61, 0x62};
   server_->EnableAlpn(server_alpn, sizeof(server_alpn));
 
   ClientHelloErrorTest(nullptr, kTlsAlertNoApplicationProtocol);
 }
 
+TEST_P(TlsExtensionTestGeneric, AlpnDisabledServer) {
+  const uint8_t client_alpn[] = {0x01, 0x61};
+  client_->EnableAlpn(client_alpn, sizeof(client_alpn));
+  server_->EnableAlpn(nullptr, 0);
+
+  ClientHelloErrorTest(nullptr, kTlsAlertUnsupportedExtension);
+}
+
+TEST_P(TlsConnectGeneric, AlpnDisabled) {
+  server_->EnableAlpn(nullptr, 0);
+  Connect();
+
+  SSLNextProtoState state;
+  uint8_t buf[255] = {0};
+  unsigned int buf_len = 3;
+  EXPECT_EQ(SECSuccess, SSL_GetNextProto(client_->ssl_fd(), &state, buf,
+                                         &buf_len, sizeof(buf)));
+  EXPECT_EQ(SSL_NEXT_PROTO_NO_SUPPORT, state);
+  EXPECT_EQ(0U, buf_len);
+}
+
 // Many of these tests fail in TLS 1.3 because the extension is encrypted, which
 // prevents modification of the value from the ServerHello.
 TEST_P(TlsExtensionTestPre13, AlpnReturnedEmptyList) {
   EnableAlpn();
   const uint8_t val[] = {0x00, 0x00};
   DataBuffer extension(val, sizeof(val));
   ServerHelloErrorTest(std::make_shared<TlsExtensionReplacer>(
       server_, ssl_app_layer_protocol_xtn, extension));
--- a/lib/ssl/ssl.h
+++ b/lib/ssl/ssl.h
@@ -398,17 +398,24 @@ SSL_IMPORT SECStatus SSL_SetNextProtoCal
  * SSL_NEXT_PROTO_SELECTED as the state.
  *
  * Because the predecessor to ALPN, NPN, used the first protocol as the fallback
  * protocol, when sending an ALPN extension, the first protocol is moved to the
  * end of the list. This indicates that the fallback protocol is the least
  * preferred. The other protocols should be in preference order.
  *
  * The supported protocols are specified in |data| in wire-format (8-bit
- * length-prefixed). For example: "\010http/1.1\006spdy/2". */
+ * length-prefixed). For example: "\010http/1.1\006spdy/2".
+ *
+ * An empty value (i.e., where |length| is 0 and |data| is any value,
+ * including NULL) forcibly disables ALPN.  In this mode, the server will
+ * reject any ClientHello that includes the ALPN extension.
+ *
+ * Calling this function overrides the callback previously set by
+ * SSL_SetNextProtoCallback. */
 SSL_IMPORT SECStatus SSL_SetNextProtoNego(PRFileDesc *fd,
                                           const unsigned char *data,
                                           unsigned int length);
 
 typedef enum SSLNextProtoState {
     SSL_NEXT_PROTO_NO_SUPPORT = 0, /* No peer support                   */
     SSL_NEXT_PROTO_NEGOTIATED = 1, /* Mutual agreement                  */
     SSL_NEXT_PROTO_NO_OVERLAP = 2, /* No protocol overlap found         */
--- a/lib/ssl/ssl3exthandle.c
+++ b/lib/ssl/ssl3exthandle.c
@@ -303,17 +303,17 @@ ssl3_SelectAppProtocol(const sslSocket *
     SECStatus rv;
     unsigned char resultBuffer[255];
     SECItem result = { siBuffer, resultBuffer, 0 };
 
     rv = ssl3_ValidateAppProtocol(data->data, data->len);
     if (rv != SECSuccess) {
         ssl3_ExtSendAlert(ss, alert_fatal, decode_error);
         PORT_SetError(SSL_ERROR_NEXT_PROTOCOL_DATA_INVALID);
-        return rv;
+        return SECFailure;
     }
 
     PORT_Assert(ss->nextProtoCallback);
     /* Neither the cipher suite nor ECH are selected yet Note that extensions
      * sometimes affect what cipher suite is selected, e.g., for ECC. */
     PORT_Assert((ss->ssl3.hs.preliminaryInfo &
                  ssl_preinfo_all & ~ssl_preinfo_cipher_suite & ~ssl_preinfo_ech) ==
                 (ssl_preinfo_all & ~ssl_preinfo_cipher_suite & ~ssl_preinfo_ech));
--- a/lib/ssl/sslsock.c
+++ b/lib/ssl/sslsock.c
@@ -2207,22 +2207,28 @@ SSL_SetNextProtoCallback(PRFileDesc *fd,
 static SECStatus
 ssl_NextProtoNegoCallback(void *arg, PRFileDesc *fd,
                           const unsigned char *protos, unsigned int protos_len,
                           unsigned char *protoOut, unsigned int *protoOutLen,
                           unsigned int protoMaxLen)
 {
     unsigned int i, j;
     sslSocket *ss = ssl_FindSocket(fd);
-
     if (!ss) {
         SSL_DBG(("%d: SSL[%d]: bad socket in ssl_NextProtoNegoCallback",
                  SSL_GETPID(), fd));
         return SECFailure;
     }
+    if (ss->opt.nextProtoNego.len == 0) {
+        SSL_DBG(("%d: SSL[%d]: ssl_NextProtoNegoCallback ALPN disabled",
+                 SSL_GETPID(), fd));
+        SSL3_SendAlert(ss, alert_fatal, unsupported_extension);
+        return SECFailure;
+    }
+
     PORT_Assert(protoMaxLen <= 255);
     if (protoMaxLen > 255) {
         PORT_SetError(SEC_ERROR_OUTPUT_LEN);
         return SECFailure;
     }
 
     /* For each protocol in client preference, see if we support it. */
     for (j = 0; j < ss->opt.nextProtoNego.len;) {
@@ -2252,30 +2258,32 @@ SSL_SetNextProtoNego(PRFileDesc *fd, con
 
     ss = ssl_FindSocket(fd);
     if (!ss) {
         SSL_DBG(("%d: SSL[%d]: bad socket in SSL_SetNextProtoNego",
                  SSL_GETPID(), fd));
         return SECFailure;
     }
 
-    if (ssl3_ValidateAppProtocol(data, length) != SECSuccess) {
+    if (length > 0 && ssl3_ValidateAppProtocol(data, length) != SECSuccess) {
         return SECFailure;
     }
 
     /* NPN required that the client's fallback protocol is first in the
      * list. However, ALPN sends protocols in preference order. So move the
      * first protocol to the end of the list. */
     ssl_GetSSL3HandshakeLock(ss);
     SECITEM_FreeItem(&ss->opt.nextProtoNego, PR_FALSE);
-    SECITEM_AllocItem(NULL, &ss->opt.nextProtoNego, length);
-    size_t firstLen = data[0] + 1;
-    /* firstLen <= length is ensured by ssl3_ValidateAppProtocol. */
-    PORT_Memcpy(ss->opt.nextProtoNego.data + (length - firstLen), data, firstLen);
-    PORT_Memcpy(ss->opt.nextProtoNego.data, data + firstLen, length - firstLen);
+    if (length > 0) {
+        SECITEM_AllocItem(NULL, &ss->opt.nextProtoNego, length);
+        size_t firstLen = data[0] + 1;
+        /* firstLen <= length is ensured by ssl3_ValidateAppProtocol. */
+        PORT_Memcpy(ss->opt.nextProtoNego.data + (length - firstLen), data, firstLen);
+        PORT_Memcpy(ss->opt.nextProtoNego.data, data + firstLen, length - firstLen);
+    }
     ssl_ReleaseSSL3HandshakeLock(ss);
 
     return SSL_SetNextProtoCallback(fd, ssl_NextProtoNegoCallback, NULL);
 }
 
 SECStatus
 SSL_GetNextProto(PRFileDesc *fd, SSLNextProtoState *state, unsigned char *buf,
                  unsigned int *bufLen, unsigned int bufLenMax)