337008 - Coverity OOM crash [@ nssList_Add - STAN_InitTokenForSlotInfo - STAN_LoadDefaultNSS3TrustDomain][@ nssList_Clone - nssList_CreateIterator - STAN_LoadDefaultNSS3TrustDomain] Dereferencing possibly NULL "(td)->tokenList"

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[timeless]

Event returned_null: Function "nssList_Create" returned NULL value (checked 15 out of 17 times)

Comment 1

[Alexei Volkov]

Attachment: if list is null release locks and return failure

Comment 2

[Alexei Volkov]

Comment on attachment 222391 [details] [diff] [review]
if list is null release locks and return failure

Asking Nelson for review. Bob is on vacation.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 222391 [details] [diff] [review]
if list is null release locks and return failure

r=nelson

Comment 4

[Wan-Teh Chang]

Comment on attachment 222391 [details] [diff] [review]
if list is null release locks and return failure

r=wtc.  This patch should silence Coverity.  But there are
still two issues.

1. This function has two other failure points that aren't
checked.

  td->tokens = nssList_CreateIterator(td->tokenList);

  g_default_crypto_context = NSSTrustDomain_CreateCryptoContext(td, NULL);

Ideally, we should check for the failure of these two
function calls, destroy what we have created so far,
and return PR_FAILURE.

2. We should review NSSTrustDomain_Destroy to ensure that
it can destroy a partially constructed NSSTrustDomain
object safely.  For example, it now says:

        /* Destroy each token in the list of tokens */
        if (td->tokens) {
            nssListIterator_Destroy(td->tokens);
            nssList_Clear(td->tokenList, token_destructor);
            nssList_Destroy(td->tokenList);
        }

This means if td->tokens is NULL but td->tokenList is
not NULL, which happens if the nssList_CreateIterator
call I mentioned above fails, we will fail to destroy
td->tokenList.

Since it'll take extraordinary work to inject artificial
failures to test the error handling code, I am not sure
if it's worthwhile to address the above two issues.  (This
is OOM during NSS initialization.)

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, I agree with your remarks in comment 4, and think they're serious
enough that your review should be construed as r-, rather than r+.

Alexei, please make another patch that addresses those additional comments.
We might as well "head Coverity off at the pass".

Comment 6

[Alexei Volkov]

The patch is already checked in. But will leave the bug open and create one more patch to fix remaining issues.

trunk:
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.87; previous revision: 1.86

3.11 branch:
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.86.28.1; previous revision: 1.86

Comment 7

[Alexei Volkov]

Attachment: incorporate wtc comments

Remove tokenList initialization outside of locks. So far tokenList is local for the function and nssList_Create does not involve any initializations outside of the function scope.

Check nssList_CreateIterator and NSSTrustDomain_CreateCryptoContext return values.

Modification to  NSSTrustDomain_Destroy to clear and to destroy tokenList in case when "!td->tokens".

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 222748 [details] [diff] [review]
incorporate wtc comments

Two comments:

>Index: pki3hack.c

>     g_default_trust_domain = td;
>     g_default_crypto_context = NSSTrustDomain_CreateCryptoContext(td, NULL);
>+    if (!g_default_crypto_context) {
>+	goto loser;

1. If we go to loser here, then we leave g_default_trust_domain pointing at 
a destroyed td.  So, instead we should not set g_default_trust_domain to td
until after NSSTrustDomain_CreateCryptoContext has succeeded.

>+    }
>     return PR_SUCCESS;
>+
>+  loser:
>+    NSSTrustDomain_Destroy(td);
>+    return PR_FAILURE;
> }

>Index: trustdomain.c

>     if (--td->refCount == 0) {
> 	/* Destroy each token in the list of tokens */
> 	if (td->tokens) {
> 	    nssListIterator_Destroy(td->tokens);
> 	    nssList_Clear(td->tokenList, token_destructor);
> 	    nssList_Destroy(td->tokenList);
>+	} else if (td->tokenList) {
>+	    nssList_Clear(td->tokenList, token_destructor);
>+	    nssList_Destroy(td->tokenList);
> 	}

I think what we want is more like this:

> 	if (td->tokens) {
> 	    nssListIterator_Destroy(td->tokens);
>           td->tokens = NULL;
> 	} 
>       if (td->tokenList) {
> 	    nssList_Clear(td->tokenList, token_destructor);
> 	    nssList_Destroy(td->tokenList);
>           td->tokenList = NULL;
> 	}

Comment 9

[Wan-Teh Chang]

Comment on attachment 222748 [details] [diff] [review]
incorporate wtc comments

I agree with Nelson's review comments.  I'm also wondering
if we should move
  td->tokens = nssList_CreateIterator(td->tokenList);
outside the critical region.

Comment 10

[Wan-Teh Chang]

Comment on attachment 222748 [details] [diff] [review]
incorporate wtc comments

>+  loser:
>+    NSSTrustDomain_Destroy(td);
>+    return PR_FAILURE;
> }

Nit: I believe the prevalent style in NSS is to not
indent labels.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

CIDs 306 307 691 880

Comment 12

[Alexei Volkov]

(In reply to comment #9)
> (From update of attachment 222748 [details] [diff] [review] [edit])
> I'm also wondering if we should move
>   td->tokens = nssList_CreateIterator(td->tokenList);
> outside the critical region.

Looks like we can not do so. nssList_CreateIterator calls funsion nssList_Clone
which access the data on the original list, that in this case is one of the things  protected by the lock. 

Comment 13

[Alexei Volkov]

Attachment: changes according nelson comments

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 225354 [details] [diff] [review]
changes according nelson comments

r=nelson
Thanks, Alexei.

Comment 15

[Wan-Teh Chang]

Comment on attachment 225354 [details] [diff] [review]
changes according nelson comments

r=wtc.  Good patch.

In trustdomain.c, NSSTrustDomain_Destroy, since
you are already nulling td's pointer members
after destroying the objects they point to, you
might as well add
  	td->tokensLock = NULL;
after
  	NSSRWLock_Destroy(td->tokensLock);

Comment 16

[Alexei Volkov]

tip:

/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.88; previous revision: 1.87
/cvsroot/mozilla/security/nss/lib/pki/trustdomain.c,v  <--  trustdomain.c
new revision: 1.52; previous revision: 1.51

3.11 branch:
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.86.28.2; previous revision: 1.86.28.1
/cvsroot/mozilla/security/nss/lib/pki/trustdomain.c,v  <--  trustdomain.c
new revision: 1.51.28.1; previous revision: 1.51