1607955 - [Static Analysis][Dereference of null pointer] kbkdf_CreateKey can dereference null pointer

Status: RESOLVED INVALID

(NSS :: Libraries, defect, P1)

Description

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

Coverity warning on Bug 1599603's patch:

Dereferencing a pointer that might be "NULL" "slot" when calling "sftk_NewObject".
The path that leads to this defect is:

    security/nss/lib/softoken/kbkdf.c:605:
        returned_null: "sftk_SlotFromSessionHandle" returns "NULL" (checked 14 out of 14 times)..

    security/nss/lib/softoken/kbkdf.c:605:
        var_assigned: Assigning: "slot" = "NULL" return value from "sftk_SlotFromSessionHandle"..

    security/nss/lib/softoken/kbkdf.c:613:
        dereference: Dereferencing a pointer that might be "NULL" "slot" when calling "sftk_NewObject"..

This looks true, that slot should be checked. Can we get a quick fixup?

Comment 1

[Alexander Scheel]

I'm not an expert here, but I'm not sure this is valid. Consider the flow starting at NSC_DeriveKey:

NSC_DeriveKey:

• Line 6559: SFTKSlot *slot = sftk_SlotFromSessionHandle(hSession);
• Line 6591...6593: if (!slot) { return CKR_SESSION_HANDLE_INVALID; }
• Line 8167: crv = kbkdf_Dispatch(mechanism, hSession, pMechanism, sourceKey, key, keySize);

So kbkdf_Dispatch is given a session, which we can resolve to a slot. Concretely, this is a hash table lookup.

kbkdf_Dispatch calls kbkdf_SaveKeys with the passed hSession, which calls kbkdf_CreateKey with the hSession, to resolve the slot.

We could check it here, but that implies that someone could've modified our hashtable under us and removed our active session.

1. Is this something we're actively trying to protect against?
2. Are calls to NSC_DeriveKey locked, or is there some other mechanism in place to protect against concurrent access to the underlying hash table and accidental removal of hSession?

I guess either way, this is internally inconsistent. I handle the (unlikely?) potential for sftk_SessionFromHandle to fail in kbkdf_FinalizeKey, even though NSC_DeriveKey has already checked that it doesn't in lines 6668 to 6673.

What do y'all recommend/prefer?

(Also, is the Coverity bot broken again? I don't think I saw this mentioned on my Moz-Phab submission...)

Comment 2

[Robert Relyea]

Such a test would be paranoia only. The PKCS #11 semantics require that the application locks operations to prevent multiple operations on the same session at the same time. NSS proper does this (either the session is locked or a new session is created before C_DeriveKey is called).

Comment 3

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

Thanks for running through that, Alexander. You're right, the session should be valid per NSC_DeriveKey. That said, in truth the existence of slot is not internally guaranteed during shutdown, as the locking slot and token destruction during module unload is not internally consistent ... but that's a problem upstream that needs repair, and a nullptr crash here from that would be in the general noise of "NSS crashes during shutdown".

I'm marking this INVALID.

But yeah, I guess Coverity didn't catch this in NSS, but it flagged in mozilla-central. Coverity did run, though: https://phabricator.services.mozilla.com/D54821#1758896