From eb6681e7a59821a477e4ee07908af5c27d0ac4a4 Mon Sep 17 00:00:00 2001
From: Martin Thomson <martin.thomson@gmail.com>
Date: Fri, 24 Feb 2017 13:40:31 +1100
Subject: [PATCH] Bug 1339418 - Reject ALPN values that weren't advertised,
 r=ttaubert

--HG--
extra : rebase_source : 281449f20c3dae18ae9bd2b2068bb80c16aafc6e
extra : amend_source : 68c6ed7de4242e56d718c3eb71fd851b269ac099
---
 gtests/nss_bogo_shim/config.json           |  2 +-
 gtests/ssl_gtest/ssl_extension_unittest.cc |  8 +++++++
 lib/ssl/ssl3exthandle.c                    | 27 ++++++++++++++++++++++
 lib/ssl/sslimpl.h                          |  2 ++
 lib/ssl/tls13con.c                         | 23 +-----------------
 5 files changed, 39 insertions(+), 23 deletions(-)

diff --git a/gtests/nss_bogo_shim/config.json b/gtests/nss_bogo_shim/config.json
index 7a52888189..f6743dde64 100644
--- a/gtests/nss_bogo_shim/config.json
+++ b/gtests/nss_bogo_shim/config.json
@@ -55,7 +55,7 @@
         "UnsolicitedServerNameAck-TLS1*":"Boring wants us to fail with an unexpected_extension alert, we simply ignore ssl_server_name_xtn.",
         "RequireAnyClientCertificate-TLS1*":"Bug 1339387",
         "SendExtensionOnClientCertificate-TLS13":"Bug 1339392",
-        "ALPNClient-Mismatch-TLS1*":"Bug 1339418"
+	"ALPNClient-Mismatch-TLS13": "NSS sends alerts in response to errors in protected handshake messages in the clear"
     },
     "ErrorMap" : {
         ":HANDSHAKE_FAILURE_ON_CLIENT_HELLO:":"SSL_ERROR_NO_CYPHER_OVERLAP",
diff --git a/gtests/ssl_gtest/ssl_extension_unittest.cc b/gtests/ssl_gtest/ssl_extension_unittest.cc
index 293878d76d..3a912848e2 100644
--- a/gtests/ssl_gtest/ssl_extension_unittest.cc
+++ b/gtests/ssl_gtest/ssl_extension_unittest.cc
@@ -476,6 +476,14 @@ TEST_P(TlsExtensionTestPre13, AlpnReturnedBadNameLength) {
       ssl_app_layer_protocol_xtn, extension));
 }
 
+TEST_P(TlsExtensionTestPre13, AlpnReturnedUnknownName) {
+  EnableAlpn();
+  const uint8_t val[] = {0x00, 0x02, 0x01, 0x67};
+  DataBuffer extension(val, sizeof(val));
+  ServerHelloErrorTest(std::make_shared<TlsExtensionReplacer>(
+      ssl_app_layer_protocol_xtn, extension), kTlsAlertIllegalParameter);
+}
+
 TEST_P(TlsExtensionTestDtls, SrtpShort) {
   EnableSrtp();
   ClientHelloErrorTest(
diff --git a/lib/ssl/ssl3exthandle.c b/lib/ssl/ssl3exthandle.c
index 04c8182cc2..5704809c3f 100644
--- a/lib/ssl/ssl3exthandle.c
+++ b/lib/ssl/ssl3exthandle.c
@@ -354,6 +354,27 @@ ssl3_ParseEncryptedSessionTicket(sslSocket *ss, SECItem *data,
     return SECSuccess;
 }
 
+PRBool
+ssl_AlpnTagAllowed(const sslSocket *ss, const SECItem *tag)
+{
+    const unsigned char *data = ss->opt.nextProtoNego.data;
+    unsigned int length = ss->opt.nextProtoNego.len;
+    unsigned int offset = 0;
+
+    if (!tag->len)
+        return PR_TRUE;
+
+    while (offset < length) {
+        unsigned int taglen = (unsigned int)data[offset];
+        if ((taglen == tag->len) &&
+            !PORT_Memcmp(data + offset + 1, tag->data, tag->len))
+            return PR_TRUE;
+        offset += 1 + taglen;
+    }
+
+    return PR_FALSE;
+}
+
 /* handle an incoming Next Protocol Negotiation extension. */
 SECStatus
 ssl3_ServerHandleNextProtoNegoXtn(const sslSocket *ss, TLSExtensionData *xtnData, PRUint16 ex_type,
@@ -568,6 +589,12 @@ ssl3_ClientHandleAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData, PRU
         return SECFailure;
     }
 
+    if (!ssl_AlpnTagAllowed(ss, &protocol_name)) {
+        ssl3_ExtSendAlert(ss, alert_fatal, illegal_parameter);
+        PORT_SetError(SSL_ERROR_NEXT_PROTOCOL_DATA_INVALID);
+        return SECFailure;
+    }
+
     SECITEM_FreeItem(&xtnData->nextProto, PR_FALSE);
     xtnData->nextProtoState = SSL_NEXT_PROTO_SELECTED;
     xtnData->negotiated[xtnData->numNegotiated++] = ex_type;
diff --git a/lib/ssl/sslimpl.h b/lib/ssl/sslimpl.h
index f4d285dce6..50eb53c020 100644
--- a/lib/ssl/sslimpl.h
+++ b/lib/ssl/sslimpl.h
@@ -1856,6 +1856,8 @@ ssl3_TLSPRFWithMasterSecret(sslSocket *ss, ssl3CipherSpec *spec,
                             const unsigned char *val, unsigned int valLen,
                             unsigned char *out, unsigned int outLen);
 
+PRBool ssl_AlpnTagAllowed(const sslSocket *ss, const SECItem *tag);
+
 #ifdef TRACE
 #define SSL_TRACE(msg) ssl_Trace msg
 #else
diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c
index 25ece54467..0a5e0bb90d 100644
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -941,27 +941,6 @@ tls13_CanResume(sslSocket *ss, const sslSessionID *sid)
     return PR_TRUE;
 }
 
-static PRBool
-tls13_AlpnTagAllowed(const sslSocket *ss, const SECItem *tag)
-{
-    const unsigned char *data = ss->opt.nextProtoNego.data;
-    unsigned int length = ss->opt.nextProtoNego.len;
-    unsigned int offset = 0;
-
-    if (!tag->len)
-        return PR_TRUE;
-
-    while (offset < length) {
-        unsigned int taglen = (unsigned int)data[offset];
-        if ((taglen == tag->len) &&
-            !PORT_Memcmp(data + offset + 1, tag->data, tag->len))
-            return PR_TRUE;
-        offset += 1 + taglen;
-    }
-
-    return PR_FALSE;
-}
-
 static PRBool
 tls13_CanNegotiateZeroRtt(sslSocket *ss, const sslSessionID *sid)
 {
@@ -4296,7 +4275,7 @@ tls13_ClientAllow0Rtt(const sslSocket *ss, const sslSessionID *sid)
         return PR_FALSE;
     if ((sid->u.ssl3.locked.sessionTicket.flags & ticket_allow_early_data) == 0)
         return PR_FALSE;
-    return tls13_AlpnTagAllowed(ss, &sid->u.ssl3.alpnSelection);
+    return ssl_AlpnTagAllowed(ss, &sid->u.ssl3.alpnSelection);
 }
 
 SECStatus