Skip to content

Commit

Permalink
crypto: fix handling of root_cert_store.
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
agl authored and MylesBorins committed Feb 22, 2017
1 parent 9b35715 commit 6e8edf9
Show file tree
Hide file tree
Showing 4 changed files with 162 additions and 66 deletions.
135 changes: 84 additions & 51 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ const char* const root_certs[] = {
std::string extra_root_certs_file; // NOLINT(runtime/string)

X509_STORE* root_cert_store;
std::vector<X509*>* root_certs_vector;

// Just to generate static methods
template class SSLWrap<TLSWrap>;
Expand Down Expand Up @@ -404,8 +405,6 @@ void SecureContext::Init(const FunctionCallbackInfo<Value>& args) {
SSL_SESS_CACHE_NO_AUTO_CLEAR);
SSL_CTX_sess_set_get_cb(sc->ctx_, SSLWrap<Connection>::GetSessionCallback);
SSL_CTX_sess_set_new_cb(sc->ctx_, SSLWrap<Connection>::NewSessionCallback);

sc->ca_store_ = nullptr;
}


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


#if OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(OPENSSL_IS_BORINGSSL)
// This section contains OpenSSL 1.1.0 functions reimplemented for OpenSSL
// 1.0.2 so that the following code can be written without lots of #if lines.

static int X509_STORE_up_ref(X509_STORE* store) {
CRYPTO_add(&store->references, 1, CRYPTO_LOCK_X509_STORE);
return 1;
}

static int X509_up_ref(X509* cert) {
CRYPTO_add(&cert->references, 1, CRYPTO_LOCK_X509);
return 1;
}
#endif // OPENSSL_VERSION_NUMBER < 0x10100000L && !OPENSSL_IS_BORINGSSL


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

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);
BIO_free(bp);

if (x509 == nullptr) {
// Parse errors from the built-in roots are fatal.
abort();
return nullptr;
}

root_certs_vector->push_back(x509);
}
}

X509_STORE* store = X509_STORE_new();
for (auto& cert : *root_certs_vector) {
X509_up_ref(cert);
X509_STORE_add_cert(store, cert);
}

return store;
}


void SecureContext::AddCACert(const FunctionCallbackInfo<Value>& args) {
bool newCAStore = false;
Environment* env = Environment::GetCurrent(args);

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

if (!sc->ca_store_) {
sc->ca_store_ = X509_STORE_new();
newCAStore = true;
}

X509* x509 = LoadX509(env, args[0]);
if (!x509)
BIO* bio = LoadBIO(env, args[0]);
if (!bio) {
return;
}

X509_STORE_add_cert(sc->ca_store_, x509);
SSL_CTX_add_client_CA(sc->ctx_, x509);

X509_free(x509);

if (newCAStore) {
SSL_CTX_set_cert_store(sc->ctx_, sc->ca_store_);
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) {
cert_store = NewRootCertStore();
SSL_CTX_set_cert_store(sc->ctx_, cert_store);
}
X509_STORE_add_cert(cert_store, x509);
SSL_CTX_add_client_CA(sc->ctx_, x509);
X509_free(x509);
}

BIO_free_all(bio);
}


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

X509_CRL *x509 =
X509_CRL* crl =
PEM_read_bio_X509_CRL(bio, nullptr, CryptoPemCallback, nullptr);

if (x509 == nullptr) {
if (crl == nullptr) {
return env->ThrowError("Failed to parse CRL");
BIO_free_all(bio);
return;
}

X509_STORE_add_crl(sc->ca_store_, x509);
X509_STORE_set_flags(sc->ca_store_, X509_V_FLAG_CRL_CHECK |
X509_V_FLAG_CRL_CHECK_ALL);
X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_);
if (cert_store == root_cert_store) {
cert_store = NewRootCertStore();
SSL_CTX_set_cert_store(sc->ctx_, cert_store);
}

X509_STORE_add_crl(cert_store, crl);
X509_STORE_set_flags(cert_store,
X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL);

BIO_free_all(bio);
X509_CRL_free(x509);
X509_CRL_free(crl);
}


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

CHECK_EQ(sc->ca_store_, nullptr);

if (!root_cert_store) {
root_cert_store = X509_STORE_new();

for (size_t i = 0; i < arraysize(root_certs); i++) {
BIO* bp = NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i]));
if (bp == nullptr) {
return;
}

X509 *x509 = PEM_read_bio_X509(bp, nullptr, CryptoPemCallback, nullptr);
if (x509 == nullptr) {
BIO_free_all(bp);
return;
}

X509_STORE_add_cert(root_cert_store, x509);

