Skip to content

Commit

Permalink
Bug 1308389 - Validate server selected cipher when resuming TLS 1.3, …
Browse files Browse the repository at this point in the history 
…r=ekr

--HG--
extra : rebase_source : fc35def176e6a23d7c0855cb7b655dacbab8d577
  • Loading branch information
@martinthomson
martinthomson committed Oct 7, 2016
1 parent 6ca6aa7 commit e7d1942
Show file tree
Hide file tree
Showing 7 changed files with 73 additions and 40 deletions.
72 changes: 51 additions & 21 deletions external_tests/ssl_gtest/ssl_resumption_unittest.cc
View file
Expand Up @@ -170,13 +170,9 @@ TEST_P(TlsConnectGeneric, ConnectResumeClientNoneServerBoth) {
}

TEST_P(TlsConnectGenericPre13, ConnectResumeWithHigherVersion) {
EnsureTlsSetup();
SetExpectedVersion(SSL_LIBRARY_VERSION_TLS_1_1);
ConfigureSessionCache(RESUME_SESSIONID, RESUME_SESSIONID);
client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
SSL_LIBRARY_VERSION_TLS_1_1);
server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
SSL_LIBRARY_VERSION_TLS_1_1);
ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_1);
SetExpectedVersion(SSL_LIBRARY_VERSION_TLS_1_1);
Connect();

Reset();
Expand Down Expand Up @@ -219,7 +215,6 @@ TEST_P(TlsConnectGeneric, ServerSNICertSwitch) {
ScopedCERTCertificate cert1(SSL_PeerCertificate(client_->ssl_fd()));

Reset();
EnsureTlsSetup();
ConfigureSessionCache(RESUME_NONE, RESUME_NONE);

server_->SetSniCallback(SwitchCertificates);
Expand All @@ -236,7 +231,6 @@ TEST_P(TlsConnectGeneric, ServerSNICertTypeSwitch) {
ScopedCERTCertificate cert1(SSL_PeerCertificate(client_->ssl_fd()));

Reset();
EnsureTlsSetup();
ConfigureSessionCache(RESUME_NONE, RESUME_NONE);

// Because we configure an RSA certificate here, it only adds a second, unused
Expand Down Expand Up @@ -360,14 +354,55 @@ TEST_P(TlsConnectTls13, TestTls13ResumeServerDifferentCipher) {
CheckKeys();
}

class SelectedCipherSuiteReplacer : public TlsHandshakeFilter {
public:
SelectedCipherSuiteReplacer(uint16_t suite) : cipher_suite_(suite) {}

protected:
PacketFilter::Action FilterHandshake(const HandshakeHeader& header,
const DataBuffer& input,
DataBuffer* output) override {
if (header.handshake_type() != kTlsHandshakeServerHello) {
return KEEP;
}

*output = input;
// Cipher suite is after version(2) and random(32).
output->Write(34, static_cast<uint32_t>(cipher_suite_), 2);
return CHANGE;
}

private:
uint16_t cipher_suite_;
};

// Test that the client won't tolerate the server picking a different cipher
// suite for resumption. (Stream only because the server is unable to decrypt
// the alert that the client sends, see bug 1304603.)
TEST_F(TlsConnectTest, TestTls13ResumeOverrideCipher) {
ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
server_->EnableSingleCipher(TLS_AES_128_GCM_SHA256);
Connect();
SendReceive();
CheckKeys(ssl_kea_ecdh, ssl_auth_rsa_sign);

Reset();
ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
server_->SetPacketFilter(
new SelectedCipherSuiteReplacer(TLS_CHACHA20_POLY1305_SHA256));
ConnectExpectFail();
client_->CheckErrorCode(SSL_ERROR_RX_MALFORMED_SERVER_HELLO);
server_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ);
}

// Test that two TLS resumptions work and produce the same ticket.
// This will change after bug 1257047 is fixed.
TEST_F(TlsConnectTest, TestTls13ResumptionTwice) {
ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
SSL_LIBRARY_VERSION_TLS_1_3);
server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
SSL_LIBRARY_VERSION_TLS_1_3);
ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);

Connect();
SendReceive(); // Need to read so that we absorb the session ticket.
CheckKeys();
Expand All @@ -376,14 +411,11 @@ TEST_F(TlsConnectTest, TestTls13ResumptionTwice) {

Reset();
ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
ExpectResumption(RESUME_TICKET);
TlsExtensionCapture* c1 =
new TlsExtensionCapture(ssl_tls13_pre_shared_key_xtn);
client_->SetPacketFilter(c1);
client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
SSL_LIBRARY_VERSION_TLS_1_3);
server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
SSL_LIBRARY_VERSION_TLS_1_3);
ExpectResumption(RESUME_TICKET);
Connect();
SendReceive();
CheckKeys(ssl_kea_ecdh, ssl_grp_ec_curve25519, ssl_auth_rsa_sign,
Expand All @@ -398,13 +430,10 @@ TEST_F(TlsConnectTest, TestTls13ResumptionTwice) {
Reset();
ClearStats();
ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
TlsExtensionCapture* c2 =
new TlsExtensionCapture(ssl_tls13_pre_shared_key_xtn);
client_->SetPacketFilter(c2);
client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
SSL_LIBRARY_VERSION_TLS_1_3);
server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
SSL_LIBRARY_VERSION_TLS_1_3);
ExpectResumption(RESUME_TICKET);
Connect();
SendReceive();
Expand All @@ -425,4 +454,5 @@ TEST_F(TlsConnectTest, TestTls13ResumptionTwice) {

ASSERT_NE(initialTicket, c2->extension());
}

} // namespace nss_test
12 changes: 4 additions & 8 deletions external_tests/ssl_gtest/ssl_v2_client_hello_unittest.cc
View file
Expand Up @@ -204,11 +204,9 @@ TEST_P(SSLv2ClientHelloTest, Connect) {

// Test negotiating TLS 1.3.
TEST_F(SSLv2ClientHelloTestF, Connect13) {
EnsureTlsSetup();
SetExpectedVersion(SSL_LIBRARY_VERSION_TLS_1_3);
client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_3,
SSL_LIBRARY_VERSION_TLS_1_3);
server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_3,
SSL_LIBRARY_VERSION_TLS_1_3);
ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);

