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

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

/ node Public

Commit 35a660e

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 e0dc0ce commit 35a660e

4 files changed

Lines changed: 162 additions & 66 deletions

File tree

‎src/node_crypto.cc‎

Lines changed: 84 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,7 @@ const char* const root_certs[] = {
123123
std::string extra_root_certs_file; // NOLINT(runtime/string)
124124

125125
X509_STORE* root_cert_store;
126+
std::vector<X509*>* root_certs_vector;
126127

127128
// Just to generate static methods
128129
template class SSLWrap<TLSWrap>;
@@ -404,8 +405,6 @@ void SecureContext::Init(const FunctionCallbackInfo<Value>& args) {
404405
SSL_SESS_CACHE_NO_AUTO_CLEAR);
405406
SSL_CTX_sess_set_get_cb(sc->ctx_, SSLWrap<Connection>::GetSessionCallback);
406407
SSL_CTX_sess_set_new_cb(sc->ctx_, SSLWrap<Connection>::NewSessionCallback);
407-
408-
sc->ca_store_ = nullptr;
409408
}
410409

411410

@@ -689,8 +688,52 @@ void SecureContext::SetCert(const FunctionCallbackInfo<Value>& args) {
689688
}
690689

691690

691+
#if OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(OPENSSL_IS_BORINGSSL)
692+
// This section contains OpenSSL 1.1.0 functions reimplemented for OpenSSL
693+
// 1.0.2 so that the following code can be written without lots of #if lines.
694+
695+
static int X509_STORE_up_ref(X509_STORE* store) {
696+
CRYPTO_add(&store->references, 1, CRYPTO_LOCK_X509_STORE);
697+
return 1;
698+
}
699+
700+
static int X509_up_ref(X509* cert) {
701+
CRYPTO_add(&cert->references, 1, CRYPTO_LOCK_X509);
702+
return 1;
703+
}
704+
#endif // OPENSSL_VERSION_NUMBER < 0x10100000L && !OPENSSL_IS_BORINGSSL
705+
706+
707+
static X509_STORE* NewRootCertStore() {
708+
if (!root_certs_vector) {
709+
root_certs_vector = new std::vector<X509*>;
710+
711+
for (size_t i = 0; i < arraysize(root_certs); i++) {
712+
BIO* bp = NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i]));
713+
X509 *x509 = PEM_read_bio_X509(bp, nullptr, CryptoPemCallback, nullptr);
714+
BIO_free(bp);
715+
716+
if (x509 == nullptr) {
717+
// Parse errors from the built-in roots are fatal.
718+
abort();
719+
return nullptr;
720+
}
721+
722+
root_certs_vector->push_back(x509);
723+
}
724+
}
725+
726+
X509_STORE* store = X509_STORE_new();
727+
for (auto& cert : *root_certs_vector) {
728+
X509_up_ref(cert);
729+
X509_STORE_add_cert(store, cert);
730+
}
731+
732+
return store;
733+
}
734+
735+
692736
void SecureContext::AddCACert(const FunctionCallbackInfo<Value>& args) {
693-
bool newCAStore = false;
694737
Environment* env = Environment::GetCurrent(args);
695738

696739
SecureContext* sc;
@@ -702,23 +745,24 @@ void SecureContext::AddCACert(const FunctionCallbackInfo<Value>& args) {
702745
return env->ThrowTypeError("Bad parameter");
703746
}
704747

705-
if (!sc->ca_store_) {
706-
sc->ca_store_ = X509_STORE_new();
707-
newCAStore = true;
708-
}
709-
710-
X509* x509 = LoadX509(env, args[0]);
711-
if (!x509)
748+
BIO* bio = LoadBIO(env, args[0]);
749+
if (!bio) {
712750
return;
751+
}
713752

714-
X509_STORE_add_cert(sc->ca_store_, x509);
715-
SSL_CTX_add_client_CA(sc->ctx_, x509);
716-
717-
X509_free(x509);
718-
719-
if (newCAStore) {
720-
SSL_CTX_set_cert_store(sc->ctx_, sc->ca_store_);
753+
X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_);
754+
while (X509* x509 =
755+
PEM_read_bio_X509(bio, nullptr, CryptoPemCallback, nullptr)) {
756+
if (cert_store == root_cert_store) {
757+
cert_store = NewRootCertStore();
758+
SSL_CTX_set_cert_store(sc->ctx_, cert_store);
759+
}
760+
X509_STORE_add_cert(cert_store, x509);
761+
SSL_CTX_add_client_CA(sc->ctx_, x509);
762+
X509_free(x509);
721763
}
764+
765+
BIO_free_all(bio);
722766
}
723767

