Last Comment Bug 353895 - klocwork Null ptr derefs in pki/pkibase.c
: klocwork Null ptr derefs in pki/pkibase.c
Status: RESOLVED FIXED
: klocwork
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
: P2 normal (vote)
: 3.12
Assigned To: Alexei Volkov
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-22 16:26 PDT by Nelson Bolyard (seldom reads bugmail)
Modified: 2007-01-04 16:25 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
check a pointer for NULL before deref it. (2.23 KB, patch)
2006-10-03 15:04 PDT, Alexei Volkov
nelson: review+
Details | Diff | Splinter Review
fix for 89058 (1.58 KB, patch)
2006-10-31 10:31 PST, Alexei Volkov
nelson: review+
Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2006-09-22 16:26:03 PDT
ID:       89049
Function: nssCertificateCollection_Create
Location: nss/lib/pki/pkibase.c : 1133

Pointer 'collection' returned from call to function 'nssPKIObjectCollection_Create' at line 1132 may be NULL and will be dereferenced at line 1133.

1131	    nssPKIObjectCollection *collection; 
1132	    collection = nssPKIObjectCollection_Create(td, NULL, nssPKIMonitor); 
1133	    collection->objectType = pkiObjectType_Certificate; 

----

ID:       89058
Function: crl_getUIDFromObject
Location: nss/lib/pki/pkibase.c : 1202

Pointer 'encoding' returned from call to function 'nssCRL_GetEncoding' at line 1201 may be NULL and will be dereferenced at line 1202.

1200	    NSSDER *encoding; 
1201	    encoding = nssCRL_GetEncoding(crl); 
1202	    uid[0] = *encoding; 

----

ID:       89059
Function: nssCRLCollection_Create
Location: nss/lib/pki/pkibase.c : 1236

Pointer 'collection' returned from call to function 'nssPKIObjectCollection_Create' at line 1235 may be NULL and will be dereferenced at line 1236.

1234	    nssPKIObjectCollection *collection; 
1235	    collection = nssPKIObjectCollection_Create(td, NULL, nssPKILock); 
1236	    collection->objectType = pkiObjectType_CRL; 

----

ID:       89068
Function: NSSTime_SetPRTime
Location: nss/lib/pki/pkibase.c : 1511

Pointer 'rvTime' returned from call to function 'nss_ZAlloc' at line 1510 
may be NULL and will be dereferenced at line 1511.

1509	    NSSTime *rvTime; 
1510	    rvTime = (timeOpt) ? timeOpt : nss_ZNEW(NULL, NSSTime); 
1511	    rvTime->prTime = prTime;
Comment 1 Alexei Volkov 2006-10-03 15:04:48 PDT
Created attachment 241113 [details] [diff] [review]
check a pointer for NULL before deref it.

I don't see a problem in case number two(ID 89058), then encoding is returned by nssCRL_GetEncoding and get dereferenced. 
"encoding" is the member of NSSCRLStr.

211 struct NSSCRLStr {
212   nssPKIObject object;
213   NSSDER encoding;
214   NSSUTF8 *url;
215   PRBool isKRL;
216 };

nssCRL_GetEncoding just returns a pointer to "encoding" field in this structure. The NSSCRLStr is always allocated when nssCRL_GetEncoding is called, and so should be allocated the "encoding" field.
 
All others are valid and fixed in the patch.
Comment 2 Nelson Bolyard (seldom reads bugmail) 2006-10-03 17:03:14 PDT
Alexei, in comment 1 you seem to be suggesting that nssCRL_GetEncoding(crl)
cannot return NULL, and therefore callers of that function need not check 
for a NULL return value.  Yet the author of that code clearly believed that 
it could.  

Julien, do you concur that nssCRL_GetEncoding(crl) cannot return NULL ?
Comment 3 Julien Pierre 2006-10-03 18:11:41 PDT
Nelson,

I'm not the author of that code, but it's a simple enough function :

NSS_IMPLEMENT NSSDER *
nssCRL_GetEncoding (
  NSSCRL *crl
)
{
    if (crl->encoding.data != NULL && crl->encoding.size > 0) {
	return &crl->encoding;
    } else {
	return (NSSDER *)NULL;
    }
}

I'm not sure if either of the 2 conditions can be false. There is only one caller, crl_getUIDFromObject, which gets the crl pointer from one of its arguments. It therefore can't be assured of its content. So, I would say those conditions could be false and nssCRL_GetEncoding could return NULL . I also note that nssCRL_GetEncoding does not check for a NULL input crl argument, and will crash dereferencing it in that case.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2006-10-20 16:42:46 PDT
Comment on attachment 241113 [details] [diff] [review]
check a pointer for NULL before deref it.

Alexei, this patch is OK for the 3 cases it fixes.  
Please add a new patch to do a NULL pointer check for ID 89058.
Comment 5 Alexei Volkov 2006-10-31 10:31:06 PST
Created attachment 244211 [details] [diff] [review]
fix for 89058
Comment 6 Nelson Bolyard (seldom reads bugmail) 2006-10-31 13:12:59 PST
Comment on attachment 244211 [details] [diff] [review]
fix for 89058

r=nelson
Comment 7 Alexei Volkov 2007-01-04 16:25:26 PST
/cvsroot/mozilla/security/nss/lib/pki/certificate.c,v  <--  certificate.c
new revision: 1.62; previous revision: 1.61
/cvsroot/mozilla/security/nss/lib/pki/pkibase.c,v  <--  pkibase.c
new revision: 1.27; previous revision: 1.26

Note You need to log in before you can comment on or make changes to this bug.