std::vector<uint16_t> cipher_suites = {TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256};
SetAvailableCipherSuites(cipher_suites);
Expand Down Expand Up @@ -317,11 +315,9 @@ TEST_P(SSLv2ClientHelloTest, RequireSafeRenegotiationWithSCSV) {
// a higher version. As the server doesn't support anything higher than TLS 1.1
// it must accept the connection.
TEST_F(SSLv2ClientHelloTestF, FallbackSCSV) {
EnsureTlsSetup();
SetExpectedVersion(SSL_LIBRARY_VERSION_TLS_1_1);
client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
SSL_LIBRARY_VERSION_TLS_1_1);
server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
SSL_LIBRARY_VERSION_TLS_1_1);
ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_1);

std::vector<uint16_t> cipher_suites = {TLS_DHE_RSA_WITH_AES_128_CBC_SHA,
TLS_FALLBACK_SCSV};
Expand Down
12 changes: 4 additions & 8 deletions external_tests/ssl_gtest/ssl_version_unittest.cc
View file
Expand Up @@ -206,21 +206,17 @@ TEST_P(TlsConnectStream, ConnectTls10AndClientRenegotiateHigher) {
}

TEST_F(TlsConnectTest, Tls13RejectsRehandshakeClient) {
client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
SSL_LIBRARY_VERSION_TLS_1_3);
server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
SSL_LIBRARY_VERSION_TLS_1_3);
EnsureTlsSetup();
ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
Connect();
SECStatus rv = SSL_ReHandshake(client_->ssl_fd(), PR_TRUE);
EXPECT_EQ(SECFailure, rv);
EXPECT_EQ(SSL_ERROR_RENEGOTIATION_NOT_ALLOWED, PORT_GetError());
}

TEST_F(TlsConnectTest, Tls13RejectsRehandshakeServer) {
client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
SSL_LIBRARY_VERSION_TLS_1_3);
server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
SSL_LIBRARY_VERSION_TLS_1_3);
EnsureTlsSetup();
ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
Connect();
SECStatus rv = SSL_ReHandshake(server_->ssl_fd(), PR_TRUE);
EXPECT_EQ(SECFailure, rv);
Expand Down
8 changes: 6 additions & 2 deletions external_tests/ssl_gtest/tls_connect.cc
View file
Expand Up @@ -199,8 +199,7 @@ void TlsConnectTestBase::Init() {
server_->SetPeer(client_);

if (version_) {
client_->SetVersionRange(version_, version_);
server_->SetVersionRange(version_, version_);
ConfigureVersion(version_);
}
}

Expand Down Expand Up @@ -374,6 +373,11 @@ void TlsConnectTestBase::ConnectExpectFail() {
ASSERT_EQ(TlsAgent::STATE_ERROR, server_->state());
}

void TlsConnectTestBase::ConfigureVersion(uint16_t version) {
client_->SetVersionRange(version, version);
server_->SetVersionRange(version, version);
}

void TlsConnectTestBase::SetExpectedVersion(uint16_t version) {
client_->SetExpectedVersion(version);
server_->SetExpectedVersion(version);
Expand Down
1 change: 1 addition & 0 deletions external_tests/ssl_gtest/tls_connect.h
View file
Expand Up @@ -81,6 +81,7 @@ class TlsConnectTestBase : public ::testing::Test {
void CheckShares(const DataBuffer& shares,
std::function<void(SSLNamedGroup)> check_group);

void ConfigureVersion(uint16_t version);
void SetExpectedVersion(uint16_t version);
// Expect resumption of a particular type.
void ExpectResumption(SessionResumptionMode expected);
Expand Down
2 changes: 1 addition & 1 deletion lib/ssl/ssl3con.c
View file
Expand Up @@ -350,7 +350,7 @@ static const ssl3KEADef kea_defs[] =
/* must use ssl_LookupCipherSuiteDef to access */
static const ssl3CipherSuiteDef cipher_suite_defs[] =
{
/* cipher_suite bulk_cipher_alg mac_alg key_exchange_alg prf_hash_alg */
/* cipher_suite bulk_cipher_alg mac_alg key_exchange_alg prf_hash */
/* Note that the prf_hash_alg is the hash function used by the PRF, see sslimpl.h. */

{TLS_NULL_WITH_NULL_NULL, cipher_null, mac_null, kea_null, ssl_hash_none},
Expand Down
6 changes: 6 additions & 0 deletions lib/ssl/tls13con.c
View file
Expand Up @@ -1983,6 +1983,12 @@ tls13_HandleServerHelloPart2(sslSocket *ss)
unexpected_message);
return SECFailure;
}

if (ss->ssl3.hs.cipher_suite != sid->u.ssl3.cipherSuite) {
FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_SERVER_HELLO,
illegal_parameter);
return SECFailure;
}
} else {
if (!ssl3_ExtensionNegotiated(ss, ssl_signature_algorithms_xtn)) {
FATAL_ERROR(ss, SSL_ERROR_MISSING_SIGNATURE_ALGORITHMS_EXTENSION,
Expand Down

0 comments on commit e7d1942

Please sign in to comment.