1771218 - Segfault in PK11_GetPublicKeyNickname after SECKEY_ConvertToPublicKey

Status: NEW

(NSS :: Libraries, defect, P5)

Description

[Alexander Scheel]

Attachment: 0001-Correctly-handle-missing-pubkey-slot-information.patch

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:100.0) Gecko/20100101 Firefox/100.0

Steps to reproduce:

            SECKEYPublicKey *candidateKey = SECKEY_ConvertToPublicKey(candidate);
            char *candidatePublicName = PK11_GetPublicKeyNickname(candidateKey); /* SEGFAULT HAPPENS HERE */ 

Actual results:

Segfault due to attempting to acquire a lock on a NULL slot without actually verifying we have a non-NULL slot.

Expected results:

PK11_GetPublicKeyNickname probably shouldn't segfault.

Comment 1

[John Schanck [:jschanck]]

That does seem like an easy mistake to make. Bob, do you have an opinion on this?

Comment 2

[Robert Relyea]

The patch is correct in that public keys don't always have a slot (if they are extracted from a certificate, for instance), so we really should check for NULL here.

There is a question of whether or not SECKEY_ConvertToPublicKey() should make sure the public key has a slot and id. The source private key definitately had a slot, and ConvertToPublicKey() has multiple ways of getting the public key. The first is just to match it in the slot (in which case it has a slot). The second is to match the cert and extract the key from the cert (which is likely this case). The third is only works with RSA: extract the public components from the private key and create a public key on the fly (which. The forth and final is to use a token derive method to produce the private key from the public key. In the first and forth methods, the public key has a representation in the token. One question is, should we import the public key in the 2nd and 3rd methods. The upside of importing is we will always have the slot, the downsides are 1) it will force the use of the key in the given token, rather than the prefered token, and 2) the import does have a minor performance cost. Given the nature of SECKEY_ConvertToPublicKey(), I think 2 isn't a major consideration, it's already a potentially expensive operation. 1 is a consideration, though it can be overridden by the application (using an explicit import command). We would need to decide if we import the key as a token key or as a session key (we have to make that same determination for the derive case, already).

Upshot. the patch is correct (even if we fix SECKEY_ConvertToPublicKey()), but we may also need to do more.

bob