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,