Bug 1688685 - land NSS fc3a4c142c16 UPGRADE_NSS_RELEASE, r=kjacobs

2021-02-04 Kevin Jacobs <kjacobs@mozilla.com> * 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 <kjacobs@mozilla.com> * automation/taskcluster/docker-builds/Dockerfile: Bug 1690421 - Install packaged libabigail in docker-builds image r=bbeurdouche [3c719b620136] 2021-01-31 Kevin Jacobs <kjacobs@mozilla.com> * 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 <kjacobs@mozilla.com> * lib/ssl/ssl3exthandle.c: Bug 1674819 - Fixup a51fae403328, enum type may be signed. r=bbeurdouche [2004338a2080] Differential Revision: https://phabricator.services.mozilla.com/D104258
2021-02-05 21:13:47 +00:00 · 2021-02-05 21:13:47 +00:00 · d901b16ba2
--- a/security/nss/TAG-INFO
+++ b/security/nss/TAG-INFO
@ -1 +1 @@
-92dcda94c1d4
+fc3a4c142c16
--- 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}
--- 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
--- 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;
 }
--- 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);
--- a/security/nss/coreconf/coreconf.dep
+++ b/security/nss/coreconf/coreconf.dep
@ -10,4 +10,3 @@
 */

 #error "Do not include this header file."
-
--- 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<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:
--- 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. */
--- 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;
--- 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;
        }
    }
--- 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