From 0b6702204ab7f830d575cfc3777ca0b05276956d Mon Sep 17 00:00:00 2001 From: Martin Thomson Date: Mon, 7 Aug 2017 14:12:33 +1000 Subject: [PATCH] Bug 1386096 - Stateless HelloRetryRequest for server, r=ekr Differential Revision: https://nss-review.dev.mozaws.net/D396 --HG-- branch : NSS_TLS13_DRAFT19_BRANCH extra : rebase_source : d14b3aa5144a153c192b35b70d4bcad1596db1ad extra : amend_source : 7efffaa368abf0457450c4b01a97c4788691642b --- gtests/ssl_gtest/ssl_extension_unittest.cc | 42 ----- gtests/ssl_gtest/ssl_hrr_unittest.cc | 186 +++++++++++++++++++++ gtests/ssl_gtest/tls_agent.h | 2 + gtests/ssl_gtest/tls_connect.cc | 24 +-- gtests/ssl_gtest/tls_connect.h | 1 + gtests/ssl_gtest/tls_filter.cc | 32 ++++ gtests/ssl_gtest/tls_filter.h | 15 ++ lib/ssl/dtlscon.c | 14 ++ lib/ssl/ssl3con.c | 3 +- lib/ssl/sslexp.h | 2 +- lib/ssl/tls13con.c | 82 +++++---- lib/ssl/tls13con.h | 1 + lib/ssl/tls13hashstate.c | 12 +- lib/ssl/tls13hashstate.h | 4 +- 14 files changed, 333 insertions(+), 87 deletions(-) diff --git a/gtests/ssl_gtest/ssl_extension_unittest.cc b/gtests/ssl_gtest/ssl_extension_unittest.cc index 74902e709b..6d5a3490b7 100644 --- a/gtests/ssl_gtest/ssl_extension_unittest.cc +++ b/gtests/ssl_gtest/ssl_extension_unittest.cc @@ -61,48 +61,6 @@ class TlsExtensionDamager : public TlsExtensionFilter { size_t index_; }; -class TlsExtensionInjector : public TlsHandshakeFilter { - public: - TlsExtensionInjector(uint16_t ext, DataBuffer& data) - : extension_(ext), data_(data) {} - - virtual PacketFilter::Action FilterHandshake(const HandshakeHeader& header, - const DataBuffer& input, - DataBuffer* output) { - TlsParser parser(input); - if (!TlsExtensionFilter::FindExtensions(&parser, header)) { - return KEEP; - } - size_t offset = parser.consumed(); - - *output = input; - - // Increase the size of the extensions. - uint16_t ext_len; - memcpy(&ext_len, output->data() + offset, sizeof(ext_len)); - ext_len = htons(ntohs(ext_len) + data_.len() + 4); - memcpy(output->data() + offset, &ext_len, sizeof(ext_len)); - - // Insert the extension type and length. - DataBuffer type_length; - type_length.Allocate(4); - type_length.Write(0, extension_, 2); - type_length.Write(2, data_.len(), 2); - output->Splice(type_length, offset + 2); - - // Insert the payload. - if (data_.len() > 0) { - output->Splice(data_, offset + 6); - } - - return CHANGE; - } - - private: - const uint16_t extension_; - const DataBuffer data_; -}; - class TlsExtensionAppender : public TlsHandshakeFilter { public: TlsExtensionAppender(uint8_t handshake_type, uint16_t ext, DataBuffer& data) diff --git a/gtests/ssl_gtest/ssl_hrr_unittest.cc b/gtests/ssl_gtest/ssl_hrr_unittest.cc index 2429d09f12..65eaefa7a3 100644 --- a/gtests/ssl_gtest/ssl_hrr_unittest.cc +++ b/gtests/ssl_gtest/ssl_hrr_unittest.cc @@ -488,6 +488,192 @@ TEST_F(TlsConnectStreamTls13, RetryCallbackWithSessionTicketToken) { EXPECT_TRUE(cb_run); } +std::shared_ptr MakeNewServer(std::shared_ptr& client) { + auto server = std::make_shared(TlsAgent::kServerRsa, + TlsAgent::SERVER, client->variant()); + server->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1, + SSL_LIBRARY_VERSION_TLS_1_3); + client->SetPeer(server); + server->SetPeer(client); + server->StartConnect(); + return server; +} + +void TriggerHelloRetryRequest(std::shared_ptr& client, + std::shared_ptr& server) { + size_t cb_called = 0; + EXPECT_EQ(SECSuccess, SSL_HelloRetryRequestCallback(server->ssl_fd(), + RetryHello, &cb_called)); + + // Start the handshake. + client->StartConnect(); + server->StartConnect(); + client->Handshake(); + server->Handshake(); + EXPECT_EQ(1U, cb_called); +} + +TEST_P(TlsConnectTls13, RetryStateless) { + ConfigureSelfEncrypt(); + EnsureTlsSetup(); + + TriggerHelloRetryRequest(client_, server_); + server_ = MakeNewServer(client_); + + Handshake(); + SendReceive(); +} + +// Stream only because DTLS drops bad packets. +TEST_F(TlsConnectStreamTls13, RetryStatelessDamageFirstClientHello) { + ConfigureSelfEncrypt(); + EnsureTlsSetup(); + + auto damage_ch = std::make_shared(0xfff3, DataBuffer()); + client_->SetPacketFilter(damage_ch); + + TriggerHelloRetryRequest(client_, server_); + server_ = MakeNewServer(client_); + + // Key exchange fails when the handshake continues because client and server + // disagree about the transcript. + client_->ExpectSendAlert(kTlsAlertBadRecordMac); + server_->ExpectSendAlert(kTlsAlertBadRecordMac); + Handshake(); + server_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ); + client_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ); +} + +TEST_F(TlsConnectStreamTls13, RetryStatelessDamageSecondClientHello) { + ConfigureSelfEncrypt(); + EnsureTlsSetup(); + + TriggerHelloRetryRequest(client_, server_); + server_ = MakeNewServer(client_); + + auto damage_ch = std::make_shared(0xfff3, DataBuffer()); + client_->SetPacketFilter(damage_ch); + + // Key exchange fails when the handshake continues because client and server + // disagree about the transcript. + client_->ExpectSendAlert(kTlsAlertBadRecordMac); + server_->ExpectSendAlert(kTlsAlertBadRecordMac); + Handshake(); + server_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ); + client_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ); +} + +TEST_P(TlsConnectTls13, RetryStatelessDisableSuiteClient) { + ConfigureSelfEncrypt(); + EnsureTlsSetup(); + + auto capture_hrr = std::make_shared( + ssl_hs_hello_retry_request); + server_->SetPacketFilter(capture_hrr); + + TriggerHelloRetryRequest(client_, server_); + server_ = MakeNewServer(client_); + + // Read the cipher suite from the HRR and disable it on the client. + uint32_t suite; + ASSERT_TRUE(capture_hrr->buffer().Read(2, 2, &suite)); + EXPECT_EQ(SECSuccess, + SSL_CipherPrefSet(client_->ssl_fd(), static_cast(suite), + PR_FALSE)); + + // The client thinks that the HelloRetryRequest is bad, even though its + // because it changed its mind about the cipher suite. + ExpectAlert(client_, kTlsAlertIllegalParameter); + Handshake(); + client_->CheckErrorCode(SSL_ERROR_RX_MALFORMED_HELLO_RETRY_REQUEST); + server_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT); +} + +TEST_P(TlsConnectTls13, RetryStatelessDisableSuiteServer) { + ConfigureSelfEncrypt(); + EnsureTlsSetup(); + + auto capture_hrr = std::make_shared( + ssl_hs_hello_retry_request); + server_->SetPacketFilter(capture_hrr); + + TriggerHelloRetryRequest(client_, server_); + server_ = MakeNewServer(client_); + + // Read the cipher suite from the HRR and disable it on the server. + uint32_t suite; + ASSERT_TRUE(capture_hrr->buffer().Read(2, 2, &suite)); + EXPECT_EQ(SECSuccess, + SSL_CipherPrefSet(server_->ssl_fd(), static_cast(suite), + PR_FALSE)); + + ExpectAlert(server_, kTlsAlertIllegalParameter); + Handshake(); + server_->CheckErrorCode(SSL_ERROR_BAD_2ND_CLIENT_HELLO); + client_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT); +} + +TEST_P(TlsConnectTls13, RetryStatelessDisableGroupClient) { + ConfigureSelfEncrypt(); + EnsureTlsSetup(); + + TriggerHelloRetryRequest(client_, server_); + server_ = MakeNewServer(client_); + + static const std::vector groups = {ssl_grp_ec_secp384r1}; + client_->ConfigNamedGroups(groups); + + // We're into undefined behavior on the client side, but - at the point this + // test was written - the client here doesn't amend its key shares because the + // server doesn't ask it to. The server notices that the key share (x25519) + // doesn't match the negotiated group (P-384) and objects. + ExpectAlert(server_, kTlsAlertIllegalParameter); + Handshake(); + server_->CheckErrorCode(SSL_ERROR_BAD_2ND_CLIENT_HELLO); + client_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT); +} + +TEST_P(TlsConnectTls13, RetryStatelessDisableGroupServer) { + ConfigureSelfEncrypt(); + EnsureTlsSetup(); + + TriggerHelloRetryRequest(client_, server_); + server_ = MakeNewServer(client_); + + static const std::vector groups = {ssl_grp_ec_secp384r1}; + server_->ConfigNamedGroups(groups); + + ExpectAlert(server_, kTlsAlertIllegalParameter); + Handshake(); + server_->CheckErrorCode(SSL_ERROR_BAD_2ND_CLIENT_HELLO); + client_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT); +} + +TEST_P(TlsConnectTls13, RetryStatelessBadCookie) { + ConfigureSelfEncrypt(); + EnsureTlsSetup(); + + TriggerHelloRetryRequest(client_, server_); + + // Now replace the self-encrypt MAC key with a garbage key. + static const uint8_t bad_hmac_key[32] = {0}; + SECItem key_item = {siBuffer, const_cast(bad_hmac_key), + sizeof(bad_hmac_key)}; + ScopedPK11SlotInfo slot(PK11_GetInternalSlot()); + PK11SymKey* hmac_key = + PK11_ImportSymKey(slot.get(), CKM_SHA256_HMAC, PK11_OriginUnwrap, + CKA_SIGN, &key_item, nullptr); + ASSERT_NE(nullptr, hmac_key); + SSLInt_SetSelfEncryptMacKey(hmac_key); // Passes ownership. + + server_ = MakeNewServer(client_); + + ExpectAlert(server_, kTlsAlertIllegalParameter); + Handshake(); + server_->CheckErrorCode(SSL_ERROR_BAD_2ND_CLIENT_HELLO); + client_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT); +} + // Stream because the server doesn't consume the alert and terminate. TEST_F(TlsConnectStreamTls13, RetryWithDifferentCipherSuite) { EnsureTlsSetup(); diff --git a/gtests/ssl_gtest/tls_agent.h b/gtests/ssl_gtest/tls_agent.h index 16f53e7124..fad1214494 100644 --- a/gtests/ssl_gtest/tls_agent.h +++ b/gtests/ssl_gtest/tls_agent.h @@ -171,6 +171,8 @@ class TlsAgent : public PollTarget { Role role() const { return role_; } std::string role_str() const { return role_ == SERVER ? "server" : "client"; } + SSLProtocolVariant variant() const { return variant_; } + State state() const { return state_; } const CERTCertificate* peer_cert() const { diff --git a/gtests/ssl_gtest/tls_connect.cc b/gtests/ssl_gtest/tls_connect.cc index 6f5ce6e0a3..16619da75f 100644 --- a/gtests/ssl_gtest/tls_connect.cc +++ b/gtests/ssl_gtest/tls_connect.cc @@ -459,21 +459,25 @@ void TlsConnectTestBase::EnableSomeEcdhCiphers() { } } +void TlsConnectTestBase::ConfigureSelfEncrypt() { + ScopedCERTCertificate cert; + ScopedSECKEYPrivateKey privKey; + ASSERT_TRUE( + TlsAgent::LoadCertificate(TlsAgent::kServerRsaDecrypt, &cert, &privKey)); + + ScopedSECKEYPublicKey pubKey(CERT_ExtractPublicKey(cert.get())); + ASSERT_TRUE(pubKey); + + EXPECT_EQ(SECSuccess, + SSL_SetSessionTicketKeyPair(pubKey.get(), privKey.get())); +} + void TlsConnectTestBase::ConfigureSessionCache(SessionResumptionMode client, SessionResumptionMode server) { client_->ConfigureSessionCache(client); server_->ConfigureSessionCache(server); if ((server & RESUME_TICKET) != 0) { - ScopedCERTCertificate cert; - ScopedSECKEYPrivateKey privKey; - ASSERT_TRUE(TlsAgent::LoadCertificate(TlsAgent::kServerRsaDecrypt, &cert, - &privKey)); - - ScopedSECKEYPublicKey pubKey(CERT_ExtractPublicKey(cert.get())); - ASSERT_TRUE(pubKey); - - EXPECT_EQ(SECSuccess, - SSL_SetSessionTicketKeyPair(pubKey.get(), privKey.get())); + ConfigureSelfEncrypt(); } } diff --git a/gtests/ssl_gtest/tls_connect.h b/gtests/ssl_gtest/tls_connect.h index 73e8dc81a9..5a84354d0e 100644 --- a/gtests/ssl_gtest/tls_connect.h +++ b/gtests/ssl_gtest/tls_connect.h @@ -95,6 +95,7 @@ class TlsConnectTestBase : public ::testing::Test { void EnableOnlyDheCiphers(); void EnableSomeEcdhCiphers(); void EnableExtendedMasterSecret(); + void ConfigureSelfEncrypt(); void ConfigureSessionCache(SessionResumptionMode client, SessionResumptionMode server); void EnableAlpn(); diff --git a/gtests/ssl_gtest/tls_filter.cc b/gtests/ssl_gtest/tls_filter.cc index 1b5bbd480e..889e01b0d0 100644 --- a/gtests/ssl_gtest/tls_filter.cc +++ b/gtests/ssl_gtest/tls_filter.cc @@ -665,6 +665,38 @@ PacketFilter::Action TlsExtensionDropper::FilterExtension( return KEEP; } +PacketFilter::Action TlsExtensionInjector::FilterHandshake( + const HandshakeHeader& header, const DataBuffer& input, + DataBuffer* output) { + TlsParser parser(input); + if (!TlsExtensionFilter::FindExtensions(&parser, header)) { + return KEEP; + } + size_t offset = parser.consumed(); + + *output = input; + + // Increase the size of the extensions. + uint16_t ext_len; + memcpy(&ext_len, output->data() + offset, sizeof(ext_len)); + ext_len = htons(ntohs(ext_len) + data_.len() + 4); + memcpy(output->data() + offset, &ext_len, sizeof(ext_len)); + + // Insert the extension type and length. + DataBuffer type_length; + type_length.Allocate(4); + type_length.Write(0, extension_, 2); + type_length.Write(2, data_.len(), 2); + output->Splice(type_length, offset + 2); + + // Insert the payload. + if (data_.len() > 0) { + output->Splice(data_, offset + 6); + } + + return CHANGE; +} + PacketFilter::Action AfterRecordN::FilterRecord(const TlsRecordHeader& header, const DataBuffer& body, DataBuffer* out) { diff --git a/gtests/ssl_gtest/tls_filter.h b/gtests/ssl_gtest/tls_filter.h index f3f748ba62..aba555f48c 100644 --- a/gtests/ssl_gtest/tls_filter.h +++ b/gtests/ssl_gtest/tls_filter.h @@ -358,6 +358,21 @@ class TlsExtensionDropper : public TlsExtensionFilter { uint16_t extension_; }; +class TlsExtensionInjector : public TlsHandshakeFilter { + public: + TlsExtensionInjector(uint16_t ext, const DataBuffer& data) + : extension_(ext), data_(data) {} + + protected: + PacketFilter::Action FilterHandshake(const HandshakeHeader& header, + const DataBuffer& input, + DataBuffer* output) override; + + private: + const uint16_t extension_; + const DataBuffer data_; +}; + class TlsAgent; typedef std::function VoidFunction; diff --git a/lib/ssl/dtlscon.c b/lib/ssl/dtlscon.c index ba28a4708d..45433f034b 100644 --- a/lib/ssl/dtlscon.c +++ b/lib/ssl/dtlscon.c @@ -333,6 +333,20 @@ dtls_HandleHandshake(sslSocket *ss, sslBuffer *origBuf) break; } + /* If we're a server and we receive what appears to be a retried + * ClientHello, and we are expecting a ClientHello, move the receive + * sequence number forward. This allows for a retried ClientHello if we + * send a stateless HelloRetryRequest. */ + if (message_seq > ss->ssl3.hs.recvMessageSeq && + message_seq == 1 && + fragment_offset == 0 && + ss->ssl3.hs.ws == wait_client_hello && + (SSLHandshakeType)type == ssl_hs_client_hello) { + SSL_TRC(5, ("%d: DTLS[%d]: Received apparent 2nd ClientHello", + SSL_GETPID(), ss->fd)); + ss->ssl3.hs.recvMessageSeq = 1; + } + /* There are three ways we could not be ready for this packet. * * 1. It's a partial next message. diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c index 64ebf21787..5ef7cf02c2 100644 --- a/lib/ssl/ssl3con.c +++ b/lib/ssl/ssl3con.c @@ -4123,7 +4123,7 @@ ssl3_UpdateHandshakeHashes(sslSocket *ss, const unsigned char *b, unsigned int l return sslBuffer_Append(&ss->ssl3.hs.messages, b, l); } - PRINT_BUF(90, (NULL, "handshake hash input:", b, l)); + PRINT_BUF(90, (ss, "handshake hash input:", b, l)); if (ss->ssl3.hs.hashType == handshake_hash_single) { PORT_Assert(ss->version >= SSL_LIBRARY_VERSION_TLS_1_3); @@ -8467,6 +8467,7 @@ ssl3_HandleClientHello(sslSocket *ss, PRUint8 *b, PRUint32 length) } #endif + /* If there is a cookie, then this is a second ClientHello (TLS 1.3). */ if (ssl3_FindExtension(ss, ssl_tls13_cookie_xtn)) { ss->ssl3.hs.helloRetry = PR_TRUE; } diff --git a/lib/ssl/sslexp.h b/lib/ssl/sslexp.h index 86b790dae6..32553ba763 100644 --- a/lib/ssl/sslexp.h +++ b/lib/ssl/sslexp.h @@ -309,7 +309,7 @@ typedef SECStatus(PR_CALLBACK *SSLExtensionHandler)( * server socket. All necessary state to continue the TLS handshake will be * included in the cookie extension. This makes it possible to use a new socket * to handle the remainder of the handshake. The existing socket can be safely - * discarded. [TODO: see Bug 1386096] + * discarded. * * If the same socket is retained, the information in the cookie will be checked * for consistency against the existing state of the socket. Any discrepancy diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c index 5b4021d385..63537cc4f0 100644 --- a/lib/ssl/tls13con.c +++ b/lib/ssl/tls13con.c @@ -1252,12 +1252,6 @@ tls13_MaybeSendHelloRetry(sslSocket *ss, const sslNamedGroupDef *requestedGroup, unsigned int tokenLen = 0; SECStatus rv; - /* We asked already, but didn't get a share. */ - if (requestedGroup && ss->ssl3.hs.helloRetry) { - FATAL_ERROR(ss, SSL_ERROR_BAD_2ND_CLIENT_HELLO, illegal_parameter); - return SECFailure; - } - if (ss->hrrCallback) { action = ss->hrrCallback(!ss->ssl3.hs.helloRetry, ss->xtnData.applicationToken.data, @@ -1341,8 +1335,9 @@ tls13_HandleClientHelloPart2(sslSocket *ss, SSL3Statistics *ssl3stats = SSL_GetStatistics(); const sslNamedGroupDef *requestedGroup = NULL; TLS13KeyShareEntry *clientShare = NULL; + ssl3CipherSuite previousCipherSuite = 0; + const sslNamedGroupDef *previousGroup = NULL; PRBool hrr = PR_FALSE; - ssl3CipherSuite previousCipherSuite; if (ssl3_ExtensionNegotiated(ss, ssl_tls13_early_data_xtn)) { ss->ssl3.hs.zeroRttState = ssl_0rtt_sent; @@ -1357,36 +1352,40 @@ tls13_HandleClientHelloPart2(sslSocket *ss, } #endif - previousCipherSuite = ss->ssl3.hs.cipher_suite; + /* Negotiate cipher suite. */ rv = ssl3_NegotiateCipherSuite(ss, suites, PR_FALSE); if (rv != SECSuccess) { FATAL_ERROR(ss, SSL_ERROR_NO_CYPHER_OVERLAP, handshake_failure); goto loser; } + /* If we are going around again, then we should make sure that the cipher * suite selection doesn't change. That's a sign of client shennanigans. */ if (ss->ssl3.hs.helloRetry) { - if (ss->ssl3.hs.cipher_suite != previousCipherSuite) { - FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_CLIENT_HELLO, handshake_failure); - goto loser; - } - if (!ss->xtnData.cookie.len) { - FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_CLIENT_HELLO, handshake_failure); - goto loser; + + /* Update sequence numbers before checking the cookie so that any alerts + * we generate are sent with the right sequence numbers. */ + if (IS_DTLS(ss)) { + /* Count the first ClientHello and the HelloRetryRequest. */ + ss->ssl3.hs.sendMessageSeq = 1; + ss->ssl3.hs.recvMessageSeq = 1; + ssl_GetSpecWriteLock(ss); + /* Increase the write sequence number. The read sequence number + * will be reset after this to early data or handshake. */ + ss->ssl3.cwSpec->write_seq_num = 1; + ssl_ReleaseSpecWriteLock(ss); } + + PORT_Assert(ss->xtnData.cookie.len); PRINT_BUF(50, (ss, "Client sent cookie", ss->xtnData.cookie.data, ss->xtnData.cookie.len)); - rv = tls13_RecoverHashState(ss, ss->xtnData.cookie.data, ss->xtnData.cookie.len); + rv = tls13_RecoverHashState(ss, ss->xtnData.cookie.data, + ss->xtnData.cookie.len, + &previousCipherSuite, + &previousGroup); if (rv != SECSuccess) { - goto loser; - } - } else { - if (ss->xtnData.cookie.len) { - /* Client shouldn't be sending a cookie if we're not doing HRR. - * TODO(ekr@rtfm.com): Remove this when we go to totally stateless. - */ - FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_CLIENT_HELLO, handshake_failure); + FATAL_ERROR(ss, SSL_ERROR_BAD_2ND_CLIENT_HELLO, illegal_parameter); goto loser; } } @@ -1430,6 +1429,26 @@ tls13_HandleClientHelloPart2(sslSocket *ss, PORT_Assert((requestedGroup && !clientShare) || (!requestedGroup && clientShare)); + /* After HelloRetryRequest, check consistency of cipher and group. */ + if (ss->ssl3.hs.helloRetry) { + PORT_Assert(previousCipherSuite); + if (ss->ssl3.hs.cipher_suite != previousCipherSuite) { + FATAL_ERROR(ss, SSL_ERROR_BAD_2ND_CLIENT_HELLO, + illegal_parameter); + goto loser; + } + if (!clientShare) { + FATAL_ERROR(ss, SSL_ERROR_BAD_2ND_CLIENT_HELLO, + illegal_parameter); + goto loser; + } + if (previousGroup && clientShare->group != previousGroup) { + FATAL_ERROR(ss, SSL_ERROR_BAD_2ND_CLIENT_HELLO, + illegal_parameter); + goto loser; + } + } + rv = tls13_MaybeSendHelloRetry(ss, requestedGroup, &hrr); if (rv != SECSuccess) { goto loser; @@ -1619,6 +1638,7 @@ SSLExp_HelloRetryRequestCallback(PRFileDesc *fd, */ SECStatus tls13_ConstructHelloRetryRequest(sslSocket *ss, + ssl3CipherSuite cipherSuite, const sslNamedGroupDef *selectedGroup, PRUint8 *cookie, unsigned int cookieLen, sslBuffer *buffer) @@ -1634,7 +1654,7 @@ tls13_ConstructHelloRetryRequest(sslSocket *ss, goto loser; } - rv = sslBuffer_AppendNumber(buffer, ss->ssl3.hs.cipher_suite, 2); + rv = sslBuffer_AppendNumber(buffer, cipherSuite, 2); if (rv != SECSuccess) { goto loser; } @@ -1692,7 +1712,8 @@ tls13_SendHelloRetryRequest(sslSocket *ss, } /* Now build the body of the message. */ - rv = tls13_ConstructHelloRetryRequest(ss, requestedGroup, + rv = tls13_ConstructHelloRetryRequest(ss, ss->ssl3.hs.cipher_suite, + requestedGroup, cookie, cookieLen, &messageBuf); if (rv != SECSuccess) { FATAL_ERROR(ss, SEC_ERROR_LIBRARY_FAILURE, internal_error); @@ -1715,6 +1736,9 @@ tls13_SendHelloRetryRequest(sslSocket *ss, if (rv != SECSuccess) { goto loser; /* error code set by ssl3_FlushHandshake */ } + /* We depend on this being exactly one record and one message. */ + PORT_Assert(!IS_DTLS(ss) || (ss->ssl3.hs.sendMessageSeq == 1 && + ss->ssl3.cwSpec->write_seq_num == 1)); ssl_ReleaseXmitBufLock(ss); ss->ssl3.hs.helloRetry = PR_TRUE; @@ -1933,6 +1957,8 @@ tls13_HandleHelloRetryRequest(sslSocket *ss, PRUint8 *b, PRUint32 length) rv = ssl_ClientConsumeCipherSuite(ss, version, &b, &length); if (rv != SECSuccess) { + FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_HELLO_RETRY_REQUEST, + illegal_parameter); return SECFailure; /* error code already set */ } @@ -3078,7 +3104,7 @@ tls13_ComputeHandshakeHashes(sslSocket *ss, SSL3Hashes *hashes) goto loser; } - PRINT_BUF(10, (NULL, "Handshake hash computed over saved messages", + PRINT_BUF(10, (ss, "Handshake hash computed over saved messages", ss->ssl3.hs.messages.buf, ss->ssl3.hs.messages.len)); @@ -3104,7 +3130,7 @@ tls13_ComputeHandshakeHashes(sslSocket *ss, SSL3Hashes *hashes) goto loser; } - PRINT_BUF(10, (NULL, "Handshake hash", hashes->u.raw, hashes->len)); + PRINT_BUF(10, (ss, "Handshake hash", hashes->u.raw, hashes->len)); PORT_Assert(hashes->len == tls13_GetHashSize(ss)); PK11_DestroyContext(ctx, PR_TRUE); diff --git a/lib/ssl/tls13con.h b/lib/ssl/tls13con.h index 1be28f485e..920f49d6c4 100644 --- a/lib/ssl/tls13con.h +++ b/lib/ssl/tls13con.h @@ -75,6 +75,7 @@ SECStatus tls13_HandleServerHelloPart2(sslSocket *ss); SECStatus tls13_HandlePostHelloHandshakeMessage(sslSocket *ss, PRUint8 *b, PRUint32 length); SECStatus tls13_ConstructHelloRetryRequest(sslSocket *ss, + ssl3CipherSuite cipherSuite, const sslNamedGroupDef *selectedGroup, PRUint8 *cookie, unsigned int cookieLen, sslBuffer *buffer); diff --git a/lib/ssl/tls13hashstate.c b/lib/ssl/tls13hashstate.c index f32e830870..5b4e011a18 100644 --- a/lib/ssl/tls13hashstate.c +++ b/lib/ssl/tls13hashstate.c @@ -88,8 +88,9 @@ tls13_MakeHrrCookie(sslSocket *ss, const sslNamedGroupDef *selectedGroup, /* Recover the hash state from the cookie. */ SECStatus tls13_RecoverHashState(sslSocket *ss, - unsigned char *cookie, - unsigned int cookieLen) + unsigned char *cookie, unsigned int cookieLen, + ssl3CipherSuite *previousCipherSuite, + const sslNamedGroupDef **previousGroup) { SECStatus rv; unsigned char plaintext[1024]; @@ -116,7 +117,7 @@ tls13_RecoverHashState(sslSocket *ss, } /* The cipher suite should be the same or there are some shenanigans. */ rv = ssl3_ConsumeNumberFromItem(&ptItem, &cipherSuite, 2); - if ((rv != SECSuccess) || cipherSuite != ss->ssl3.hs.cipher_suite) { + if (rv != SECSuccess) { FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_CLIENT_HELLO, illegal_parameter); return SECFailure; } @@ -164,7 +165,8 @@ tls13_RecoverHashState(sslSocket *ss, } /* And finally reinject the HRR. */ - rv = tls13_ConstructHelloRetryRequest(ss, selectedGroup, + rv = tls13_ConstructHelloRetryRequest(ss, cipherSuite, + selectedGroup, cookie, cookieLen, &messageBuf); if (rv != SECSuccess) { @@ -178,5 +180,7 @@ tls13_RecoverHashState(sslSocket *ss, return SECFailure; } + *previousCipherSuite = cipherSuite; + *previousGroup = selectedGroup; return SECSuccess; } diff --git a/lib/ssl/tls13hashstate.h b/lib/ssl/tls13hashstate.h index 3357a78856..e9a4aa84f3 100644 --- a/lib/ssl/tls13hashstate.h +++ b/lib/ssl/tls13hashstate.h @@ -19,5 +19,7 @@ SECStatus tls13_MakeHrrCookie(sslSocket *ss, const sslNamedGroupDef *selectedGro SECStatus tls13_GetHrrCookieLength(sslSocket *ss, unsigned int *length); SECStatus tls13_RecoverHashState(sslSocket *ss, unsigned char *cookie, - unsigned int cookieLen); + unsigned int cookieLen, + ssl3CipherSuite *previousCipherSuite, + const sslNamedGroupDef **previousGroup); #endif