Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Bug 1487597 - Improve 0-RTT data delivery, r=ekr
Summary:
This improves the code that delivers 0-RTT.  When the caller provided a read
buffer to small to hold an entire record, the previous code reported errors.
Those errors might cause the connection to be dropped by the caller, but the
socket was still usable.  If the socket was used again, there would be a gap in
the stream.

This fixes that bug and adds a bunch of tests around 0-RTT delivery.  More tests
check the order of operations.

For instance, in TLS, we strictly maintain ordering between 0-RTT data delivery
and handshake completion.  That is not the case for DTLS, where this allows
0-RTT records that arrive before the handshake completes to be read afterwards.
We do drop keys as soon as we see EndOfEarlyData (this is going away for DTLS,
so I assume Certificate/Finished will be the trigger eventually).  The tests
added here confirm that late arrival causes 0-RTT to be dropped.  Another test
confirms that any early arrival that is only read late will be delivered.

Reviewers: ekr

Subscribers: mt, ekr

Tags: #secure-revision, PHID-PROJ-ffhf7tdvqze7zrdn6dh3

Bug #: 1487597

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

--HG--
extra : rebase_source : 540d790d678828a155457e9d0f5a3e34527391c0
extra : amend_source : 3856c989ac5b323d6683d33304fa8887d6fd7ac0
  • Loading branch information
martinthomson committed Sep 10, 2018
1 parent 47b5b1a commit e81e063
Show file tree
Hide file tree
Showing 5 changed files with 261 additions and 16 deletions.
221 changes: 221 additions & 0 deletions gtests/ssl_gtest/ssl_0rtt_unittest.cc
Expand Up @@ -649,6 +649,227 @@ TEST_P(TlsConnectTls13, ZeroRttOrdering) {
EXPECT_EQ(2U, step);
}

// Early data remains available after the handshake completes for TLS.
TEST_F(TlsConnectStreamTls13, ZeroRttLateReadTls) {
SetupForZeroRtt();
client_->Set0RttEnabled(true);
server_->Set0RttEnabled(true);
ExpectResumption(RESUME_TICKET);
client_->Handshake(); // ClientHello

// Write some early data.
const uint8_t data[] = {1, 2, 3, 4, 5, 6, 7, 8};
PRInt32 rv = PR_Write(client_->ssl_fd(), data, sizeof(data));
EXPECT_EQ(static_cast<PRInt32>(sizeof(data)), rv);

// Consume the ClientHello and generate ServerHello..Finished.
server_->Handshake();

// Read some of the data.
std::vector<uint8_t> small_buffer(1 + sizeof(data) / 2);
rv = PR_Read(server_->ssl_fd(), small_buffer.data(), small_buffer.size());
EXPECT_EQ(static_cast<PRInt32>(small_buffer.size()), rv);
EXPECT_EQ(0, memcmp(data, small_buffer.data(), small_buffer.size()));

Handshake(); // Complete the handshake.
ExpectEarlyDataAccepted(true);
CheckConnected();

// After the handshake, it should be possible to read the remainder.
uint8_t big_buf[100];
rv = PR_Read(server_->ssl_fd(), big_buf, sizeof(big_buf));
EXPECT_EQ(static_cast<PRInt32>(sizeof(data) - small_buffer.size()), rv);
EXPECT_EQ(0, memcmp(&data[small_buffer.size()], big_buf,
sizeof(data) - small_buffer.size()));

// And that's all there is to read.
rv = PR_Read(server_->ssl_fd(), big_buf, sizeof(big_buf));
EXPECT_GT(0, rv);
EXPECT_EQ(PR_WOULD_BLOCK_ERROR, PORT_GetError());
}

// Early data that arrives before the handshake can be read after the handshake
// is complete.
TEST_F(TlsConnectDatagram13, ZeroRttLateReadDtls) {
SetupForZeroRtt();
client_->Set0RttEnabled(true);
server_->Set0RttEnabled(true);
ExpectResumption(RESUME_TICKET);
client_->Handshake(); // ClientHello

// Write some early data.
const uint8_t data[] = {1, 2, 3};
PRInt32 written = PR_Write(client_->ssl_fd(), data, sizeof(data));
EXPECT_EQ(static_cast<PRInt32>(sizeof(data)), written);

Handshake(); // Complete the handshake.
ExpectEarlyDataAccepted(true);
CheckConnected();

// Reading at the server should return the early data, which was buffered.
uint8_t buf[sizeof(data) + 1] = {0};
PRInt32 read = PR_Read(server_->ssl_fd(), buf, sizeof(buf));
EXPECT_EQ(static_cast<PRInt32>(sizeof(data)), read);
EXPECT_EQ(0, memcmp(data, buf, sizeof(data)));
}

class PacketHolder : public PacketFilter {
public:
PacketHolder() = default;

virtual Action Filter(const DataBuffer& input, DataBuffer* output) {
packet_ = input;
Disable();
return DROP;
}

const DataBuffer& packet() const { return packet_; }

private:
DataBuffer packet_;
};

