From d901b16ba22cb7b4a9a9011857d3dbb6b7a201e3 Mon Sep 17 00:00:00 2001 From: Benjamin Beurdouche Date: Fri, 5 Feb 2021 21:13:47 +0000 Subject: [PATCH] Bug 1688685 - land NSS fc3a4c142c16 UPGRADE_NSS_RELEASE, r=kjacobs 2021-02-04 Kevin Jacobs * gtests/ssl_gtest/ssl_recordsize_unittest.cc, lib/ssl/ssl3ext.c: 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. [fc3a4c142c16] [tip] * automation/abi-check/expected-report-libnss3.so.txt: Bug 1690421 - Adjust 3.62 ABI report formatting for new libabigail. r=bbeurdouche [a1ed44dba32e] 2021-02-03 Kevin Jacobs * automation/taskcluster/docker-builds/Dockerfile: Bug 1690421 - Install packaged libabigail in docker-builds image r=bbeurdouche [3c719b620136] 2021-01-31 Kevin Jacobs * cmd/selfserv/selfserv.c, cmd/tstclnt/tstclnt.c, lib/ssl/tls13hashstate.c, lib/ssl/tls13hashstate.h: Bug 1689228 - Minor ECH -09 fixes for interop testing, fuzzing. r=mt A few minor ECH -09 fixes for interop testing and fuzzing: - selfserv now takes a PKCS8 keypair for ECH. This is more maintainable and significantly less terrible than parsing the ECHConfigs and cobbling one together within selfserv (e.g. we can support other KEMs without modifying the server). - Get rid of the newline character in tstclnt retry_configs output. - Fuzzer fixes in tls13_HandleHrrCookie: - We shouldn't use internal_error when PK11_HPKE_ImportContext fails. Cookies are unprotected in fuzzer mode, so this can be expected to occur. - Only restore the application token when recovering hash state, otherwise the copy could happen twice, leaking one of the allocations. [8bbea1902024] 2021-01-25 Kevin Jacobs * lib/ssl/ssl3exthandle.c: Bug 1674819 - Fixup a51fae403328, enum type may be signed. r=bbeurdouche [2004338a2080] Differential Revision: https://phabricator.services.mozilla.com/D104258 --- security/nss/TAG-INFO | 2 +- .../abi-check/expected-report-libnss3.so.txt | 4 +-- .../taskcluster/docker-builds/Dockerfile | 11 +----- security/nss/cmd/selfserv/selfserv.c | 36 +++++-------------- security/nss/cmd/tstclnt/tstclnt.c | 5 +++ security/nss/coreconf/coreconf.dep | 1 - .../ssl_gtest/ssl_recordsize_unittest.cc | 21 +++++++++++ security/nss/lib/ssl/ssl3ext.c | 7 ++-- security/nss/lib/ssl/ssl3exthandle.c | 2 +- security/nss/lib/ssl/tls13hashstate.c | 28 ++++++++------- security/nss/lib/ssl/tls13hashstate.h | 2 +- 11 files changed, 58 insertions(+), 61 deletions(-) diff --git a/security/nss/TAG-INFO b/security/nss/TAG-INFO index 60e30d16e298..bb156686dcab 100644 --- a/security/nss/TAG-INFO +++ b/security/nss/TAG-INFO @@ -1 +1 @@ -92dcda94c1d4 \ No newline at end of file +fc3a4c142c16 \ No newline at end of file diff --git a/security/nss/automation/abi-check/expected-report-libnss3.so.txt b/security/nss/automation/abi-check/expected-report-libnss3.so.txt index f758e8bec79e..20c74c71156a 100644 --- a/security/nss/automation/abi-check/expected-report-libnss3.so.txt +++ b/security/nss/automation/abi-check/expected-report-libnss3.so.txt @@ -1,5 +1,5 @@ 2 Added functions: - [A] 'function SECStatus PK11_HPKE_ExportContext(const HpkeContext*, PK11SymKey*, SECItem**)' {PK11_HPKE_ExportContext@@NSS_3.62} - [A] 'function HpkeContext* PK11_HPKE_ImportContext(const SECItem*, PK11SymKey*)' {PK11_HPKE_ImportContext@@NSS_3.62} + 'function SECStatus PK11_HPKE_ExportContext(const HpkeContext*, PK11SymKey*, SECItem**)' {PK11_HPKE_ExportContext@@NSS_3.62} + 'function HpkeContext* PK11_HPKE_ImportContext(const SECItem*, PK11SymKey*)' {PK11_HPKE_ImportContext@@NSS_3.62} diff --git a/security/nss/automation/taskcluster/docker-builds/Dockerfile b/security/nss/automation/taskcluster/docker-builds/Dockerfile index 0ce4e80c6f97..97436902cb82 100644 --- a/security/nss/automation/taskcluster/docker-builds/Dockerfile +++ b/security/nss/automation/taskcluster/docker-builds/Dockerfile @@ -43,16 +43,7 @@ RUN update-alternatives --install /usr/bin/clang-format \ # Latest version of abigail-tools RUN apt-get update \ - && apt-get install -y --no-install-recommends automake libtool libxml2-dev \ - && git clone git://sourceware.org/git/libabigail.git /tmp/libabigail \ - && cd /tmp/libabigail \ - && autoreconf -fi \ - && ./configure --prefix=/usr --disable-static --disable-apidoc --disable-manual \ - && make && make install \ - && rm -rf /tmp/libabigail \ - && apt-get remove -y automake libtool libxml2-dev \ - && rm -rf /var/lib/apt/lists/* \ - && apt-get autoremove -y && apt-get clean -y + && apt-get install -y libabigail-dev abigail-tools ENV SHELL /bin/bash ENV USER worker diff --git a/security/nss/cmd/selfserv/selfserv.c b/security/nss/cmd/selfserv/selfserv.c index be703318a175..6b6f53a353e2 100644 --- a/security/nss/cmd/selfserv/selfserv.c +++ b/security/nss/cmd/selfserv/selfserv.c @@ -1968,12 +1968,14 @@ configureEchWithData(PRFileDesc *model_sock) { /* The input should be a Base64-encoded ECHKey struct: * struct { - * opaque sk<0..2^16-1>; - * ECHConfig config<0..2^16>; // draft-ietf-tls-esni-09 + * opaque pkcs8_ech_keypair<0..2^16-1>; + * ECHConfigs configs<0..2^16>; // draft-ietf-tls-esni-09 * } ECHKey; * * This is not a standardized format, rather it's designed for * interoperability with https://github.com/xvzcf/tls-interop-runner. + * It is the user's responsibility to ensure that the PKCS8 keypair + * corresponds to the public key embedded in the ECHConfigs. */ #define REMAINING_BYTES(rdr, buf) \ @@ -1984,9 +1986,9 @@ configureEchWithData(PRFileDesc *model_sock) unsigned char *reader; PK11SlotInfo *slot = NULL; SECItem *decoded = NULL; - SECItem *pkcs8Key = NULL; SECKEYPublicKey *pk = NULL; SECKEYPrivateKey *sk = NULL; + SECItem pkcs8Key = { siBuffer, NULL, 0 }; decoded = NSSBase64_DecodeBuffer(NULL, NULL, echParamsStr, PORT_Strlen(echParamsStr)); if (!decoded || decoded->len < 2) { @@ -2001,32 +2003,14 @@ configureEchWithData(PRFileDesc *model_sock) errWarn("Bad ECHParams encoding"); goto loser; } - /* Importing a raw KEM private key is generally awful, - * however since we only support X25519, we can hardcode - * all the OID data. */ - const PRUint8 pkcs8Start[] = { 0x30, 0x67, 0x02, 0x01, 0x00, 0x30, 0x14, 0x06, - 0x07, 0x2A, 0x86, 0x48, 0xCE, 0x3D, 0x02, 0x01, - 0x06, 0x09, 0x2B, 0x06, 0x01, 0x04, 0x01, 0xDA, - 0x47, 0x0F, 0x01, 0x04, 0x4C, 0x30, 0x4A, 0x02, - 0x01, 0x01, 0x04, 0x20 }; - const PRUint8 pkcs8End[] = { 0xA1, 0x23, 0x03, 0x21, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00 }; - pkcs8Key = SECITEM_AllocItem(NULL, NULL, sizeof(pkcs8Start) + len + sizeof(pkcs8End)); - if (!pkcs8Key) { - goto loser; - } - PORT_Memcpy(pkcs8Key->data, pkcs8Start, sizeof(pkcs8Start)); - PORT_Memcpy(&pkcs8Key->data[sizeof(pkcs8Start)], reader, len); - PORT_Memcpy(&pkcs8Key->data[sizeof(pkcs8Start) + len], pkcs8End, sizeof(pkcs8End)); + pkcs8Key.data = reader; + pkcs8Key.len = len; reader += len; /* Convert the key bytes to key handles */ slot = PK11_GetInternalKeySlot(); rv = PK11_ImportDERPrivateKeyInfoAndReturnKey( - slot, pkcs8Key, NULL, NULL, PR_FALSE, PR_FALSE, KU_ALL, &sk, NULL); + slot, &pkcs8Key, NULL, NULL, PR_FALSE, PR_FALSE, KU_ALL, &sk, NULL); if (rv != SECSuccess || !sk) { errWarn("ECH key import failed"); goto loser; @@ -2037,7 +2021,7 @@ configureEchWithData(PRFileDesc *model_sock) goto loser; } - /* Remainder is the ECHConfig. */ + /* Remainder is the ECHConfigs. */ rv = SSL_SetServerEchConfigs(model_sock, pk, sk, reader, REMAINING_BYTES(reader, decoded)); if (rv != SECSuccess) { @@ -2048,7 +2032,6 @@ configureEchWithData(PRFileDesc *model_sock) PK11_FreeSlot(slot); SECKEY_DestroyPrivateKey(sk); SECKEY_DestroyPublicKey(pk); - SECITEM_FreeItem(pkcs8Key, PR_TRUE); SECITEM_FreeItem(decoded, PR_TRUE); return SECSuccess; loser: @@ -2057,7 +2040,6 @@ loser: } SECKEY_DestroyPrivateKey(sk); SECKEY_DestroyPublicKey(pk); - SECITEM_FreeItem(pkcs8Key, PR_TRUE); SECITEM_FreeItem(decoded, PR_TRUE); return SECFailure; } diff --git a/security/nss/cmd/tstclnt/tstclnt.c b/security/nss/cmd/tstclnt/tstclnt.c index 74887b49cc54..639cf4f2414b 100644 --- a/security/nss/cmd/tstclnt/tstclnt.c +++ b/security/nss/cmd/tstclnt/tstclnt.c @@ -1279,6 +1279,11 @@ printEchRetryConfigs(PRFileDesc *s) return SECFailure; } + // Remove the newline characters that NSSBase64_EncodeItem unhelpfully inserts. + char *newline = strstr(retriesBase64, "\r\n"); + if (newline) { + memmove(newline, newline + 2, strlen(newline + 2) + 1); + } fprintf(stderr, "Received ECH retry_configs: \n%s\n", retriesBase64); PORT_Free(retriesBase64); SECITEM_FreeItem(&retries, PR_FALSE); diff --git a/security/nss/coreconf/coreconf.dep b/security/nss/coreconf/coreconf.dep index 590d1bfaeee3..5182f75552c8 100644 --- a/security/nss/coreconf/coreconf.dep +++ b/security/nss/coreconf/coreconf.dep @@ -10,4 +10,3 @@ */ #error "Do not include this header file." - diff --git a/security/nss/gtests/ssl_gtest/ssl_recordsize_unittest.cc b/security/nss/gtests/ssl_gtest/ssl_recordsize_unittest.cc index 8926b5551ed4..a18510440429 100644 --- a/security/nss/gtests/ssl_gtest/ssl_recordsize_unittest.cc +++ b/security/nss/gtests/ssl_gtest/ssl_recordsize_unittest.cc @@ -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(client_, kTlsHandshakeClientHello); + + // Add PSK with label long enough to push CH length into [256, 511]. + std::vector 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: diff --git a/security/nss/lib/ssl/ssl3ext.c b/security/nss/lib/ssl/ssl3ext.c index 199cf459ae7c..cdbb803fab40 100644 --- a/security/nss/lib/ssl/ssl3ext.c +++ b/security/nss/lib/ssl/ssl3ext.c @@ -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. */ @@ -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. */ diff --git a/security/nss/lib/ssl/ssl3exthandle.c b/security/nss/lib/ssl/ssl3exthandle.c index fa1c66ee2170..edc79cd6bc06 100644 --- a/security/nss/lib/ssl/ssl3exthandle.c +++ b/security/nss/lib/ssl/ssl3exthandle.c @@ -921,7 +921,7 @@ ssl_ParseSessionTicket(sslSocket *ss, const SECItem *decryptedTicket, #ifndef UNSAFE_FUZZER_MODE PORT_Assert(temp < ssl_auth_size); #else - temp %= (8 * sizeof(SSLAuthType)); + temp %= (8 * sizeof(SSLAuthType)) - 1; #endif parsedTicket->authType = (SSLAuthType)temp; diff --git a/security/nss/lib/ssl/tls13hashstate.c b/security/nss/lib/ssl/tls13hashstate.c index f2f55ba0fd74..ada22b6e316a 100644 --- a/security/nss/lib/ssl/tls13hashstate.c +++ b/security/nss/lib/ssl/tls13hashstate.c @@ -131,7 +131,8 @@ tls13_MakeHrrCookie(sslSocket *ss, const sslNamedGroupDef *selectedGroup, /* Given a cookie and cookieLen, decrypt and parse, returning * any values that were requested via the "previous_" params. If - * recoverHashState is true, the transcript state is recovered */ + * recoverState is true, the transcript state and application + * token are restored. */ SECStatus tls13_HandleHrrCookie(sslSocket *ss, unsigned char *cookie, unsigned int cookieLen, @@ -142,7 +143,7 @@ tls13_HandleHrrCookie(sslSocket *ss, HpkeAeadId *previousEchAeadId, SECItem *previousEchConfigId, HpkeContext **previousEchHpkeCtx, - PRBool recoverHashState) + PRBool recoverState) { SECStatus rv; unsigned char plaintext[1024]; @@ -217,18 +218,11 @@ tls13_HandleHrrCookie(sslSocket *ss, } /* Application token. */ - PORT_Assert(ss->xtnData.applicationToken.len == 0); rv = sslRead_ReadNumber(&reader, 2, &appTokenLen); if (rv != SECSuccess) { FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_CLIENT_HELLO, illegal_parameter); return SECFailure; } - if (SECITEM_AllocItem(NULL, &ss->xtnData.applicationToken, - appTokenLen) == NULL) { - FATAL_ERROR(ss, PORT_GetError(), internal_error); - return SECFailure; - } - ss->xtnData.applicationToken.len = appTokenLen; sslReadBuffer appTokenReader = { 0 }; rv = sslRead_Read(&reader, appTokenLen, &appTokenReader); if (rv != SECSuccess) { @@ -236,10 +230,18 @@ tls13_HandleHrrCookie(sslSocket *ss, return SECFailure; } PORT_Assert(appTokenReader.len == appTokenLen); - PORT_Memcpy(ss->xtnData.applicationToken.data, appTokenReader.buf, appTokenLen); - /* The remainder is the hash. */ - if (recoverHashState) { + if (recoverState) { + PORT_Assert(ss->xtnData.applicationToken.len == 0); + if (SECITEM_AllocItem(NULL, &ss->xtnData.applicationToken, + appTokenLen) == NULL) { + FATAL_ERROR(ss, PORT_GetError(), internal_error); + return SECFailure; + } + PORT_Memcpy(ss->xtnData.applicationToken.data, appTokenReader.buf, appTokenLen); + ss->xtnData.applicationToken.len = appTokenLen; + + /* The remainder is the hash. */ unsigned int hashLen = SSL_READER_REMAINING(&reader); if (hashLen != tls13_GetHashSize(ss)) { FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_CLIENT_HELLO, illegal_parameter); @@ -279,7 +281,7 @@ tls13_HandleHrrCookie(sslSocket *ss, echHpkeBuf.len }; hpkeContext = PK11_HPKE_ImportContext(&hpkeItem, NULL); if (!hpkeContext) { - FATAL_ERROR(ss, PORT_GetError(), internal_error); + FATAL_ERROR(ss, PORT_GetError(), illegal_parameter); return SECFailure; } } diff --git a/security/nss/lib/ssl/tls13hashstate.h b/security/nss/lib/ssl/tls13hashstate.h index 48832f04a408..2ea7b493b67c 100644 --- a/security/nss/lib/ssl/tls13hashstate.h +++ b/security/nss/lib/ssl/tls13hashstate.h @@ -26,5 +26,5 @@ SECStatus tls13_HandleHrrCookie(sslSocket *ss, HpkeAeadId *previousEchAeadId, SECItem *previousEchConfigId, HpkeContext **previousEchHpkeCtx, - PRBool recoverHashState); + PRBool recoverState); #endif