353895 - klocwork Null ptr derefs in pki/pkibase.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

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]

Attachment: 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)]

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]

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)]

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]

Attachment: fix for 89058

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 244211 [details] [diff] [review]
fix for 89058

r=nelson

Comment 7

[Alexei Volkov]

/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