135052 - Leaked reference to the internal key slot in p12d.c

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

In mozilla/security/nss/lib/pkcs12/p12d.c, we call
PK11_GetInternalKeySlot() in SEC_PKCS12DecoderStart()
if the second argument 'slot' to SEC_PKCS12DecoderStart()
is NULL, but we are not calling PK11_FreeSlot() in
SEC_PKCS12DecoderFinish(), so this slot reference is
leaked.

Note that we may also call PK11_GetInternalKeySlot()
in sec_pkcs12_decoder_get_decrypt_key():

    /* if no slot specified, use the internal key slot */
    if(!keyPwd->slot) {
        keyPwd->slot = PK11_GetInternalKeySlot();
    }

But I believe that we never call this function with a
NULL keyPwd->slot, so this code is never executed.  I
will verify that.

Comment 1

[Wan-Teh Chang]

I am pretty sure that in sec_pkcs12_decoder_get_decrypt_key,
keyPwd->slot is not NULL.  Below is the notes I took while
tracing the source code.  You can skip these notes.

1. SEC_PKCS12DecoderStart creates p12dcx and sets p12dcx->slot.
p12dcx->slot is not NULL.  Then it specifies p12dcx as the
argument to sec_pkcs12_decoder_pfx_notify_proc.

2. sec_pkcs12_decoder_pfx_notify_proc is a static function and
is only referenced once. It passes p12dcx to
sec_pkcs12_decode_start_asafes_cinfo.

3. sec_pkcs12_decode_start_asafes_cinfo is a static function
and is only referenced once.  It specifies p12dcx as the
argument to sec_pkcs12_decoder_asafes_notify.

4. sec_pkcs12_decoder_asafes_notify is a static function and
is only referenced once.  It sets p12dcx->currentASafeKeyPwd->slot
to p12dcx->slot and specifies p12dcx->currentASafeKeyPwd as
the argument to sec_pkcs12_decoder_get_decrypt_key.

5. Finally, sec_pkcs12_decoder_get_decrypt_key is a static
function and is only referenced once.  Its argument is
called keyPwd.  We can assert that keyPwd->slot is not NULL.

Comment 2

[Wan-Teh Chang]

Attachment: Proposed patch

This patch requires some explanation.

1. Although I am pretty sure that in sec_pkcs12_decoder_get_decrypt_key,
keyPwd->slot is not NULL, I am not 100% sure.  So in this function, I
decided to fix the slot reference leak if keyPwd->slot is NULL and we
have to use the internal key slot.

Note that my patch changes the function's behavior slightly.  If
keyPwd->slot is NULL, on return keyPwd->slot will still be NULL
if my patch is applied; in the original code keyPwd->slot will
be equial to the internal key slot.

2. In SEC_PKCS12DecoderStart I simply deleted the code for the
case 'slot' is NULL.  This is because SEC_PKCS12DecoderStart
returns NULL if 'slot' is NULL, so we can't possibly pass a
NULL 'slot' to this function.

Comment 3

[Wan-Teh Chang]

Attachment: Proposed patch

This patch requires some explanation.

1. Although I am pretty sure that in sec_pkcs12_decoder_get_decrypt_key,
keyPwd->slot is not NULL, I am not 100% sure.  So in this function, I
decided to fix the slot reference leak if keyPwd->slot is NULL and we
have to use the internal key slot.

Note that my patch changes the function's behavior slightly.  If
keyPwd->slot is NULL, on return keyPwd->slot will still be NULL
if my patch is applied; in the original code keyPwd->slot will
be equial to the internal key slot.

2. In SEC_PKCS12DecoderStart I simply deleted the code for the
case 'slot' is NULL.  This is because SEC_PKCS12DecoderStart
returns NULL if 'slot' is NULL, so we can't possibly pass a
NULL 'slot' to this function.

Comment 4

[Wan-Teh Chang]

Comment on attachment 78472 [details] [diff] [review]
Proposed patch

Attached the same patch twice.	Sorry.

Comment 5

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 6

[Wan-Teh Chang]

Attachment: More cleanup in SEC_PKCS12DecoderStart

In rev. 1.10 of p12d.c, Bob fixed the leak of
the internal key slot reference allocated in
SEC_PKCS12DecoderStart() (for bug 133584).

So the only thing left to do for this bug is
to determine whether the internal key slot
reference in sec_pkcs12_decoder_get_decrypt_key()
is leaked.  Bob, could you take a look at that?

In reviewing the current SEC_PKCS12DecoderStart()
function I found that we need to free the slot on
error return and that it doesn't make sense to
consider a NULL slot an error because a few lines
ahead we have handled that case by using the internal
key slot.  This patch addresses these two issues.

Comment 7

[Wan-Teh Chang]

Assigned the bug to Bob.

Comment 8

[Robert Relyea]

This is not a leak, because SymKeys are expected to own the reference to the
slot. It would only be a leak if the symkey is not freed (which it is much later).

bob

Comment 9

[Wan-Teh Chang]

Comment on attachment 82983 [details] [diff] [review]
More cleanup in SEC_PKCS12DecoderStart

Bob,

Thanks for looking at sec_pkcs12_decoder_get_decrypt_key().

Could you review this patch too?

Comment 10

[Robert Relyea]

Comment on attachment 82983 [details] [diff] [review]
More cleanup in SEC_PKCS12DecoderStart

Yup.

Comment 11

[Wan-Teh Chang]

Gracias.

Marked the bug fixed.

Comment 12

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) for checkin to the 1.0 branch. Pls check this in
asap. thanks! 

Comment 13

[lchiang]

[e-mail] - can you verify this bug fix in 1.01 branch? 
When verified, pls replace fixed1.0.1 keyword with verified1.0.1.  Thanks.

Comment 14

[Bishakha Banerjee]

marking verified