// Early data that arrives late is discarded for DTLS.
TEST_F(TlsConnectDatagram13, ZeroRttLateArrivalDtls) {
SetupForZeroRtt();
client_->Set0RttEnabled(true);
server_->Set0RttEnabled(true);
ExpectResumption(RESUME_TICKET);
client_->Handshake(); // ClientHello

// Write some early data. Twice, so that we can read bits of it.
const uint8_t data[] = {1, 2, 3};
PRInt32 written = PR_Write(client_->ssl_fd(), data, sizeof(data));
EXPECT_EQ(static_cast<PRInt32>(sizeof(data)), written);

// Block and capture the next packet.
auto holder = std::make_shared<PacketHolder>();
client_->SetFilter(holder);
written = PR_Write(client_->ssl_fd(), data, sizeof(data));
EXPECT_EQ(static_cast<PRInt32>(sizeof(data)), written);
EXPECT_FALSE(holder->enabled()) << "the filter should disable itself";

// Consume the ClientHello and generate ServerHello..Finished.
server_->Handshake();

// Read some of the data.
std::vector<uint8_t> small_buffer(sizeof(data));
PRInt32 read =
PR_Read(server_->ssl_fd(), small_buffer.data(), small_buffer.size());

EXPECT_EQ(static_cast<PRInt32>(small_buffer.size()), read);
EXPECT_EQ(0, memcmp(data, small_buffer.data(), small_buffer.size()));

Handshake(); // Complete the handshake.
ExpectEarlyDataAccepted(true);
CheckConnected();

server_->SendDirect(holder->packet());

// Reading now should return nothing, even though a valid packet was
// delivered.
read = PR_Read(server_->ssl_fd(), small_buffer.data(), small_buffer.size());
EXPECT_GT(0, read);
EXPECT_EQ(PR_WOULD_BLOCK_ERROR, PORT_GetError());
}

// Early data reads in TLS should be coalesced.
TEST_F(TlsConnectStreamTls13, ZeroRttCoalesceReadTls) {
SetupForZeroRtt();
client_->Set0RttEnabled(true);
server_->Set0RttEnabled(true);
ExpectResumption(RESUME_TICKET);
client_->Handshake(); // ClientHello

// Write some early data. In two writes.
const uint8_t data[] = {1, 2, 3, 4, 5, 6};
PRInt32 written = PR_Write(client_->ssl_fd(), data, 1);
EXPECT_EQ(1, written);

written = PR_Write(client_->ssl_fd(), data + 1, sizeof(data) - 1);
EXPECT_EQ(static_cast<PRInt32>(sizeof(data) - 1), written);

// Consume the ClientHello and generate ServerHello..Finished.
server_->Handshake();

// Read all of the data.
std::vector<uint8_t> buffer(sizeof(data));
PRInt32 read = PR_Read(server_->ssl_fd(), buffer.data(), buffer.size());
EXPECT_EQ(static_cast<PRInt32>(sizeof(data)), read);
EXPECT_EQ(0, memcmp(data, buffer.data(), sizeof(data)));

Handshake(); // Complete the handshake.
ExpectEarlyDataAccepted(true);
CheckConnected();
}

// Early data reads in DTLS should not be coalesced.
TEST_F(TlsConnectDatagram13, ZeroRttNoCoalesceReadDtls) {
SetupForZeroRtt();
client_->Set0RttEnabled(true);
server_->Set0RttEnabled(true);
ExpectResumption(RESUME_TICKET);
client_->Handshake(); // ClientHello

// Write some early data. In two writes.
const uint8_t data[] = {1, 2, 3, 4, 5, 6};
PRInt32 written = PR_Write(client_->ssl_fd(), data, 1);
EXPECT_EQ(1, written);

written = PR_Write(client_->ssl_fd(), data + 1, sizeof(data) - 1);
EXPECT_EQ(static_cast<PRInt32>(sizeof(data) - 1), written);

// Consume the ClientHello and generate ServerHello..Finished.
server_->Handshake();

// Try to read all of the data.
std::vector<uint8_t> buffer(sizeof(data));
PRInt32 read = PR_Read(server_->ssl_fd(), buffer.data(), buffer.size());
EXPECT_EQ(1, read);
EXPECT_EQ(0, memcmp(data, buffer.data(), 1));

// Read the remainder.
read = PR_Read(server_->ssl_fd(), buffer.data(), buffer.size());
EXPECT_EQ(static_cast<PRInt32>(sizeof(data) - 1), read);
EXPECT_EQ(0, memcmp(data + 1, buffer.data(), sizeof(data) - 1));

Handshake(); // Complete the handshake.
ExpectEarlyDataAccepted(true);
CheckConnected();
}

