Coverity errors reported for libPKIX and related functions

NEW
Assigned to

Status

NSS
Libraries
P1
normal
9 years ago
9 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Alexei Volkov)

Tracking

({coverity})

3.12
3.12.5
coverity

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX)

The latest full coverity run for Firefox (run 278) reports numerous 
issues with libPKIX and related code.  I will add information about 
those errors as comments to this bug.  Note: Firefox is using an old
snapshot of NSS, so it's possible that some (perhaps all) of these are
already fixed, but I imagine most still remain.  Also, it's possible
that some of these are not errors. 

CID 1293 - NULL ptr dereference

In file nss/lib/libpkix/pkix/top/pkix_validate.c function pkix_CheckChain 
at label "cleanup", Coverity claims that, when all the following conditions
are true:
  - stdVars.aPkixErrorResult       is NULL
  - stdVars.aPkixTempErrorReceived is NOT NULL
  - cert                           is NOT NULL
then variable stdVars.aPkixErrorResult (which is NULL) is dereferenced.
CID 1292 - Variable "hcv1" tracked as NULL was dereferenced.

In file nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_aiamgr.c, function pkix_pl_AIAMgr_GetHTTPCerts, if all the following conditions are true:

- "nbio" == NULL, 
- httpClient->version != 1
and at label cleanup,
- stdVars.aPkixErrorReceived != 0
- aiaMgr != 0 
and either 
- aiaMgr->client.hdata.requestSession != NULL or
- aiaMgr->client.hdata.serverSession != NULL

then Variable "hcv1" tracked as NULL is dereferenced.
Priority: -- → P1
CID 1295

In file nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_colcertstore.c
function pkix_pl_CollectionCertStoreContext_PopulateCert
if these conditions are all true:

- dir      is non-zero
- dirEntry is non-zero
- prError  is zero
- PL_strrstr(dirEntry->name, ".crt") is NOT equal to 
             dirEntry->name + PL_strlen(dirEntry->name) - 4)
and, after the line
             dirEntry = PR_ReadDir(dir, PR_SKIP_HIDDEN | PR_SKIP_BOTH);
- dirEntry is NULL and 
- prError != 0 and prError != PR_NO_MORE_FILES_ERROR 

then Variable "prErrorText" (tracked as NULL) is passed to function 
PR_GetErrorText, which dereferences it.
CID 1294 - NULL ptr dereference

In file nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_colcertstore.c
function pkix_pl_CollectionCertStoreContext_PopulateCRL 
if these conditions are all true:

- dir      is non-zero
- dirEntry is non-zero
- prError  is zero
- PL_strrstr(dirEntry->name, ".crl") is NOT equal to 
             dirEntry->name + PL_strlen(dirEntry->name) - 4
and, after the line
             dirEntry = PR_ReadDir(dir, PR_SKIP_HIDDEN | PR_SKIP_BOTH);
- dirEntry is NULL and 
- prError != 0 and prError != PR_NO_MORE_FILES_ERROR 

then Variable "prErrorText" (tracked as NULL) is passed to function 
PR_GetErrorText, which dereferences it.
CID 1350 - memory leak

In file nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_httpdefaultclient.c
in function pkix_pl_HttpDefaultClient_HdrCheckComplete
The allocated memory block known as "copy", allocated at

177     PKIX_CHECK(PKIX_PL_Malloc(headerLength + 1, (void **)&copy, plContext),
178                PKIX_MALLOCFAILED);

Is leaked at label cleanup.
CID 1350, part 2 - memory leak

Also in function pkix_pl_HttpDefaultClient_HdrCheckComplete
The allocated memory block known as "body", allocated at

336  	 PKIX_CHECK(PKIX_PL_Malloc(contentLength, (void **)&body, plContext),
337  	            PKIX_MALLOCFAILED);

is leaked at cleanup.
CID 1346 - memory leak

Coverity definitely believes that PKIX_PL_Malloc can allocate memory 
and store its address in the output variable, then return an error code.
I'm not at all sure that is true, but we should double check it.  
ONLY IF that *IS* true, can the following bug report possibly be correct.

In file nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_ldapresponse.c
function pkix_pl_LdapResponse_Create, if all these conditions are true:

- totalLength != 0
- this call throws an error
     PKIX_CHECK(PKIX_PL_Malloc(totalLength, &data, plContext),
                PKIX_MALLOCFAILED);
  making stdVars.aPkixErrorReceived != 0, but does make data non-NULL
