From 76c2efc0e902d425ca8bc56acdf0b8283cd25117 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 31 Aug 2015 14:24:29 -0400 Subject: [PATCH] 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 --- crypto/err/ssl.errordata | 1 + include/openssl/ssl.h | 1 + ssl/ssl_lib.c | 4 ++-- ssl/t1_lib.c | 14 ++++++++++++ ssl/test/runner/common.go | 9 +++++--- ssl/test/runner/handshake_messages.go | 20 ++++++++++++++++- ssl/test/runner/handshake_server.go | 4 +++- ssl/test/runner/runner.go | 32 +++++++++++++++++++++++++++ 8 files changed, 78 insertions(+), 7 deletions(-) diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata index 7825cce1..0b30b13e 100644 --- a/crypto/err/ssl.errordata +++ b/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 diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index e9081084..332a62ff 100644 --- a/include/openssl/ssl.h +++ b/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 diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index bc01568c..0aebdb23 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 2eeffabd..40b97521 100644 --- a/ssl/t1_lib.c +++ b/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; diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index f5786310..273a75c4 100644 --- a/ssl/test/runner/common.go +++ b/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() { diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go index f5303a65..79265335 100644 --- a/ssl/test/runner/handshake_messages.go +++ b/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 diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index 34828aef..d23bf713 100644 --- a/ssl/test/runner/handshake_server.go +++ b/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 diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index d0859134..adcb4058 100644 --- a/ssl/test/runner/runner.go +++ b/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,