diff options
author | Martin Thomson <martin.thomson@gmail.com> | 2016-04-18 11:52:29 +1000 |
---|---|---|
committer | Martin Thomson <martin.thomson@gmail.com> | 2016-04-18 11:52:29 +1000 |
commit | 7be20a6f3ad0f8d5f410ae75d1f69831421c5b6d (patch) | |
tree | 5f8fde972d6a17719cb8c1dcd1809436cde811fa | |
parent | c54bdf09daa8f965bef7aef3f7426e6d327bb276 (diff) | |
download | nss-hg-7be20a6f3ad0f8d5f410ae75d1f69831421c5b6d.tar.gz |
Bug 1249939 - Improve TLS 1.3 draft extension handling, r=ekr
-rw-r--r-- | external_tests/ssl_gtest/ssl_extension_unittest.cc | 52 | ||||
-rw-r--r-- | lib/ssl/ssl3con.c | 16 | ||||
-rw-r--r-- | lib/ssl/ssl3ext.c | 15 |
3 files changed, 73 insertions, 10 deletions
diff --git a/external_tests/ssl_gtest/ssl_extension_unittest.cc b/external_tests/ssl_gtest/ssl_extension_unittest.cc index 59d05ba8b..a9e235e36 100644 --- a/external_tests/ssl_gtest/ssl_extension_unittest.cc +++ b/external_tests/ssl_gtest/ssl_extension_unittest.cc @@ -5,6 +5,7 @@ * You can obtain one at http://mozilla.org/MPL/2.0/. */ #include "ssl.h" +#include "sslerr.h" #include "sslproto.h" #include <memory> @@ -15,6 +16,21 @@ namespace nss_test { +class TlsExtensionDropper : public TlsExtensionFilter { + public: + TlsExtensionDropper(uint16_t extension) + : extension_(extension) {} + virtual PacketFilter::Action FilterExtension( + uint16_t extension_type, const DataBuffer&, DataBuffer*) { + if (extension_type == extension_) { + return DROP; + } + return KEEP; + } + private: + uint16_t extension_; +}; + class TlsExtensionTruncator : public TlsExtensionFilter { public: TlsExtensionTruncator(uint16_t extension, size_t length) @@ -603,6 +619,42 @@ TEST_P(TlsExtensionTest13, EmptyClientKeyShare) { kTlsAlertHandshakeFailure); } +TEST_P(TlsExtensionTest13, DropDraftVersion) { + EnsureTlsSetup(); + client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2, + SSL_LIBRARY_VERSION_TLS_1_3); + server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2, + SSL_LIBRARY_VERSION_TLS_1_3); + client_->SetPacketFilter( + new TlsExtensionDropper(ssl_tls13_draft_version_xtn)); + ConnectExpectFail(); + // This will still fail (we can't just modify ClientHello without consequence) + // but the error is discovered later. + EXPECT_EQ(SSL_ERROR_DECRYPT_ERROR_ALERT, client_->error_code()); + EXPECT_EQ(SSL_ERROR_BAD_HANDSHAKE_HASH_VALUE, server_->error_code()); +} + +TEST_P(TlsExtensionTest13, DropDraftVersionAndFail) { + EnsureTlsSetup(); + // Since this is setup as TLS 1.3 only, expect the handshake to fail rather + // than just falling back to TLS 1.2. + client_->SetPacketFilter( + new TlsExtensionDropper(ssl_tls13_draft_version_xtn)); + ConnectExpectFail(); + EXPECT_EQ(SSL_ERROR_PROTOCOL_VERSION_ALERT, client_->error_code()); + EXPECT_EQ(SSL_ERROR_UNSUPPORTED_VERSION, server_->error_code()); +} + +TEST_P(TlsExtensionTest13, ModifyDraftVersionAndFail) { + EnsureTlsSetup(); + // As above, dropping back to 1.2 fails. + client_->SetPacketFilter( + new TlsExtensionDamager(ssl_tls13_draft_version_xtn, 1)); + ConnectExpectFail(); + EXPECT_EQ(SSL_ERROR_PROTOCOL_VERSION_ALERT, client_->error_code()); + EXPECT_EQ(SSL_ERROR_UNSUPPORTED_VERSION, server_->error_code()); +} + INSTANTIATE_TEST_CASE_P(ExtensionStream, TlsExtensionTestGeneric, ::testing::Combine( TlsConnectTestBase::kTlsModesStream, diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c index 3654edbdd..2635ac18b 100644 --- a/lib/ssl/ssl3con.c +++ b/lib/ssl/ssl3con.c @@ -8714,6 +8714,22 @@ ssl3_HandleClientHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length) } } + /* TODO: For TLS 1.3 final, downgrade when the extension IS present. */ + if (ss->version == SSL_LIBRARY_VERSION_TLS_1_3 && + !ssl3_ExtensionNegotiated(ss, ssl_tls13_draft_version_xtn)) { + SSL_TRC(30, ("%d: SSL3[%d]: Unsupported version of TLS 1.3 " + "advertised, expected %d", + SSL_GETPID(), ss->fd, TLS_1_3_DRAFT_VERSION)); + ss->version = SSL_LIBRARY_VERSION_TLS_1_2; + /* Maybe TLS 1.2 is disabled... */ + if (ss->version < ss->vrange.min) { + desc = protocol_version; + level = alert_fatal; + errCode = SSL_ERROR_UNSUPPORTED_VERSION; + goto alert_loser; + } + } + if (!ssl3_ExtensionNegotiated(ss, ssl_renegotiation_info_xtn)) { /* If we didn't receive an RI extension, look for the SCSV, * and if found, treat it just like an empty RI extension diff --git a/lib/ssl/ssl3ext.c b/lib/ssl/ssl3ext.c index 07d114ee4..27f15eeab 100644 --- a/lib/ssl/ssl3ext.c +++ b/lib/ssl/ssl3ext.c @@ -2789,19 +2789,14 @@ ssl3_ServerHandleDraftVersionXtn(sslSocket *ss, PRUint16 ex_type, return SECFailure; } - /* Keep track of negotiated extensions. */ - ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ex_type; - - if (draft_version != TLS_1_3_DRAFT_VERSION) { - /* - * Incompatible/broken TLS 1.3 implementation. Fall back to TLS 1.2. - * TODO(ekr@rtfm.com): It's not entirely clear it's safe to roll back - * here. Need to double-check. - */ + if (draft_version == TLS_1_3_DRAFT_VERSION) { + /* Mark this as negotiated only if the version matches. The code in + * ssl3_HandleClientHello will then reduce the version if needed. */ + ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ex_type; + } else { SSL_TRC(30, ("%d: SSL3[%d]: Incompatible version of TLS 1.3 (%d), " "expected %d", SSL_GETPID(), ss->fd, draft_version, TLS_1_3_DRAFT_VERSION)); - ss->version = SSL_LIBRARY_VERSION_TLS_1_2; } return SECSuccess; |