so that at label cleanup
- ldapResponse != 0
- stdVars.aPkixTempResult != 0
- "data" is non-NULL,
then the allocated memory block known as "data" is leaked.
CID 1345 - memory leak

Here again, Coverity believes that PKIX_PL_Malloc can allocate memory 
and store its address in the output variable, then return an error code.
It alleges that this causes a leak when PKIX_PL_NssContext_Create calls

67     PKIX_CHECK(PKIX_PL_Malloc
68                (sizeof(PKIX_PL_NssContext), (void **)&context, NULL),
69   	                   PKIX_MALLOCFAILED);
There were several more CIDs that all believe that PKIX_PL_Malloc can 
allocate memory and then return a non-NULL error code address.  I will 
not report them here.  If that proves to be true, then the solution will
be to fix PKIX_PL_Malloc, not every one of its callers. 

Here's a different error reported by Coverity.

CID 1296 - NULL ptr dereference

In file nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_ocspresponse.c
function pkix_pl_OcspResponse_VerifySignature, if all the following are true:

- On entry, response->nssOCSPResponse is NULL, then at label cleanup, 
variable "signature" (tracked as NULL) is dereferenced.
Here's one more.  CID 1259, 

In its analysis of function pkix_Cert2ASCII in file 
nss/lib/certhigh/certvfypkixprint.c, Coverity makes an interesting 
comment about the return value from PKIX_PL_Cert_GetSubject.
It says:

133  	        errorResult = PKIX_PL_Cert_GetSubject(cert, &subject, NULL);

Event const: After this line, the value of "errorResult" is equal to 0

134  	        if (errorResult) goto cleanup;

This means that coverity thinks that PKIX_PL_Cert_GetSubject never 
returns an error code.  If that is true, that seems like a bad bug in PKIX_PL_Cert_GetSubject.
(Assignee)

Comment 10

9 years ago
(In reply to comment #0)

> In file nss/lib/libpkix/pkix/top/pkix_validate.c function pkix_CheckChain 
> at label "cleanup", Coverity claims that, when all the following conditions
> are true:
>   - stdVars.aPkixErrorResult       is NULL
>   - stdVars.aPkixTempErrorReceived is NOT NULL
>   - cert                           is NOT NULL
> then variable stdVars.aPkixErrorResult (which is NULL) is dereferenced.

Potential problem. Should be fixed.
(Assignee)

Comment 11

9 years ago
(In reply to comment #1)
> - "nbio" == NULL, 
> - httpClient->version != 1
> and at label cleanup,
> - stdVars.aPkixErrorReceived != 0
> - aiaMgr != 0 
> and either 
> - aiaMgr->client.hdata.requestSession != NULL or
> - aiaMgr->client.hdata.serverSession != NULL
> 
> then Variable "hcv1" tracked as NULL is dereferenced.
Should be fixed.
(Assignee)

Comment 12

9 years ago
(In reply to comment #2)
> CID 1295
> 
> In file nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_colcertstore.c
> function pkix_pl_CollectionCertStoreContext_PopulateCert
> if these conditions are all true:

Will not fix any think in Collection Cert Store. Used only for testing.
(Assignee)

Comment 13

9 years ago
(In reply to comment #4)
> CID 1350 - memory leak
> 
> In file nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_httpdefaultclient.c
> in function pkix_pl_HttpDefaultClient_HdrCheckComplete
> The allocated memory block known as "copy", allocated at
> 
> 177     PKIX_CHECK(PKIX_PL_Malloc(headerLength + 1, (void **)&copy, plContext),
> 178                PKIX_MALLOCFAILED);
> 
> Is leaked at label cleanup.

177         /* allocate space to copy header (and for the NULL terminator) */
178         PKIX_CHECK(PKIX_PL_Malloc(headerLength + 1, (void **)&copy, plContext),
179                 PKIX_MALLOCFAILED);
180 
181         /* copy header data before we corrupt it (by storing NULLs) */
182         PORT_Memcpy(copy, client->rcvBuf, headerLength);
183         /* Store the NULL terminator */
184         copy[headerLength] = '\0';
185         client->rcvHeaders = copy;

PKIX_PL_Malloc can not allocate and return an error at the same time. Later, the buffer pointer is saved in rcvHeaders variable of HTTPClient structure and will be freed with the HTTPClient.
(Reporter)

Updated

9 years ago
Target Milestone: 3.12.3 → 3.12.5
(Reporter)

Updated

9 years ago
Whiteboard: PKIX
You need to log in before you can comment on or make changes to this bug.