Bug 1402738 - remove NPN and fix ALPN, r= mt

Also fixes bug 1284412.

Differential Revision: https://phabricator.services.mozilla.com/D908

8 files changed, 73 insertions, 195 deletions

diff --git a/lib/ssl/SSLerrs.h b/lib/ssl/SSLerrs.h
index 0256b4683..1fe7197c0 100644
--- a/lib/ssl/SSLerrs.h
+++ b/lib/ssl/SSLerrs.h
@@ -374,7 +374,7 @@ ER3(SSL_ERROR_WEAK_SERVER_EPHEMERAL_DH_KEY, (SSL_ERROR_BASE + 115),
     "SSL received a weak ephemeral Diffie-Hellman key in Server Key Exchange handshake message.")
 
 ER3(SSL_ERROR_NEXT_PROTOCOL_DATA_INVALID, (SSL_ERROR_BASE + 116),
-    "SSL received invalid NPN extension data.")
+    "SSL received invalid ALPN extension data.")
 
 ER3(SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_SSL2, (SSL_ERROR_BASE + 117),
     "SSL feature not supported for SSL 2.0 connections.")
diff --git a/lib/ssl/ssl.h b/lib/ssl/ssl.h
index ad8ec0f8b..0280f73c3 100644
--- a/lib/ssl/ssl.h
+++ b/lib/ssl/ssl.h
@@ -158,23 +158,18 @@ SSL_IMPORT PRFileDesc *DTLS_ImportFD(PRFileDesc *model, PRFileDesc *fd);
 #define SSL_CBC_RANDOM_IV 23
 #define SSL_ENABLE_OCSP_STAPLING 24 /* Request OCSP stapling (client) */
 
-/* SSL_ENABLE_NPN controls whether the NPN extension is enabled for the initial
- * handshake when application layer protocol negotiation is used.
- * SSL_SetNextProtoCallback or SSL_SetNextProtoNego must be used to control the
- * application layer protocol negotiation; otherwise, the NPN extension will
- * not be negotiated. SSL_ENABLE_NPN is currently enabled by default but this
- * may change in future versions.
- */
+/* SSL_ENABLE_NPN is defunct and defaults to false.
+ * Using this option will not have any effect but won't produce an error. */
 #define SSL_ENABLE_NPN 25
 
 /* SSL_ENABLE_ALPN controls whether the ALPN extension is enabled for the
  * initial handshake when application layer protocol negotiation is used.
- * SSL_SetNextProtoNego (not SSL_SetNextProtoCallback) must be used to control
- * the application layer protocol negotiation; otherwise, the ALPN extension
- * will not be negotiated. ALPN is not negotiated for renegotiation handshakes,
- * even though the ALPN specification defines a way to use ALPN during
- * renegotiations. SSL_ENABLE_ALPN is currently disabled by default, but this
- * may change in future versions.
+ * SSL_SetNextProtoNego or SSL_SetNextProtoCallback can be used to control
+ * the application layer protocol negotiation;
+ * ALPN is not negotiated for renegotiation handshakes, even though the ALPN
+ * specification defines a way to use ALPN during renegotiations.
+ * SSL_ENABLE_ALPN is currently enabled by default, but this may change in
+ * future versions.
  */
 #define SSL_ENABLE_ALPN 26
 
@@ -283,10 +278,10 @@ SSL_IMPORT SECStatus SSL_OptionSetDefault(PRInt32 option, PRIntn val);
 SSL_IMPORT SECStatus SSL_OptionGetDefault(PRInt32 option, PRIntn *val);
 SSL_IMPORT SECStatus SSL_CertDBHandleSet(PRFileDesc *fd, CERTCertDBHandle *dbHandle);
 
