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>

crypto/err/ssl.errordata

@@ -85,6 +85,7 @@ SSL,175,MISSING_TMP_DH_KEY
 SSL,176,MISSING_TMP_ECDH_KEY
 SSL,177,MIXED_SPECIAL_OPERATOR_WITH_GROUPS
 SSL,178,MTU_TOO_SMALL
+SSL,286,NEGOTIATED_BOTH_NPN_AND_ALPN
 SSL,179,NESTED_GROUP
 SSL,180,NO_CERTIFICATES_RETURNED
 SSL,181,NO_CERTIFICATE_ASSIGNED

include/openssl/ssl.h

@@ -3017,6 +3017,7 @@ OPENSSL_EXPORT const char *SSLeay_version(int unused);
 #define SSL_R_MISSING_EXTENSION 283
 #define SSL_R_CUSTOM_EXTENSION_CONTENTS_TOO_LARGE 284
 #define SSL_R_CUSTOM_EXTENSION_ERROR 285
+#define SSL_R_NEGOTIATED_BOTH_NPN_AND_ALPN 286
 #define SSL_R_SSLV3_ALERT_CLOSE_NOTIFY 1000
 #define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010
 #define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020

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. */

ssl/t1_lib.c

@@ -1410,6 +1410,13 @@ static int ext_npn_parse_serverhello(SSL *ssl, uint8_t *out_alert,
   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 @@ static int ext_alpn_parse_serverhello(SSL *ssl, uint8_t *out_alert,
   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;

ssl/test/runner/common.go

@@ -544,9 +544,8 @@ type ProtocolBugs struct {
 	// 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 @@ type ProtocolBugs struct {
 	// 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() {

ssl/test/runner/handshake_messages.go

@@ -650,6 +650,7 @@ type serverHelloMsg struct {
 	srtpMasterKeyIdentifier string
 	sctList                 []byte
 	customExtension         string
+	npnLast                 bool
 }
 
 func (m *serverHelloMsg) marshal() []byte {
@@ -742,7 +743,7 @@ func (m *serverHelloMsg) marshal() []byte {
 		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 @@ func (m *serverHelloMsg) marshal() []byte {
 		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
 

ssl/test/runner/handshake_server.go

@@ -213,6 +213,7 @@ func (hs *serverHandshakeState) readClientHello() (isResume bool, err error) {
 	hs.hello = &serverHelloMsg{
 		isDTLS:          c.isDTLS,
 		customExtension: config.Bugs.CustomExtension,
+		npnLast:         config.Bugs.SwapNPNAndALPN,
 	}
 
 	supportedCurve := false
@@ -297,7 +298,8 @@ Curves:
 			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

ssl/test/runner/runner.go

@@ -3106,6 +3106,38 @@ func addExtensionTests() {
 		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,