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.

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

crypto: fix handling of root_cert_store #9409

Closed
wants to merge 2 commits into from
Closed

Conversation

agl
Copy link
Contributor

@agl agl commented Nov 1, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

Description of change

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.

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 1, 2016
@agl agl mentioned this pull request Nov 1, 2016
2 tasks
@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Nov 1, 2016
@mscdex
Copy link
Contributor

mscdex commented Nov 1, 2016

/cc @nodejs/crypto

@jasnell
Copy link
Member

jasnell commented Nov 2, 2016

Would you mind adding some tests that verify the corrected behavior?

@mhdawson
Copy link
Member

mhdawson commented Nov 4, 2016

@sam-github

@agl
Copy link
Contributor Author

agl commented Nov 4, 2016

Would you mind adding some tests that verify the corrected behavior?

I don't really know Javascript I'm afraid, but I've amended the change with a monkey-see-monkey-do test that checks the most obvious of the issues (that adding a CA can alter other SecureContexts). That test fails on master but passes after this change.

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM! Just a few nits before landing. Thank you!

@@ -121,6 +121,7 @@ const char* const root_certs[] = {
};

X509_STORE* root_cert_store;
std::vector<X509 *>* root_certs_vector;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask you to make it X509* here? We generally try to keep asterisk with the type name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

CRYPTO_add(&cert->references, 1, CRYPTO_LOCK_X509);
return 1;
}
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably needs a // comment with the condition at #if

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


