Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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
  • Loading branch information
franziskuskiefer committed Apr 13, 2018
1 parent 03d42c5 commit 0eeb165
Show file tree
Hide file tree
Showing 14 changed files with 178 additions and 207 deletions.
4 changes: 2 additions & 2 deletions gtests/ssl_gtest/ssl_0rtt_unittest.cc
Expand Up @@ -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<uint8_t> alpn({0x01, 0x62}); // "b"
EnableAlpn(alpn);
client_->Set0RttEnabled(true);
server_->Set0RttEnabled(true);
ExpectResumption(RESUME_TICKET);
Expand Down
48 changes: 48 additions & 0 deletions gtests/ssl_gtest/ssl_loopback_unittest.cc
Expand Up @@ -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<uint8_t> 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<uint8_t> 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_));
Expand All @@ -157,6 +178,33 @@ TEST_P(TlsConnectGeneric, ConnectAlpnClone) {
CheckAlpn("a");
}

TEST_P(TlsConnectGeneric, ConnectAlpnWithCustomCallbackA) {
// "ab" "alpn"
const std::vector<uint8_t> 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<uint8_t> 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<uint8_t> 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();
Expand Down
10 changes: 4 additions & 6 deletions gtests/ssl_gtest/tls_agent.cc
Expand Up @@ -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<unsigned char*>(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);
Expand Down
2 changes: 2 additions & 0 deletions gtests/ssl_gtest/tls_agent.h
Expand Up @@ -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[];

Expand Down
49 changes: 46 additions & 3 deletions gtests/ssl_gtest/tls_connect.cc
Expand Up @@ -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<TlsAgent*>(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<const char*>(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<uint8_t>& 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<uint8_t>& vals) {
client_->EnableAlpn(vals.data(), vals.size());
server_->EnableAlpn(vals.data(), vals.size());
}

void TlsConnectTestBase::EnsureModelSockets() {
Expand Down
4 changes: 3 additions & 1 deletion gtests/ssl_gtest/tls_connect.h
Expand Up @@ -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<uint8_t>& client,
std::string server_choice);
void EnableAlpn(const std::vector<uint8_t>& vals);
void EnsureModelSockets();
void CheckAlpn(const std::string& val);
void EnableSrtp();
Expand Down
2 changes: 1 addition & 1 deletion lib/ssl/SSLerrs.h
Expand Up @@ -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.")
Expand Down
48 changes: 20 additions & 28 deletions lib/ssl/ssl.h
Expand Up @@ -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

Expand Down Expand Up @@ -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|.
Expand All @@ -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". */
Expand Down
12 changes: 0 additions & 12 deletions lib/ssl/ssl3con.c
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 0 additions & 4 deletions lib/ssl/ssl3ext.c
Expand Up @@ -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 },
Expand All @@ -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 },
Expand Down Expand Up @@ -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 },
Expand Down Expand Up @@ -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 }
};

Expand Down

0 comments on commit 0eeb165

Please sign in to comment.