Last Comment Bug 250687 - NSS Crashes or leaks Cert references if bad certs are passed up by PKCS #11 modules.
: NSS Crashes or leaks Cert references if bad certs are passed up by PKCS #11 m...
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.9.2
: x86 Windows 2000
: P2 normal (vote)
: 3.9.3
Assigned To: Robert Relyea
: Bishakha Banerjee
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-07-09 16:19 PDT by Robert Relyea
Modified: 2004-09-27 16:48 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix leak and crash conditions. (13.45 KB, patch)
2004-07-12 15:14 PDT, Robert Relyea
nelson: superreview-
Details | Diff | Splinter Review
Same as previous patch except it only drops the one bad cert in a token. (12.29 KB, patch)
2004-07-13 18:08 PDT, Robert Relyea
nelson: superreview-
Details | Diff | Splinter Review
Fix review issue in pk11cert.c & comment in pki3hack.c (13.19 KB, patch)
2004-07-15 14:24 PDT, Robert Relyea
nelson: review+
bugz: superreview+
Details | Diff | Splinter Review

Description Robert Relyea 2004-07-09 16:19:45 PDT
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 2004-07-12 15:14:46 PDT
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.
Comment 2 Robert Relyea 2004-07-12 15:18:08 PDT
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 2004-07-13 18:08:39 PDT
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
Comment 4 Robert Relyea 2004-07-13 18:10:10 PDT
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) 2004-07-14 21:22:03 PDT
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) 2004-07-14 21:27:35 PDT
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 2004-07-15 14:24:31 PDT
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.
Comment 8 Robert Relyea 2004-07-15 14:27:27 PDT
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) 2004-07-15 15:07:24 PDT
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 2004-07-19 15:18:58 PDT
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 2004-07-19 17:50:57 PDT
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 2004-07-21 11:18:34 PDT
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

Note You need to log in before you can comment on or make changes to this bug.