// Early data reads in DTLS should fail if the buffer is too small.
TEST_F(TlsConnectDatagram13, ZeroRttShortReadDtls) {
SetupForZeroRtt();
client_->Set0RttEnabled(true);
server_->Set0RttEnabled(true);
ExpectResumption(RESUME_TICKET);
client_->Handshake(); // ClientHello

// Write some early data. In two writes.
const uint8_t data[] = {1, 2, 3, 4, 5, 6};
PRInt32 written = PR_Write(client_->ssl_fd(), data, sizeof(data));
EXPECT_EQ(static_cast<PRInt32>(sizeof(data)), written);

// Consume the ClientHello and generate ServerHello..Finished.
server_->Handshake();

// Try to read all of the data into a small buffer.
std::vector<uint8_t> buffer(sizeof(data));
PRInt32 read = PR_Read(server_->ssl_fd(), buffer.data(), 1);
EXPECT_GT(0, read);
EXPECT_EQ(SSL_ERROR_RX_SHORT_DTLS_READ, PORT_GetError());

// Read again with more space.
read = PR_Read(server_->ssl_fd(), buffer.data(), buffer.size());
EXPECT_EQ(static_cast<PRInt32>(sizeof(data)), read);
EXPECT_EQ(0, memcmp(data, buffer.data(), sizeof(data)));

Handshake(); // Complete the handshake.
ExpectEarlyDataAccepted(true);
CheckConnected();
}

#ifndef NSS_DISABLE_TLS_1_3
INSTANTIATE_TEST_CASE_P(Tls13ZeroRttReplayTest, TlsZeroRttReplayTest,
TlsConnectTestBase::kTlsVariantsAll);
Expand Down
4 changes: 3 additions & 1 deletion gtests/ssl_gtest/test_io.h
Expand Up @@ -33,9 +33,11 @@ class PacketFilter {
CHANGE, // change the packet to a different value
DROP // drop the packet
};
PacketFilter(bool enabled = true) : enabled_(enabled) {}
explicit PacketFilter(bool enabled = true) : enabled_(enabled) {}
virtual ~PacketFilter() {}

bool enabled() const { return enabled_; }

virtual Action Process(const DataBuffer& input, DataBuffer* output) {
if (!enabled_) {
return KEEP;
Expand Down
1 change: 1 addition & 0 deletions gtests/ssl_gtest/tls_connect.cc
Expand Up @@ -250,6 +250,7 @@ void TlsConnectTestBase::Reset(const std::string& server_name,
server_->SkipVersionChecks();
}

std::cerr << "Reset" << std::endl;
Init();
}

Expand Down
5 changes: 3 additions & 2 deletions lib/ssl/sslimpl.h
Expand Up @@ -576,8 +576,9 @@ struct TLS13KeyShareEntryStr {
};

typedef struct TLS13EarlyDataStr {
PRCList link; /* The linked list link */
SECItem data; /* The data */
PRCList link; /* The linked list link */
unsigned int consumed; /* How much has been read. */
SECItem data; /* The data */
} TLS13EarlyData;

typedef enum {
Expand Down
46 changes: 33 additions & 13 deletions lib/ssl/tls13con.c
Expand Up @@ -5544,23 +5544,43 @@ tls13_MaybeDo0RTTHandshake(sslSocket *ss)
PRInt32
tls13_Read0RttData(sslSocket *ss, void *buf, PRInt32 len)
{
TLS13EarlyData *msg;

PORT_Assert(!PR_CLIST_IS_EMPTY(&ss->ssl3.hs.bufferedEarlyData));
msg = (TLS13EarlyData *)PR_NEXT_LINK(&ss->ssl3.hs.bufferedEarlyData);
PRInt32 offset = 0;
while (!PR_CLIST_IS_EMPTY(&ss->ssl3.hs.bufferedEarlyData)) {
TLS13EarlyData *msg =
(TLS13EarlyData *)PR_NEXT_LINK(&ss->ssl3.hs.bufferedEarlyData);
unsigned int tocpy = msg->data.len - msg->consumed;

if (tocpy > (len - offset)) {
if (IS_DTLS(ss)) {
/* In DTLS, we only return entire records.
* So offset and consumed are always zero. */
PORT_Assert(offset == 0);
PORT_Assert(msg->consumed == 0);
PORT_SetError(SSL_ERROR_RX_SHORT_DTLS_READ);
return -1;
}

PR_REMOVE_LINK(&msg->link);
if (msg->data.len > len) {
PORT_SetError(SSL_ERROR_ILLEGAL_PARAMETER_ALERT);
return SECFailure;
}
len = msg->data.len;
tocpy = len - offset;
}

PORT_Memcpy(buf + offset, msg->data.data + msg->consumed, tocpy);
offset += tocpy;
msg->consumed += tocpy;

PORT_Memcpy(buf, msg->data.data, msg->data.len);
SECITEM_ZfreeItem(&msg->data, PR_FALSE);
PORT_ZFree(msg, sizeof(*msg));
if (msg->consumed == msg->data.len) {
PR_REMOVE_LINK(&msg->link);
SECITEM_ZfreeItem(&msg->data, PR_FALSE);
PORT_ZFree(msg, sizeof(*msg));
}

/* We are done after one record for DTLS; otherwise, when the buffer fills up. */
if (IS_DTLS(ss) || offset == len) {
break;
}
}

return len;
return offset;
}

static SECStatus
Expand Down

0 comments on commit e81e063

Please sign in to comment.