From 5eacfc3d2dd223e95f02c07249df7ff3c03299d0 Mon Sep 17 00:00:00 2001 From: Martin Thomson Date: Mon, 4 Sep 2017 15:05:11 +1000 Subject: [PATCH] Bug 1375837 - Allow TLS 1.3 when renegotiation isn't enabled, r=ekr --HG-- extra : rebase_source : 7ba119b3f792ff3100e367888c3b97a8c9c859aa --- gtests/ssl_gtest/manifest.mn | 1 + gtests/ssl_gtest/ssl_cert_ext_unittest.cc | 25 +-- gtests/ssl_gtest/ssl_dhe_unittest.cc | 24 +-- gtests/ssl_gtest/ssl_gtest.gyp | 1 + gtests/ssl_gtest/ssl_loopback_unittest.cc | 22 +-- .../ssl_gtest/ssl_renegotiation_unittest.cc | 144 ++++++++++++++++++ gtests/ssl_gtest/ssl_resumption_unittest.cc | 9 +- gtests/ssl_gtest/ssl_staticrsa_unittest.cc | 4 +- .../ssl_gtest/ssl_v2_client_hello_unittest.cc | 11 +- gtests/ssl_gtest/ssl_version_unittest.cc | 100 +----------- gtests/ssl_gtest/tls_agent.cc | 84 ++-------- gtests/ssl_gtest/tls_agent.h | 5 +- lib/ssl/ssl3con.c | 44 +++--- lib/ssl/ssl3exthandle.c | 8 +- 14 files changed, 216 insertions(+), 266 deletions(-) create mode 100644 gtests/ssl_gtest/ssl_renegotiation_unittest.cc diff --git a/gtests/ssl_gtest/manifest.mn b/gtests/ssl_gtest/manifest.mn index e7d3e10878..967ee9ca9a 100644 --- a/gtests/ssl_gtest/manifest.mn +++ b/gtests/ssl_gtest/manifest.mn @@ -33,6 +33,7 @@ CPPSRCS = \ ssl_misc_unittest.cc \ ssl_record_unittest.cc \ ssl_resumption_unittest.cc \ + ssl_renegotiation_unittest.cc \ ssl_skip_unittest.cc \ ssl_staticrsa_unittest.cc \ ssl_v2_client_hello_unittest.cc \ diff --git a/gtests/ssl_gtest/ssl_cert_ext_unittest.cc b/gtests/ssl_gtest/ssl_cert_ext_unittest.cc index 3463782e09..36ee104afd 100644 --- a/gtests/ssl_gtest/ssl_cert_ext_unittest.cc +++ b/gtests/ssl_gtest/ssl_cert_ext_unittest.cc @@ -82,9 +82,8 @@ TEST_P(TlsConnectGenericPre13, SignedCertificateTimestampsLegacy) { ssl_kea_rsa)); EXPECT_EQ(SECSuccess, SSL_SetSignedCertTimestamps(server_->ssl_fd(), &kSctItem, ssl_kea_rsa)); - EXPECT_EQ(SECSuccess, - SSL_OptionSet(client_->ssl_fd(), SSL_ENABLE_SIGNED_CERT_TIMESTAMPS, - PR_TRUE)); + + client_->SetOption(SSL_ENABLE_SIGNED_CERT_TIMESTAMPS, PR_TRUE); SignedCertificateTimestampsExtractor timestamps_extractor(client_); Connect(); @@ -96,9 +95,7 @@ TEST_P(TlsConnectGeneric, SignedCertificateTimestampsSuccess) { EnsureTlsSetup(); EXPECT_TRUE( server_->ConfigServerCert(TlsAgent::kServerRsa, true, &kExtraSctData)); - EXPECT_EQ(SECSuccess, - SSL_OptionSet(client_->ssl_fd(), SSL_ENABLE_SIGNED_CERT_TIMESTAMPS, - PR_TRUE)); + client_->SetOption(SSL_ENABLE_SIGNED_CERT_TIMESTAMPS, PR_TRUE); SignedCertificateTimestampsExtractor timestamps_extractor(client_); Connect(); @@ -120,9 +117,7 @@ TEST_P(TlsConnectGeneric, SignedCertificateTimestampsInactiveClient) { TEST_P(TlsConnectGeneric, SignedCertificateTimestampsInactiveServer) { EnsureTlsSetup(); - EXPECT_EQ(SECSuccess, - SSL_OptionSet(client_->ssl_fd(), SSL_ENABLE_SIGNED_CERT_TIMESTAMPS, - PR_TRUE)); + client_->SetOption(SSL_ENABLE_SIGNED_CERT_TIMESTAMPS, PR_TRUE); SignedCertificateTimestampsExtractor timestamps_extractor(client_); Connect(); @@ -173,16 +168,14 @@ TEST_P(TlsConnectGeneric, OcspNotRequested) { // Even if the client asks, the server has nothing unless it is configured. TEST_P(TlsConnectGeneric, OcspNotProvided) { EnsureTlsSetup(); - EXPECT_EQ(SECSuccess, SSL_OptionSet(client_->ssl_fd(), - SSL_ENABLE_OCSP_STAPLING, PR_TRUE)); + client_->SetOption(SSL_ENABLE_OCSP_STAPLING, PR_TRUE); client_->SetAuthCertificateCallback(CheckNoOCSP); Connect(); } TEST_P(TlsConnectGenericPre13, OcspMangled) { EnsureTlsSetup(); - EXPECT_EQ(SECSuccess, SSL_OptionSet(client_->ssl_fd(), - SSL_ENABLE_OCSP_STAPLING, PR_TRUE)); + client_->SetOption(SSL_ENABLE_OCSP_STAPLING, PR_TRUE); EXPECT_TRUE( server_->ConfigServerCert(TlsAgent::kServerRsa, true, &kOcspExtraData)); @@ -197,8 +190,7 @@ TEST_P(TlsConnectGenericPre13, OcspMangled) { TEST_P(TlsConnectGeneric, OcspSuccess) { EnsureTlsSetup(); - EXPECT_EQ(SECSuccess, SSL_OptionSet(client_->ssl_fd(), - SSL_ENABLE_OCSP_STAPLING, PR_TRUE)); + client_->SetOption(SSL_ENABLE_OCSP_STAPLING, PR_TRUE); auto capture_ocsp = std::make_shared(ssl_cert_status_xtn); server_->SetPacketFilter(capture_ocsp); @@ -225,8 +217,7 @@ TEST_P(TlsConnectGeneric, OcspSuccess) { TEST_P(TlsConnectGeneric, OcspHugeSuccess) { EnsureTlsSetup(); - EXPECT_EQ(SECSuccess, SSL_OptionSet(client_->ssl_fd(), - SSL_ENABLE_OCSP_STAPLING, PR_TRUE)); + client_->SetOption(SSL_ENABLE_OCSP_STAPLING, PR_TRUE); uint8_t hugeOcspValue[16385]; memset(hugeOcspValue, 0xa1, sizeof(hugeOcspValue)); diff --git a/gtests/ssl_gtest/ssl_dhe_unittest.cc b/gtests/ssl_gtest/ssl_dhe_unittest.cc index 97943303ad..da8680cfe9 100644 --- a/gtests/ssl_gtest/ssl_dhe_unittest.cc +++ b/gtests/ssl_gtest/ssl_dhe_unittest.cc @@ -59,8 +59,7 @@ TEST_P(TlsConnectTls13, SharesForBothEcdheAndDhe) { TEST_P(TlsConnectGeneric, ConnectFfdheClient) { EnableOnlyDheCiphers(); - EXPECT_EQ(SECSuccess, SSL_OptionSet(client_->ssl_fd(), - SSL_REQUIRE_DH_NAMED_GROUPS, PR_TRUE)); + client_->SetOption(SSL_REQUIRE_DH_NAMED_GROUPS, PR_TRUE); auto groups_capture = std::make_shared(ssl_supported_groups_xtn); auto shares_capture = @@ -90,8 +89,7 @@ TEST_P(TlsConnectGeneric, ConnectFfdheClient) { // because the client automatically sends the supported groups extension. TEST_P(TlsConnectGenericPre13, ConnectFfdheServer) { EnableOnlyDheCiphers(); - EXPECT_EQ(SECSuccess, SSL_OptionSet(server_->ssl_fd(), - SSL_REQUIRE_DH_NAMED_GROUPS, PR_TRUE)); + server_->SetOption(SSL_REQUIRE_DH_NAMED_GROUPS, PR_TRUE); if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) { Connect(); @@ -126,8 +124,7 @@ class TlsDheServerKeyExchangeDamager : public TlsHandshakeFilter { // the signature until everything else has been checked. TEST_P(TlsConnectGenericPre13, DamageServerKeyShare) { EnableOnlyDheCiphers(); - EXPECT_EQ(SECSuccess, SSL_OptionSet(client_->ssl_fd(), - SSL_REQUIRE_DH_NAMED_GROUPS, PR_TRUE)); + client_->SetOption(SSL_REQUIRE_DH_NAMED_GROUPS, PR_TRUE); server_->SetPacketFilter(std::make_shared()); ConnectExpectAlert(client_, kTlsAlertIllegalParameter); @@ -289,8 +286,7 @@ class TlsDamageDHYTest TEST_P(TlsDamageDHYTest, DamageServerY) { EnableOnlyDheCiphers(); if (std::get<3>(GetParam())) { - EXPECT_EQ(SECSuccess, SSL_OptionSet(client_->ssl_fd(), - SSL_REQUIRE_DH_NAMED_GROUPS, PR_TRUE)); + client_->SetOption(SSL_REQUIRE_DH_NAMED_GROUPS, PR_TRUE); } TlsDheSkeChangeY::ChangeYTo change = std::get<2>(GetParam()); server_->SetPacketFilter( @@ -320,8 +316,7 @@ TEST_P(TlsDamageDHYTest, DamageServerY) { TEST_P(TlsDamageDHYTest, DamageClientY) { EnableOnlyDheCiphers(); if (std::get<3>(GetParam())) { - EXPECT_EQ(SECSuccess, SSL_OptionSet(client_->ssl_fd(), - SSL_REQUIRE_DH_NAMED_GROUPS, PR_TRUE)); + client_->SetOption(SSL_REQUIRE_DH_NAMED_GROUPS, PR_TRUE); } // The filter on the server is required to capture the prime. auto server_filter = @@ -445,8 +440,7 @@ TEST_P(TlsConnectGenericPre13, PadDheP) { // Note: This test case can take ages to generate the weak DH key. TEST_P(TlsConnectGenericPre13, WeakDHGroup) { EnableOnlyDheCiphers(); - EXPECT_EQ(SECSuccess, SSL_OptionSet(client_->ssl_fd(), - SSL_REQUIRE_DH_NAMED_GROUPS, PR_TRUE)); + client_->SetOption(SSL_REQUIRE_DH_NAMED_GROUPS, PR_TRUE); EXPECT_EQ(SECSuccess, SSL_EnableWeakDHEPrimeGroup(server_->ssl_fd(), PR_TRUE)); @@ -496,8 +490,7 @@ TEST_P(TlsConnectTls13, NamedGroupMismatch13) { // custom group in contrast to the previous test. TEST_P(TlsConnectGenericPre13, RequireNamedGroupsMismatchPre13) { EnableOnlyDheCiphers(); - EXPECT_EQ(SECSuccess, SSL_OptionSet(client_->ssl_fd(), - SSL_REQUIRE_DH_NAMED_GROUPS, PR_TRUE)); + client_->SetOption(SSL_REQUIRE_DH_NAMED_GROUPS, PR_TRUE); static const std::vector server_groups = {ssl_grp_ffdhe_3072}; static const std::vector client_groups = {ssl_grp_ec_secp256r1, ssl_grp_ffdhe_2048}; @@ -525,8 +518,7 @@ TEST_P(TlsConnectGenericPre13, PreferredFfdhe) { TEST_P(TlsConnectGenericPre13, MismatchDHE) { EnableOnlyDheCiphers(); - EXPECT_EQ(SECSuccess, SSL_OptionSet(client_->ssl_fd(), - SSL_REQUIRE_DH_NAMED_GROUPS, PR_TRUE)); + client_->SetOption(SSL_REQUIRE_DH_NAMED_GROUPS, PR_TRUE); static const SSLDHEGroupType serverGroups[] = {ssl_ff_dhe_3072_group}; EXPECT_EQ(SECSuccess, SSL_DHEGroupPrefSet(server_->ssl_fd(), serverGroups, PR_ARRAY_SIZE(serverGroups))); diff --git a/gtests/ssl_gtest/ssl_gtest.gyp b/gtests/ssl_gtest/ssl_gtest.gyp index 9bf147e4f5..5c43673260 100644 --- a/gtests/ssl_gtest/ssl_gtest.gyp +++ b/gtests/ssl_gtest/ssl_gtest.gyp @@ -34,6 +34,7 @@ 'ssl_misc_unittest.cc', 'ssl_record_unittest.cc', 'ssl_resumption_unittest.cc', + 'ssl_renegotiation_unittest.cc', 'ssl_skip_unittest.cc', 'ssl_staticrsa_unittest.cc', 'ssl_v2_client_hello_unittest.cc', diff --git a/gtests/ssl_gtest/ssl_loopback_unittest.cc b/gtests/ssl_gtest/ssl_loopback_unittest.cc index be97291efb..074b516300 100644 --- a/gtests/ssl_gtest/ssl_loopback_unittest.cc +++ b/gtests/ssl_gtest/ssl_loopback_unittest.cc @@ -168,24 +168,6 @@ TEST_P(TlsConnectDatagram, ConnectSrtp) { SendReceive(); } -// 1.3 is disabled in the next few tests because we don't -// presently support resumption in 1.3. -TEST_P(TlsConnectStreamPre13, ConnectAndClientRenegotiate) { - Connect(); - server_->PrepareForRenegotiate(); - client_->StartRenegotiate(); - Handshake(); - CheckConnected(); -} - -TEST_P(TlsConnectStreamPre13, ConnectAndServerRenegotiate) { - Connect(); - client_->PrepareForRenegotiate(); - server_->StartRenegotiate(); - Handshake(); - CheckConnected(); -} - TEST_P(TlsConnectGeneric, ConnectSendReceive) { Connect(); SendReceive(); @@ -230,8 +212,8 @@ TEST_P(TlsConnectStream, ShortRead) { // so we should never get it. TEST_P(TlsConnectGeneric, ConnectWithCompressionEnabled) { EnsureTlsSetup(); - client_->EnableCompression(); - server_->EnableCompression(); + client_->SetOption(SSL_ENABLE_DEFLATE, PR_TRUE); + server_->SetOption(SSL_ENABLE_DEFLATE, PR_TRUE); Connect(); EXPECT_FALSE(client_->is_compressed()); SendReceive(); diff --git a/gtests/ssl_gtest/ssl_renegotiation_unittest.cc b/gtests/ssl_gtest/ssl_renegotiation_unittest.cc new file mode 100644 index 0000000000..759fdcb24f --- /dev/null +++ b/gtests/ssl_gtest/ssl_renegotiation_unittest.cc @@ -0,0 +1,144 @@ +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ +/* vim: set ts=2 et sw=2 tw=80: */ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#include +#include +#include "secerr.h" +#include "ssl.h" +#include "sslerr.h" +#include "sslproto.h" + +#include "gtest_utils.h" +#include "tls_connect.h" + +namespace nss_test { + +// 1.3 is disabled in the next few tests because we don't +// presently support resumption in 1.3. +TEST_P(TlsConnectStreamPre13, RenegotiateClient) { + Connect(); + server_->PrepareForRenegotiate(); + client_->StartRenegotiate(); + Handshake(); + CheckConnected(); +} + +TEST_P(TlsConnectStreamPre13, RenegotiateServer) { + Connect(); + client_->PrepareForRenegotiate(); + server_->StartRenegotiate(); + Handshake(); + CheckConnected(); +} + +// The renegotiation options shouldn't cause an error if TLS 1.3 is chosen. +TEST_F(TlsConnectTest, RenegotiationConfigTls13) { + EnsureTlsSetup(); + ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3); + server_->SetOption(SSL_ENABLE_RENEGOTIATION, SSL_RENEGOTIATE_UNRESTRICTED); + server_->SetOption(SSL_REQUIRE_SAFE_NEGOTIATION, PR_TRUE); + Connect(); + SendReceive(); + CheckKeys(); +} + +TEST_P(TlsConnectStream, ConnectTls10AndServerRenegotiateHigher) { + if (version_ == SSL_LIBRARY_VERSION_TLS_1_0) { + return; + } + // Set the client so it will accept any version from 1.0 + // to |version_|. + client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_0, version_); + server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_0, + SSL_LIBRARY_VERSION_TLS_1_0); + // Reset version so that the checks succeed. + uint16_t test_version = version_; + version_ = SSL_LIBRARY_VERSION_TLS_1_0; + Connect(); + + // Now renegotiate, with the server being set to do + // |version_|. + client_->PrepareForRenegotiate(); + server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_0, test_version); + // Reset version and cipher suite so that the preinfo callback + // doesn't fail. + server_->ResetPreliminaryInfo(); + server_->StartRenegotiate(); + + if (test_version >= SSL_LIBRARY_VERSION_TLS_1_3) { + ExpectAlert(server_, kTlsAlertUnexpectedMessage); + } else { + ExpectAlert(client_, kTlsAlertIllegalParameter); + } + + Handshake(); + if (test_version >= SSL_LIBRARY_VERSION_TLS_1_3) { + // In TLS 1.3, the server detects this problem. + client_->CheckErrorCode(SSL_ERROR_HANDSHAKE_UNEXPECTED_ALERT); + server_->CheckErrorCode(SSL_ERROR_RENEGOTIATION_NOT_ALLOWED); + } else { + client_->CheckErrorCode(SSL_ERROR_UNSUPPORTED_VERSION); + server_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT); + } +} + +TEST_P(TlsConnectStream, ConnectTls10AndClientRenegotiateHigher) { + if (version_ == SSL_LIBRARY_VERSION_TLS_1_0) { + return; + } + // Set the client so it will accept any version from 1.0 + // to |version_|. + client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_0, version_); + server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_0, + SSL_LIBRARY_VERSION_TLS_1_0); + // Reset version so that the checks succeed. + uint16_t test_version = version_; + version_ = SSL_LIBRARY_VERSION_TLS_1_0; + Connect(); + + // Now renegotiate, with the server being set to do + // |version_|. + server_->PrepareForRenegotiate(); + server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_0, test_version); + // Reset version and cipher suite so that the preinfo callback + // doesn't fail. + server_->ResetPreliminaryInfo(); + client_->StartRenegotiate(); + if (test_version >= SSL_LIBRARY_VERSION_TLS_1_3) { + ExpectAlert(server_, kTlsAlertUnexpectedMessage); + } else { + ExpectAlert(client_, kTlsAlertIllegalParameter); + } + Handshake(); + if (test_version >= SSL_LIBRARY_VERSION_TLS_1_3) { + // In TLS 1.3, the server detects this problem. + client_->CheckErrorCode(SSL_ERROR_HANDSHAKE_UNEXPECTED_ALERT); + server_->CheckErrorCode(SSL_ERROR_RENEGOTIATION_NOT_ALLOWED); + } else { + client_->CheckErrorCode(SSL_ERROR_UNSUPPORTED_VERSION); + server_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT); + } +} + +TEST_F(TlsConnectTest, Tls13RejectsRehandshakeClient) { + 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) { + EnsureTlsSetup(); + ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3); + Connect(); + SECStatus rv = SSL_ReHandshake(server_->ssl_fd(), PR_TRUE); + EXPECT_EQ(SECFailure, rv); + EXPECT_EQ(SSL_ERROR_RENEGOTIATION_NOT_ALLOWED, PORT_GetError()); +} + +} // namespace nss_test diff --git a/gtests/ssl_gtest/ssl_resumption_unittest.cc b/gtests/ssl_gtest/ssl_resumption_unittest.cc index ce0e3ca8db..42d89ed183 100644 --- a/gtests/ssl_gtest/ssl_resumption_unittest.cc +++ b/gtests/ssl_gtest/ssl_resumption_unittest.cc @@ -355,10 +355,7 @@ TEST_P(TlsConnectGenericPre13, ConnectEcdheTwiceReuseKey) { // This test parses the ServerKeyExchange, which isn't in 1.3 TEST_P(TlsConnectGenericPre13, ConnectEcdheTwiceNewKey) { - server_->EnsureTlsSetup(); - SECStatus rv = - SSL_OptionSet(server_->ssl_fd(), SSL_REUSE_SERVER_ECDHE_KEY, PR_FALSE); - EXPECT_EQ(SECSuccess, rv); + server_->SetOption(SSL_REUSE_SERVER_ECDHE_KEY, PR_FALSE); auto i1 = std::make_shared( kTlsHandshakeServerKeyExchange); server_->SetPacketFilter(i1); @@ -369,9 +366,7 @@ TEST_P(TlsConnectGenericPre13, ConnectEcdheTwiceNewKey) { // Restart Reset(); - server_->EnsureTlsSetup(); - rv = SSL_OptionSet(server_->ssl_fd(), SSL_REUSE_SERVER_ECDHE_KEY, PR_FALSE); - EXPECT_EQ(SECSuccess, rv); + server_->SetOption(SSL_REUSE_SERVER_ECDHE_KEY, PR_FALSE); auto i2 = std::make_shared( kTlsHandshakeServerKeyExchange); server_->SetPacketFilter(i2); diff --git a/gtests/ssl_gtest/ssl_staticrsa_unittest.cc b/gtests/ssl_gtest/ssl_staticrsa_unittest.cc index 8db1f30e1b..e7fe44d92f 100644 --- a/gtests/ssl_gtest/ssl_staticrsa_unittest.cc +++ b/gtests/ssl_gtest/ssl_staticrsa_unittest.cc @@ -71,7 +71,7 @@ TEST_P(TlsConnectGenericPre13, ConnectStaticRSABogusPMSVersionIgnore) { EnableOnlyStaticRsaCiphers(); client_->SetPacketFilter( std::make_shared(server_)); - server_->DisableRollbackDetection(); + server_->SetOption(SSL_ROLLBACK_DETECTION, PR_FALSE); Connect(); } @@ -102,7 +102,7 @@ TEST_P(TlsConnectStreamPre13, EnableExtendedMasterSecret(); client_->SetPacketFilter( std::make_shared(server_)); - server_->DisableRollbackDetection(); + server_->SetOption(SSL_ROLLBACK_DETECTION, PR_FALSE); Connect(); } diff --git a/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc b/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc index 110e3e0b6f..2f8ddd6fed 100644 --- a/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc +++ b/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc @@ -153,13 +153,6 @@ class SSLv2ClientHelloTestF : public TlsConnectTestBase { client_->SetPacketFilter(filter_); } - void RequireSafeRenegotiation() { - server_->EnsureTlsSetup(); - SECStatus rv = - SSL_OptionSet(server_->ssl_fd(), SSL_REQUIRE_SAFE_NEGOTIATION, PR_TRUE); - EXPECT_EQ(rv, SECSuccess); - } - void SetExpectedVersion(uint16_t version) { TlsConnectTestBase::SetExpectedVersion(version); filter_->SetVersion(version); @@ -319,7 +312,7 @@ TEST_P(SSLv2ClientHelloTest, BigClientRandom) { // Connection must fail if we require safe renegotiation but the client doesn't // include TLS_EMPTY_RENEGOTIATION_INFO_SCSV in the list of cipher suites. TEST_P(SSLv2ClientHelloTest, RequireSafeRenegotiation) { - RequireSafeRenegotiation(); + server_->SetOption(SSL_REQUIRE_SAFE_NEGOTIATION, PR_TRUE); SetAvailableCipherSuite(TLS_DHE_RSA_WITH_AES_128_CBC_SHA); ConnectExpectAlert(server_, kTlsAlertHandshakeFailure); EXPECT_EQ(SSL_ERROR_UNSAFE_NEGOTIATION, server_->error_code()); @@ -328,7 +321,7 @@ TEST_P(SSLv2ClientHelloTest, RequireSafeRenegotiation) { // Connection must succeed when requiring safe renegotiation and the client // includes TLS_EMPTY_RENEGOTIATION_INFO_SCSV in the list of cipher suites. TEST_P(SSLv2ClientHelloTest, RequireSafeRenegotiationWithSCSV) { - RequireSafeRenegotiation(); + server_->SetOption(SSL_REQUIRE_SAFE_NEGOTIATION, PR_TRUE); std::vector cipher_suites = {TLS_DHE_RSA_WITH_AES_128_CBC_SHA, TLS_EMPTY_RENEGOTIATION_INFO_SCSV}; SetAvailableCipherSuites(cipher_suites); diff --git a/gtests/ssl_gtest/ssl_version_unittest.cc b/gtests/ssl_gtest/ssl_version_unittest.cc index 379a67e350..b13092bff4 100644 --- a/gtests/ssl_gtest/ssl_version_unittest.cc +++ b/gtests/ssl_gtest/ssl_version_unittest.cc @@ -128,12 +128,12 @@ TEST_F(TlsConnectTest, TestFallbackFromTls13) { #endif TEST_P(TlsConnectGeneric, TestFallbackSCSVVersionMatch) { - client_->SetFallbackSCSVEnabled(true); + client_->SetOption(SSL_ENABLE_FALLBACK_SCSV, PR_TRUE); Connect(); } TEST_P(TlsConnectGenericPre13, TestFallbackSCSVVersionMismatch) { - client_->SetFallbackSCSVEnabled(true); + client_->SetOption(SSL_ENABLE_FALLBACK_SCSV, PR_TRUE); server_->SetVersionRange(version_, version_ + 1); ConnectExpectAlert(server_, kTlsAlertInappropriateFallback); client_->CheckErrorCode(SSL_ERROR_INAPPROPRIATE_FALLBACK_ALERT); @@ -155,102 +155,6 @@ TEST_F(TlsConnectTest, DisallowSSLv3HelloWithTLSv13Enabled) { EXPECT_EQ(SECFailure, rv); } -TEST_P(TlsConnectStream, ConnectTls10AndServerRenegotiateHigher) { - if (version_ == SSL_LIBRARY_VERSION_TLS_1_0) { - return; - } - // Set the client so it will accept any version from 1.0 - // to |version_|. - client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_0, version_); - server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_0, - SSL_LIBRARY_VERSION_TLS_1_0); - // Reset version so that the checks succeed. - uint16_t test_version = version_; - version_ = SSL_LIBRARY_VERSION_TLS_1_0; - Connect(); - - // Now renegotiate, with the server being set to do - // |version_|. - client_->PrepareForRenegotiate(); - server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_0, test_version); - // Reset version and cipher suite so that the preinfo callback - // doesn't fail. - server_->ResetPreliminaryInfo(); - server_->StartRenegotiate(); - - if (test_version >= SSL_LIBRARY_VERSION_TLS_1_3) { - ExpectAlert(server_, kTlsAlertUnexpectedMessage); - } else { - ExpectAlert(client_, kTlsAlertIllegalParameter); - } - - Handshake(); - if (test_version >= SSL_LIBRARY_VERSION_TLS_1_3) { - // In TLS 1.3, the server detects this problem. - client_->CheckErrorCode(SSL_ERROR_HANDSHAKE_UNEXPECTED_ALERT); - server_->CheckErrorCode(SSL_ERROR_RENEGOTIATION_NOT_ALLOWED); - } else { - client_->CheckErrorCode(SSL_ERROR_UNSUPPORTED_VERSION); - server_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT); - } -} - -TEST_P(TlsConnectStream, ConnectTls10AndClientRenegotiateHigher) { - if (version_ == SSL_LIBRARY_VERSION_TLS_1_0) { - return; - } - // Set the client so it will accept any version from 1.0 - // to |version_|. - client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_0, version_); - server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_0, - SSL_LIBRARY_VERSION_TLS_1_0); - // Reset version so that the checks succeed. - uint16_t test_version = version_; - version_ = SSL_LIBRARY_VERSION_TLS_1_0; - Connect(); - - // Now renegotiate, with the server being set to do - // |version_|. - server_->PrepareForRenegotiate(); - server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_0, test_version); - // Reset version and cipher suite so that the preinfo callback - // doesn't fail. - server_->ResetPreliminaryInfo(); - client_->StartRenegotiate(); - if (test_version >= SSL_LIBRARY_VERSION_TLS_1_3) { - ExpectAlert(server_, kTlsAlertUnexpectedMessage); - } else { - ExpectAlert(client_, kTlsAlertIllegalParameter); - } - Handshake(); - if (test_version >= SSL_LIBRARY_VERSION_TLS_1_3) { - // In TLS 1.3, the server detects this problem. - client_->CheckErrorCode(SSL_ERROR_HANDSHAKE_UNEXPECTED_ALERT); - server_->CheckErrorCode(SSL_ERROR_RENEGOTIATION_NOT_ALLOWED); - } else { - client_->CheckErrorCode(SSL_ERROR_UNSUPPORTED_VERSION); - server_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT); - } -} - -TEST_F(TlsConnectTest, Tls13RejectsRehandshakeClient) { - 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) { - EnsureTlsSetup(); - ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3); - Connect(); - SECStatus rv = SSL_ReHandshake(server_->ssl_fd(), PR_TRUE); - EXPECT_EQ(SECFailure, rv); - EXPECT_EQ(SSL_ERROR_RENEGOTIATION_NOT_ALLOWED, PORT_GetError()); -} - TEST_P(TlsConnectGeneric, AlertBeforeServerHello) { EnsureTlsSetup(); client_->ExpectReceiveAlert(kTlsAlertUnrecognizedName, kTlsAlertWarning); diff --git a/gtests/ssl_gtest/tls_agent.cc b/gtests/ssl_gtest/tls_agent.cc index 7d76cffb26..0a64686b7e 100644 --- a/gtests/ssl_gtest/tls_agent.cc +++ b/gtests/ssl_gtest/tls_agent.cc @@ -259,13 +259,10 @@ void TlsAgent::CheckCipherSuite(uint16_t cipher_suite) { } void TlsAgent::RequestClientAuth(bool requireAuth) { - EXPECT_TRUE(EnsureTlsSetup()); ASSERT_EQ(SERVER, role_); - EXPECT_EQ(SECSuccess, - SSL_OptionSet(ssl_fd(), SSL_REQUEST_CERTIFICATE, PR_TRUE)); - EXPECT_EQ(SECSuccess, SSL_OptionSet(ssl_fd(), SSL_REQUIRE_CERTIFICATE, - requireAuth ? PR_TRUE : PR_FALSE)); + SetOption(SSL_REQUEST_CERTIFICATE, PR_TRUE); + SetOption(SSL_REQUIRE_CERTIFICATE, requireAuth ? PR_TRUE : PR_FALSE); EXPECT_EQ(SECSuccess, SSL_AuthCertificateHook( ssl_fd(), &TlsAgent::ClientAuthenticated, this)); @@ -377,35 +374,8 @@ void TlsAgent::ConfigNamedGroups(const std::vector& groups) { EXPECT_EQ(SECSuccess, rv); } -void TlsAgent::SetSessionTicketsEnabled(bool en) { - EXPECT_TRUE(EnsureTlsSetup()); - - SECStatus rv = SSL_OptionSet(ssl_fd(), SSL_ENABLE_SESSION_TICKETS, - en ? PR_TRUE : PR_FALSE); - EXPECT_EQ(SECSuccess, rv); -} - -void TlsAgent::SetSessionCacheEnabled(bool en) { - EXPECT_TRUE(EnsureTlsSetup()); - - SECStatus rv = SSL_OptionSet(ssl_fd(), SSL_NO_CACHE, en ? PR_FALSE : PR_TRUE); - EXPECT_EQ(SECSuccess, rv); -} - void TlsAgent::Set0RttEnabled(bool en) { - EXPECT_TRUE(EnsureTlsSetup()); - - SECStatus rv = - SSL_OptionSet(ssl_fd(), SSL_ENABLE_0RTT_DATA, en ? PR_TRUE : PR_FALSE); - EXPECT_EQ(SECSuccess, rv); -} - -void TlsAgent::SetFallbackSCSVEnabled(bool en) { - EXPECT_TRUE(role_ == CLIENT && EnsureTlsSetup()); - - SECStatus rv = SSL_OptionSet(ssl_fd(), SSL_ENABLE_FALLBACK_SCSV, - en ? PR_TRUE : PR_FALSE); - EXPECT_EQ(SECSuccess, rv); + SetOption(SSL_ENABLE_0RTT_DATA, en ? PR_TRUE : PR_FALSE); } void TlsAgent::SetShortHeadersEnabled() { @@ -577,8 +547,7 @@ void TlsAgent::EnableFalseStart() { falsestart_enabled_ = true; EXPECT_EQ(SECSuccess, SSL_SetCanFalseStartCallback( ssl_fd(), CanFalseStartCallback, this)); - EXPECT_EQ(SECSuccess, - SSL_OptionSet(ssl_fd(), SSL_ENABLE_FALSE_START, PR_TRUE)); + SetOption(SSL_ENABLE_FALSE_START, PR_TRUE); } void TlsAgent::ExpectResumption() { expect_resumption_ = true; } @@ -586,7 +555,7 @@ void TlsAgent::ExpectResumption() { expect_resumption_ = true; } void TlsAgent::EnableAlpn(const uint8_t* val, size_t len) { EXPECT_TRUE(EnsureTlsSetup()); - EXPECT_EQ(SECSuccess, SSL_OptionSet(ssl_fd(), SSL_ENABLE_ALPN, PR_TRUE)); + SetOption(SSL_ENABLE_ALPN, PR_TRUE); EXPECT_EQ(SECSuccess, SSL_SetNextProtoNego(ssl_fd(), val, len)); } @@ -781,12 +750,7 @@ void TlsAgent::Connected() { } void TlsAgent::EnableExtendedMasterSecret() { - ASSERT_TRUE(EnsureTlsSetup()); - - SECStatus rv = - SSL_OptionSet(ssl_fd(), SSL_ENABLE_EXTENDED_MASTER_SECRET, PR_TRUE); - - ASSERT_EQ(SECSuccess, rv); + SetOption(SSL_ENABLE_EXTENDED_MASTER_SECRET, PR_TRUE); } void TlsAgent::CheckExtendedMasterSecret(bool expected) { @@ -809,21 +773,6 @@ void TlsAgent::CheckSecretsDestroyed() { ASSERT_EQ(PR_TRUE, SSLInt_CheckSecretsDestroyed(ssl_fd())); } -void TlsAgent::DisableRollbackDetection() { - ASSERT_TRUE(EnsureTlsSetup()); - - SECStatus rv = SSL_OptionSet(ssl_fd(), SSL_ROLLBACK_DETECTION, PR_FALSE); - - ASSERT_EQ(SECSuccess, rv); -} - -void TlsAgent::EnableCompression() { - ASSERT_TRUE(EnsureTlsSetup()); - - SECStatus rv = SSL_OptionSet(ssl_fd(), SSL_ENABLE_DEFLATE, PR_TRUE); - ASSERT_EQ(SECSuccess, rv); -} - void TlsAgent::SetDowngradeCheckVersion(uint16_t version) { ASSERT_TRUE(EnsureTlsSetup()); @@ -959,23 +908,20 @@ void TlsAgent::ReadBytes(size_t amount) { void TlsAgent::ResetSentBytes() { send_ctr_ = 0; } -void TlsAgent::ConfigureSessionCache(SessionResumptionMode mode) { - EXPECT_TRUE(EnsureTlsSetup()); - - SECStatus rv = SSL_OptionSet(ssl_fd(), SSL_NO_CACHE, - mode & RESUME_SESSIONID ? PR_FALSE : PR_TRUE); - EXPECT_EQ(SECSuccess, rv); +void TlsAgent::SetOption(int32_t option, int value) { + ASSERT_TRUE(EnsureTlsSetup()); + EXPECT_EQ(SECSuccess, SSL_OptionSet(ssl_fd(), option, value)); +} - rv = SSL_OptionSet(ssl_fd(), SSL_ENABLE_SESSION_TICKETS, - mode & RESUME_TICKET ? PR_TRUE : PR_FALSE); - EXPECT_EQ(SECSuccess, rv); +void TlsAgent::ConfigureSessionCache(SessionResumptionMode mode) { + SetOption(SSL_NO_CACHE, mode & RESUME_SESSIONID ? PR_FALSE : PR_TRUE); + SetOption(SSL_ENABLE_SESSION_TICKETS, + mode & RESUME_TICKET ? PR_TRUE : PR_FALSE); } void TlsAgent::DisableECDHEServerKeyReuse() { - ASSERT_TRUE(EnsureTlsSetup()); ASSERT_EQ(TlsAgent::SERVER, role_); - SECStatus rv = SSL_OptionSet(ssl_fd(), SSL_REUSE_SERVER_ECDHE_KEY, PR_FALSE); - EXPECT_EQ(SECSuccess, rv); + SetOption(SSL_REUSE_SERVER_ECDHE_KEY, PR_FALSE); } static const std::string kTlsRolesAllArr[] = {"CLIENT", "SERVER"}; diff --git a/gtests/ssl_gtest/tls_agent.h b/gtests/ssl_gtest/tls_agent.h index 2ae10bbd0a..36050ec998 100644 --- a/gtests/ssl_gtest/tls_agent.h +++ b/gtests/ssl_gtest/tls_agent.h @@ -121,9 +121,8 @@ class TlsAgent : public PollTarget { void SetupClientAuth(); void RequestClientAuth(bool requireAuth); + void SetOption(int32_t option, int value); void ConfigureSessionCache(SessionResumptionMode mode); - void SetSessionTicketsEnabled(bool en); - void SetSessionCacheEnabled(bool en); void Set0RttEnabled(bool en); void SetFallbackSCSVEnabled(bool en); void SetShortHeadersEnabled(); @@ -157,8 +156,6 @@ class TlsAgent : public PollTarget { void EnableExtendedMasterSecret(); void CheckExtendedMasterSecret(bool expected); void CheckEarlyDataAccepted(bool expected); - void DisableRollbackDetection(); - void EnableCompression(); void SetDowngradeCheckVersion(uint16_t version); void CheckSecretsDestroyed(); void ConfigNamedGroups(const std::vector& groups); diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c index 8daad7c506..a36c88ed4e 100644 --- a/lib/ssl/ssl3con.c +++ b/lib/ssl/ssl3con.c @@ -8539,29 +8539,33 @@ ssl3_HandleClientHello(sslSocket *ss, PRUint8 *b, PRUint32 length) } } } + /* This is a second check for TLS 1.3 and re-handshake to stop us * from re-handshake up to TLS 1.3, so it happens after version * negotiation. */ - if (ss->firstHsDone && ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) { - desc = unexpected_message; - errCode = SSL_ERROR_RENEGOTIATION_NOT_ALLOWED; - goto alert_loser; - } - if (ss->firstHsDone && - (ss->opt.enableRenegotiation == SSL_RENEGOTIATE_REQUIRES_XTN || - ss->opt.enableRenegotiation == SSL_RENEGOTIATE_TRANSITIONAL) && - !ssl3_ExtensionNegotiated(ss, ssl_renegotiation_info_xtn)) { - desc = no_renegotiation; - level = alert_warning; - errCode = SSL_ERROR_RENEGOTIATION_NOT_ALLOWED; - goto alert_loser; - } - if ((ss->opt.requireSafeNegotiation || - (ss->firstHsDone && ss->peerRequestedProtection)) && - !ssl3_ExtensionNegotiated(ss, ssl_renegotiation_info_xtn)) { - desc = handshake_failure; - errCode = SSL_ERROR_UNSAFE_NEGOTIATION; - goto alert_loser; + if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) { + if (ss->firstHsDone) { + desc = unexpected_message; + errCode = SSL_ERROR_RENEGOTIATION_NOT_ALLOWED; + goto alert_loser; + } + } else { + if (ss->firstHsDone && + (ss->opt.enableRenegotiation == SSL_RENEGOTIATE_REQUIRES_XTN || + ss->opt.enableRenegotiation == SSL_RENEGOTIATE_TRANSITIONAL) && + !ssl3_ExtensionNegotiated(ss, ssl_renegotiation_info_xtn)) { + desc = no_renegotiation; + level = alert_warning; + errCode = SSL_ERROR_RENEGOTIATION_NOT_ALLOWED; + goto alert_loser; + } + if ((ss->opt.requireSafeNegotiation || + (ss->firstHsDone && ss->peerRequestedProtection)) && + !ssl3_ExtensionNegotiated(ss, ssl_renegotiation_info_xtn)) { + desc = handshake_failure; + errCode = SSL_ERROR_UNSAFE_NEGOTIATION; + goto alert_loser; + } } /* We do stateful resumes only if we are in TLS < 1.3 and diff --git a/lib/ssl/ssl3exthandle.c b/lib/ssl/ssl3exthandle.c index 370bd8b3e4..bda3d375fc 100644 --- a/lib/ssl/ssl3exthandle.c +++ b/lib/ssl/ssl3exthandle.c @@ -1497,12 +1497,12 @@ ssl3_SendRenegotiationInfoXtn( PRInt32 len = 0; PRInt32 needed; - /* In draft-ietf-tls-renegotiation-03, it is NOT RECOMMENDED to send - * both the SCSV and the empty RI, so when we send SCSV in - * the initial handshake, we don't also send RI. + /* In RFC 5746, it is NOT RECOMMENDED to send both the SCSV and the empty + * RI, so when we send SCSV in the initial handshake, we don't also send RI. */ - if (!ss || ss->ssl3.hs.sendingSCSV) + if (ss->ssl3.hs.sendingSCSV) { return 0; + } if (ss->firstHsDone) { len = ss->sec.isServer ? ss->ssl3.hs.finishedBytes * 2 : ss->ssl3.hs.finishedBytes;