1517574 - LSan leak in WPT /WebCryptoAPI/generateKey/ test

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Andrew McCreight [:mccr8]]

Here's the LSan report:

Direct leak of 38 byte(s) in 38 object(s) allocated from:
    #0 0x5599cf6c2943 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:146:3
    #1 0x7fb99d157b90 in PORT_Alloc_Util /builds/worker/workspace/build/src/security/nss/lib/util/secport.c:87:14
    #2 0x7fb99d15474d in SECITEM_AllocItem_Util /builds/worker/workspace/build/src/security/nss/lib/util/secitem.c:43:28
    #3 0x7fb96e304ad9  (<unknown module>)
    #4 0x7fb96e2f4608  (<unknown module>)
    #5 0x7fb99c329266 in PK11_GenerateKeyPairWithOpFlags /builds/worker/workspace/build/src/security/nss/lib/pk11wrap/pk11akey.c:1530:11
    #6 0x7fb99c329c02 in PK11_GenerateKeyPairWithFlags /builds/worker/workspace/build/src/security/nss/lib/pk11wrap/pk11akey.c:1617:12
    #7 0x7fb99c329c02 in PK11_GenerateKeyPair /builds/worker/workspace/build/src/security/nss/lib/pk11wrap/pk11akey.c:1641
    #8 0x7fb985a34489 in mozilla::dom::GenerateAsymmetricKeyTask::DoCrypto() /builds/worker/workspace/build/src/dom/crypto/WebCryptoTask.cpp:2414:40
    #9 0x7fb985a2fb67 in CalculateResult /builds/worker/workspace/build/src/dom/crypto/WebCryptoTask.cpp:387:10
    #10 0x7fb985a2fb67 in mozilla::dom::WebCryptoTask::Run() /builds/worker/workspace/build/src/dom/crypto/WebCryptoTask.cpp:348
    #11 0x7fb97f8f29cf in nsThreadPool::Run() /builds/worker/workspace/build/src/xpcom/threads

The code where this is happening is:
  mPrivateKey = UniqueSECKEYPrivateKey(PK11_GenerateKeyPair(
      slot.get(), mMechanism, param, &pubKey, PR_FALSE, PR_FALSE, nullptr));

Maybe something isn't being freed in some error case?

Comment 1

[Andrew McCreight [:mccr8]]

If somebody fixes this, please also delete the lsan-allowed: [PORT_Alloc_Util] entry from testing/web-platform/meta/WebCryptoAPI/generateKey/__dir__.ini, or just delete that file entirely if there's nothing else in it.

Comment 2

[Andrew McCreight [:mccr8]]

I ran all of the tests in this directory individually. Three of them seem to trigger the leak, when run as follows:
./mach wpt WebCryptoAPI/generateKey/failures_RSA-OAEP.https.any.html
./mach wpt WebCryptoAPI/generateKey/failures_RSA-PSS.https.any.html
./mach wpt WebCryptoAPI/generateKey/failures_RSASSA-PKCS1-v1_5.https.any.html

Comment 4

[Daniel Holbert [:dholbert]]

The allocation comes from GenerateAsymmetricKeyTask::DoCrypto() and specifically this line:

  mPrivateKey = UniqueSECKEYPrivateKey(PK11_GenerateKeyPair(
      slot.get(), mMechanism, param, &pubKey, PR_FALSE, PR_FALSE, nullptr));

https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/dom/crypto/WebCryptoTask.cpp#2414

That line seems to be storing the result of PK11_GenerateKeyPair in a UniqueSECKEYPrivateKey, which is a custom UniquePtr. Typically, you'd think that a UniquePtr should prevent memory from leaking (but apparently doesn't in this case). Note that this custom-UniquePtr machinery allows you to provide your own deletion function, and in this case we use SECKEY_DestroyPrivateKey, defined here:
https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/security/nss/lib/cryptohi/seckey.c#240

Comment 5

[Andrew McCreight [:mccr8]]

Yeah, I'm not sure what exactly is leaking. It is a single byte allocation, leaking 38 times. My guess is we hit some error state which leaves some data structure in a partially-set up case, and then whatever cleanup method fails to clean up some single byte thing hanging off of it.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Aha - I found at least one leak at https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/security/nss/lib/softoken/pkcs11c.c#4834

            /* extract the exponent */
            crv = sftk_Attribute2SSecItem(NULL, &pubExp, publicKey, CKA_PUBLIC_EXPONENT);
            if (crv != CKR_OK)
                break;
            bitSize = sftk_GetLengthInBits(pubExp.data, pubExp.len);
            if (bitSize < 2) {
                crv = CKR_ATTRIBUTE_VALUE_INVALID;
                break;
            }
            crv = sftk_AddAttributeType(privateKey, CKA_PUBLIC_EXPONENT,
                                        sftk_item_expand(&pubExp));
            if (crv != CKR_OK) {
                PORT_Free(pubExp.data);
                break;
            }

            rsaPriv = RSA_NewKey(public_modulus_bits, &pubExp);
            PORT_Free(pubExp.data);

If we pass in an exponent with value 1, we'll break without freeing the memory pointed to by pubExp.data.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: bug 1517574 - fix leak in NSC_GenerateKeyPair when rejecting public RSA exponents that are too small r?jcj

Comment 8

[Andrew McCreight [:mccr8]]

Thanks for looking at this! If you have ASan set up, you could try deleting the file testing/web-platform/meta/WebCryptoAPI/generateKey/dir.ini and then running ./mach wpt WebCryptoAPI/generateKey/ and see if there are any leaks reported. If not, you could delete the file as part of your patch.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Ah - thanks for the pointer. Since this is an NSS change, it won't go directly into mozilla-central when this bug is closed, so I'll file a follow-up to remove that.

https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=9b518a646aacc092b81e94421c09aa9b87f2cab1

Comment 10

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/projects/nss/rev/794984bb11694c33936ef71d2c79c69ec0723559