static X509_STORE* NewRootCertStore() {
if (!root_certs_vector) {
root_certs_vector = new std::vector<X509 *>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asterisk to the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// certificate.
clientOptions.secureContext = contextWithoutCert;
clientOptions.port = server.address().port;
var client = tls.connect(clientOptions, common.fail);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With ES6 we try to always use const/let instead of var.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

key: loadPEM('agent1-key'),
cert: loadPEM('agent1-cert'),
};
var server = tls.createServer(serverOptions, function() { });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// This time it should connect because contextWithCert includes the needed
// CA certificate.
clientOptions.secureContext = contextWithCert;
var client2 = tls.connect(clientOptions, function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, common.mustCall.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -141,3 +141,7 @@ assert.throws(function() {

// Make sure memory isn't released before being returned
console.log(crypto.randomBytes(16));

assert.throws(function() {
tls.createSecureContext({crl: 'not a CRL'});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ crl: ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -725,55 +773,43 @@ void SecureContext::AddCRL(const FunctionCallbackInfo<Value>& args) {
if (!bio)
return;

X509_CRL *x509 =
X509_CRL *crl =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it was this way before, but let's put asterisk to the left here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

X509_free(x509);
cert_count += 1;
X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_);
while (X509 *x509 =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asterisk.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


for (size_t i = 0; i < arraysize(root_certs); i++) {
BIO* bp = NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i]));
X509 *x509 = PEM_read_bio_X509(bp, nullptr, CryptoPemCallback, nullptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asterisk ;)

@agl agl changed the title Certstore crypto: fix handling of root_cert_store Nov 5, 2016
@agl agl force-pushed the certstore branch 2 times, most recently from bfc983a to 03ee8ec Compare November 5, 2016 17:05
#if OPENSSL_VERSION_NUMBER >= 0x10002000L
CRYPTO_add(&store->references, 1, CRYPTO_LOCK_X509_STORE);
#else
// Prior to OpenSSL 1.0.2, the reference count for an X509_STORE was ignored
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need not to support openssl-1.0.1 or before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll remove this. (I only included it because 1.0.1 will still be able to compile this code, but it'll corrupt memory. Hopefully the rest of this code won't compile with 1.0.1.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did not check openssl version explicitly. It will need to be considered in the future major upgrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I've removed the check for 1.0.1 in the latest version.)

X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_);
while (X509* x509 =
PEM_read_bio_X509(bio, nullptr, CryptoPemCallback, nullptr)) {
if (cert_store == root_cert_store) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason to put this in while loop. It seems that both are not updated more than twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll write it whichever way you prefer, but the thought was that, if no CA certificates are found then we can save making an new X509_STORE. (This was explicitly done in the previous version of the code; see cert_count.)

Copy link
Contributor

@shigeki shigeki Nov 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got that secureContext.context.addCACert('') has that code path. Please leave this as is.

if (!sc->ca_store_) {
sc->ca_store_ = X509_STORE_new();
SSL_CTX_set_cert_store(sc->ctx_, sc->ca_store_);
if (cert_store == root_cert_store) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Same intent as in the previous case.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave this as is.

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, assuming that nits reported by @nodejs/crrypto are fixed.

Copy link
Contributor Author

@agl agl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Pending feedback before making changes.)

#if OPENSSL_VERSION_NUMBER >= 0x10002000L
CRYPTO_add(&store->references, 1, CRYPTO_LOCK_X509_STORE);
#else
// Prior to OpenSSL 1.0.2, the reference count for an X509_STORE was ignored
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll remove this. (I only included it because 1.0.1 will still be able to compile this code, but it'll corrupt memory. Hopefully the rest of this code won't compile with 1.0.1.)

X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_);
while (X509* x509 =
PEM_read_bio_X509(bio, nullptr, CryptoPemCallback, nullptr)) {
if (cert_store == root_cert_store) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll write it whichever way you prefer, but the thought was that, if no CA certificates are found then we can save making an new X509_STORE. (This was explicitly done in the previous version of the code; see cert_count.)

if (!sc->ca_store_) {
sc->ca_store_ = X509_STORE_new();
SSL_CTX_set_cert_store(sc->ctx_, sc->ca_store_);
if (cert_store == root_cert_store) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Same intent as in the previous case.)

@@ -141,3 +141,8 @@ assert.throws(function() {

// Make sure memory isn't released before being returned
console.log(crypto.randomBytes(16));

assert.throws(function() {
common.fail();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes a lint error.

146:7  error  Expected indentation of 2 spaces but found 6  indent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. (That was actually something that I should have removed before committing.)

client2.on('error', (e) => {
console.log(e);
});
}))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this has a lint error.

59:6  error  Missing semicolon  semi

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

AdamMajer and others added 2 commits November 8, 2016 10:43
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
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.
@shigeki
Copy link
Contributor

shigeki commented Nov 9, 2016

@shigeki
Copy link
Contributor

shigeki commented Nov 9, 2016

This PR includes the commit of 8dbe1aa written by @AdamMajer . Do we need his permission?

@sam-github
Copy link
Contributor

@shigeki No, he PRed that commit in #8334, requesting it be merged, and the commit is still attributed to him.

He'll have to rebase his PR onto master after this merges.

@shigeki
Copy link
Contributor

shigeki commented Nov 9, 2016

@sam-github Okay, thanks for your answer.

@sam-github
Copy link
Contributor

@shigeki thanks for your careful review

@shigeki
Copy link
Contributor

shigeki commented Nov 9, 2016

CI has only one test failure due to timeout in arm platform. Landed in 9259bf3 and 34febfb. Thanks.

MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
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: #9409
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
Fix leaking the BIO in the error path.  Introduced in commit 34febfb
("crypto: fix handling of root_cert_store").

PR-URL: #9604
Refs: #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>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
Setting reference count at the time of setting cert_store instead of
trying to manage it by modifying internal states in destructor.

PR-URL: #9409
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
This was referenced Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
Setting reference count at the time of setting cert_store instead of
trying to manage it by modifying internal states in destructor.

PR-URL: #9409
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
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: #9409
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
Fix leaking the BIO in the error path.  Introduced in commit 34febfb
("crypto: fix handling of root_cert_store").

PR-URL: #9604
Refs: #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>
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
Setting reference count at the time of setting cert_store instead of
trying to manage it by modifying internal states in destructor.

PR-URL: #9409
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
sam-github pushed a commit to sam-github/node that referenced this pull request Feb 14, 2017
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>
sam-github pushed a commit to sam-github/node that referenced this pull request Feb 14, 2017
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>
sam-github added a commit to sam-github/node that referenced this pull request Feb 14, 2017
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>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2017
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: #9409
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2017
Fix leaking the BIO in the error path.  Introduced in commit 34febfb
("crypto: fix handling of root_cert_store").

PR-URL: #9604
Refs: #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>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2017
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: #9409 (comment)

PR-URL: #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>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2017
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: #9409
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2017
Fix leaking the BIO in the error path.  Introduced in commit 34febfb
("crypto: fix handling of root_cert_store").

PR-URL: #9604
Refs: #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>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2017
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: #9409 (comment)

PR-URL: #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>
MylesBorins pushed a commit that referenced this pull request Mar 7, 2017
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: #9409 (comment)

PR-URL: #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>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
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: #9409 (comment)

PR-URL: #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>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
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: #9409
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Fix leaking the BIO in the error path.  Introduced in commit 34febfb
("crypto: fix handling of root_cert_store").

PR-URL: #9604
Refs: #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>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
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: #9409 (comment)

PR-URL: #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>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. confirmed-bug Issues with confirmed bugs. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.