1254656 - nssList_Destroy() does not check for NULL, causes crash.

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Robert Pufky]

Attachment: nsslist-destroy.patch

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.75 Safari/537.36

Steps to reproduce:

After traversing the certificates in routine NSSTrustDomain_TraverseCertificates(), Firefox cleans up the collection in nssPKIObjectCollection_Destroy(). The latter makes calls to nssList_Destroy() which does not handle a case where the argument list is NULL.

This specific case happens on OSX using a PKCS #11 API to access the keychain; if the keychain is locked, no certificates are returned, resulting in NULL being returned.


Actual results:

Firefox crashes to desktop.


Expected results:

nssList_Destroy should have returned success, as there is no list to destroy.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

I can't quite reproduce the call stack by looking at the mentioned call sites but I think we should fix this by NULL-checking the list before calling nssList_Destroy() or nssList_Count(). All other places seem to do that and we're probably just missing it here. Can you please implement NULL-checks before we call the nssList_* functions? Thanks!

Comment 2

[Franziskus Kiefer [:franziskus]]

Attachment: nsslist-destroy.patch

this file's a mess...

I agree that if there's contract that the *list handed to nssList_* functions must not be null we don't have to check them. Unfortunately there are too many calls in too many different places to make this feasible.

This patch adds some null checks in nssList_* functions as well as checking outside and should solve this bug as well as bug 1254655. But we might want to rewrite list.c at some point to be more lenient to misuse.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8757252 [details] [diff] [review]
nsslist-destroy.patch

Review of attachment 8757252 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/pki/pkistore.c
@@ +271,5 @@
>      if (subjectList) {
>  	/* Remove the cert from the subject hash */
>  	nssList_Remove(subjectList, cert);
>  	nssHash_Remove(store->subject, &cert->subject);
> +	if (subjectList) {

Shouldn't that be:

if (subjectList && nssList_Count(subjectList) == 0) {

::: lib/pki/tdcache.c
@@ +991,5 @@
>  	if (certListOpt) {
>  	    collectList = certListOpt;
>  	} else {
>  	    collectList = nssList_Create(NULL, PR_FALSE);
> +	    if (!collectList && collectList) {

That seems unlikely to ever be true :)

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8757252 [details] [diff] [review]
nsslist-destroy.patch

Review of attachment 8757252 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/pki/pkistore.c
@@ +271,5 @@
>      if (subjectList) {
>  	/* Remove the cert from the subject hash */
>  	nssList_Remove(subjectList, cert);
>  	nssHash_Remove(store->subject, &cert->subject);
> +	if (subjectList) {

No, wait. We can just leave this as is because we check |subjectList| on line 271 above already.

Comment 5

[Franziskus Kiefer [:franziskus]]

Attachment: nsslist-destroy.patch

that patch clearly needed another look... this one should work better.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8759031 [details] [diff] [review]
nsslist-destroy.patch

Review of attachment 8759031 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/base/list.c
@@ +348,5 @@
>          rvIterator->lock = PZ_NewLock(nssILockOther);
>          if (!rvIterator->lock) {
> +            if (rvIterator->list) {
> +                nssList_Destroy(rvIterator->list);
> +            }

I think we don't need this check. We null-check rvIterator->list on line 342 and dereference it on line 346 above.

Comment 7

[Franziskus Kiefer [:franziskus]]

landed as https://hg.mozilla.org/projects/nss/rev/4d5f8d533df4