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

RESOLVED FIXED in 3.9.3

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Robert Relyea, Assigned: Robert Relyea)

Tracking

3.9.2
3.9.3
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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).
(Assignee)

Comment 1

13 years ago
Created attachment 152983 [details] [diff] [review]
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.
(Assignee)

Comment 2

13 years ago
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
Attachment #152983 - Flags: superreview?(nelson)
Attachment #152983 - Flags: review?(bugz)
(Assignee)

Comment 3

13 years ago
Created attachment 153098 [details] [diff] [review]
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
(Assignee)

Comment 4

13 years ago
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
Attachment #153098 - Flags: superreview?(nelson)
Attachment #153098 - Flags: review?(bugz)
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.
Attachment #152983 - Flags: superreview?(nelson)
Attachment #152983 - Flags: superreview-
Attachment #152983 - Flags: review?(bugz)
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.
Attachment #153098 - Flags: superreview?(nelson)
Attachment #153098 - Flags: superreview-
Attachment #153098 - Flags: review?(bugz)
(Assignee)

Comment 7

13 years ago
Created attachment 153333 [details] [diff] [review]
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.
Attachment #152983 - Attachment is obsolete: true
Attachment #153098 - Attachment is obsolete: true
(Assignee)

Comment 8

13 years ago
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.
Attachment #153333 - Flags: superreview?(nelson)
Attachment #153333 - Flags: review?(bugz)
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
Attachment #153333 - Flags: superreview?(nelson)
Attachment #153333 - Flags: superreview?(bugz)
Attachment #153333 - Flags: review?(bugz)
Attachment #153333 - Flags: review+
(Assignee)

Comment 10

13 years ago
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

Status: NEW → ASSIGNED
Target Milestone: --- → 3.9.3

Comment 11

13 years ago
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.
Attachment #153333 - Flags: superreview?(bugz) → superreview+
(Assignee)

Comment 12

13 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.