BIO_free_all(bp);
X509_free(x509);
}
root_cert_store = NewRootCertStore();

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

sc->ca_store_ = root_cert_store;
// Increment reference count so global store is not deleted along with CTX.
CRYPTO_add(&root_cert_store->references, 1, CRYPTO_LOCK_X509_STORE);
SSL_CTX_set_cert_store(sc->ctx_, sc->ca_store_);
X509_STORE_up_ref(root_cert_store);
SSL_CTX_set_cert_store(sc->ctx_, root_cert_store);
}


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

X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_);

if (d2i_PKCS12_bio(in, &p12) &&
PKCS12_parse(p12, pass, &pkey, &cert, &extra_certs) &&
SSL_CTX_use_certificate_chain(sc->ctx_,
Expand All @@ -1046,11 +1079,11 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
for (int i = 0; i < sk_X509_num(extra_certs); i++) {
X509* ca = sk_X509_value(extra_certs, i);

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) {
cert_store = NewRootCertStore();
SSL_CTX_set_cert_store(sc->ctx_, cert_store);
}
X509_STORE_add_cert(sc->ca_store_, ca);
X509_STORE_add_cert(cert_store, ca);
SSL_CTX_add_client_CA(sc->ctx_, ca);
}
ret = true;
Expand Down
27 changes: 12 additions & 15 deletions src/node_crypto.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ class SecureContext : public BaseObject {

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

X509_STORE* ca_store_;
SSL_CTX* ctx_;
X509* cert_;
X509* issuer_;
Expand Down Expand Up @@ -131,7 +130,6 @@ class SecureContext : public BaseObject {

SecureContext(Environment* env, v8::Local<v8::Object> wrap)
: BaseObject(env, wrap),
ca_store_(nullptr),
ctx_(nullptr),
cert_(nullptr),
issuer_(nullptr) {
Expand All @@ -140,20 +138,19 @@ class SecureContext : public BaseObject {
}

void FreeCTXMem() {
if (ctx_) {
env()->isolate()->AdjustAmountOfExternalAllocatedMemory(-kExternalSize);
SSL_CTX_free(ctx_);
if (cert_ != nullptr)
X509_free(cert_);
if (issuer_ != nullptr)
X509_free(issuer_);
ctx_ = nullptr;
ca_store_ = nullptr;
cert_ = nullptr;
issuer_ = nullptr;
} else {
CHECK_EQ(ca_store_, nullptr);
if (!ctx_) {
return;
}

env()->isolate()->AdjustAmountOfExternalAllocatedMemory(-kExternalSize);
SSL_CTX_free(ctx_);
if (cert_ != nullptr)
X509_free(cert_);
if (issuer_ != nullptr)
X509_free(issuer_);
ctx_ = nullptr;
cert_ = nullptr;
issuer_ = nullptr;
}
};

Expand Down
4 changes: 4 additions & 0 deletions test/parallel/test-crypto.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,3 +140,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' });
}, /Failed to parse CRL/);
62 changes: 62 additions & 0 deletions test/parallel/test-tls-addca.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
'use strict';
const common = require('../common');
const fs = require('fs');

if (!common.hasCrypto) {
common.skip('missing crypto');
return;
}
const tls = require('tls');

function filenamePEM(n) {
return require('path').join(common.fixturesDir, 'keys', n + '.pem');
}

function loadPEM(n) {
return fs.readFileSync(filenamePEM(n));
}

const caCert = loadPEM('ca1-cert');
const contextWithoutCert = tls.createSecureContext({});
const contextWithCert = tls.createSecureContext({});
// Adding a CA certificate to contextWithCert should not also add it to
// contextWithoutCert. This is tested by trying to connect to a server that
// depends on that CA using contextWithoutCert.
contextWithCert.context.addCACert(caCert);

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

const clientOptions = {
port: undefined,
ca: [caCert],
servername: 'agent1',
rejectUnauthorized: true,
};

function startTest() {
// This client should fail to connect because it doesn't trust the CA
// certificate.
clientOptions.secureContext = contextWithoutCert;
clientOptions.port = server.address().port;
const client = tls.connect(clientOptions, common.fail);
client.on('error', common.mustCall(() => {
client.destroy();

// This time it should connect because contextWithCert includes the needed
// CA certificate.
clientOptions.secureContext = contextWithCert;
const client2 = tls.connect(clientOptions, common.mustCall(() => {
client2.destroy();
server.close();
}));
client2.on('error', (e) => {
console.log(e);
});
}));
}

server.listen(0, startTest);

0 comments on commit 6e8edf9

Please sign in to comment.