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

v4 backport: crypto: fix handling of root_cert_store. #10969

Closed
wants to merge 4 commits into from
Closed

v4 backport: crypto: fix handling of root_cert_store. #10969

wants to merge 4 commits into from

Commits on Feb 14, 2017

  1. 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.

PR-URL: nodejs#9409
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
    @agl @sam-github
    agl authored and sam-github committed Feb 14, 2017
    Configuration menu
    Copy the full SHA
    13c8240 View commit details
    Browse the repository at this point in the history

  2. src: fix memory leak introduced in 34febfb 

    Fix leaking the BIO in the error path.  Introduced in commit 34febfb
("crypto: fix handling of root_cert_store").

PR-URL: nodejs#9604
Refs: nodejs#9409
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
    @bnoordhuis @sam-github
    bnoordhuis authored and sam-github committed Feb 14, 2017
    Configuration menu
    Copy the full SHA
    6d0ce65 View commit details
    Browse the repository at this point in the history

  3. src: use ABORT() macro instead of abort() 

    This makes sure that we dump a backtrace and use raise(SIGABRT) on
Windows.

PR-URL: nodejs#9613
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
    @evanlucas @sam-github
    evanlucas authored and sam-github committed Feb 14, 2017
    Configuration menu
    Copy the full SHA
    129be41 View commit details
    Browse the repository at this point in the history

  4. crypto: use CHECK_NE instead of ABORT or abort 

    Use of abort() was added in 34febfb, and changed to ABORT()
in 21826ef, but conditional+ABORT() is better expressesed
using a CHECK_xxx() macro.

See: nodejs#9409 (comment)

PR-URL: nodejs#10413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
    @sam-github
    sam-github committed Feb 14, 2017
    Configuration menu
    Copy the full SHA
    f923028 View commit details
    Browse the repository at this point in the history