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

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Jan 23, 2017 •
edited
Loading

backport #9409 (comment) and its fix, #9604, and its cleanups, #9613 and #10413

/to @agl, PTAL

/to @MylesBorins I am still building it locally and will self-review again closely to make sure it makes sense.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. v4.x labels Jan 23, 2017
This was referenced Jan 23, 2017
Merged
Merged
@bnoordhuis
Copy link
Member

Lint error in test/parallel/test-crypto.js: 146:4 error Unexpected string as second argument assert-throws-arguments

@sam-github
Copy link
Contributor Author

fixed-up

@sam-github
Copy link
Contributor Author

@MylesBorins did you see this?

@sam-github
Copy link
Contributor Author

ci: https://ci.nodejs.org/job/node-test-pull-request/6053/

@sam-github
Copy link
Contributor Author

@nodejs/crypto PTAL

@MylesBorins
Copy link
Contributor

@sam-github this needs a rebase

agl and others added 4 commits February 14, 2017 11:24
@agl @sam-github
crypto: fix handling of root_cert_store.
13c8240 
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 <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
@bnoordhuis @sam-github
src: fix memory leak introduced in 34febfb
6d0ce65 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@evanlucas @sam-github
src: use ABORT() macro instead of abort()
129be41 
This makes sure that we dump a backtrace and use raise(SIGABRT) on
Windows.

PR-URL: nodejs#9613
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@sam-github
crypto: use CHECK_NE instead of ABORT or abort
f923028 
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 <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@sam-github
Copy link
Contributor Author

ci: https://ci.nodejs.org/job/node-test-pull-request/6411/

I'm also doing a git rebase -x "make test" to ensure each individual commit passes for me locally

@MylesBorins
Copy link
Contributor

MylesBorins commented Feb 21, 2017 •
edited
Loading

landed in

ed634d0...bba3dcf

Will have to wait for next release to land

@sam-github sam-github deleted the backport-9409-to-v4 branch April 17, 2017 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@sam-github @bnoordhuis @MylesBorins @jasnell @nodejs-github-bot @agl @evanlucas