Forbid a server from negotiating both ALPN and NPN.

If the two extensions select different next protocols (quite possible since one
is server-selected and the other is client-selected), things will break. This
matches the behavior of NSS (Firefox) and Go.

Change-Id: Ie1da97bf062b91a370c85c12bc61423220a22f36
Reviewed-on: https://boringssl-review.googlesource.com/5780
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index bc01568..0aebdb2 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -155,9 +155,9 @@
 #include "../crypto/internal.h"
 
 
-/* |SSL_R_UKNOWN_PROTOCOL| is no longer emitted, but continue to define it
+/* |SSL_R_UNKNOWN_PROTOCOL| is no longer emitted, but continue to define it
  * to avoid downstream churn. */
-OPENSSL_DECLARE_ERROR_REASON(SSL, SSL_R_UKNOWN_PROTOCOL)
+OPENSSL_DECLARE_ERROR_REASON(SSL, UNKNOWN_PROTOCOL)
 
 /* Some error codes are special. Ensure the make_errors.go script never
  * regresses this. */
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 2eeffab..40b9752 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -1410,6 +1410,13 @@
   assert(!SSL_IS_DTLS(ssl));
   assert(ssl->ctx->next_proto_select_cb != NULL);
 
+  if (ssl->s3->alpn_selected != NULL) {
+    /* NPN and ALPN may not be negotiated in the same connection. */
+    *out_alert = SSL_AD_ILLEGAL_PARAMETER;
+    OPENSSL_PUT_ERROR(SSL, SSL_R_NEGOTIATED_BOTH_NPN_AND_ALPN);
+    return 0;
+  }
+
   const uint8_t *const orig_contents = CBS_data(contents);
   const size_t orig_len = CBS_len(contents);
 
@@ -1585,6 +1592,13 @@
   assert(!ssl->s3->initial_handshake_complete);
   assert(ssl->alpn_client_proto_list != NULL);
 
+  if (ssl->s3->next_proto_neg_seen) {
+    /* NPN and ALPN may not be negotiated in the same connection. */
+    *out_alert = SSL_AD_ILLEGAL_PARAMETER;
+    OPENSSL_PUT_ERROR(SSL, SSL_R_NEGOTIATED_BOTH_NPN_AND_ALPN);
+    return 0;
+  }
+
   /* The extension data consists of a ProtocolNameList which must have
    * exactly one ProtocolName. Each of these is length-prefixed. */
   CBS protocol_name_list, protocol_name;
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index f578631..273a75c 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -544,9 +544,8 @@
 	// must specify in the server_name extension.
 	ExpectServerName string
 
-	// SwapNPNAndALPN switches the relative order between NPN and
-	// ALPN on the server. This is to test that server preference
-	// of ALPN works regardless of their relative order.
+	// SwapNPNAndALPN switches the relative order between NPN and ALPN in
+	// both ClientHello and ServerHello.
 	SwapNPNAndALPN bool
 
 	// ALPNProtocol, if not nil, sets the ALPN protocol that a server will
@@ -766,6 +765,10 @@
 	// SendLargeRecords, if true, allows outgoing records to be sent
 	// arbitrarily large.
 	SendLargeRecords bool
+
+	// NegotiateALPNAndNPN, if true, causes the server to negotiate both
+	// ALPN and NPN in the same connetion.
+	NegotiateALPNAndNPN bool
 }
 
 func (c *Config) serverInit() {
diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go
index f5303a6..7926533 100644
--- a/ssl/test/runner/handshake_messages.go
+++ b/ssl/test/runner/handshake_messages.go
@@ -650,6 +650,7 @@
 	srtpMasterKeyIdentifier string
 	sctList                 []byte
 	customExtension         string
+	npnLast                 bool
 }
 
 func (m *serverHelloMsg) marshal() []byte {
@@ -742,7 +743,7 @@
 		z[1] = 0xff
 		z = z[4:]
 	}
-	if m.nextProtoNeg {
+	if m.nextProtoNeg && !m.npnLast {
 		z[0] = byte(extensionNextProtoNeg >> 8)
 		z[1] = byte(extensionNextProtoNeg & 0xff)
 		z[2] = byte(nextProtoLen >> 8)
@@ -841,6 +842,23 @@
 		copy(z[4:], []byte(m.customExtension))
 		z = z[4+l:]
 	}
+	if m.nextProtoNeg && m.npnLast {
+		z[0] = byte(extensionNextProtoNeg >> 8)
+		z[1] = byte(extensionNextProtoNeg & 0xff)
+		z[2] = byte(nextProtoLen >> 8)
+		z[3] = byte(nextProtoLen)
+		z = z[4:]
+
+		for _, v := range m.nextProtos {
+			l := len(v)
+			if l > 255 {
+				l = 255
+			}
+			z[0] = byte(l)
+			copy(z[1:], []byte(v[0:l]))
+			z = z[1+l:]
+		}
+	}
 
 	m.raw = x
 
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index 34828ae..d23bf71 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -213,6 +213,7 @@
 	hs.hello = &serverHelloMsg{
 		isDTLS:          c.isDTLS,
 		customExtension: config.Bugs.CustomExtension,
+		npnLast:         config.Bugs.SwapNPNAndALPN,
 	}
 
 	supportedCurve := false
@@ -297,7 +298,8 @@
 			c.clientProtocol = selectedProto
 			c.usedALPN = true
 		}
-	} else {
+	}
+	if len(hs.clientHello.alpnProtocols) == 0 || c.config.Bugs.NegotiateALPNAndNPN {
 		// Although sending an empty NPN extension is reasonable, Firefox has
 		// had a bug around this. Best to send nothing at all if
 		// config.NextProtos is empty. See
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index d085913..adcb405 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -3106,6 +3106,38 @@
 		shouldFail:    true,
 		expectedError: ":PARSE_TLSEXT:",
 	})
+	// Test that negotiating both NPN and ALPN is forbidden.
+	testCases = append(testCases, testCase{
+		name: "NegotiateALPNAndNPN",
+		config: Config{
+			NextProtos: []string{"foo", "bar", "baz"},
+			Bugs: ProtocolBugs{
+				NegotiateALPNAndNPN: true,
+			},
+		},
+		flags: []string{
+			"-advertise-alpn", "\x03foo",
+			"-select-next-proto", "foo",
+		},
+		shouldFail:    true,
+		expectedError: ":NEGOTIATED_BOTH_NPN_AND_ALPN:",
+	})
+	testCases = append(testCases, testCase{
+		name: "NegotiateALPNAndNPN-Swapped",
+		config: Config{
+			NextProtos: []string{"foo", "bar", "baz"},
+			Bugs: ProtocolBugs{
+				NegotiateALPNAndNPN: true,
+				SwapNPNAndALPN:      true,
+			},
+		},
+		flags: []string{
+			"-advertise-alpn", "\x03foo",
+			"-select-next-proto", "foo",
+		},
+		shouldFail:    true,
+		expectedError: ":NEGOTIATED_BOTH_NPN_AND_ALPN:",
+	})
 	// Resume with a corrupt ticket.
 	testCases = append(testCases, testCase{
 		testType: serverTest,