Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Bug 1690583 - Fix CH padding extension size calculation. r=mt
Bug 1654332 changed the way that NSS constructs Client Hello messages. `ssl_CalculatePaddingExtLen` now receives a `clientHelloLength` value that includes the 4B handshake header. This looks okay per the inline comment (which states that only the record header is omitted from the length), but the function actually assumes that the handshake header is also omitted.

This patch removes the addition of the handshake header length. Those bytes are already included in the buffered CH.

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

--HG--
extra : moz-landing-system : lando
  • Loading branch information
Kevin Jacobs committed Feb 4, 2021
1 parent 6f1f30e commit 1b6a857
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 5 deletions.
21 changes: 21 additions & 0 deletions gtests/ssl_gtest/ssl_recordsize_unittest.cc
Expand Up @@ -266,6 +266,27 @@ TEST_P(TlsConnectTls13, RecordSizeCiphertextExceed) {
server_->CheckErrorCode(SSL_ERROR_RECORD_OVERFLOW_ALERT);
}

TEST_F(TlsConnectStreamTls13, ClientHelloF5Padding) {
EnsureTlsSetup();
ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
ScopedPK11SymKey key(
PK11_KeyGen(slot.get(), CKM_NSS_CHACHA20_POLY1305, nullptr, 32, nullptr));

auto filter =
MakeTlsFilter<TlsHandshakeRecorder>(client_, kTlsHandshakeClientHello);

// Add PSK with label long enough to push CH length into [256, 511].
std::vector<uint8_t> label(100);
EXPECT_EQ(SECSuccess,
SSL_AddExternalPsk(client_->ssl_fd(), key.get(), label.data(),
label.size(), ssl_hash_sha256));
StartConnect();
client_->Handshake();

// Filter removes the 4B handshake header.
EXPECT_EQ(508UL, filter->buffer().len());
}

// This indiscriminately adds padding to application data records.
class TlsRecordPadder : public TlsRecordFilter {
public:
Expand Down
7 changes: 2 additions & 5 deletions lib/ssl/ssl3ext.c
Expand Up @@ -837,9 +837,6 @@ ssl_SendEmptyExtension(const sslSocket *ss, TLSExtensionData *xtnData,
static unsigned int
ssl_CalculatePaddingExtLen(const sslSocket *ss, unsigned int clientHelloLength)
{
unsigned int recordLength = 1 /* handshake message type */ +
3 /* handshake message length */ +
clientHelloLength;
unsigned int extensionLen;

/* Don't pad for DTLS, for SSLv3, or for renegotiation. */
Expand All @@ -853,11 +850,11 @@ ssl_CalculatePaddingExtLen(const sslSocket *ss, unsigned int clientHelloLength)
* the ClientHello doesn't have a length between 256 and 511 bytes
* (inclusive). Initial ClientHello records with such lengths trigger bugs
* in F5 devices. */
if (recordLength < 256 || recordLength >= 512) {
if (clientHelloLength < 256 || clientHelloLength >= 512) {
return 0;
}

extensionLen = 512 - recordLength;
extensionLen = 512 - clientHelloLength;
/* Extensions take at least four bytes to encode. Always include at least
* one byte of data if we are padding. Some servers will time out or
* terminate the connection if the last ClientHello extension is empty. */
Expand Down

0 comments on commit 1b6a857

Please sign in to comment.