-/* SSLNextProtoCallback is called during the handshake for the client, when a
- * Next Protocol Negotiation (NPN) extension has been received from the server.
- * |protos| and |protosLen| define a buffer which contains the server's
- * advertisement. This data is guaranteed to be well formed per the NPN spec.
+/* SSLNextProtoCallback is called during the handshake for the server, when an
+ * Application-Layer Protocol Negotiation (ALPN) extension has been received
+ * from the client. |protos| and |protosLen| define a buffer which contains the
+ * client's advertisement.
  * |protoOut| is a buffer provided by the caller, of length 255 (the maximum
  * allowed by the protocol). On successful return, the protocol to be announced
  * to the server will be in |protoOut| and its length in |*protoOutLen|.
@@ -302,27 +297,24 @@ typedef SECStatus(PR_CALLBACK *SSLNextProtoCallback)(
     unsigned int *protoOutLen,
     unsigned int protoMaxOut);
 
-/* SSL_SetNextProtoCallback sets a callback function to handle Next Protocol
- * Negotiation. It causes a client to advertise NPN. */
+/* SSL_SetNextProtoCallback sets a callback function to handle ALPN Negotiation.
+ * It causes a client to advertise ALPN. */
 SSL_IMPORT SECStatus SSL_SetNextProtoCallback(PRFileDesc *fd,
                                               SSLNextProtoCallback callback,
                                               void *arg);
 
 /* SSL_SetNextProtoNego can be used as an alternative to
- * SSL_SetNextProtoCallback. It also causes a client to advertise NPN and
- * installs a default callback function which selects the first supported
- * protocol in server-preference order. If no matching protocol is found it
- * selects the first supported protocol.
+ * SSL_SetNextProtoCallback.
  *
- * Using this function also allows the client to transparently support ALPN.
+ * Using this function allows client and server to transparently support ALPN.
  * The same set of protocols will be advertised via ALPN and, if the server
  * uses ALPN to select a protocol, SSL_GetNextProto will return
  * SSL_NEXT_PROTO_SELECTED as the state.
  *
- * Since NPN uses 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.
+ * 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". */
diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index 7346245a6..83040a8e9 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -7321,10 +7321,6 @@ ssl3_SendClientSecondRound(sslSocket *ss)
      * certificate to an attacker that does not have a valid cert for the
      * domain we are connecting to.
      *
-     * XXX: We should do the same for the NPN extension, but for that we
-     * need an option to give the application the ability to leak the NPN
-     * information to get better performance.
-     *
      * During the initial handshake on a connection, we never send/receive
      * application data until we have authenticated the server's certificate;
      * i.e. we have fully authenticated the handshake before using the cipher
