FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: fix handling of root_cert_store. · nodejs/node@f299333 · GitHub

/ node Public

Commit f299333

Browse files
agl
authored and
MylesBorins
committed
crypto: fix handling of root_cert_store.
SecureContext::AddRootCerts only parses the root certificates once and keeps the result in root_cert_store, a global X509_STORE. This change addresses the following issues: 1. SecureContext::AddCACert would add certificates to whatever X509_STORE was being used, even if that happened to be root_cert_store. Thus adding a CA certificate to a SecureContext would also cause it to be included in unrelated SecureContexts. 2. AddCRL would crash if neither AddRootCerts nor AddCACert had been called first. 3. Calling AddCACert without calling AddRootCerts first, and with an input that didn't contain any certificates, would leak an X509_STORE. 4. AddCRL would add the CRL to whatever X509_STORE was being used. Thus, like AddCACert, unrelated SecureContext objects could be affected. The following, non-obvious behaviour remains: calling AddRootCerts doesn't /add/ them, rather it sets the CA certs to be the root set and overrides any previous CA certificates. Points 1–3 are probably unimportant because the SecureContext is typically configured by `createSecureContext` in `lib/_tls_common.js`. This function either calls AddCACert or AddRootCerts and only calls AddCRL after setting up CA certificates. Point four could still apply in the unlikely case that someone configures a CRL without explicitly configuring the CAs. PR-URL: #9409 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
1 parent 4849d03 commit f299333

4 files changed

Lines changed: 160 additions & 68 deletions

File tree

‎src/node_crypto.cc‎

Lines changed: 82 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,7 @@ const char* const root_certs[] = {
121121
};
122122

123123
X509_STORE* root_cert_store;
124+
std::vector<X509*>* root_certs_vector;
124125

125126
// Just to generate static methods
126127
template class SSLWrap<TLSWrap>;
@@ -402,8 +403,6 @@ void SecureContext::Init(const FunctionCallbackInfo<Value>& args) {
402403
SSL_SESS_CACHE_NO_AUTO_CLEAR);
403404
SSL_CTX_sess_set_get_cb(sc->ctx_, SSLWrap<Connection>::GetSessionCallback);
404405
SSL_CTX_sess_set_new_cb(sc->ctx_, SSLWrap<Connection>::NewSessionCallback);
405-
406-
sc->ca_store_ = nullptr;
407406
}
408407

409408

@@ -672,8 +671,52 @@ void SecureContext::SetCert(const FunctionCallbackInfo<Value>& args) {
672671
}
673672

674673