724768

@@ -739,19 +783,27 @@ void SecureContext::AddCRL(const FunctionCallbackInfo<Value>& args) {
739783
if (!bio)
740784
return;
741785

742-
X509_CRL *x509 =
786+
X509_CRL* crl =
743787
PEM_read_bio_X509_CRL(bio, nullptr, CryptoPemCallback, nullptr);
744788

745-
if (x509 == nullptr) {
789+
if (crl == nullptr) {
790+
return env->ThrowError("Failed to parse CRL");
746791
BIO_free_all(bio);
747792
return;
748793
}
749794

750-
X509_STORE_add_crl(sc->ca_store_, x509);
751-
X509_STORE_set_flags(sc->ca_store_, X509_V_FLAG_CRL_CHECK |
752-
X509_V_FLAG_CRL_CHECK_ALL);
795+
X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_);
796+
if (cert_store == root_cert_store) {
797+
cert_store = NewRootCertStore();
798+
SSL_CTX_set_cert_store(sc->ctx_, cert_store);
799+
}
800+
801+
X509_STORE_add_crl(cert_store, crl);
802+
X509_STORE_set_flags(cert_store,
803+
X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL);
804+
753805
BIO_free_all(bio);
754-
X509_CRL_free(x509);
806+
X509_CRL_free(crl);
755807
}
756808

757809

@@ -794,28 +846,8 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args) {
794846
ClearErrorOnReturn clear_error_on_return;
795847
(void) &clear_error_on_return; // Silence compiler warning.
796848

797-
CHECK_EQ(sc->ca_store_, nullptr);
798-
799849
if (!root_cert_store) {
800-
root_cert_store = X509_STORE_new();
801-
802-
for (size_t i = 0; i < arraysize(root_certs); i++) {
803-
BIO* bp = NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i]));
804-
if (bp == nullptr) {
805-
return;
806-
}
807-
808-
X509 *x509 = PEM_read_bio_X509(bp, nullptr, CryptoPemCallback, nullptr);
809-
if (x509 == nullptr) {
810-
BIO_free_all(bp);
811-
return;
812-
}
813-
814-
X509_STORE_add_cert(root_cert_store, x509);
815-
816-
BIO_free_all(bp);
817-
X509_free(x509);
818-
}
850+
root_cert_store = NewRootCertStore();
819851

820852
if (!extra_root_certs_file.empty()) {
821853
unsigned long err = AddCertsFromFile( // NOLINT(runtime/int)
@@ -830,10 +862,9 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args) {
830862
}
831863
}
832864

833-
sc->ca_store_ = root_cert_store;
834865
// Increment reference count so global store is not deleted along with CTX.
835-
CRYPTO_add(&root_cert_store->references, 1, CRYPTO_LOCK_X509_STORE);
836-
SSL_CTX_set_cert_store(sc->ctx_, sc->ca_store_);
866+
X509_STORE_up_ref(root_cert_store);
867+
SSL_CTX_set_cert_store(sc->ctx_, root_cert_store);
837868
}
838869

839870

@@ -1034,6 +1065,8 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
10341065
sc->cert_ = nullptr;
10351066
}
10361067

1068+
X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_);
1069+
10371070
if (d2i_PKCS12_bio(in, &p12) &&
10381071
PKCS12_parse(p12, pass, &pkey, &cert, &extra_certs) &&
10391072
SSL_CTX_use_certificate_chain(sc->ctx_,
@@ -1046,11 +1079,11 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
10461079
for (int i = 0; i < sk_X509_num(extra_certs); i++) {
10471080
X509* ca = sk_X509_value(extra_certs, i);
10481081

1049-
if (!sc->ca_store_) {
1050-
sc->ca_store_ = X509_STORE_new();
1051-
SSL_CTX_set_cert_store(sc->ctx_, sc->ca_store_);
1082+
if (cert_store == root_cert_store) {
1083+
cert_store = NewRootCertStore();
1084+
SSL_CTX_set_cert_store(sc->ctx_, cert_store);
10521085
}
1053-
X509_STORE_add_cert(sc->ca_store_, ca);
1086+
X509_STORE_add_cert(cert_store, ca);
10541087
SSL_CTX_add_client_CA(sc->ctx_, ca);
10551088
}
10561089
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