Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Bug 1600144 - Treat ClientHello with message_seq of 1 as a second Cli…
…entHello, r=kjacobs

Summary:
The logic that deals with stateless HelloRetryRequest in DTLS
allows this one-off increment to the message_seq field in case the server was
operating statelessly.  However, when it does, it should insist on the
ClientHello carrying a cookie; concretely, it should set the flag that says that
a HelloRetryRequest was sent, even if it doesn't currently remember that it sent
one. That is the only way that this condition could be met.

Differential Revision: https://phabricator.services.mozilla.com/D55210

--HG--
extra : rebase_source : 07acedf98f6619e53706a9c3a738a81198178747
extra : amend_source : fb1f0cd771db8464901a9103f1b6989f94f71a20
  • Loading branch information
martinthomson committed Nov 29, 2019
1 parent 5438617 commit 330d7ef
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 0 deletions.
33 changes: 33 additions & 0 deletions gtests/ssl_gtest/ssl_hrr_unittest.cc
Expand Up @@ -1055,6 +1055,39 @@ TEST_F(TlsConnectTest, Select12AfterHelloRetryRequest) {
EXPECT_EQ(SSL_ERROR_RX_MALFORMED_SERVER_HELLO, client_->error_code());
}

// This class increments the low byte of the first Handshake.message_seq
// field in every handshake record.
class MessageSeqIncrementer : public TlsRecordFilter {
public:
MessageSeqIncrementer(const std::shared_ptr<TlsAgent>& a) : TlsRecordFilter(a) {}

protected:
PacketFilter::Action FilterRecord(const TlsRecordHeader& header,
const DataBuffer& data,
DataBuffer* changed) override {
if (header.content_type() != ssl_ct_handshake) {
return KEEP;
}

*changed = data;
// struct { uint8 msg_type; uint24 length; uint16 message_seq; ... } Handshake;
changed->data()[5]++;
EXPECT_NE(0, changed->data()[5]); // Check for overflow.
return CHANGE;
}
};

// A server that receives a ClientHello with message_seq == 1
// assumes that this is after a stateless HelloRetryRequest.
// However, it should reject the ClientHello if it lacks a cookie.
TEST_F(TlsConnectDatagram13, MessageSeq1ClientHello) {
EnsureTlsSetup();
MakeTlsFilter<MessageSeqIncrementer>(client_);
ConnectExpectAlert(server_, kTlsAlertMissingExtension);
EXPECT_EQ(SSL_ERROR_MISSING_COOKIE_EXTENSION, server_->error_code());
EXPECT_EQ(SSL_ERROR_MISSING_EXTENSION_ALERT, client_->error_code());
}

class HelloRetryRequestAgentTest : public TlsAgentTestClient {
protected:
void SetUp() override {
Expand Down
1 change: 1 addition & 0 deletions lib/ssl/dtlscon.c
Expand Up @@ -343,6 +343,7 @@ dtls_HandleHandshake(sslSocket *ss, DTLSEpoch epoch, sslSequenceNumber seqNum,
SSL_TRC(5, ("%d: DTLS[%d]: Received apparent 2nd ClientHello",
SSL_GETPID(), ss->fd));
ss->ssl3.hs.recvMessageSeq = 1;
ss->ssl3.hs.helloRetry = PR_TRUE;
}

/* There are three ways we could not be ready for this packet.
Expand Down

0 comments on commit 330d7ef

Please sign in to comment.