674+
#if OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(OPENSSL_IS_BORINGSSL)
675+
// This section contains OpenSSL 1.1.0 functions reimplemented for OpenSSL
676+
// 1.0.2 so that the following code can be written without lots of #if lines.
677+
678+
static int X509_STORE_up_ref(X509_STORE* store) {
679+
CRYPTO_add(&store->references, 1, CRYPTO_LOCK_X509_STORE);
680+
return 1;
681+
}
682+
683+
static int X509_up_ref(X509* cert) {
684+
CRYPTO_add(&cert->references, 1, CRYPTO_LOCK_X509);
685+
return 1;
686+
}
687+
#endif // OPENSSL_VERSION_NUMBER < 0x10100000L && !OPENSSL_IS_BORINGSSL
688+
689+
690+
static X509_STORE* NewRootCertStore() {
691+
if (!root_certs_vector) {
692+
root_certs_vector = new std::vector<X509*>;
693+
694+
for (size_t i = 0; i < arraysize(root_certs); i++) {
695+
BIO* bp = NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i]));
696+
X509 *x509 = PEM_read_bio_X509(bp, nullptr, CryptoPemCallback, nullptr);
697+
BIO_free(bp);
698+
699+
if (x509 == nullptr) {
700+
// Parse errors from the built-in roots are fatal.
701+
abort();
702+
return nullptr;
703+
}
704+
705+
root_certs_vector->push_back(x509);
706+
}
707+
}
708+
709+
X509_STORE* store = X509_STORE_new();
710+
for (auto& cert : *root_certs_vector) {
711+
X509_up_ref(cert);
712+
X509_STORE_add_cert(store, cert);
713+
}
714+
715+
return store;
716+
}
717+
718+
675719
void SecureContext::AddCACert(const FunctionCallbackInfo<Value>& args) {
676-
bool newCAStore = false;
677720
Environment* env = Environment::GetCurrent(args);
678721

679722
SecureContext* sc;
@@ -685,26 +728,24 @@ void SecureContext::AddCACert(const FunctionCallbackInfo<Value>& args) {
685728
return env->ThrowTypeError("CA certificate argument is mandatory");
686729
}
687730

688-
if (!sc->ca_store_) {
689-
sc->ca_store_ = X509_STORE_new();
690-
newCAStore = true;
731+
BIO* bio = LoadBIO(env, args[0]);
732+
if (!bio) {
733+
return;
691734
}
692735

693-
unsigned cert_count = 0;
694-
if (BIO* bio = LoadBIO(env, args[0])) {
695-
while (X509* x509 =
696-
PEM_read_bio_X509(bio, nullptr, CryptoPemCallback, nullptr)) {
697-
X509_STORE_add_cert(sc->ca_store_, x509);
698-
SSL_CTX_add_client_CA(sc->ctx_, x509);
699-
X509_free(x509);
700-
cert_count += 1;
736+
X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_);
737+
while (X509* x509 =
738+
PEM_read_bio_X509(bio, nullptr, CryptoPemCallback, nullptr)) {
739+
if (cert_store == root_cert_store) {
740+
cert_store = NewRootCertStore();
741+
SSL_CTX_set_cert_store(sc->ctx_, cert_store);
701742
}
702-
BIO_free_all(bio);
743+
X509_STORE_add_cert(cert_store, x509);
744+
SSL_CTX_add_client_CA(sc->ctx_, x509);
745+
X509_free(x509);
703746
}
704747

705-
if (cert_count > 0 && newCAStore) {
706-
SSL_CTX_set_cert_store(sc->ctx_, sc->ca_store_);
707-
}
748+
BIO_free_all(bio);
708749
}
709750

710751

@@ -725,57 +766,43 @@ void SecureContext::AddCRL(const FunctionCallbackInfo<Value>& args) {
725766
if (!bio)
726767
return;
727768

728-
X509_CRL *x509 =
769+
X509_CRL* crl =
729770
PEM_read_bio_X509_CRL(bio, nullptr, CryptoPemCallback, nullptr);
730771

731-
if (x509 == nullptr) {
772+
if (crl == nullptr) {
773+
return env->ThrowError("Failed to parse CRL");
732774
BIO_free_all(bio);
733775
return;
734776
}
735777

736-
X509_STORE_add_crl(sc->ca_store_, x509);
737-
X509_STORE_set_flags(sc->ca_store_, X509_V_FLAG_CRL_CHECK |
738-
X509_V_FLAG_CRL_CHECK_ALL);
778+
X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_);
779+
if (cert_store == root_cert_store) {
780+
cert_store = NewRootCertStore();
781+
SSL_CTX_set_cert_store(sc->ctx_, cert_store);
782+
}
783+
784+
X509_STORE_add_crl(cert_store, crl);
785+
X509_STORE_set_flags(cert_store,
786+
X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL);
787+
739788
BIO_free_all(bio);
740-
X509_CRL_free(x509);
789+
X509_CRL_free(crl);
741790
}
742791

743792

744-
745793
void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args) {
746794
SecureContext* sc;
747795
ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder());
748796
ClearErrorOnReturn clear_error_on_return;
749797
(void) &clear_error_on_return; // Silence compiler warning.
750798

751-
CHECK_EQ(sc->ca_store_, nullptr);
752-
753799
if (!root_cert_store) {
754-
root_cert_store = X509_STORE_new();
755-
756-
for (size_t i = 0; i < arraysize(root_certs); i++) {
757-
BIO* bp = NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i]));
758-
if (bp == nullptr) {
759-
return;
760-
}
761-
762-
X509 *x509 = PEM_read_bio_X509(bp, nullptr, CryptoPemCallback, nullptr);
763-
if (x509 == nullptr) {
764-
BIO_free_all(bp);
765-
return;
766-
}
767-
768-
X509_STORE_add_cert(root_cert_store, x509);
769-
770-
BIO_free_all(bp);
771-
X509_free(x509);
772-
}
800+
root_cert_store = NewRootCertStore();
773801
}
774802

775-
sc->ca_store_ = root_cert_store;
776803
// Increment reference count so global store is not deleted along with CTX.
777-
CRYPTO_add(&root_cert_store->references, 1, CRYPTO_LOCK_X509_STORE);
778-
SSL_CTX_set_cert_store(sc->ctx_, sc->ca_store_);
804+
X509_STORE_up_ref(root_cert_store);
805+
SSL_CTX_set_cert_store(sc->ctx_, root_cert_store);
779806
}
780807

781808

@@ -985,6 +1012,8 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
9851012
sc->cert_ = nullptr;
9861013
}
9871014

