Skip to content

Commit

Permalink
Bug 1304762 - accept groups other than the preferred if close enough,…
Browse files Browse the repository at this point in the history 
… r=mt,ekr

Differential Revision: https://nss-dev.phacility.com/D33

--HG--
extra : rebase_source : 3db95318d8fa732fe4c9f1e60470b26c2b7d0613
extra : amend_source : c7e5027261a2ce96d4a4a428595397a9a4898c98
  • Loading branch information
@franziskuskiefer
franziskuskiefer committed Oct 6, 2016
1 parent ce56d93 commit dda02ee
Show file tree
Hide file tree
Showing 9 changed files with 312 additions and 121 deletions.
8 changes: 4 additions & 4 deletions external_tests/ssl_gtest/ssl_auth_unittest.cc
View file
Expand Up @@ -97,7 +97,7 @@ TEST_P(TlsConnectTls12, ServerAuthCheckSigAlg) {
EXPECT_EQ(3U, buffer.data()[0]) << "curve_type == named_curve";
uint32_t tmp;
EXPECT_TRUE(buffer.Read(1, 2, &tmp)) << "read NamedCurve";
EXPECT_EQ(ssl_grp_ec_secp256r1, tmp);
EXPECT_EQ(ssl_grp_ec_curve25519, tmp);
EXPECT_TRUE(buffer.Read(3, 1, &tmp)) << " read ECPoint";
CheckSigScheme(capture_ske, 4 + tmp, client_, kTlsSigSchemeRsaPssSha256,
1024);
Expand Down Expand Up @@ -142,11 +142,11 @@ static const SSLSignatureAndHashAlg SignatureRsaSha256[] = {
static SSLNamedGroup NamedGroupForEcdsa384(uint16_t version) {
// NSS tries to match the group size to the symmetric cipher. In TLS 1.1 and
// 1.0, TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA is the highest priority suite, so
// we use P-384. With TLS 1.2 on we pick AES-128 GCM so use P-256.
// we use P-384. With TLS 1.2 on we pick AES-128 GCM so use x25519.
if (version <= SSL_LIBRARY_VERSION_TLS_1_1) {
return ssl_grp_ec_secp384r1;
}
return ssl_grp_ec_secp256r1;
return ssl_grp_ec_curve25519;
}

// When signature algorithms match up, this should connect successfully; even
Expand Down Expand Up @@ -197,7 +197,7 @@ TEST_P(TlsConnectTls12, SignatureSchemeCurveMismatch12) {
Connect();
// The scheme is reported as using secp384r1, but this is just the generic
// ECDSA + SHA-384 codepoint as defined in TLS 1.2.
CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp256r1, ssl_auth_ecdsa,
CheckKeys(ssl_kea_ecdh, ssl_grp_ec_curve25519, ssl_auth_ecdsa,
ssl_sig_ecdsa_secp384r1_sha384);
}

Expand Down
196 changes: 178 additions & 18 deletions external_tests/ssl_gtest/ssl_ecdh_unittest.cc
View file
Expand Up @@ -93,11 +93,11 @@ TEST_P(TlsConnectGeneric, ConnectEcdheP384Server) {
// This test will fail when we add other groups that identify as ECDHE.
TEST_P(TlsConnectGeneric, ConnectEcdheGroupMismatch) {
EnsureTlsSetup();
const std::vector<SSLNamedGroup> clientGroups = {ssl_grp_ec_secp256r1,
ssl_grp_ffdhe_2048};
const std::vector<SSLNamedGroup> serverGroups = {ssl_grp_ffdhe_2048};
client_->ConfigNamedGroups(clientGroups);
server_->ConfigNamedGroups(serverGroups);
const std::vector<SSLNamedGroup> client_groups = {ssl_grp_ec_secp256r1,
ssl_grp_ffdhe_2048};
const std::vector<SSLNamedGroup> server_groups = {ssl_grp_ffdhe_2048};
client_->ConfigNamedGroups(client_groups);
server_->ConfigNamedGroups(server_groups);

Connect();
CheckKeys(ssl_kea_dh, ssl_auth_rsa_sign);
Expand Down Expand Up @@ -167,9 +167,9 @@ TEST_P(TlsConnectGenericPre13, P384PriorityOnServer) {
client_->EnableCiphersByKeyExchange(ssl_kea_ecdh);

// The server prefers P384. It has to win.
const std::vector<SSLNamedGroup> serverGroups = {
const std::vector<SSLNamedGroup> server_groups = {
ssl_grp_ec_secp384r1, ssl_grp_ec_secp256r1, ssl_grp_ec_secp521r1};
server_->ConfigNamedGroups(serverGroups);
server_->ConfigNamedGroups(server_groups);

Connect();

Expand Down Expand Up @@ -223,23 +223,23 @@ TEST_P(TlsConnectStreamPre13, ConfiguredGroupsRenegotiate) {
client_->DisableAllCiphers();
client_->EnableCiphersByKeyExchange(ssl_kea_ecdh);

const std::vector<SSLNamedGroup> serverGroups = {ssl_grp_ec_secp256r1,
ssl_grp_ec_secp384r1};
const std::vector<SSLNamedGroup> clientGroups = {ssl_grp_ec_secp384r1};
server_->ConfigNamedGroups(clientGroups);
client_->ConfigNamedGroups(serverGroups);
const std::vector<SSLNamedGroup> client_groups = {ssl_grp_ec_secp256r1};
const std::vector<SSLNamedGroup> server_groups = {ssl_grp_ec_secp256r1,
ssl_grp_ec_secp256r1};
client_->ConfigNamedGroups(client_groups);
server_->ConfigNamedGroups(server_groups);

Connect();

CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp384r1, ssl_auth_rsa_sign,
CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp256r1, ssl_auth_rsa_sign,
ssl_sig_rsa_pss_sha256);
CheckConnected();

// The renegotiation has to use the same preferences as the original session.
server_->PrepareForRenegotiate();
client_->StartRenegotiate();
Handshake();
CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp384r1, ssl_auth_rsa_sign,
CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp256r1, ssl_auth_rsa_sign,
ssl_sig_rsa_pss_sha256);
}

Expand All @@ -257,33 +257,162 @@ TEST_P(TlsKeyExchangeTest, Curve25519) {
CheckKEXDetails(groups, shares);
}

TEST_P(TlsConnectGeneric, P256andCurve25519OnlyServer) {
TEST_P(TlsConnectGenericPre13, GroupPreferenceServerPriority) {
EnsureTlsSetup();
client_->DisableAllCiphers();
client_->EnableCiphersByKeyExchange(ssl_kea_ecdh);

// the client sends a P256 key share while the server prefers 25519
// The client prefers P256 while the server prefers 25519.
// The server's preference has to win.
const std::vector<SSLNamedGroup> client_groups = {ssl_grp_ec_secp256r1,
ssl_grp_ec_curve25519};
const std::vector<SSLNamedGroup> server_groups = {ssl_grp_ec_curve25519,
ssl_grp_ec_secp256r1};
client_->ConfigNamedGroups(client_groups);
server_->ConfigNamedGroups(server_groups);

Connect();

CheckKeys(ssl_kea_ecdh, ssl_grp_ec_curve25519, ssl_auth_rsa_sign,
ssl_sig_rsa_pss_sha256);
}

#ifndef NSS_DISABLE_TLS_1_3
TEST_P(TlsKeyExchangeTest13, Curve25519P256EqualPriorityClient13) {
EnsureKeyShareSetup();

// The client sends a P256 key share while the server prefers 25519.
// We have to accept P256 without retry.
const std::vector<SSLNamedGroup> client_groups = {ssl_grp_ec_secp256r1,
ssl_grp_ec_curve25519};
const std::vector<SSLNamedGroup> server_groups = {ssl_grp_ec_curve25519,
ssl_grp_ec_secp256r1};
client_->ConfigNamedGroups(client_groups);
server_->ConfigNamedGroups(server_groups);

Connect();

CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp256r1, ssl_auth_rsa_sign,
ssl_sig_rsa_pss_sha256);
const std::vector<SSLNamedGroup> shares = {ssl_grp_ec_secp256r1};
CheckKEXDetails(client_groups, shares, false);
}

TEST_P(TlsKeyExchangeTest13, Curve25519P256EqualPriorityServer13) {
EnsureKeyShareSetup();

// The client sends a 25519 key share while the server prefers P256.
// We have to accept 25519 without retry.
const std::vector<SSLNamedGroup> client_groups = {ssl_grp_ec_curve25519,
ssl_grp_ec_secp256r1};
const std::vector<SSLNamedGroup> server_groups = {ssl_grp_ec_secp256r1,
ssl_grp_ec_curve25519};
client_->ConfigNamedGroups(client_groups);
server_->ConfigNamedGroups(server_groups);

Connect();

CheckKeys(ssl_kea_ecdh, ssl_grp_ec_curve25519, ssl_auth_rsa_sign,
ssl_sig_rsa_pss_sha256);
const std::vector<SSLNamedGroup> shares = {ssl_grp_ec_curve25519};
CheckKEXDetails(client_groups, shares, false);
}

TEST_P(TlsKeyExchangeTest13, EqualPriorityTestRetryECServer13) {
EnsureKeyShareSetup();

// The client sends a 25519 key share while the server prefers P256.
// The server prefers P-384 over x25519, so it must not consider P-256 and
// x25519 to be equivalent. It will therefore request a P-256 share
// with a HelloRetryRequest.
const std::vector<SSLNamedGroup> client_groups = {
ssl_grp_ec_curve25519, ssl_grp_ec_secp256r1, ssl_grp_ec_secp384r1};
const std::vector<SSLNamedGroup> server_groups = {
ssl_grp_ec_secp256r1, ssl_grp_ec_secp384r1, ssl_grp_ec_curve25519};
client_->ConfigNamedGroups(client_groups);
server_->ConfigNamedGroups(server_groups);

Connect();

CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp256r1, ssl_auth_rsa_sign,
ssl_sig_rsa_pss_sha256);
const std::vector<SSLNamedGroup> shares = {ssl_grp_ec_curve25519};
CheckKEXDetails(client_groups, shares, true);
}

TEST_P(TlsKeyExchangeTest13, NotEqualPriorityWithIntermediateGroup13) {
EnsureKeyShareSetup();

// The client sends a 25519 key share while the server prefers P256.
// The server prefers ffdhe_2048 over x25519, so it must not consider the
// P-256 and x25519 to be equivalent. It will therefore request a P-256 share
// with a HelloRetryRequest.
const std::vector<SSLNamedGroup> client_groups = {
ssl_grp_ec_curve25519, ssl_grp_ec_secp256r1, ssl_grp_ffdhe_2048};
const std::vector<SSLNamedGroup> server_groups = {
ssl_grp_ec_secp256r1, ssl_grp_ffdhe_2048, ssl_grp_ec_curve25519};
client_->ConfigNamedGroups(client_groups);
server_->ConfigNamedGroups(server_groups);

Connect();

CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp256r1, ssl_auth_rsa_sign,
ssl_sig_rsa_pss_sha256);
const std::vector<SSLNamedGroup> shares = {ssl_grp_ec_curve25519};
CheckKEXDetails(client_groups, shares, true);
}

TEST_P(TlsKeyExchangeTest13,
NotEqualPriorityWithUnsupportedIntermediateGroup13) {
EnsureKeyShareSetup();

// As in the previous test, the server prefers ffdhe_2048. Thus, even though
// the client doesn't support this group, the server must not regard x25519 as
// equivalent to P-256.
const std::vector<SSLNamedGroup> client_groups = {ssl_grp_ec_curve25519,
ssl_grp_ec_secp256r1};
const std::vector<SSLNamedGroup> server_groups = {
ssl_grp_ec_secp256r1, ssl_grp_ffdhe_2048, ssl_grp_ec_curve25519};
client_->ConfigNamedGroups(client_groups);
server_->ConfigNamedGroups(server_groups);

Connect();

CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp256r1, ssl_auth_rsa_sign,
ssl_sig_rsa_pss_sha256);
const std::vector<SSLNamedGroup> shares = {ssl_grp_ec_curve25519};
CheckKEXDetails(client_groups, shares, true);
}

TEST_P(TlsKeyExchangeTest13, EqualPriority13) {
EnsureKeyShareSetup();

// The client sends a 25519 key share while the server prefers P256.
// We have to accept 25519 without retry because it's considered equivalent to
// P256 by the server.
const std::vector<SSLNamedGroup> client_groups = {
ssl_grp_ec_curve25519, ssl_grp_ffdhe_2048, ssl_grp_ec_secp256r1};
const std::vector<SSLNamedGroup> server_groups = {ssl_grp_ec_secp256r1,
ssl_grp_ec_curve25519};
client_->ConfigNamedGroups(client_groups);
server_->ConfigNamedGroups(server_groups);

Connect();

CheckKeys();
const std::vector<SSLNamedGroup> shares = {ssl_grp_ec_curve25519};
CheckKEXDetails(client_groups, shares, false);
}
#endif

TEST_P(TlsConnectGeneric, P256ClientAndCurve25519Server) {
EnsureTlsSetup();
client_->DisableAllCiphers();
client_->EnableCiphersByKeyExchange(ssl_kea_ecdh);

// the client sends a P256 key share while the server prefers 25519.
const std::vector<SSLNamedGroup> server_groups = {ssl_grp_ec_curve25519};
// The client sends a P256 key share while the server prefers 25519.
const std::vector<SSLNamedGroup> client_groups = {ssl_grp_ec_secp256r1};
const std::vector<SSLNamedGroup> server_groups = {ssl_grp_ec_curve25519};

client_->ConfigNamedGroups(client_groups);
server_->ConfigNamedGroups(server_groups);
Expand All @@ -293,6 +422,31 @@ TEST_P(TlsConnectGeneric, P256ClientAndCurve25519Server) {
server_->CheckErrorCode(SSL_ERROR_NO_CYPHER_OVERLAP);
}

TEST_P(TlsKeyExchangeTest13, MultipleClientShares) {
EnsureKeyShareSetup();

// The client sends 25519 and P256 key shares. The server prefers P256,
// which must be chosen here.
const std::vector<SSLNamedGroup> client_groups = {ssl_grp_ec_curve25519,
ssl_grp_ec_secp256r1};
const std::vector<SSLNamedGroup> server_groups = {ssl_grp_ec_secp256r1,
ssl_grp_ec_curve25519};
client_->ConfigNamedGroups(client_groups);
server_->ConfigNamedGroups(server_groups);

// Generate a key share on the client for both curves.
EXPECT_EQ(SECSuccess, SSL_SendAdditionalKeyShares(client_->ssl_fd(), 1));

Connect();

// The server would accept 25519 but its preferred group (P256) has to win.
CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp256r1, ssl_auth_rsa_sign,
ssl_sig_rsa_pss_sha256);
const std::vector<SSLNamedGroup> shares = {ssl_grp_ec_curve25519,
ssl_grp_ec_secp256r1};
CheckKEXDetails(client_groups, shares, false);
}

// Replace the point in the client key exchange message with an empty one
class ECCClientKEXFilter : public TlsHandshakeFilter {
public:
Expand Down Expand Up @@ -355,4 +509,10 @@ INSTANTIATE_TEST_CASE_P(KeyExchangeTest, TlsKeyExchangeTest,
::testing::Combine(TlsConnectTestBase::kTlsModesAll,
TlsConnectTestBase::kTlsV11Plus));

#ifndef NSS_DISABLE_TLS_1_3
INSTANTIATE_TEST_CASE_P(KeyExchangeTest, TlsKeyExchangeTest13,
::testing::Combine(TlsConnectTestBase::kTlsModesAll,
TlsConnectTestBase::kTlsV13));
#endif

} // namespace nss_test
12 changes: 6 additions & 6 deletions external_tests/ssl_gtest/ssl_hrr_unittest.cc
View file
Expand Up @@ -115,14 +115,14 @@ TEST_F(TlsConnectDatagram13, DropClientSecondFlightWithHelloRetry) {

class TlsKeyExchange13 : public TlsKeyExchangeTest {};

// This should work, with an HRR, because the server prefers P-256 and the
// This should work, with an HRR, because the server prefers x25519 and the
// client generates a share for P-384 on the initial ClientHello.
TEST_P(TlsKeyExchange13, ConnectEcdhePreferenceMismatchHrr) {
EnsureKeyShareSetup();
static const std::vector<SSLNamedGroup> client_groups = {
ssl_grp_ec_secp384r1, ssl_grp_ec_secp256r1};
ssl_grp_ec_secp384r1, ssl_grp_ec_curve25519};
static const std::vector<SSLNamedGroup> server_groups = {
ssl_grp_ec_secp256r1, ssl_grp_ec_secp384r1};
ssl_grp_ec_curve25519, ssl_grp_ec_secp384r1};
client_->ConfigNamedGroups(client_groups);
server_->ConfigNamedGroups(server_groups);
Connect();
Expand All @@ -132,14 +132,14 @@ TEST_P(TlsKeyExchange13, ConnectEcdhePreferenceMismatchHrr) {
CheckKEXDetails(client_groups, expectedShares, true /* expect_hrr */);
}

// This should work, but not use HRR because the key share for P-256 was
// This should work, but not use HRR because the key share for x25519 was
// pre-generated by the client.
TEST_P(TlsKeyExchange13, ConnectEcdhePreferenceMismatchHrrExtraShares) {
EnsureKeyShareSetup();
static const std::vector<SSLNamedGroup> client_groups = {
ssl_grp_ec_secp384r1, ssl_grp_ec_secp256r1};
ssl_grp_ec_secp384r1, ssl_grp_ec_curve25519};
static const std::vector<SSLNamedGroup> server_groups = {
ssl_grp_ec_secp256r1, ssl_grp_ec_secp384r1};
ssl_grp_ec_curve25519, ssl_grp_ec_secp384r1};
client_->ConfigNamedGroups(client_groups);
server_->ConfigNamedGroups(server_groups);
EXPECT_EQ(SECSuccess, SSL_SendAdditionalKeyShares(client_->ssl_fd(), 1));
Expand Down
4 changes: 2 additions & 2 deletions external_tests/ssl_gtest/ssl_resumption_unittest.cc
View file
Expand Up @@ -386,7 +386,7 @@ TEST_F(TlsConnectTest, TestTls13ResumptionTwice) {
ExpectResumption(RESUME_TICKET);
Connect();
SendReceive();
CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp256r1, ssl_auth_rsa_sign,
CheckKeys(ssl_kea_ecdh, ssl_grp_ec_curve25519, ssl_auth_rsa_sign,
ssl_sig_none);
// The filter will go away when we reset, so save the captured extension.
DataBuffer initialTicket(c1->extension());
Expand All @@ -408,7 +408,7 @@ TEST_F(TlsConnectTest, TestTls13ResumptionTwice) {
ExpectResumption(RESUME_TICKET);
Connect();
SendReceive();
CheckKeys(ssl_kea_ecdh, ssl_grp_ec_secp256r1, ssl_auth_rsa_sign,
CheckKeys(ssl_kea_ecdh, ssl_grp_ec_curve25519, ssl_auth_rsa_sign,
ssl_sig_none);
ASSERT_LT(0U, c2->extension().len());

Expand Down
14 changes: 9 additions & 5 deletions external_tests/ssl_gtest/tls_agent.cc
View file
Expand Up @@ -234,20 +234,21 @@ void TlsAgent::DisableAllCiphers() {
// Not actually all groups, just the onece that we are actually willing
// to use.
const std::vector<SSLNamedGroup> kAllDHEGroups = {
ssl_grp_ec_secp256r1, ssl_grp_ec_secp384r1, ssl_grp_ec_secp521r1,
ssl_grp_ffdhe_2048, ssl_grp_ffdhe_3072, ssl_grp_ffdhe_4096,
ssl_grp_ffdhe_6144, ssl_grp_ffdhe_8192};
ssl_grp_ec_curve25519, ssl_grp_ec_secp256r1, ssl_grp_ec_secp384r1,
ssl_grp_ec_secp521r1, ssl_grp_ffdhe_2048, ssl_grp_ffdhe_3072,
ssl_grp_ffdhe_4096, ssl_grp_ffdhe_6144, ssl_grp_ffdhe_8192};

const std::vector<SSLNamedGroup> kECDHEGroups = {
ssl_grp_ec_secp256r1, ssl_grp_ec_secp384r1, ssl_grp_ec_secp521r1};
ssl_grp_ec_curve25519, ssl_grp_ec_secp256r1, ssl_grp_ec_secp384r1,
ssl_grp_ec_secp521r1};

const std::vector<SSLNamedGroup> kFFDHEGroups = {
ssl_grp_ffdhe_2048, ssl_grp_ffdhe_3072, ssl_grp_ffdhe_4096,
ssl_grp_ffdhe_6144, ssl_grp_ffdhe_8192};

// Defined because the big DHE groups are ridiculously slow.
const std::vector<SSLNamedGroup> kFasterDHEGroups = {
ssl_grp_ec_secp256r1, ssl_grp_ec_secp384r1, ssl_grp_ec_secp521r1,
ssl_grp_ec_curve25519, ssl_grp_ec_secp256r1, ssl_grp_ec_secp384r1,
ssl_grp_ffdhe_2048, ssl_grp_ffdhe_3072};

void TlsAgent::EnableCiphersByKeyExchange(SSLKEAType kea) {
Expand Down Expand Up @@ -843,6 +844,9 @@ void TlsAgentTestBase::EnsureInit() {
if (!agent_) {
Init();
}
const std::vector<SSLNamedGroup> groups = {
ssl_grp_ec_secp256r1, ssl_grp_ec_secp384r1, ssl_grp_ffdhe_2048};
agent_->ConfigNamedGroups(groups);
}

void TlsAgentTestBase::ProcessMessage(const DataBuffer& buffer,
Expand Down

0 comments on commit dda02ee

Please sign in to comment.