353745 - klocwork null ptr dereference in PKCS12 decoder

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Klocwork ID 87898
File     nss/lib/pkcs12/p12d.c
function sec_pkcs12_get_nickname

1679	    if(!bag) { 
1680		bag->problem = PR_TRUE;            // D'oh!
1681		bag->error = SEC_ERROR_NO_MEMORY;  // D'oh!
1682		return NULL; 
1683	    }

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Klocwork ID 87940
File     nss/lib/pkcs12/p12dec.c
Function SEC_PKCS12PutPFX

Pointer 'safe_contents' returned from call to function 'sec_pkcs12_create_safe_contents' at line 621 may be NULL and will be 
dereferenced at line 622.

621		safe_contents = sec_pkcs12_create_safe_contents(asafe->poolp); 
622		safe_contents->swapUnicode = pfx->swapUnicode; 

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Klocwork ID 92547
File     nss/lib/pkcs12/p12d.c 
Function sec_pkcs12_add_key

Pointer 'nickName' returned from call to function 'sec_pkcs12_get_nickname' 
at line 2449 may be NULL and may be dereferenced by passing argument 4 to 
function 'PK11_ImportEncryptedPrivateKeyInfo' at line 2460.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

klocwork ID 87941
file     nss/lib/pkcs12/p12dec.c
function SEC_PKCS12PutPFX   (see all comment 1 above)

Suspicious dereference of pointer 'safe_contents' before NULL check at line 623