1015+
X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_);
1016+
9881017
if (d2i_PKCS12_bio(in, &p12) &&
9891018
PKCS12_parse(p12, pass, &pkey, &cert, &extra_certs) &&
9901019
SSL_CTX_use_certificate_chain(sc->ctx_,
@@ -997,11 +1026,11 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
9971026
for (int i = 0; i < sk_X509_num(extra_certs); i++) {
9981027
X509* ca = sk_X509_value(extra_certs, i);
9991028

1000-
if (!sc->ca_store_) {
1001-
sc->ca_store_ = X509_STORE_new();
1002-
SSL_CTX_set_cert_store(sc->ctx_, sc->ca_store_);
1029+
if (cert_store == root_cert_store) {
1030+
cert_store = NewRootCertStore();
1031+
SSL_CTX_set_cert_store(sc->ctx_, cert_store);
10031032
}
1004-
X509_STORE_add_cert(sc->ca_store_, ca);
1033+
X509_STORE_add_cert(cert_store, ca);
10051034
SSL_CTX_add_client_CA(sc->ctx_, ca);
10061035
}
10071036
ret = true;

‎src/node_crypto.h‎

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,6 @@ class SecureContext : public BaseObject {
7676

7777
static void Initialize(Environment* env, v8::Local<v8::Object> target);
7878

79-
X509_STORE* ca_store_;
8079
SSL_CTX* ctx_;
8180
X509* cert_;
8281
X509* issuer_;
@@ -131,7 +130,6 @@ class SecureContext : public BaseObject {
131130

132131
SecureContext(Environment* env, v8::Local<v8::Object> wrap)
133132
: BaseObject(env, wrap),
134-
ca_store_(nullptr),
135133
ctx_(nullptr),
136134
cert_(nullptr),
137135
issuer_(nullptr) {
@@ -140,20 +138,19 @@ class SecureContext : public BaseObject {
140138
}
141139

142140
void FreeCTXMem() {
143-
if (ctx_) {
144-
env()->isolate()->AdjustAmountOfExternalAllocatedMemory(-kExternalSize);
145-
SSL_CTX_free(ctx_);
146-
if (cert_ != nullptr)
147-
X509_free(cert_);
148-
if (issuer_ != nullptr)
149-
X509_free(issuer_);
150-
ctx_ = nullptr;
151-
ca_store_ = nullptr;
152-
cert_ = nullptr;
153-
issuer_ = nullptr;
154-
} else {
155-
CHECK_EQ(ca_store_, nullptr);
141+
if (!ctx_) {
142+
return;
156143
}
144+
145+
env()->isolate()->AdjustAmountOfExternalAllocatedMemory(-kExternalSize);
146+
SSL_CTX_free(ctx_);
147+
if (cert_ != nullptr)
148+
X509_free(cert_);
149+
if (issuer_ != nullptr)
150+
X509_free(issuer_);
151+
ctx_ = nullptr;
152+
cert_ = nullptr;
153+
issuer_ = nullptr;
157154
}
158155
};
159156

‎test/parallel/test-crypto.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,3 +140,7 @@ assert.throws(function() {
140140

141141
// Make sure memory isn't released before being returned
142142
console.log(crypto.randomBytes(16));
143+
144+
assert.throws(function() {
145+
tls.createSecureContext({ crl: 'not a CRL' });
146+
}, /Failed to parse CRL/);

‎test/parallel/test-tls-addca.js‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
'use strict';
2+
const common = require('../common');
3+
const fs = require('fs');
4+
5+
if (!common.hasCrypto) {
6+
common.skip('missing crypto');
7+
return;
8+
}
9+
const tls = require('tls');
10+
11+
function filenamePEM(n) {
12+
return require('path').join(common.fixturesDir, 'keys', n + '.pem');
13+
}
14+
15+
function loadPEM(n) {
16+
return fs.readFileSync(filenamePEM(n));
17+
}
18+
19+
const caCert = loadPEM('ca1-cert');
20+
const contextWithoutCert = tls.createSecureContext({});
21+
const contextWithCert = tls.createSecureContext({});
22+
// Adding a CA certificate to contextWithCert should not also add it to
23+
// contextWithoutCert. This is tested by trying to connect to a server that
24+
// depends on that CA using contextWithoutCert.
25+
contextWithCert.context.addCACert(caCert);
26+
27+
const serverOptions = {
28+
key: loadPEM('agent1-key'),
29+
cert: loadPEM('agent1-cert'),
30+
};
31+
const server = tls.createServer(serverOptions, function() {});
32+
33+
const clientOptions = {
34+
port: undefined,
35+
ca: [caCert],
36+
servername: 'agent1',
37+
rejectUnauthorized: true,
38+
};
39+
40+
function startTest() {
41+
// This client should fail to connect because it doesn't trust the CA
42+
// certificate.
43+
clientOptions.secureContext = contextWithoutCert;
44+
clientOptions.port = server.address().port;
45+
const client = tls.connect(clientOptions, common.fail);
46+
client.on('error', common.mustCall(() => {
47+
client.destroy();
48+
49+
// This time it should connect because contextWithCert includes the needed
50+
// CA certificate.
51+
clientOptions.secureContext = contextWithCert;
52+
const client2 = tls.connect(clientOptions, common.mustCall(() => {
53+
client2.destroy();
54+
server.close();
55+
}));
56+
client2.on('error', (e) => {
57+
console.log(e);
58+
});
59+
}));
60+
}
61+
62+
server.listen(0, startTest);

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL