From 0eeb1650313c96c405ad70cb72b7e72882dfd8d6 Mon Sep 17 00:00:00 2001 From: Franziskus Kiefer Date: Fri, 13 Apr 2018 13:24:21 +0200 Subject: [PATCH] Bug 1402738 - remove NPN and fix ALPN, r= mt Also fixes bug 1284412. Differential Revision: https://phabricator.services.mozilla.com/D908 --HG-- extra : amend_source : f2e3fa5f8e30abcfbe0ed8c81f9fb09b0ed7985a --- gtests/ssl_gtest/ssl_0rtt_unittest.cc | 4 +- gtests/ssl_gtest/ssl_loopback_unittest.cc | 48 +++++++++ gtests/ssl_gtest/tls_agent.cc | 10 +- gtests/ssl_gtest/tls_agent.h | 2 + gtests/ssl_gtest/tls_connect.cc | 49 ++++++++- gtests/ssl_gtest/tls_connect.h | 4 +- lib/ssl/SSLerrs.h | 2 +- lib/ssl/ssl.h | 48 ++++----- lib/ssl/ssl3con.c | 12 --- lib/ssl/ssl3ext.c | 4 - lib/ssl/ssl3exthandle.c | 117 +++------------------- lib/ssl/sslimpl.h | 7 +- lib/ssl/sslsock.c | 66 ++++++------ lib/ssl/tls13con.c | 12 +-- 14 files changed, 178 insertions(+), 207 deletions(-) diff --git a/gtests/ssl_gtest/ssl_0rtt_unittest.cc b/gtests/ssl_gtest/ssl_0rtt_unittest.cc index 08781af711..28fdc66318 100644 --- a/gtests/ssl_gtest/ssl_0rtt_unittest.cc +++ b/gtests/ssl_gtest/ssl_0rtt_unittest.cc @@ -345,8 +345,8 @@ TEST_P(TlsConnectTls13, TestTls13ZeroRttNoAlpnClient) { TEST_P(TlsConnectTls13, TestTls13ZeroRttAlpnChangeBoth) { EnableAlpn(); SetupForZeroRtt(); - static const uint8_t alpn[] = {0x01, 0x62}; // "b" - EnableAlpn(alpn, sizeof(alpn)); + static const std::vector alpn({0x01, 0x62}); // "b" + EnableAlpn(alpn); client_->Set0RttEnabled(true); server_->Set0RttEnabled(true); ExpectResumption(RESUME_TICKET); diff --git a/gtests/ssl_gtest/ssl_loopback_unittest.cc b/gtests/ssl_gtest/ssl_loopback_unittest.cc index b4a74b99ea..5adbd9dc71 100644 --- a/gtests/ssl_gtest/ssl_loopback_unittest.cc +++ b/gtests/ssl_gtest/ssl_loopback_unittest.cc @@ -149,6 +149,27 @@ TEST_P(TlsConnectGeneric, ConnectAlpn) { CheckAlpn("a"); } +TEST_P(TlsConnectGeneric, ConnectAlpnPriorityA) { + // "alpn" "npn" + // alpn is the fallback here. npn has the highest priority and should be + // picked. + const std::vector alpn = {0x04, 0x61, 0x6c, 0x70, 0x6e, + 0x03, 0x6e, 0x70, 0x6e}; + EnableAlpn(alpn); + Connect(); + CheckAlpn("npn"); +} + +TEST_P(TlsConnectGeneric, ConnectAlpnPriorityB) { + // "alpn" "npn" "http" + // npn has the highest priority and should be picked. + const std::vector alpn = {0x04, 0x61, 0x6c, 0x70, 0x6e, 0x03, 0x6e, + 0x70, 0x6e, 0x04, 0x68, 0x74, 0x74, 0x70}; + EnableAlpn(alpn); + Connect(); + CheckAlpn("npn"); +} + TEST_P(TlsConnectGeneric, ConnectAlpnClone) { EnsureModelSockets(); client_model_->EnableAlpn(alpn_dummy_val_, sizeof(alpn_dummy_val_)); @@ -157,6 +178,33 @@ TEST_P(TlsConnectGeneric, ConnectAlpnClone) { CheckAlpn("a"); } +TEST_P(TlsConnectGeneric, ConnectAlpnWithCustomCallbackA) { + // "ab" "alpn" + const std::vector client_alpn = {0x02, 0x61, 0x62, 0x04, + 0x61, 0x6c, 0x70, 0x6e}; + EnableAlpnWithCallback(client_alpn, "alpn"); + Connect(); + CheckAlpn("alpn"); +} + +TEST_P(TlsConnectGeneric, ConnectAlpnWithCustomCallbackB) { + // "ab" "alpn" + const std::vector client_alpn = {0x02, 0x61, 0x62, 0x04, + 0x61, 0x6c, 0x70, 0x6e}; + EnableAlpnWithCallback(client_alpn, "ab"); + Connect(); + CheckAlpn("ab"); +} + +TEST_P(TlsConnectGeneric, ConnectAlpnWithCustomCallbackC) { + // "cd" "npn" "alpn" + const std::vector client_alpn = {0x02, 0x63, 0x64, 0x03, 0x6e, 0x70, + 0x6e, 0x04, 0x61, 0x6c, 0x70, 0x6e}; + EnableAlpnWithCallback(client_alpn, "npn"); + Connect(); + CheckAlpn("npn"); +} + TEST_P(TlsConnectDatagram, ConnectSrtp) { EnableSrtp(); Connect(); diff --git a/gtests/ssl_gtest/tls_agent.cc b/gtests/ssl_gtest/tls_agent.cc index 3ee0b68f44..4f0402bf52 100644 --- a/gtests/ssl_gtest/tls_agent.cc +++ b/gtests/ssl_gtest/tls_agent.cc @@ -599,22 +599,20 @@ void TlsAgent::ExpectResumption() { expect_resumption_ = true; } void TlsAgent::EnableAlpn(const uint8_t* val, size_t len) { EXPECT_TRUE(EnsureTlsSetup()); - - SetOption(SSL_ENABLE_ALPN, PR_TRUE); EXPECT_EQ(SECSuccess, SSL_SetNextProtoNego(ssl_fd(), val, len)); } void TlsAgent::CheckAlpn(SSLNextProtoState expected_state, const std::string& expected) const { - SSLNextProtoState npn_state; + SSLNextProtoState alpn_state; char chosen[10]; unsigned int chosen_len; - SECStatus rv = SSL_GetNextProto(ssl_fd(), &npn_state, + SECStatus rv = SSL_GetNextProto(ssl_fd(), &alpn_state, reinterpret_cast(chosen), &chosen_len, sizeof(chosen)); EXPECT_EQ(SECSuccess, rv); - EXPECT_EQ(expected_state, npn_state); - if (npn_state == SSL_NEXT_PROTO_NO_SUPPORT) { + EXPECT_EQ(expected_state, alpn_state); + if (alpn_state == SSL_NEXT_PROTO_NO_SUPPORT) { EXPECT_EQ("", expected); } else { EXPECT_NE("", expected); diff --git a/gtests/ssl_gtest/tls_agent.h b/gtests/ssl_gtest/tls_agent.h index a00bd4be17..786efb7605 100644 --- a/gtests/ssl_gtest/tls_agent.h +++ b/gtests/ssl_gtest/tls_agent.h @@ -268,6 +268,8 @@ class TlsAgent : public PollTarget { void ExpectReceiveAlert(uint8_t alert, uint8_t level = 0); void ExpectSendAlert(uint8_t alert, uint8_t level = 0); + std::string alpn_value_to_use_ = ""; + private: const static char* states[]; diff --git a/gtests/ssl_gtest/tls_connect.cc b/gtests/ssl_gtest/tls_connect.cc index 8567b392fd..68f6d21e9c 100644 --- a/gtests/ssl_gtest/tls_connect.cc +++ b/gtests/ssl_gtest/tls_connect.cc @@ -571,14 +571,57 @@ void TlsConnectTestBase::CheckResumption(SessionResumptionMode expected) { } } +static SECStatus NextProtoCallbackServer(void* arg, PRFileDesc* fd, + const unsigned char* protos, + unsigned int protos_len, + unsigned char* protoOut, + unsigned int* protoOutLen, + unsigned int protoMaxLen) { + EXPECT_EQ(protoMaxLen, 255U); + TlsAgent* agent = reinterpret_cast(arg); + // Check that agent->alpn_value_to_use_ is in protos. + if (protos_len < 1) { + return SECFailure; + } + for (size_t i = 0; i < protos_len;) { + size_t l = protos[i]; + EXPECT_LT(i + l, protos_len); + if (i + l >= protos_len) { + return SECFailure; + } + std::string protos_s(reinterpret_cast(protos + i + 1), l); + if (protos_s == agent->alpn_value_to_use_) { + size_t s_len = agent->alpn_value_to_use_.size(); + EXPECT_LE(s_len, 255U); + memcpy(protoOut, &agent->alpn_value_to_use_[0], s_len); + *protoOutLen = s_len; + return SECSuccess; + } + i += l + 1; + } + return SECFailure; +} + void TlsConnectTestBase::EnableAlpn() { client_->EnableAlpn(alpn_dummy_val_, sizeof(alpn_dummy_val_)); server_->EnableAlpn(alpn_dummy_val_, sizeof(alpn_dummy_val_)); } -void TlsConnectTestBase::EnableAlpn(const uint8_t* val, size_t len) { - client_->EnableAlpn(val, len); - server_->EnableAlpn(val, len); +void TlsConnectTestBase::EnableAlpnWithCallback( + const std::vector& client_vals, std::string server_choice) { + EnsureTlsSetup(); + server_->alpn_value_to_use_ = server_choice; + EXPECT_EQ(SECSuccess, + SSL_SetNextProtoNego(client_->ssl_fd(), client_vals.data(), + client_vals.size())); + SECStatus rv = SSL_SetNextProtoCallback( + server_->ssl_fd(), NextProtoCallbackServer, server_.get()); + EXPECT_EQ(SECSuccess, rv); +} + +void TlsConnectTestBase::EnableAlpn(const std::vector& vals) { + client_->EnableAlpn(vals.data(), vals.size()); + server_->EnableAlpn(vals.data(), vals.size()); } void TlsConnectTestBase::EnsureModelSockets() { diff --git a/gtests/ssl_gtest/tls_connect.h b/gtests/ssl_gtest/tls_connect.h index 7dffe7f8aa..0004945011 100644 --- a/gtests/ssl_gtest/tls_connect.h +++ b/gtests/ssl_gtest/tls_connect.h @@ -110,7 +110,9 @@ class TlsConnectTestBase : public ::testing::Test { void ConfigureSessionCache(SessionResumptionMode client, SessionResumptionMode server); void EnableAlpn(); - void EnableAlpn(const uint8_t* val, size_t len); + void EnableAlpnWithCallback(const std::vector& client, + std::string server_choice); + void EnableAlpn(const std::vector& vals); void EnsureModelSockets(); void CheckAlpn(const std::string& val); void EnableSrtp(); diff --git a/lib/ssl/SSLerrs.h b/lib/ssl/SSLerrs.h index 0256b46834..1fe7197c00 100644 --- a/lib/ssl/SSLerrs.h +++ b/lib/ssl/SSLerrs.h @@ -374,7 +374,7 @@ ER3(SSL_ERROR_WEAK_SERVER_EPHEMERAL_DH_KEY, (SSL_ERROR_BASE + 115), "SSL received a weak ephemeral Diffie-Hellman key in Server Key Exchange handshake message.") ER3(SSL_ERROR_NEXT_PROTOCOL_DATA_INVALID, (SSL_ERROR_BASE + 116), - "SSL received invalid NPN extension data.") + "SSL received invalid ALPN extension data.") ER3(SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_SSL2, (SSL_ERROR_BASE + 117), "SSL feature not supported for SSL 2.0 connections.") diff --git a/lib/ssl/ssl.h b/lib/ssl/ssl.h index ad8ec0f8b9..0280f73c38 100644 --- a/lib/ssl/ssl.h +++ b/lib/ssl/ssl.h @@ -158,23 +158,18 @@ SSL_IMPORT PRFileDesc *DTLS_ImportFD(PRFileDesc *model, PRFileDesc *fd); #define SSL_CBC_RANDOM_IV 23 #define SSL_ENABLE_OCSP_STAPLING 24 /* Request OCSP stapling (client) */ -/* SSL_ENABLE_NPN controls whether the NPN extension is enabled for the initial - * handshake when application layer protocol negotiation is used. - * SSL_SetNextProtoCallback or SSL_SetNextProtoNego must be used to control the - * application layer protocol negotiation; otherwise, the NPN extension will - * not be negotiated. SSL_ENABLE_NPN is currently enabled by default but this - * may change in future versions. - */ +/* SSL_ENABLE_NPN is defunct and defaults to false. + * Using this option will not have any effect but won't produce an error. */ #define SSL_ENABLE_NPN 25 /* SSL_ENABLE_ALPN controls whether the ALPN extension is enabled for the * initial handshake when application layer protocol negotiation is used. - * SSL_SetNextProtoNego (not SSL_SetNextProtoCallback) must be used to control - * the application layer protocol negotiation; otherwise, the ALPN extension - * will not be negotiated. ALPN is not negotiated for renegotiation handshakes, - * even though the ALPN specification defines a way to use ALPN during - * renegotiations. SSL_ENABLE_ALPN is currently disabled by default, but this - * may change in future versions. + * SSL_SetNextProtoNego or SSL_SetNextProtoCallback can be used to control + * the application layer protocol negotiation; + * ALPN is not negotiated for renegotiation handshakes, even though the ALPN + * specification defines a way to use ALPN during renegotiations. + * SSL_ENABLE_ALPN is currently enabled by default, but this may change in + * future versions. */ #define SSL_ENABLE_ALPN 26 @@ -283,10 +278,10 @@ SSL_IMPORT SECStatus SSL_OptionSetDefault(PRInt32 option, PRIntn val); SSL_IMPORT SECStatus SSL_OptionGetDefault(PRInt32 option, PRIntn *val); SSL_IMPORT SECStatus SSL_CertDBHandleSet(PRFileDesc *fd, CERTCertDBHandle *dbHandle); -/* SSLNextProtoCallback is called during the handshake for the client, when a - * Next Protocol Negotiation (NPN) extension has been received from the server. - * |protos| and |protosLen| define a buffer which contains the server's - * advertisement. This data is guaranteed to be well formed per the NPN spec. +/* SSLNextProtoCallback is called during the handshake for the server, when an + * Application-Layer Protocol Negotiation (ALPN) extension has been received + * from the client. |protos| and |protosLen| define a buffer which contains the + * client's advertisement. * |protoOut| is a buffer provided by the caller, of length 255 (the maximum * allowed by the protocol). On successful return, the protocol to be announced * to the server will be in |protoOut| and its length in |*protoOutLen|. @@ -302,27 +297,24 @@ typedef SECStatus(PR_CALLBACK *SSLNextProtoCallback)( unsigned int *protoOutLen, unsigned int protoMaxOut); -/* SSL_SetNextProtoCallback sets a callback function to handle Next Protocol - * Negotiation. It causes a client to advertise NPN. */ +/* SSL_SetNextProtoCallback sets a callback function to handle ALPN Negotiation. + * It causes a client to advertise ALPN. */ SSL_IMPORT SECStatus SSL_SetNextProtoCallback(PRFileDesc *fd, SSLNextProtoCallback callback, void *arg); /* SSL_SetNextProtoNego can be used as an alternative to - * SSL_SetNextProtoCallback. It also causes a client to advertise NPN and - * installs a default callback function which selects the first supported - * protocol in server-preference order. If no matching protocol is found it - * selects the first supported protocol. + * SSL_SetNextProtoCallback. * - * Using this function also allows the client to transparently support ALPN. + * Using this function allows client and server to transparently support ALPN. * The same set of protocols will be advertised via ALPN and, if the server * uses ALPN to select a protocol, SSL_GetNextProto will return * SSL_NEXT_PROTO_SELECTED as the state. * - * Since NPN uses the first protocol as the fallback protocol, when sending an - * ALPN extension, the first protocol is moved to the end of the list. This - * indicates that the fallback protocol is the least preferred. The other - * protocols should be in preference order. + * Because the predecessor to ALPN, NPN, used the first protocol as the fallback + * protocol, when sending an ALPN extension, the first protocol is moved to the + * end of the list. This indicates that the fallback protocol is the least + * 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". */ diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c index 7346245a6b..83040a8e97 100644 --- a/lib/ssl/ssl3con.c +++ b/lib/ssl/ssl3con.c @@ -7321,10 +7321,6 @@ ssl3_SendClientSecondRound(sslSocket *ss) * certificate to an attacker that does not have a valid cert for the * domain we are connecting to. * - * XXX: We should do the same for the NPN extension, but for that we - * need an option to give the application the ability to leak the NPN - * information to get better performance. - * * During the initial handshake on a connection, we never send/receive * application data until we have authenticated the server's certificate; * i.e. we have fully authenticated the handshake before using the cipher @@ -7398,14 +7394,6 @@ ssl3_SendClientSecondRound(sslSocket *ss) ss->enoughFirstHsDone = PR_TRUE; if (!ss->firstHsDone) { - /* XXX: If the server's certificate hasn't been authenticated by this - * point, then we may be leaking this NPN message to an attacker. - */ - rv = ssl3_SendNextProto(ss); - if (rv != SECSuccess) { - goto loser; /* err code was set. */ - } - if (ss->opt.enableFalseStart) { if (!ss->ssl3.hs.authCertificatePending) { /* When we fix bug 589047, we will need to know whether we are diff --git a/lib/ssl/ssl3ext.c b/lib/ssl/ssl3ext.c index 28c2611b1f..782425e572 100644 --- a/lib/ssl/ssl3ext.c +++ b/lib/ssl/ssl3ext.c @@ -39,7 +39,6 @@ static const ssl3ExtensionHandler clientHelloHandlers[] = { { ssl_ec_point_formats_xtn, &ssl3_HandleSupportedPointFormatsXtn }, { ssl_session_ticket_xtn, &ssl3_ServerHandleSessionTicketXtn }, { ssl_renegotiation_info_xtn, &ssl3_HandleRenegotiationInfoXtn }, - { ssl_next_proto_nego_xtn, &ssl3_ServerHandleNextProtoNegoXtn }, { ssl_app_layer_protocol_xtn, &ssl3_ServerHandleAppProtoXtn }, { ssl_use_srtp_xtn, &ssl3_ServerHandleUseSRTPXtn }, { ssl_cert_status_xtn, &ssl3_ServerHandleStatusRequestXtn }, @@ -61,7 +60,6 @@ static const ssl3ExtensionHandler serverHelloHandlersTLS[] = { /* TODO: add a handler for ssl_ec_point_formats_xtn */ { ssl_session_ticket_xtn, &ssl3_ClientHandleSessionTicketXtn }, { ssl_renegotiation_info_xtn, &ssl3_HandleRenegotiationInfoXtn }, - { ssl_next_proto_nego_xtn, &ssl3_ClientHandleNextProtoNegoXtn }, { ssl_app_layer_protocol_xtn, &ssl3_ClientHandleAppProtoXtn }, { ssl_use_srtp_xtn, &ssl3_ClientHandleUseSRTPXtn }, { ssl_cert_status_xtn, &ssl3_ClientHandleStatusRequestXtn }, @@ -122,7 +120,6 @@ static const sslExtensionBuilder clientHelloSendersTLS[] = { ssl_supported_groups_xtn, &ssl_SendSupportedGroupsXtn }, { ssl_ec_point_formats_xtn, &ssl3_SendSupportedPointFormatsXtn }, { ssl_session_ticket_xtn, &ssl3_ClientSendSessionTicketXtn }, - { ssl_next_proto_nego_xtn, &ssl3_ClientSendNextProtoNegoXtn }, { ssl_app_layer_protocol_xtn, &ssl3_ClientSendAppProtoXtn }, { ssl_use_srtp_xtn, &ssl3_ClientSendUseSRTPXtn }, { ssl_cert_status_xtn, &ssl3_ClientSendStatusRequestXtn }, @@ -183,7 +180,6 @@ static const struct { { ssl_tls13_psk_key_exchange_modes_xtn, ssl_ext_native_only }, { ssl_tls13_ticket_early_data_info_xtn, ssl_ext_native_only }, { ssl_tls13_certificate_authorities_xtn, ssl_ext_native }, - { ssl_next_proto_nego_xtn, ssl_ext_none }, { ssl_renegotiation_info_xtn, ssl_ext_native } }; diff --git a/lib/ssl/ssl3exthandle.c b/lib/ssl/ssl3exthandle.c index e6388945e1..d08e5e435e 100644 --- a/lib/ssl/ssl3exthandle.c +++ b/lib/ssl/ssl3exthandle.c @@ -242,33 +242,11 @@ ssl_AlpnTagAllowed(const sslSocket *ss, const SECItem *tag) return PR_FALSE; } -/* handle an incoming Next Protocol Negotiation extension. */ -SECStatus -ssl3_ServerHandleNextProtoNegoXtn(const sslSocket *ss, TLSExtensionData *xtnData, - SECItem *data) -{ - PORT_Assert(ss->version < SSL_LIBRARY_VERSION_TLS_1_3); - - if (ss->firstHsDone || data->len != 0) { - /* Clients MUST send an empty NPN extension, if any. */ - PORT_SetError(SSL_ERROR_NEXT_PROTOCOL_DATA_INVALID); - return SECFailure; - } - - xtnData->negotiated[xtnData->numNegotiated++] = ssl_next_proto_nego_xtn; - - /* TODO: server side NPN support would require calling - * ssl3_RegisterServerHelloExtensionSender here in order to echo the - * extension back to the client. */ - - return SECSuccess; -} - -/* ssl3_ValidateNextProtoNego checks that the given block of data is valid: none +/* ssl3_ValidateAppProtocol checks that the given block of data is valid: none * of the lengths may be 0 and the sum of the lengths must equal the length of * the block. */ SECStatus -ssl3_ValidateNextProtoNego(const unsigned char *data, unsigned int length) +ssl3_ValidateAppProtocol(const unsigned char *data, unsigned int length) { unsigned int offset = 0; @@ -286,7 +264,7 @@ ssl3_ValidateNextProtoNego(const unsigned char *data, unsigned int length) return SECSuccess; } -/* protocol selection handler for ALPN (server side) and NPN (client side) */ +/* Protocol selection handler for ALPN. */ static SECStatus ssl3_SelectAppProtocol(const sslSocket *ss, TLSExtensionData *xtnData, PRUint16 extension, SECItem *data) @@ -295,7 +273,7 @@ ssl3_SelectAppProtocol(const sslSocket *ss, TLSExtensionData *xtnData, unsigned char resultBuffer[255]; SECItem result = { siBuffer, resultBuffer, 0 }; - rv = ssl3_ValidateNextProtoNego(data->data, data->len); + rv = ssl3_ValidateAppProtocol(data->data, data->len); if (rv != SECSuccess) { ssl3_ExtSendAlert(ss, alert_fatal, decode_error); PORT_SetError(SSL_ERROR_NEXT_PROTOCOL_DATA_INVALID); @@ -303,11 +281,13 @@ ssl3_SelectAppProtocol(const sslSocket *ss, TLSExtensionData *xtnData, } PORT_Assert(ss->nextProtoCallback); - /* For ALPN, the cipher suite isn't selected yet. Note that extensions + /* The cipher suite isn't selected yet. Note that extensions * sometimes affect what cipher suite is selected, e.g., for ECC. */ PORT_Assert((ss->ssl3.hs.preliminaryInfo & ssl_preinfo_all & ~ssl_preinfo_cipher_suite) == (ssl_preinfo_all & ~ssl_preinfo_cipher_suite)); + /* The callback has to make sure that either rv != SECSuccess or that result + * is not set if there is no common protocol. */ rv = ss->nextProtoCallback(ss->nextProtoArg, ss->fd, data->data, data->len, result.data, &result.len, sizeof(resultBuffer)); if (rv != SECSuccess) { @@ -320,21 +300,20 @@ ssl3_SelectAppProtocol(const sslSocket *ss, TLSExtensionData *xtnData, * stack. */ if (result.len > sizeof(resultBuffer)) { PORT_SetError(SEC_ERROR_OUTPUT_LEN); - /* TODO: crash */ + PORT_Assert(PR_FALSE); return SECFailure; } SECITEM_FreeItem(&xtnData->nextProto, PR_FALSE); - if (extension == ssl_app_layer_protocol_xtn && - xtnData->nextProtoState != SSL_NEXT_PROTO_NEGOTIATED) { - /* The callback might say OK, but then it picks a default value - one - * that was not listed. That's OK for NPN, but not ALPN. */ + if (result.len < 1 || !result.data) { + /* Check that we actually got a result. */ ssl3_ExtSendAlert(ss, alert_fatal, no_application_protocol); PORT_SetError(SSL_ERROR_NEXT_PROTOCOL_NO_PROTOCOL); return SECFailure; } + xtnData->nextProtoState = SSL_NEXT_PROTO_NEGOTIATED; xtnData->negotiated[xtnData->numNegotiated++] = extension; return SECITEM_CopyItem(NULL, &xtnData->nextProto, &result); } @@ -356,7 +335,7 @@ ssl3_ServerHandleAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData, return SECFailure; } - /* Unlike NPN, ALPN has extra redundant length information so that + /* ALPN has extra redundant length information so that * the extension is the same in both ClientHello and ServerHello. */ rv = ssl3_ExtConsumeHandshakeNumber(ss, &count, 2, &data->data, &data->len); if (rv != SECSuccess || count != data->len) { @@ -388,39 +367,6 @@ ssl3_ServerHandleAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData, return SECSuccess; } -SECStatus -ssl3_ClientHandleNextProtoNegoXtn(const sslSocket *ss, TLSExtensionData *xtnData, - SECItem *data) -{ - PORT_Assert(ss->version < SSL_LIBRARY_VERSION_TLS_1_3); - PORT_Assert(!ss->firstHsDone); - - if (ssl3_ExtensionNegotiated(ss, ssl_app_layer_protocol_xtn)) { - /* If the server negotiated ALPN then it has already told us what - * protocol to use, so it doesn't make sense for us to try to negotiate - * a different one by sending the NPN handshake message. However, if - * we've negotiated NPN then we're required to send the NPN handshake - * message. Thus, these two extensions cannot both be negotiated on the - * same connection. */ - ssl3_ExtSendAlert(ss, alert_fatal, illegal_parameter); - PORT_SetError(SSL_ERROR_BAD_SERVER); - return SECFailure; - } - - /* We should only get this call if we sent the extension, so - * ss->nextProtoCallback needs to be non-NULL. However, it is possible - * that an application erroneously cleared the callback between the time - * we sent the ClientHello and now. */ - if (!ss->nextProtoCallback) { - PORT_Assert(0); - ssl3_ExtSendAlert(ss, alert_fatal, internal_error); - PORT_SetError(SSL_ERROR_NEXT_PROTOCOL_NO_CALLBACK); - return SECFailure; - } - - return ssl3_SelectAppProtocol(ss, xtnData, ssl_next_proto_nego_xtn, data); -} - SECStatus ssl3_ClientHandleAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData, SECItem *data) @@ -474,19 +420,6 @@ ssl3_ClientHandleAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData, return SECITEM_CopyItem(NULL, &xtnData->nextProto, &protocol_name); } -SECStatus -ssl3_ClientSendNextProtoNegoXtn(const sslSocket *ss, TLSExtensionData *xtnData, - sslBuffer *buf, PRBool *added) -{ - /* Renegotiations do not send this extension. */ - if (!ss->opt.enableNPN || !ss->nextProtoCallback || ss->firstHsDone) { - return SECSuccess; - } - - *added = PR_TRUE; - return SECSuccess; -} - SECStatus ssl3_ClientSendAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData, sslBuffer *buf, PRBool *added) @@ -499,35 +432,15 @@ ssl3_ClientSendAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData, return SECSuccess; } - /* NPN requires that the client's fallback protocol is first in the - * list. However, ALPN sends protocols in preference order. So move the - * first protocol to the end of the list. */ - if (len > 0) { /* Each protocol string is prefixed with a single byte length. */ - unsigned int i; - rv = sslBuffer_AppendNumber(buf, len, 2); if (rv != SECSuccess) { return SECFailure; } - - i = ss->opt.nextProtoNego.data[0] + 1; - if (i <= len) { - rv = sslBuffer_Append(buf, &ss->opt.nextProtoNego.data[i], len - i); - if (rv != SECSuccess) { - return SECFailure; - } - rv = sslBuffer_Append(buf, ss->opt.nextProtoNego.data, i); - if (rv != SECSuccess) { - return SECFailure; - } - } else { - /* This seems to be invalid data so we'll send as-is. */ - rv = sslBuffer_Append(buf, ss->opt.nextProtoNego.data, len); - if (rv != SECSuccess) { - return SECFailure; - } + rv = sslBuffer_Append(buf, ss->opt.nextProtoNego.data, len); + if (rv != SECSuccess) { + return SECFailure; } } diff --git a/lib/ssl/sslimpl.h b/lib/ssl/sslimpl.h index 3c01ab4368..f992dd1624 100644 --- a/lib/ssl/sslimpl.h +++ b/lib/ssl/sslimpl.h @@ -251,7 +251,6 @@ typedef struct sslOptionsStr { unsigned int enableFalseStart : 1; unsigned int cbcRandomIV : 1; unsigned int enableOCSPStapling : 1; - unsigned int enableNPN : 1; unsigned int enableALPN : 1; unsigned int reuseServerECDHEKey : 1; unsigned int enableFallbackSCSV : 1; @@ -443,7 +442,7 @@ struct sslSessionIDStr { */ SECItem signedCertTimestamps; - /* The NPN/ALPN value negotiated in the original connection. + /* The ALPN value negotiated in the original connection. * Used for TLS 1.3. */ SECItem alpnSelection; @@ -1547,8 +1546,8 @@ SECStatus ssl_GetSelfEncryptKeys(sslSocket *ss, unsigned char *keyName, PK11SymKey **encKey, PK11SymKey **macKey); void ssl_ResetSelfEncryptKeys(); -extern SECStatus ssl3_ValidateNextProtoNego(const unsigned char *data, - unsigned int length); +extern SECStatus ssl3_ValidateAppProtocol(const unsigned char *data, + unsigned int length); /* Construct a new NSPR socket for the app to use */ extern PRFileDesc *ssl_NewPRSocket(sslSocket *ss, PRFileDesc *fd); diff --git a/lib/ssl/sslsock.c b/lib/ssl/sslsock.c index f5d829d4ee..5b98a3de4f 100644 --- a/lib/ssl/sslsock.c +++ b/lib/ssl/sslsock.c @@ -72,7 +72,6 @@ static sslOptions ssl_defaults = { .enableFalseStart = PR_FALSE, .cbcRandomIV = PR_TRUE, .enableOCSPStapling = PR_FALSE, - .enableNPN = PR_FALSE, .enableALPN = PR_TRUE, .reuseServerECDHEKey = PR_TRUE, .enableFallbackSCSV = PR_FALSE, @@ -919,7 +918,7 @@ SSL_OptionGet(PRFileDesc *fd, PRInt32 which, PRIntn *pVal) val = ss->opt.enableOCSPStapling; break; case SSL_ENABLE_NPN: - val = ss->opt.enableNPN; + val = PR_FALSE; break; case SSL_ENABLE_ALPN: val = ss->opt.enableALPN; @@ -1045,7 +1044,7 @@ SSL_OptionGetDefault(PRInt32 which, PRIntn *pVal) val = ssl_defaults.enableOCSPStapling; break; case SSL_ENABLE_NPN: - val = ssl_defaults.enableNPN; + val = PR_FALSE; break; case SSL_ENABLE_ALPN: val = ssl_defaults.enableALPN; @@ -1910,10 +1909,7 @@ DTLS_ImportFD(PRFileDesc *model, PRFileDesc *fd) } /* SSL_SetNextProtoCallback is used to select an application protocol - * for ALPN and NPN. For ALPN, this runs on the server; for NPN it - * runs on the client. */ -/* Note: The ALPN version doesn't allow for the use of a default, setting a - * status of SSL_NEXT_PROTO_NO_OVERLAP is treated as a failure. */ + * for ALPN. */ SECStatus SSL_SetNextProtoCallback(PRFileDesc *fd, SSLNextProtoCallback callback, void *arg) @@ -1934,7 +1930,7 @@ SSL_SetNextProtoCallback(PRFileDesc *fd, SSLNextProtoCallback callback, return SECSuccess; } -/* ssl_NextProtoNegoCallback is set as an ALPN/NPN callback when +/* ssl_NextProtoNegoCallback is set as an ALPN callback when * SSL_SetNextProtoNego is used. */ static SECStatus @@ -1944,7 +1940,6 @@ ssl_NextProtoNegoCallback(void *arg, PRFileDesc *fd, unsigned int protoMaxLen) { unsigned int i, j; - const unsigned char *result; sslSocket *ss = ssl_FindSocket(fd); if (!ss) { @@ -1952,37 +1947,29 @@ ssl_NextProtoNegoCallback(void *arg, PRFileDesc *fd, SSL_GETPID(), fd)); return SECFailure; } + PORT_Assert(protoMaxLen <= 255); + if (protoMaxLen > 255) { + PORT_SetError(SEC_ERROR_OUTPUT_LEN); + return SECFailure; + } - /* For each protocol in server preference, see if we support it. */ - for (i = 0; i < protos_len;) { - for (j = 0; j < ss->opt.nextProtoNego.len;) { + /* For each protocol in client preference, see if we support it. */ + for (j = 0; j < ss->opt.nextProtoNego.len;) { + for (i = 0; i < protos_len;) { if (protos[i] == ss->opt.nextProtoNego.data[j] && PORT_Memcmp(&protos[i + 1], &ss->opt.nextProtoNego.data[j + 1], protos[i]) == 0) { /* We found a match. */ - ss->xtnData.nextProtoState = SSL_NEXT_PROTO_NEGOTIATED; - result = &protos[i]; - goto found; + const unsigned char *result = &protos[i]; + memcpy(protoOut, result + 1, result[0]); + *protoOutLen = result[0]; + return SECSuccess; } - j += 1 + (unsigned int)ss->opt.nextProtoNego.data[j]; + i += 1 + (unsigned int)protos[i]; } - i += 1 + (unsigned int)protos[i]; + j += 1 + (unsigned int)ss->opt.nextProtoNego.data[j]; } - /* The other side supports the extension, and either doesn't have any - * protocols configured, or none of its options match ours. In this case we - * request our favoured protocol. */ - /* This will be treated as a failure for ALPN. */ - ss->xtnData.nextProtoState = SSL_NEXT_PROTO_NO_OVERLAP; - result = ss->opt.nextProtoNego.data; - -found: - if (protoMaxLen < result[0]) { - PORT_SetError(SEC_ERROR_OUTPUT_LEN); - return SECFailure; - } - memcpy(protoOut, result + 1, result[0]); - *protoOutLen = result[0]; return SECSuccess; } @@ -1991,8 +1978,6 @@ SSL_SetNextProtoNego(PRFileDesc *fd, const unsigned char *data, unsigned int length) { sslSocket *ss; - SECStatus rv; - SECItem dataItem = { siBuffer, (unsigned char *)data, length }; ss = ssl_FindSocket(fd); if (!ss) { @@ -2001,17 +1986,22 @@ SSL_SetNextProtoNego(PRFileDesc *fd, const unsigned char *data, return SECFailure; } - if (ssl3_ValidateNextProtoNego(data, length) != SECSuccess) + if (ssl3_ValidateAppProtocol(data, length) != SECSuccess) { return SECFailure; + } + /* NPN required that the client's fallback protocol is first in the + * list. However, ALPN sends protocols in preference order. So move the + * first protocol to the end of the list. */ ssl_GetSSL3HandshakeLock(ss); SECITEM_FreeItem(&ss->opt.nextProtoNego, PR_FALSE); - rv = SECITEM_CopyItem(NULL, &ss->opt.nextProtoNego, &dataItem); + 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); - if (rv != SECSuccess) - return rv; - return SSL_SetNextProtoCallback(fd, ssl_NextProtoNegoCallback, NULL); } diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c index b12cbb17bb..3592f30f7b 100644 --- a/lib/ssl/tls13con.c +++ b/lib/ssl/tls13con.c @@ -3584,7 +3584,7 @@ tls13_HandleEncryptedExtensions(sslSocket *ss, PRUint8 *b, PRUint32 length) { SECStatus rv; PRUint32 innerLength; - SECItem oldNpn = { siBuffer, NULL, 0 }; + SECItem oldAlpn = { siBuffer, NULL, 0 }; PORT_Assert(ss->opt.noLocks || ssl_HaveRecvBufLock(ss)); PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss)); @@ -3608,11 +3608,11 @@ tls13_HandleEncryptedExtensions(sslSocket *ss, PRUint8 *b, PRUint32 length) return SECFailure; } - /* If we are doing 0-RTT, then we already have an NPN value. Stash + /* If we are doing 0-RTT, then we already have an ALPN value. Stash * it for comparison. */ if (ss->ssl3.hs.zeroRttState == ssl_0rtt_sent && ss->xtnData.nextProtoState == SSL_NEXT_PROTO_EARLY_VALUE) { - oldNpn = ss->xtnData.nextProto; + oldAlpn = ss->xtnData.nextProto; ss->xtnData.nextProto.data = NULL; ss->xtnData.nextProtoState = SSL_NEXT_PROTO_NO_SUPPORT; } @@ -3632,8 +3632,8 @@ tls13_HandleEncryptedExtensions(sslSocket *ss, PRUint8 *b, PRUint32 length) ss->ssl3.hs.zeroRttState = ssl_0rtt_accepted; /* Check that the server negotiated the same ALPN (if any). */ - if (SECITEM_CompareItem(&oldNpn, &ss->xtnData.nextProto)) { - SECITEM_FreeItem(&oldNpn, PR_FALSE); + if (SECITEM_CompareItem(&oldAlpn, &ss->xtnData.nextProto)) { + SECITEM_FreeItem(&oldAlpn, PR_FALSE); FATAL_ERROR(ss, SSL_ERROR_NEXT_PROTOCOL_DATA_INVALID, illegal_parameter); return SECFailure; @@ -3655,7 +3655,7 @@ tls13_HandleEncryptedExtensions(sslSocket *ss, PRUint8 *b, PRUint32 length) ss->ssl3.hs.zeroRttState == ssl_0rtt_ignored)); } - SECITEM_FreeItem(&oldNpn, PR_FALSE); + SECITEM_FreeItem(&oldAlpn, PR_FALSE); if (ss->ssl3.hs.kea_def->authKeyType == ssl_auth_psk) { TLS13_SET_HS_STATE(ss, wait_finished); } else {