621		safe_contents = sec_pkcs12_create_safe_contents(asafe->poolp); 
622		safe_contents->swapUnicode = pfx->swapUnicode; 
623		if(safe_contents == NULL) { 

This is pretty silly!

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

ID:       92195
Function: sec_pkcs12_decoder_nested_safe_contents_update
Location: nss/lib/pkcs12/p12d.c : 645

dereference of pointer 'safeContentsCtx->safeContentsDcx' by passing 
argument 1 to function 'SEC_ASN1DecoderUpdate' at line 645 before NULL 
check at line 656

(Null check should occur before first dereference.)

----

ID:       92196
Function: sec_pkcs12_decoder_safe_contents_callback
Location: nss/lib/pkcs12/p12d.c : 736

dereference of pointer 'safeContentsCtx->safeContentsDcx' by passing 
argument 1 to function 'SEC_ASN1DecoderUpdate' at line 736 before 
NULL check at line 751

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

ID:       87919
Function: sec_pkcs12_install_bags
Location: nss/lib/pkcs12/p12d.c : 2844

Null pointer 'publicValue' that comes from line 2830 may be dereferenced by passing argument 2 to function 'sec_pkcs12_add_key' at line 2844.

2830	    SECItem *publicValue = NULL; 
2838	    certList = sec_pkcs12_find_certs_for_key(safeBags, 
2839						     keyList[i]); 
2840	    if(certList) { 
2841		publicValue = sec_pkcs12_get_public_value_and_type(certList[0], 
2842							&keyType, &keyUsage); 
2843	    } 
2844	    rv = sec_pkcs12_add_key(keyList[i], publicValue, keyType, keyUsage, 
2845									wincx); 

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

See also bug 339906

Comment 7

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1

Patch v1.

* Addressed all the bugs listed below (or tried too).

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 249915 [details] [diff] [review]
Patch v1

Ryan, This patch appears to be logically correct, but 
there are a few issues that need to be fixed.

>     if(!safeContentsCtx || !safeContentsCtx->p12dcx 
>-			|| safeContentsCtx->p12dcx->error) {
>+			|| safeContentsCtx->p12dcx->error || !safeContentsCtx->safeContentsDcx) {
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

The line above exceeds 80 columns in width and needs to be wrapped.

>     /* check for error */  
>     if(!safeContentsCtx || !safeContentsCtx->p12dcx 
>-		|| safeContentsCtx->p12dcx->error) {
>+		|| safeContentsCtx->p12dcx->error || !safeContentsCtx->safeContentsDcx) {
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

Likewise the line above needs to be wrapped.

>-    if(!key) {
>+    // We should always have values for "key" and "publicValue"
>+    // so they can't be dereferenced later.

Those are c++ style comments, but this is a c source file, so they 
need to be turned into c style comments.

I think that last comment should read: so they can be dereferenced later
(it should read can, not can't).

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 249915 [details] [diff] [review]
Patch v1

Ryan, This patch appears to be logically correct, but 
there are a few issues that need to be fixed.

>     if(!safeContentsCtx || !safeContentsCtx->p12dcx 
>-			|| safeContentsCtx->p12dcx->error) {
>+			|| safeContentsCtx->p12dcx->error || !safeContentsCtx->safeContentsDcx) {
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

The line above exceeds 80 columns in width and needs to be wrapped.

>     /* check for error */  
>     if(!safeContentsCtx || !safeContentsCtx->p12dcx 
>-		|| safeContentsCtx->p12dcx->error) {
>+		|| safeContentsCtx->p12dcx->error || !safeContentsCtx->safeContentsDcx) {
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

Likewise the line above needs to be wrapped.

>-    if(!key) {
>+    // We should always have values for "key" and "publicValue"
>+    // so they can't be dereferenced later.

Those are c++ style comments, but this is a c source file, so they 
need to be turned into c style comments.

I think that last comment should read: so they can be dereferenced later
(it should read can, not can't).

Comment 10

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2

Patch v2

* Addressed comments brought up by Nelson Bolyard.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 249999 [details] [diff] [review]
Patch v2

r=nelson for trunk.

Neil, please commit your patch on the trunk for Bug 339906 
and then commit this patch on the trunk for Ryan.

Comment 12

[Neil Williams]

In this part of the patch

Index: security/nss/lib/pkcs12/p12d.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/lib/pkcs12/p12d.c,v
retrieving revision 1.29.2.2
diff -u -8 -p -r1.29.2.2 p12d.c
--- security/nss/lib/pkcs12/p12d.c	18 May 2006 19:39:58 -0000	1.29.2.2
+++ security/nss/lib/pkcs12/p12d.c	30 Dec 2006 15:19:33 -0000
...
@@ -2433,25 +2433,30 @@ sec_pkcs12_add_cert(sec_PKCS12SafeBag *c
 
 static SECStatus
 sec_pkcs12_add_key(sec_PKCS12SafeBag *key, SECItem *publicValue, 
 			KeyType keyType, unsigned int keyUsage, void *wincx)
 {
     SECStatus rv;
     SECItem *nickName;
 
...
 
     nickName = sec_pkcs12_get_nickname(key);
+    if(!nickName) {
+	return SECFailure;
+    }
 

it appears this is not a bug. It is legal for nickname to be NULL in both PK11_ImportPrivateKeyInfo() and PK11_ImportEncryptedPrivateKeyInfo() in which case the CKA_LABEL attribute is not added to the key object.

Nelson, would you take a look at this and confirm deny this assertion?

Comment 13

[Neil Williams]

Attachment: same as previous but without the null nickname test

This doesn't need to be reviewed. I built it because the previous was built from the 3.11 branch and did not apply to the trunk. I'll checkin this patch when we decide about the question of comment 12.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Neil, I confirm your diagnosis in comment 12.  Good catch!  I missed that.
Code reviewing really makes a big difference!

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 251244 [details] [diff] [review]
same as previous but without the null nickname test

It appears that Klocwork ID 92547 was inadvertently fixed by my checkin
of revision 1.15 to file pk11wrap/pk11obj.c to fix bug 335481.
So, this patch will suffice to address the remaining KlocWork IDs 
reported in this bug.

Be sure to credit Ryan Jones when you commit this patch.

Comment 16

[Neil Williams]

Ryan, thanks for your help.

Klocwork ID 92547 (comment 12) was fixed by bug 335481 so I removed that part of the fix. 

Checking in p12d.c;
/cvsroot/mozilla/security/nss/lib/pkcs12/p12d.c,v  <--  p12d.c
new revision: 1.36; previous revision: 1.35
done
Checking in p12dec.c;
/cvsroot/mozilla/security/nss/lib/pkcs12/p12dec.c,v  <--  p12dec.c
new revision: 1.6; previous revision: 1.5
done