diff options
author | Martin Thomson <mt@lowentropy.net> | 2021-06-10 08:39:07 +0000 |
---|---|---|
committer | Martin Thomson <mt@lowentropy.net> | 2021-06-10 08:39:07 +0000 |
commit | 25815ee1016daa1a252f32f62bfcac85af61f732 (patch) | |
tree | b54985fabf350bb885987ddf50f663c342ba4111 | |
parent | f4ba7f612305cee932cffc93fb2e8a4e0abd2658 (diff) | |
download | nss-hg-25815ee1016daa1a252f32f62bfcac85af61f732.tar.gz |
Bug 1683710 - Add a means to disable ALPN, r=bbeurdoucheNSS_3_67_BETA1
We've recently learned the value of ALPN and SNI when it comes to protecting
against cross-protocol attacks. However, some protocols don't have ALPN yet.
For servers that terminate connections for those connections, validating that
the client has not offered ALPN provides a way to protect against cross-protocol
attacks. If the cross-protocol attack uses a protocol that does include ALPN,
being able to reject those connections safely reduces exposure.
This modifies SSL_SetNextProtoNego() to accept a zero-length buffer as an
argument. Previously, this would have crashed. Now it causes the server to
reject a handshake if ALPN is offered by the client.
It was always possible to implement this by passing a function that always
returns SECFailure to SSL_SetNextProtoCallback(). This approach has the
advantage that the server generates a no_application_protocol alert, which is
not something that user-provided code can do.
Differential Revision: https://phabricator.services.mozilla.com/D110887
-rw-r--r-- | gtests/ssl_gtest/ssl_extension_unittest.cc | 21 | ||||
-rw-r--r-- | lib/ssl/ssl.h | 9 | ||||
-rw-r--r-- | lib/ssl/ssl3exthandle.c | 2 | ||||
-rw-r--r-- | lib/ssl/sslsock.c | 22 |
4 files changed, 45 insertions, 9 deletions
diff --git a/gtests/ssl_gtest/ssl_extension_unittest.cc b/gtests/ssl_gtest/ssl_extension_unittest.cc index f60697900..2e201a6e8 100644 --- a/gtests/ssl_gtest/ssl_extension_unittest.cc +++ b/gtests/ssl_gtest/ssl_extension_unittest.cc @@ -326,6 +326,27 @@ TEST_P(TlsExtensionTestGeneric, AlpnMismatch) { ClientHelloErrorTest(nullptr, kTlsAlertNoApplicationProtocol); } +TEST_P(TlsExtensionTestGeneric, AlpnDisabledServer) { + const uint8_t client_alpn[] = {0x01, 0x61}; + client_->EnableAlpn(client_alpn, sizeof(client_alpn)); + server_->EnableAlpn(nullptr, 0); + + ClientHelloErrorTest(nullptr, kTlsAlertUnsupportedExtension); +} + +TEST_P(TlsConnectGeneric, AlpnDisabled) { + server_->EnableAlpn(nullptr, 0); + Connect(); + + SSLNextProtoState state; + uint8_t buf[255] = {0}; + unsigned int buf_len = 3; + EXPECT_EQ(SECSuccess, SSL_GetNextProto(client_->ssl_fd(), &state, buf, + &buf_len, sizeof(buf))); + EXPECT_EQ(SSL_NEXT_PROTO_NO_SUPPORT, state); + EXPECT_EQ(0U, buf_len); +} + // Many of these tests fail in TLS 1.3 because the extension is encrypted, which // prevents modification of the value from the ServerHello. TEST_P(TlsExtensionTestPre13, AlpnReturnedEmptyList) { diff --git a/lib/ssl/ssl.h b/lib/ssl/ssl.h index 43a0f3228..a2c80beaf 100644 --- a/lib/ssl/ssl.h +++ b/lib/ssl/ssl.h @@ -403,7 +403,14 @@ SSL_IMPORT SECStatus SSL_SetNextProtoCallback(PRFileDesc *fd, * preferred. The other protocols should be in preference order. * * The supported protocols are specified in |data| in wire-format (8-bit - * length-prefixed). For example: "\010http/1.1\006spdy/2". */ + * length-prefixed). For example: "\010http/1.1\006spdy/2". + * + * An empty value (i.e., where |length| is 0 and |data| is any value, + * including NULL) forcibly disables ALPN. In this mode, the server will + * reject any ClientHello that includes the ALPN extension. + * + * Calling this function overrides the callback previously set by + * SSL_SetNextProtoCallback. */ SSL_IMPORT SECStatus SSL_SetNextProtoNego(PRFileDesc *fd, const unsigned char *data, unsigned int length); diff --git a/lib/ssl/ssl3exthandle.c b/lib/ssl/ssl3exthandle.c index edc79cd6b..188496e56 100644 --- a/lib/ssl/ssl3exthandle.c +++ b/lib/ssl/ssl3exthandle.c @@ -308,7 +308,7 @@ ssl3_SelectAppProtocol(const sslSocket *ss, TLSExtensionData *xtnData, if (rv != SECSuccess) { ssl3_ExtSendAlert(ss, alert_fatal, decode_error); PORT_SetError(SSL_ERROR_NEXT_PROTOCOL_DATA_INVALID); - return rv; + return SECFailure; } PORT_Assert(ss->nextProtoCallback); diff --git a/lib/ssl/sslsock.c b/lib/ssl/sslsock.c index aa89f04e3..561411811 100644 --- a/lib/ssl/sslsock.c +++ b/lib/ssl/sslsock.c @@ -2212,12 +2212,18 @@ ssl_NextProtoNegoCallback(void *arg, PRFileDesc *fd, { unsigned int i, j; sslSocket *ss = ssl_FindSocket(fd); - if (!ss) { SSL_DBG(("%d: SSL[%d]: bad socket in ssl_NextProtoNegoCallback", SSL_GETPID(), fd)); return SECFailure; } + if (ss->opt.nextProtoNego.len == 0) { + SSL_DBG(("%d: SSL[%d]: ssl_NextProtoNegoCallback ALPN disabled", + SSL_GETPID(), fd)); + SSL3_SendAlert(ss, alert_fatal, unsupported_extension); + return SECFailure; + } + PORT_Assert(protoMaxLen <= 255); if (protoMaxLen > 255) { PORT_SetError(SEC_ERROR_OUTPUT_LEN); @@ -2257,7 +2263,7 @@ SSL_SetNextProtoNego(PRFileDesc *fd, const unsigned char *data, return SECFailure; } - if (ssl3_ValidateAppProtocol(data, length) != SECSuccess) { + if (length > 0 && ssl3_ValidateAppProtocol(data, length) != SECSuccess) { return SECFailure; } @@ -2266,11 +2272,13 @@ SSL_SetNextProtoNego(PRFileDesc *fd, const unsigned char *data, * first protocol to the end of the list. */ ssl_GetSSL3HandshakeLock(ss); SECITEM_FreeItem(&ss->opt.nextProtoNego, PR_FALSE); - SECITEM_AllocItem(NULL, &ss->opt.nextProtoNego, length); - size_t firstLen = data[0] + 1; - /* firstLen <= length is ensured by ssl3_ValidateAppProtocol. */ - PORT_Memcpy(ss->opt.nextProtoNego.data + (length - firstLen), data, firstLen); - PORT_Memcpy(ss->opt.nextProtoNego.data, data + firstLen, length - firstLen); + if (length > 0) { + SECITEM_AllocItem(NULL, &ss->opt.nextProtoNego, length); + size_t firstLen = data[0] + 1; + /* firstLen <= length is ensured by ssl3_ValidateAppProtocol. */ + PORT_Memcpy(ss->opt.nextProtoNego.data + (length - firstLen), data, firstLen); + PORT_Memcpy(ss->opt.nextProtoNego.data, data + firstLen, length - firstLen); + } ssl_ReleaseSSL3HandshakeLock(ss); return SSL_SetNextProtoCallback(fd, ssl_NextProtoNegoCallback, NULL); |