# HG changeset patch # User Martin Thomson # Date 1523260140 -36000 # Mon Apr 09 17:49:00 2018 +1000 # Node ID 350b7210e90758de454feb4339379ef7f6b9b470 # Parent 5db9e969c74a2a02c4b1d918792827014d1a9d5e Bug 1452549 - Discard application data that arrives before DTLS handshake completes, r=ekr diff --git a/gtests/ssl_gtest/ssl_drop_unittest.cc b/gtests/ssl_gtest/ssl_drop_unittest.cc --- a/gtests/ssl_gtest/ssl_drop_unittest.cc +++ b/gtests/ssl_gtest/ssl_drop_unittest.cc @@ -884,6 +884,45 @@ TEST_P(TlsConnectDatagram12Plus, MissAWi SendReceive(); } +// This filter replaces the first record it sees with junk application data. +class TlsReplaceFirstRecordWithJunk : public TlsRecordFilter { + public: + TlsReplaceFirstRecordWithJunk(const std::shared_ptr& a) + : TlsRecordFilter(a), replaced_(false) {} + + protected: + PacketFilter::Action FilterRecord(const TlsRecordHeader& header, + const DataBuffer& record, size_t* offset, + DataBuffer* output) override { + if (replaced_) { + return KEEP; + } + replaced_ = true; + TlsRecordHeader out_header(header.variant(), header.version(), + kTlsApplicationDataType, + header.sequence_number()); + + static const uint8_t junk[] = {1, 2, 3, 4}; + *offset = out_header.Write(output, *offset, DataBuffer(junk, sizeof(junk))); + return CHANGE; + } + + private: + bool replaced_; +}; + +// DTLS needs to discard application_data that it receives prior to handshake +// completion, not generate an error. +TEST_P(TlsConnectDatagram, ReplaceFirstServerRecordWithApplicationData) { + MakeTlsFilter(server_); + Connect(); +} + +TEST_P(TlsConnectDatagram, ReplaceFirstClientRecordWithApplicationData) { + MakeTlsFilter(client_); + Connect(); +} + INSTANTIATE_TEST_CASE_P(Datagram12Plus, TlsConnectDatagram12Plus, TlsConnectTestBase::kTlsV12Plus); INSTANTIATE_TEST_CASE_P(DatagramPre13, TlsConnectDatagramPre13, diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c --- a/lib/ssl/ssl3con.c +++ b/lib/ssl/ssl3con.c @@ -12216,23 +12216,33 @@ ssl3_HandleRecord(sslSocket *ss, SSL3Cip } } -#ifdef UNSAFE_FUZZER_MODE + /* Most record types aside from protected TLS 1.3 records carry the content + * type in the first octet. TLS 1.3 will override this value later. */ rType = cText->hdr[0]; - rv = Null_Cipher(NULL, plaintext->buf, (int *)&plaintext->len, - plaintext->space, cText->buf->buf, cText->buf->len); + /* Encrypted application data records could arrive before the handshake + * completes in DTLS 1.3. These can look like valid TLS 1.2 application_data + * records in epoch 0, which is never valid. Pretend they didn't decrypt. */ + if (spec->epoch == 0 && rType == content_application_data) { + PORT_SetError(SSL_ERROR_RX_UNEXPECTED_APPLICATION_DATA); + alert = unexpected_message; + rv = SECFailure; + } else { +#ifdef UNSAFE_FUZZER_MODE + rv = Null_Cipher(NULL, plaintext->buf, (int *)&plaintext->len, + plaintext->space, cText->buf->buf, cText->buf->len); #else - /* IMPORTANT: Unprotect functions MUST NOT send alerts - * because we still hold the spec read lock. Instead, if they - * return SECFailure, they set *alert to the alert to be sent. */ - if (spec->version < SSL_LIBRARY_VERSION_TLS_1_3 || - spec->cipherDef->calg == ssl_calg_null) { - /* Unencrypted TLS 1.3 records use the pre-TLS 1.3 format. */ - rType = cText->hdr[0]; - rv = ssl3_UnprotectRecord(ss, spec, cText, plaintext, &alert); - } else { - rv = tls13_UnprotectRecord(ss, spec, cText, plaintext, &rType, &alert); - } + /* IMPORTANT: Unprotect functions MUST NOT send alerts + * because we still hold the spec read lock. Instead, if they + * return SECFailure, they set *alert to the alert to be sent. */ + if (spec->version < SSL_LIBRARY_VERSION_TLS_1_3 || + spec->epoch == 0) { + rv = ssl3_UnprotectRecord(ss, spec, cText, plaintext, &alert); + } else { + rv = tls13_UnprotectRecord(ss, spec, cText, plaintext, &rType, + &alert); + } #endif + } if (rv != SECSuccess) { ssl_ReleaseSpecReadLock(ss); /***************************/ @@ -12242,10 +12252,10 @@ ssl3_HandleRecord(sslSocket *ss, SSL3Cip /* Ensure that we don't process this data again. */ plaintext->len = 0; - /* Ignore a CCS if the alternative handshake is negotiated. Note that - * this will fail if the server fails to negotiate the alternative - * handshake type in a 0-RTT session that is resumed from a session that - * did negotiate it. We don't care about that corner case right now. */ + /* Ignore a CCS if compatibility mode is negotiated. Note that this + * will fail if the server fails to negotiate compatibility mode in a + * 0-RTT session that is resumed from a session that did negotiate it. + * We don't care about that corner case right now. */ if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3 && cText->hdr[0] == content_change_cipher_spec && ss->ssl3.hs.ws != idle_handshake && @@ -12254,19 +12264,20 @@ ssl3_HandleRecord(sslSocket *ss, SSL3Cip /* Ignore the CCS. */ return SECSuccess; } + if (IS_DTLS(ss) || (ss->sec.isServer && ss->ssl3.hs.zeroRttIgnore == ssl_0rtt_ignore_trial)) { /* Silently drop the packet */ return SECSuccess; - } else { - int errCode = PORT_GetError(); - SSL3_SendAlert(ss, alert_fatal, alert); - /* Reset the error code in case SSL3_SendAlert called - * PORT_SetError(). */ - PORT_SetError(errCode); - return SECFailure; - } + } + + int errCode = PORT_GetError(); + SSL3_SendAlert(ss, alert_fatal, alert); + /* Reset the error code in case SSL3_SendAlert called + * PORT_SetError(). */ + PORT_SetError(errCode); + return SECFailure; } /* SECSuccess */