@@ -7398,14 +7394,6 @@ ssl3_SendClientSecondRound(sslSocket *ss)
     ss->enoughFirstHsDone = PR_TRUE;
 
     if (!ss->firstHsDone) {
-        /* XXX: If the server's certificate hasn't been authenticated by this
-         * point, then we may be leaking this NPN message to an attacker.
-         */
-        rv = ssl3_SendNextProto(ss);
-        if (rv != SECSuccess) {
-            goto loser; /* err code was set. */
-        }
-
         if (ss->opt.enableFalseStart) {
             if (!ss->ssl3.hs.authCertificatePending) {
                 /* When we fix bug 589047, we will need to know whether we are
diff --git a/lib/ssl/ssl3ext.c b/lib/ssl/ssl3ext.c
index 28c2611b1..782425e57 100644
--- a/lib/ssl/ssl3ext.c
+++ b/lib/ssl/ssl3ext.c
@@ -39,7 +39,6 @@ static const ssl3ExtensionHandler clientHelloHandlers[] = {
     { ssl_ec_point_formats_xtn, &ssl3_HandleSupportedPointFormatsXtn },
     { ssl_session_ticket_xtn, &ssl3_ServerHandleSessionTicketXtn },
     { ssl_renegotiation_info_xtn, &ssl3_HandleRenegotiationInfoXtn },
-    { ssl_next_proto_nego_xtn, &ssl3_ServerHandleNextProtoNegoXtn },
     { ssl_app_layer_protocol_xtn, &ssl3_ServerHandleAppProtoXtn },
     { ssl_use_srtp_xtn, &ssl3_ServerHandleUseSRTPXtn },
     { ssl_cert_status_xtn, &ssl3_ServerHandleStatusRequestXtn },
@@ -61,7 +60,6 @@ static const ssl3ExtensionHandler serverHelloHandlersTLS[] = {
     /* TODO: add a handler for ssl_ec_point_formats_xtn */
     { ssl_session_ticket_xtn, &ssl3_ClientHandleSessionTicketXtn },
     { ssl_renegotiation_info_xtn, &ssl3_HandleRenegotiationInfoXtn },
-    { ssl_next_proto_nego_xtn, &ssl3_ClientHandleNextProtoNegoXtn },
     { ssl_app_layer_protocol_xtn, &ssl3_ClientHandleAppProtoXtn },
     { ssl_use_srtp_xtn, &ssl3_ClientHandleUseSRTPXtn },
     { ssl_cert_status_xtn, &ssl3_ClientHandleStatusRequestXtn },
@@ -122,7 +120,6 @@ static const sslExtensionBuilder clientHelloSendersTLS[] =
       { ssl_supported_groups_xtn, &ssl_SendSupportedGroupsXtn },
       { ssl_ec_point_formats_xtn, &ssl3_SendSupportedPointFormatsXtn },
       { ssl_session_ticket_xtn, &ssl3_ClientSendSessionTicketXtn },
-      { ssl_next_proto_nego_xtn, &ssl3_ClientSendNextProtoNegoXtn },
       { ssl_app_layer_protocol_xtn, &ssl3_ClientSendAppProtoXtn },
       { ssl_use_srtp_xtn, &ssl3_ClientSendUseSRTPXtn },
       { ssl_cert_status_xtn, &ssl3_ClientSendStatusRequestXtn },
@@ -183,7 +180,6 @@ static const struct {
     { ssl_tls13_psk_key_exchange_modes_xtn, ssl_ext_native_only },
     { ssl_tls13_ticket_early_data_info_xtn, ssl_ext_native_only },
     { ssl_tls13_certificate_authorities_xtn, ssl_ext_native },
-    { ssl_next_proto_nego_xtn, ssl_ext_none },
     { ssl_renegotiation_info_xtn, ssl_ext_native }
 };
 
diff --git a/lib/ssl/ssl3exthandle.c b/lib/ssl/ssl3exthandle.c
index e6388945e..d08e5e435 100644
--- a/lib/ssl/ssl3exthandle.c
+++ b/lib/ssl/ssl3exthandle.c
@@ -242,33 +242,11 @@ ssl_AlpnTagAllowed(const sslSocket *ss, const SECItem *tag)
     return PR_FALSE;
 }
 
-/* handle an incoming Next Protocol Negotiation extension. */
-SECStatus
-ssl3_ServerHandleNextProtoNegoXtn(const sslSocket *ss, TLSExtensionData *xtnData,
-                                  SECItem *data)
-{
-    PORT_Assert(ss->version < SSL_LIBRARY_VERSION_TLS_1_3);
-
-    if (ss->firstHsDone || data->len != 0) {
-        /* Clients MUST send an empty NPN extension, if any. */
-        PORT_SetError(SSL_ERROR_NEXT_PROTOCOL_DATA_INVALID);
-        return SECFailure;
-    }
-
-    xtnData->negotiated[xtnData->numNegotiated++] = ssl_next_proto_nego_xtn;
-
-    /* TODO: server side NPN support would require calling
-     * ssl3_RegisterServerHelloExtensionSender here in order to echo the
-     * extension back to the client. */
-
-    return SECSuccess;
-}
-
-/* ssl3_ValidateNextProtoNego checks that the given block of data is valid: none
+/* ssl3_ValidateAppProtocol checks that the given block of data is valid: none
  * of the lengths may be 0 and the sum of the lengths must equal the length of
  * the block. */
 SECStatus
-ssl3_ValidateNextProtoNego(const unsigned char *data, unsigned int length)
+ssl3_ValidateAppProtocol(const unsigned char *data, unsigned int length)
 {
     unsigned int offset = 0;
 
@@ -286,7 +264,7 @@ ssl3_ValidateNextProtoNego(const unsigned char *data, unsigned int length)
     return SECSuccess;
 }
 
-/* protocol selection handler for ALPN (server side) and NPN (client side) */
+/* Protocol selection handler for ALPN. */
 static SECStatus
 ssl3_SelectAppProtocol(const sslSocket *ss, TLSExtensionData *xtnData,
                        PRUint16 extension, SECItem *data)
@@ -295,7 +273,7 @@ ssl3_SelectAppProtocol(const sslSocket *ss, TLSExtensionData *xtnData,
     unsigned char resultBuffer[255];
     SECItem result = { siBuffer, resultBuffer, 0 };
 
-    rv = ssl3_ValidateNextProtoNego(data->data, data->len);
+    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);
@@ -303,11 +281,13 @@ ssl3_SelectAppProtocol(const sslSocket *ss, TLSExtensionData *xtnData,
     }
 
     PORT_Assert(ss->nextProtoCallback);
-    /* For ALPN, the cipher suite isn't selected yet.  Note that extensions
+    /* The cipher suite isn't 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_all & ~ssl_preinfo_cipher_suite));
+    /* The callback has to make sure that either rv != SECSuccess or that result
+     * is not set if there is no common protocol. */
     rv = ss->nextProtoCallback(ss->nextProtoArg, ss->fd, data->data, data->len,
                                result.data, &result.len, sizeof(resultBuffer));
     if (rv != SECSuccess) {
@@ -320,21 +300,20 @@ ssl3_SelectAppProtocol(const sslSocket *ss, TLSExtensionData *xtnData,
      * stack. */
     if (result.len > sizeof(resultBuffer)) {
         PORT_SetError(SEC_ERROR_OUTPUT_LEN);
-        /* TODO: crash */
+        PORT_Assert(PR_FALSE);
         return SECFailure;
     }
 
     SECITEM_FreeItem(&xtnData->nextProto, PR_FALSE);
 
-    if (extension == ssl_app_layer_protocol_xtn &&
-        xtnData->nextProtoState != SSL_NEXT_PROTO_NEGOTIATED) {
-        /* The callback might say OK, but then it picks a default value - one
-         * that was not listed.  That's OK for NPN, but not ALPN. */
+    if (result.len < 1 || !result.data) {
+        /* Check that we actually got a result. */
         ssl3_ExtSendAlert(ss, alert_fatal, no_application_protocol);
         PORT_SetError(SSL_ERROR_NEXT_PROTOCOL_NO_PROTOCOL);
         return SECFailure;
     }
 
+    xtnData->nextProtoState = SSL_NEXT_PROTO_NEGOTIATED;
     xtnData->negotiated[xtnData->numNegotiated++] = extension;
     return SECITEM_CopyItem(NULL, &xtnData->nextProto, &result);
 }
@@ -356,7 +335,7 @@ ssl3_ServerHandleAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData,
         return SECFailure;
     }
 
-    /* Unlike NPN, ALPN has extra redundant length information so that
+    /* ALPN has extra redundant length information so that
      * the extension is the same in both ClientHello and ServerHello. */
     rv = ssl3_ExtConsumeHandshakeNumber(ss, &count, 2, &data->data, &data->len);
     if (rv != SECSuccess || count != data->len) {
@@ -389,39 +368,6 @@ ssl3_ServerHandleAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData,
 }
 
 SECStatus
-ssl3_ClientHandleNextProtoNegoXtn(const sslSocket *ss, TLSExtensionData *xtnData,
-                                  SECItem *data)
-{
-    PORT_Assert(ss->version < SSL_LIBRARY_VERSION_TLS_1_3);
-    PORT_Assert(!ss->firstHsDone);
-
-    if (ssl3_ExtensionNegotiated(ss, ssl_app_layer_protocol_xtn)) {
-        /* If the server negotiated ALPN then it has already told us what
-         * protocol to use, so it doesn't make sense for us to try to negotiate
-         * a different one by sending the NPN handshake message. However, if
-         * we've negotiated NPN then we're required to send the NPN handshake
-         * message. Thus, these two extensions cannot both be negotiated on the
-         * same connection. */
-        ssl3_ExtSendAlert(ss, alert_fatal, illegal_parameter);
-        PORT_SetError(SSL_ERROR_BAD_SERVER);
-        return SECFailure;
-    }
-
-    /* We should only get this call if we sent the extension, so
-     * ss->nextProtoCallback needs to be non-NULL.  However, it is possible
-     * that an application erroneously cleared the callback between the time
-     * we sent the ClientHello and now. */
-    if (!ss->nextProtoCallback) {
-        PORT_Assert(0);
-        ssl3_ExtSendAlert(ss, alert_fatal, internal_error);
-        PORT_SetError(SSL_ERROR_NEXT_PROTOCOL_NO_CALLBACK);
-        return SECFailure;
-    }
-
-    return ssl3_SelectAppProtocol(ss, xtnData, ssl_next_proto_nego_xtn, data);
-}
-
-SECStatus
 ssl3_ClientHandleAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData,
                              SECItem *data)
 {
@@ -475,19 +421,6 @@ ssl3_ClientHandleAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData,
 }
 
 SECStatus
-ssl3_ClientSendNextProtoNegoXtn(const sslSocket *ss, TLSExtensionData *xtnData,
-                                sslBuffer *buf, PRBool *added)
-{
-    /* Renegotiations do not send this extension. */
-    if (!ss->opt.enableNPN || !ss->nextProtoCallback || ss->firstHsDone) {
-        return SECSuccess;
-    }
-
-    *added = PR_TRUE;
-    return SECSuccess;
-}
-
-SECStatus
 ssl3_ClientSendAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData,
                            sslBuffer *buf, PRBool *added)
 {
@@ -499,35 +432,15 @@ ssl3_ClientSendAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData,
         return SECSuccess;
     }
 
-    /* NPN requires 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. */
-
     if (len > 0) {
         /* Each protocol string is prefixed with a single byte length. */
-        unsigned int i;
-
         rv = sslBuffer_AppendNumber(buf, len, 2);
         if (rv != SECSuccess) {
             return SECFailure;
         }
-
-        i = ss->opt.nextProtoNego.data[0] + 1;
-        if (i <= len) {
-            rv = sslBuffer_Append(buf, &ss->opt.nextProtoNego.data[i], len - i);
-            if (rv != SECSuccess) {
-                return SECFailure;
-            }
-            rv = sslBuffer_Append(buf, ss->opt.nextProtoNego.data, i);
-            if (rv != SECSuccess) {
-                return SECFailure;
-            }
-        } else {
-            /* This seems to be invalid data so we'll send as-is. */
-            rv = sslBuffer_Append(buf, ss->opt.nextProtoNego.data, len);
-            if (rv != SECSuccess) {
-                return SECFailure;
-            }
+        rv = sslBuffer_Append(buf, ss->opt.nextProtoNego.data, len);
+        if (rv != SECSuccess) {
+            return SECFailure;
         }
     }
 
diff --git a/lib/ssl/sslimpl.h b/lib/ssl/sslimpl.h
index 3c01ab436..f992dd162 100644
--- a/lib/ssl/sslimpl.h
+++ b/lib/ssl/sslimpl.h
@@ -251,7 +251,6 @@ typedef struct sslOptionsStr {
     unsigned int enableFalseStart : 1;
     unsigned int cbcRandomIV : 1;
     unsigned int enableOCSPStapling : 1;
-    unsigned int enableNPN : 1;
     unsigned int enableALPN : 1;
     unsigned int reuseServerECDHEKey : 1;
     unsigned int enableFallbackSCSV : 1;
@@ -443,7 +442,7 @@ struct sslSessionIDStr {
             */
             SECItem signedCertTimestamps;
 
-            /* The NPN/ALPN value negotiated in the original connection.
+            /* The ALPN value negotiated in the original connection.
              * Used for TLS 1.3. */
             SECItem alpnSelection;
 
@@ -1547,8 +1546,8 @@ SECStatus ssl_GetSelfEncryptKeys(sslSocket *ss, unsigned char *keyName,
                                  PK11SymKey **encKey, PK11SymKey **macKey);
 void ssl_ResetSelfEncryptKeys();
 
-extern SECStatus ssl3_ValidateNextProtoNego(const unsigned char *data,
-                                            unsigned int length);
+extern SECStatus ssl3_ValidateAppProtocol(const unsigned char *data,
+                                          unsigned int length);
 
 /* Construct a new NSPR socket for the app to use */
 extern PRFileDesc *ssl_NewPRSocket(sslSocket *ss, PRFileDesc *fd);
diff --git a/lib/ssl/sslsock.c b/lib/ssl/sslsock.c
index f5d829d4e..5b98a3de4 100644
--- a/lib/ssl/sslsock.c
+++ b/lib/ssl/sslsock.c
@@ -72,7 +72,6 @@ static sslOptions ssl_defaults = {
     .enableFalseStart = PR_FALSE,
     .cbcRandomIV = PR_TRUE,
     .enableOCSPStapling = PR_FALSE,
-    .enableNPN = PR_FALSE,
     .enableALPN = PR_TRUE,
     .reuseServerECDHEKey = PR_TRUE,
     .enableFallbackSCSV = PR_FALSE,
@@ -919,7 +918,7 @@ SSL_OptionGet(PRFileDesc *fd, PRInt32 which, PRIntn *pVal)
             val = ss->opt.enableOCSPStapling;
             break;
         case SSL_ENABLE_NPN:
-            val = ss->opt.enableNPN;
+            val = PR_FALSE;
             break;
         case SSL_ENABLE_ALPN:
             val = ss->opt.enableALPN;
@@ -1045,7 +1044,7 @@ SSL_OptionGetDefault(PRInt32 which, PRIntn *pVal)
             val = ssl_defaults.enableOCSPStapling;
             break;
         case SSL_ENABLE_NPN:
-            val = ssl_defaults.enableNPN;
+            val = PR_FALSE;
             break;
         case SSL_ENABLE_ALPN:
             val = ssl_defaults.enableALPN;
@@ -1910,10 +1909,7 @@ DTLS_ImportFD(PRFileDesc *model, PRFileDesc *fd)
 }
 
 /* SSL_SetNextProtoCallback is used to select an application protocol
- * for ALPN and NPN.  For ALPN, this runs on the server; for NPN it
- * runs on the client. */
-/* Note: The ALPN version doesn't allow for the use of a default, setting a
- * status of SSL_NEXT_PROTO_NO_OVERLAP is treated as a failure. */
+ * for ALPN. */
 SECStatus
 SSL_SetNextProtoCallback(PRFileDesc *fd, SSLNextProtoCallback callback,
                          void *arg)
@@ -1934,7 +1930,7 @@ SSL_SetNextProtoCallback(PRFileDesc *fd, SSLNextProtoCallback callback,
     return SECSuccess;
 }
 
-/* ssl_NextProtoNegoCallback is set as an ALPN/NPN callback when
+/* ssl_NextProtoNegoCallback is set as an ALPN callback when
  * SSL_SetNextProtoNego is used.
  */
 static SECStatus
@@ -1944,7 +1940,6 @@ ssl_NextProtoNegoCallback(void *arg, PRFileDesc *fd,
                           unsigned int protoMaxLen)
 {
     unsigned int i, j;
-    const unsigned char *result;
     sslSocket *ss = ssl_FindSocket(fd);
 
     if (!ss) {
@@ -1952,37 +1947,29 @@ ssl_NextProtoNegoCallback(void *arg, PRFileDesc *fd,
                  SSL_GETPID(), fd));
         return SECFailure;
     }
+    PORT_Assert(protoMaxLen <= 255);
+    if (protoMaxLen > 255) {
+        PORT_SetError(SEC_ERROR_OUTPUT_LEN);
+        return SECFailure;
+    }
 
-    /* For each protocol in server preference, see if we support it. */
-    for (i = 0; i < protos_len;) {
-        for (j = 0; j < ss->opt.nextProtoNego.len;) {
+    /* For each protocol in client preference, see if we support it. */
+    for (j = 0; j < ss->opt.nextProtoNego.len;) {
+        for (i = 0; i < protos_len;) {
             if (protos[i] == ss->opt.nextProtoNego.data[j] &&
                 PORT_Memcmp(&protos[i + 1], &ss->opt.nextProtoNego.data[j + 1],
                             protos[i]) == 0) {
                 /* We found a match. */
-                ss->xtnData.nextProtoState = SSL_NEXT_PROTO_NEGOTIATED;
-                result = &protos[i];
-                goto found;
+                const unsigned char *result = &protos[i];
+                memcpy(protoOut, result + 1, result[0]);
+                *protoOutLen = result[0];
+                return SECSuccess;
             }
-            j += 1 + (unsigned int)ss->opt.nextProtoNego.data[j];
+            i += 1 + (unsigned int)protos[i];
         }
-        i += 1 + (unsigned int)protos[i];
+        j += 1 + (unsigned int)ss->opt.nextProtoNego.data[j];
     }
 
-    /* The other side supports the extension, and either doesn't have any
-     * protocols configured, or none of its options match ours. In this case we
-     * request our favoured protocol. */
-    /* This will be treated as a failure for ALPN. */
-    ss->xtnData.nextProtoState = SSL_NEXT_PROTO_NO_OVERLAP;
-    result = ss->opt.nextProtoNego.data;
-
-found:
-    if (protoMaxLen < result[0]) {
-        PORT_SetError(SEC_ERROR_OUTPUT_LEN);
-        return SECFailure;
-    }
-    memcpy(protoOut, result + 1, result[0]);
-    *protoOutLen = result[0];
     return SECSuccess;
 }
 
@@ -1991,8 +1978,6 @@ SSL_SetNextProtoNego(PRFileDesc *fd, const unsigned char *data,
                      unsigned int length)
 {
     sslSocket *ss;
-    SECStatus rv;
-    SECItem dataItem = { siBuffer, (unsigned char *)data, length };
 
     ss = ssl_FindSocket(fd);
     if (!ss) {
@@ -2001,17 +1986,22 @@ SSL_SetNextProtoNego(PRFileDesc *fd, const unsigned char *data,
         return SECFailure;
     }
 
-    if (ssl3_ValidateNextProtoNego(data, length) != SECSuccess)
+    if (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);
-    rv = SECITEM_CopyItem(NULL, &ss->opt.nextProtoNego, &dataItem);
+    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);
 
-    if (rv != SECSuccess)
-        return rv;
-
     return SSL_SetNextProtoCallback(fd, ssl_NextProtoNegoCallback, NULL);
 }
 
diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c
index b12cbb17b..3592f30f7 100644
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -3584,7 +3584,7 @@ tls13_HandleEncryptedExtensions(sslSocket *ss, PRUint8 *b, PRUint32 length)
 {
     SECStatus rv;
     PRUint32 innerLength;
-    SECItem oldNpn = { siBuffer, NULL, 0 };
+    SECItem oldAlpn = { siBuffer, NULL, 0 };
 
     PORT_Assert(ss->opt.noLocks || ssl_HaveRecvBufLock(ss));
     PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
@@ -3608,11 +3608,11 @@ tls13_HandleEncryptedExtensions(sslSocket *ss, PRUint8 *b, PRUint32 length)
         return SECFailure;
     }
 
-    /* If we are doing 0-RTT, then we already have an NPN value. Stash
+    /* If we are doing 0-RTT, then we already have an ALPN value. Stash
      * it for comparison. */
     if (ss->ssl3.hs.zeroRttState == ssl_0rtt_sent &&
         ss->xtnData.nextProtoState == SSL_NEXT_PROTO_EARLY_VALUE) {
-        oldNpn = ss->xtnData.nextProto;
+        oldAlpn = ss->xtnData.nextProto;
         ss->xtnData.nextProto.data = NULL;
         ss->xtnData.nextProtoState = SSL_NEXT_PROTO_NO_SUPPORT;
     }
@@ -3632,8 +3632,8 @@ tls13_HandleEncryptedExtensions(sslSocket *ss, PRUint8 *b, PRUint32 length)
         ss->ssl3.hs.zeroRttState = ssl_0rtt_accepted;
 
         /* Check that the server negotiated the same ALPN (if any). */
-        if (SECITEM_CompareItem(&oldNpn, &ss->xtnData.nextProto)) {
-            SECITEM_FreeItem(&oldNpn, PR_FALSE);
+        if (SECITEM_CompareItem(&oldAlpn, &ss->xtnData.nextProto)) {
+            SECITEM_FreeItem(&oldAlpn, PR_FALSE);
             FATAL_ERROR(ss, SSL_ERROR_NEXT_PROTOCOL_DATA_INVALID,
                         illegal_parameter);
             return SECFailure;
@@ -3655,7 +3655,7 @@ tls13_HandleEncryptedExtensions(sslSocket *ss, PRUint8 *b, PRUint32 length)
                      ss->ssl3.hs.zeroRttState == ssl_0rtt_ignored));
     }
 
-    SECITEM_FreeItem(&oldNpn, PR_FALSE);
+    SECITEM_FreeItem(&oldAlpn, PR_FALSE);
     if (ss->ssl3.hs.kea_def->authKeyType == ssl_auth_psk) {
         TLS13_SET_HS_STATE(ss, wait_finished);
     } else {