Closed
Bug 353745
Opened 18 years ago
Closed 18 years ago
klocwork null ptr dereference in PKCS12 decoder
Categories
(NSS :: Libraries, defect, P2)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.12
People
(Reporter: nelson, Assigned: sciguyryan)
References
Details
(Keywords: klocwork)
Attachments
(2 files, 1 obsolete file)
3.75 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
2.76 KB,
patch
|
nelson
:
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•18 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•18 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•18 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•18 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•18 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 | ||
Updated•18 years ago
|
Priority: -- → P2
Target Milestone: --- → 3.12
Assignee | ||
Comment 7•18 years ago
|
||
Patch v1. * Addressed all the bugs listed below (or tried too).
Attachment #249915 -
Flags: review?(nelson)
Reporter | ||
Comment 8•18 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•18 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•18 years ago
|
||
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•18 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•18 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•18 years ago
|
||
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•18 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•18 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•18 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
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•