250687 - NSS Crashes or leaks Cert references if bad certs are passed up by PKCS #11 modules.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Robert Relyea]

The current NSS code has some implicit assumptions that all certs from PKCS #11 
modules (including the softoken) are valid certificates.

If this is not the case, it is possible that STAN_GetCERTCertificate() can 
fail. Most places attempt to recover from these failures, but end up leaking 
the NSSCertificate reference. There are some places that do not recover 
(traversal functions, for instance).

Comment 1

[Robert Relyea]

Attachment: Fix leak and crash conditions.

Some notes:
CERT_DestroyCertificate() handles the case where you pass a NULL cert, so
calling:
CERT_DestroyCertificate(STAN_GetCERTCertificateOrRelease(c))
Will safely free a certificate.

Most of the changes involve using the new function
'STAN_GetCERTCertificateOrRelease()' whenever we mean for the the
NSSCertificate reference to be adopted through the CERTCertificate. In some
cases we don't want that to happen, so we continue to call the
'STAN_GetCERTCertificate' call.

The changes to fix the crash are currently coded to reject any cert traversal
on encountering one of these certs. I'm leaning toward the semantic of simply
rejecting the single cert that failed, and I'll attach a different patch that
includes that code. In the meantime I'd like your import on which you prefer.

Comment 2

[Robert Relyea]

Comment on attachment 152983 [details] [diff] [review]
Fix leak and crash conditions.

nelson & ian, please review.

If you prefer the semantic of droping the damaged cert, go ahead and r-, but
let me know if that's the only reason for the r-.

Thanks,
bob

Comment 3

[Robert Relyea]

Attachment: Same as previous patch except it only drops the one bad cert in a token.

This patch is the same as attachemet 152983 except:
  1) it ignores bad certs in a token, rather than failing all cert reads, and
  2) it replaces the new function to destroy cert arrays with the existing Stan
function.

Please pick the semantic (case 1 above) you prefer and review that patch,
giving the other one an r-. When I check in I'll check in with the case 2 from
this patch.

Also I prefer the semantic of dropping the cert rather than failing the whole
token.

bob

Comment 4

[Robert Relyea]

Comment on attachment 153098 [details] [diff] [review]
Same as previous patch except it only drops the one bad cert in a token.

please choose a patch an review (r- the other).

bob

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 152983 [details] [diff] [review]
Fix leak and crash conditions.

Both of these patches have a problem that may lead to a double-free.
It's in pk11_FindCertByIssuerAndSNOnToken in pk11cert.c

>@@ -2171,7 +2174,7 @@
>     }
>     object = NULL; /* adopted by the previous call */
>     nssTrustDomain_AddCertsToCache(td, &cert,1);
>-    rvCert = STAN_GetCERTCertificate(cert);
>+    rvCert = STAN_GetCERTCertificateOrRelease(cert);
>     if (!rvCert) {
> 	goto loser;
>     }

At label loser, the function calls   nssCertificate_Destroy(cert);
which destroys cert a second time.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 153098 [details] [diff] [review]
Same as previous patch except it only drops the one bad cert in a token.

I'm marking this patch r- also, for the same reason as the other patch.

Except for that issue, I would have marked this patch r+ .
Dropping the bad cert seems preferable during traversals.  
It allows more of the code to keep working, possibly producing
a useful result, even in the presence of a flawed token cert.

Comment 7

[Robert Relyea]

Attachment: Fix review issue in pk11cert.c & comment in pki3hack.c

This fixes the double free by restoring the code's use of
STAN_GetCERTCertificate. It would also have worked to simply return
STAN_GetCERTCertificateOrReference(), but I felt that the latter would have
been harder for someone new at the code to follow.

Comment 8

[Robert Relyea]

Comment on attachment 153333 [details] [diff] [review]
Fix review issue in pk11cert.c & comment in pki3hack.c

Please review, Since Nelson & I prefer the version that drops the single cert
and continues, I have only supplied a new patch of that version.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 153333 [details] [diff] [review]
Fix review issue in pk11cert.c & comment in pki3hack.c

This patch adds some new SEC_ERROR codes that don't seem to be
related to the rest of this patch.  I'm assuming those were
included by mistake, and this review excludes them.

r=nelson    

Please correct the typo shown below:


>+ * many callers of STAN_GetCERTCertificate() intend that
>+ * the CERTCertificate returned inherits the reference to the 
>+ * NSSCertificate. For these callers it's convinent to have 

convinent -> convenient

Comment 10

[Robert Relyea]

Checked into 3.9

Checking in pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.128.4.3; previous revision: 1.128.4.2
done
Checking in certdb/stanpcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.62.2.1; previous revision: 1.62
done
Checking in pki/pki3hack.c;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.79.14.1; previous revision: 1.79
done
Checking in pki/pki3hack.h;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.h,v  <--  pki3hack.h
new revision: 1.15.16.1; previous revision: 1.15
done

Comment 11

[Ian McGreer]

Comment on attachment 153333 [details] [diff] [review]
Fix review issue in pk11cert.c & comment in pki3hack.c

I thought I sr+'ed this over the weekend, but I guess I forgot.  I agree with
this set of changes.

Comment 12

[Robert Relyea]

Thanks Ian,

Here's the checking log for the tip..

Checking in certdb/stanpcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.66; previous revision: 1.65
done
Checking in pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.136; previous revision: 1.135
done
Checking in pki/pki3hack.c;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.82; previous revision: 1.81
done
Checking in pki/pki3hack.h;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.h,v  <--  pki3hack.h
new revision: 1.17; previous revision: 1.16
done