-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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
Conversation
/cc @nodejs/crypto |
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 |
There was a problem hiding this 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 *>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asterisk to the type.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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() { }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{}
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, common.mustCall
.
There was a problem hiding this comment.
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'}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ crl: ... }
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asterisk.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asterisk ;)
bfc983a
to
03ee8ec
Compare
#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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.)
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave this as is.
There was a problem hiding this 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.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); | ||
}); | ||
})) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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.
CI is running on https://ci.nodejs.org/job/node-test-pull-request/4810/ |
This PR includes the commit of 8dbe1aa written by @AdamMajer . Do we need his permission? |
@sam-github Okay, thanks for your answer. |
@shigeki thanks for your careful review |
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected 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:
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.
AddCRL would crash if neither AddRootCerts nor AddCACert had been called first.
Calling AddCACert without calling AddRootCerts first, and with an input that didn't contain any certificates, would leak an X509_STORE.
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
inlib/_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.