Closed Bug 353745 Opened 18 years ago Closed 18 years ago

klocwork null ptr dereference in PKCS12 decoder

Categories

(NSS :: Libraries, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nelson, Assigned: sciguyryan)

References

Details

(Keywords: klocwork)

Attachments

(2 files, 1 obsolete file)

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	    }
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; 
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.
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!
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
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); 
See also bug 339906
Depends on: 339906
Priority: -- → P2
Target Milestone: --- → 3.12
Attached patch Patch v1 (obsolete) — Splinter Review
Patch v1.

* Addressed all the bugs listed below (or tried too).
Attachment #249915 - Flags: review?(nelson)
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).
Attachment #249915 - Flags: review?(nelson) → review-
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).
Attached patch Patch v2Splinter Review
Patch v2

* Addressed comments brought up by Nelson Bolyard.
Assignee: neil.williams → sciguyryan+bugzilla
Attachment #249915 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #249999 - Flags: review?(nelson)
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.
Attachment #249999 - Flags: review?(nelson) → review+
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?
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.
Neil, I confirm your diagnosis in comment 12.  Good catch!  I missed that.
Code reviewing really makes a big difference!
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.
Attachment #251244 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: