Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

crypto: fix handling of root_cert_store #9409

Closed
wants to merge 2 commits into from
Closed

crypto: fix handling of root_cert_store #9409

wants to merge 2 commits into from

Commits on Nov 8, 2016

  1. crypto: Use reference count to manage cert_store 

    Setting reference count at the time of setting cert_store instead of
trying to manage it by modifying internal states in destructor.

PR-URL: nodejs#8334
    @AdamMajer @agl
    AdamMajer authored and agl committed Nov 8, 2016
    Configuration menu
    Copy the full SHA
    8dbe1aa View commit details
    Browse the repository at this point in the history

  2. crypto: fix handling of root_cert_store. 

    SecureContext::AddRootCerts only parses the root certificates once and
keeps the result in root_cert_store, a global X509_STORE. This change
addresses the following issues:

1. SecureContext::AddCACert would add certificates to whatever
X509_STORE was being used, even if that happened to be root_cert_store.
Thus adding a CA certificate to a SecureContext would also cause it to
be included in unrelated SecureContexts.

2. AddCRL would crash if neither AddRootCerts nor AddCACert had been
called first.

3. Calling AddCACert without calling AddRootCerts first, and with an
input that didn't contain any certificates, would leak an X509_STORE.

4. AddCRL would add the CRL to whatever X509_STORE was being used. Thus,
like AddCACert, unrelated SecureContext objects could be affected.

The following, non-obvious behaviour remains: calling AddRootCerts
doesn't /add/ them, rather it sets the CA certs to be the root set and
overrides any previous CA certificates.

Points 1–3 are probably unimportant because the SecureContext is
typically configured by `createSecureContext` in `lib/_tls_common.js`.
This function either calls AddCACert or AddRootCerts and only calls
AddCRL after setting up CA certificates. Point four could still apply in
the unlikely case that someone configures a CRL without explicitly
configuring the CAs.
    @agl
    agl committed Nov 8, 2016
    Configuration menu
    Copy the full SHA
    6e886af View commit details
    Browse the repository at this point in the history