crypto: fix handling of root_cert_store by agl · Pull Request #9409 · nodejs/node · GitHub

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:

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

/cc @nodejs/crypto

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

@sam-github

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.

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

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

Done.

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

Done.

Asterisk to the type.

Done.

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

Done.

{}

Done.

Also, common.mustCall.

Done.

{ crl: ... }

Done.

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

Done.

Asterisk.

Done.

Asterisk ;)

We need not to support openssl-1.0.1 or before.

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

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

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

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

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

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

Ditto.

(Same intent as in the previous case.)

Leave this as is.

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

(Pending feedback before making changes.)

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

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

(Same intent as in the previous case.)

This causes a lint error.

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

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

Also this has a lint error.

59:6  error  Missing semicolon  semi

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?

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

@sam-github Okay, thanks for your answer.

@shigeki thanks for your careful review

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

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: #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>

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>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 1, 2016

agl mentioned this pull request Nov 1, 2016

Make it build using openssl 1.1.0 #8491

Closed

2 tasks

mscdex added the tls Issues and PRs related to the tls subsystem. label Nov 1, 2016

agl force-pushed the certstore branch from cdfc180 to 4c25834 Compare November 4, 2016 21:06

agl force-pushed the certstore branch from 4c25834 to dd78489 Compare November 4, 2016 21:13

indutny suggested changes Nov 4, 2016

View reviewed changes

agl changed the title Certstore crypto: fix handling of root_cert_store Nov 5, 2016

agl force-pushed the certstore branch 2 times, most recently from bfc983a to 03ee8ec Compare November 5, 2016 17:05

shigeki suggested changes Nov 7, 2016

View reviewed changes

shigeki reviewed Nov 7, 2016

View reviewed changes

indutny approved these changes Nov 7, 2016

View reviewed changes

agl commented Nov 7, 2016

View reviewed changes

shigeki reviewed Nov 8, 2016

View reviewed changes

AdamMajer and others added 2 commits November 8, 2016 10:43

crypto: Use reference count to manage cert_store

8dbe1aa

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

agl force-pushed the certstore branch from 03ee8ec to 6e886af Compare November 8, 2016 18:55

shigeki approved these changes Nov 9, 2016

View reviewed changes

This was referenced Jan 24, 2017

v4.8.0 proposal #10973

Merged

v6.10.0 proposal #10974

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 7, 2017

MylesBorins mentioned this pull request Mar 9, 2017

v4.8.1 proposal #11760

Merged

davidben mentioned this pull request Aug 14, 2017

crypto: account for OpenSSL 1.1.0 function signature change. #14761

Closed

2 tasks

crypto: fix handling of root_cert_store by agl · Pull Request #9409 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: fix handling of root_cert_store - #9409

crypto: fix handling of root_cert_store#9409agl wants to merge 2 commits intonodejs:masternodejs/node:masterfrom agl:certstoreagl/node:certstoreCopy head branch name to clipboard

Conversation

agl commented Nov 1, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

mscdex commented Nov 1, 2016

Uh oh!

jasnell commented Nov 2, 2016

Uh oh!

mhdawson commented Nov 4, 2016

Uh oh!

agl commented Nov 4, 2016

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shigeki Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crypto: fix handling of root_cert_store#9409
agl wants to merge 2 commits into
nodejs:masterfrom
agl:certstore

shigeki Nov 8, 2016 •
edited
Loading