336971 - Coverity crash [@ SEC_PKCS12DecoderGetCerts] "p12dcx" Pointer dereferenced before NULL check

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[timeless]

If you really don't think it's legal for someone to pass null here, then please remove the null check and add an assert.

Based on the fact that the code now tries to use PORT_SetError(SEC_ERROR_INVALID_ARGS), it seems pretty clear in my mind that the null check should be moved before the dereference

Comment 1

[timeless]

Attachment: look before leaping

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 221184 [details] [diff] [review]
look before leaping

Here's a simpler patch that is preferable, IMO.

>Index: mozilla/security/nss/lib/pkcs12/p12d.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/pkcs12/p12d.c,v
>retrieving revision 1.32
>diff -u -r1.32 mozilla/security/nss/lib/pkcs12/p12d.c
>--- mozilla/security/nss/lib/pkcs12/p12d.c
>+++ mozilla/security/nss/lib/pkcs12/p12d.c
>@@ -2540,10 +2540,16 @@
> SEC_PKCS12DecoderGetCerts(SEC_PKCS12DecoderContext *p12dcx)
> {
>     CERTCertList *certList = NULL;
>-    sec_PKCS12SafeBag **safeBags = p12dcx->safeBags;
>+    sec_PKCS12SafeBag **safeBags;
>     int i;
> 
>     if (!p12dcx || !p12dcx->safeBags || !p12dcx->safeBags[0]) {
> 	PORT_SetError(SEC_ERROR_INVALID_ARGS);
> 	return NULL;
>     }
>+    safeBags = p12dcx->safeBags;

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

taking.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for NSS_3_11_BRANCH

This patch is for the NSS_3_11_BRANCH, which has diverged from the trunk. :(
Alexei, please review.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for the trunk

This is the corresponding patch for the trunk.
The difference between the two patches is the presence of 
a new PORT_SetError call.

Reviewers, please ensure that the statement 
    safeBags = p12dcx->safeBags;
occurs after the block that detects that p12dcx is NULL.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Checking in p12d.c; new revision: 1.33; previous revision: 1.32
Checking in p12d.c; new revision: 1.29.2.2; previous revision: 1.29.2.1

Thanks for the quick reviews.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

CID 625