klocwork null ptr dereference in PKCS12 decoder

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Ryan Jones)

Tracking

({klocwork})

trunk
3.12
klocwork

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

3.75 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
2.76 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
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	    }
(Reporter)

Comment 1

11 years ago
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; 
(Reporter)

Comment 2

11 years ago
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.
(Reporter)

Comment 3

11 years ago
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!
(Reporter)

Comment 4

11 years ago
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
(Reporter)

Comment 5

11 years ago
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); 
(Reporter)

Comment 6

11 years ago
See also bug 339906
Depends on: 339906
(Reporter)

Updated

11 years ago
Priority: -- → P2
Target Milestone: --- → 3.12
(Assignee)

Comment 7

11 years ago
Created attachment 249915 [details] [diff] [review]
Patch v1

Patch v1.

* Addressed all the bugs listed below (or tried too).
Attachment #249915 - Flags: review?(nelson)
(Reporter)

Comment 8

11 years ago
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-
(Reporter)

Comment 9

11 years ago
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).
(Assignee)

Comment 10

11 years ago
Created attachment 249999 [details] [diff] [review]
Patch v2

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)
(Reporter)

Comment 11

11 years ago
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+

Comment 12

10 years ago
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

10 years ago
Created attachment 251244 [details] [diff] [review]
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.
(Reporter)

Comment 14

10 years ago
Neil, I confirm your diagnosis in comment 12.  Good catch!  I missed that.
Code reviewing really makes a big difference!
(Reporter)

Comment 15

10 years ago
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+

Comment 16

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.