Skip to content
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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

crypto: fix handling of root_cert_store #9409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8dbe1aa
crypto: Use reference count to manage cert_store
AdamMajer Aug 30, 2016
6e886af
crypto: fix handling of root_cert_store.
agl Oct 30, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 83 additions & 52 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ const char* const root_certs[] = {
};

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

// Just to generate static methods
template class SSLWrap<TLSWrap>;
Expand Down Expand Up @@ -402,8 +403,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 @@ -672,8 +671,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;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been CHECK_NE(x509, 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 @@ -685,26 +728,24 @@ void SecureContext::AddCACert(const FunctionCallbackInfo<Value>& args) {
return env->ThrowTypeError("CA certificate argument is mandatory");
}

if (!sc->ca_store_) {
sc->ca_store_ = X509_STORE_new();
newCAStore = true;
BIO* bio = LoadBIO(env, args[0]);
if (!bio) {
return;
}

unsigned cert_count = 0;
if (BIO* bio = LoadBIO(env, args[0])) {
while (X509* x509 =
PEM_read_bio_X509(bio, nullptr, CryptoPemCallback, nullptr)) {
X509_STORE_add_cert(sc->ca_store_, x509);
SSL_CTX_add_client_CA(sc->ctx_, x509);
X509_free(x509);
cert_count += 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) {
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

@shigeki shigeki Nov 8, 2016 •
edited
Loading

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.

cert_store = NewRootCertStore();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shigeki @AdamMajer This is a bug, seems to me. It used to be that use of the .ca property replaced the compiled in root certs, because it used a X509_STORE_new() store.

With this change, the .ca property creates a new store with NewRootCertStore() that contains the compiled in root certs, then adds more certs to those compiled in.

Please take another look at this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nodejs/security

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sam-github Thanks for notifying me but I don't have time in this weekend. I will take a look at this next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the existing store is root_cert_store (i.e. the global root store) then it'll be replaced with a local copy of the same root store. Otherwise modifying the store would change it for every context.

However, if the cert store has not been set to be the root store (by calling AddRootCerts) then this function will simple append the CA cert to whatever was previously included. (And the default cert store is empty.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the existing store is root_cert_store (i.e. the global root store) then it'll be replaced with a local copy of the same root store. Otherwise modifying the store would change it for every context.

I understand the bug you fixed, but you changed the behaviour. We need to fix the bug wherein the root store gets modified - because the root store should never be modified. That was good work.

However, unless I am very much mistaken, you introduced an unintended side-effect at the same time. All use of the ca: option is intended to remove the root CAs, and replace them (for this specific secure context). For better or worse, that is the documented behaviour and pre-existing behaviour.

I would like to add a unit test to prevent regressions here, but its quite difficult, because I need an identity signed by one of the CAs in our default roots, and to prove that using the ca: option makes that identity no longer verifiable. Or, I could make an outgoing connection to google.com, and prove that once an explict ca: is set, I can't verify the cert, but the network-required tests are annoying. Still, maybe less annoying than getting a cert issued to me.

@shigeki do you have any ideas how I would approach this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think nodejs.org (and example.org) should be fine to hit in CI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sam-github As far as I know, there is no sign to prohibit using NativeSecureContext. Most of works can be accomplished without it, I can't imagine why some users dare to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if code is calling these functions directly, I think it'll still be safe. While I think that the semantics for at least AddRootsCerts should be changed to be more consistent, this patch shouldn't have altered them except in the ways mentioned in the commit message.

To be specific:

AddRootCerts: both before and after overrides the previous root certs and sets the root store to the built-in root certificates.

AddCACert: adds the certificate to the existing set.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sam-github we used it directly in the past, the signs that you see are the remnants of old API that we hadn't scrubbed over yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the default cert store is empty.

OK, I've confirmed the behaviour is the same. I was confused because the cert store used to be created using X509_Store_New(), and now it relies on the SSL_CTX having a new store already, by default. Sorry for the noise, and thanks for the clarifications.

SSL_CTX_set_cert_store(sc->ctx_, cert_store);
}
BIO_free_all(bio);
X509_STORE_add_cert(cert_store, x509);
SSL_CTX_add_client_CA(sc->ctx_, x509);
X509_free(x509);
}

if (cert_count > 0 && newCAStore) {
SSL_CTX_set_cert_store(sc->ctx_, sc->ca_store_);
}
BIO_free_all(bio);
}


Expand All @@ -725,55 +766,43 @@ 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);
}



void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args) {
SecureContext* sc;
ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder());
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();
}

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


Expand Down Expand Up @@ -983,6 +1012,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 @@ -995,11 +1026,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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave this as is.

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
34 changes: 12 additions & 22 deletions src/node_crypto.h
View file
Open in desktop
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,27 +138,19 @@ class SecureContext : public BaseObject {
}

void FreeCTXMem() {
if (ctx_) {
env()->isolate()->AdjustAmountOfExternalAllocatedMemory(-kExternalSize);
if (ctx_->cert_store == root_cert_store) {
// SSL_CTX_free() will attempt to free the cert_store as well.
// Since we want our root_cert_store to stay around forever
// we just clear the field. Hopefully OpenSSL will not modify this
// struct in future versions.
ctx_->cert_store = nullptr;
}
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' });
}, '/Failed to parse CRL/');
62 changes: 62 additions & 0 deletions test/parallel/test-tls-addca.js
View file
Open in desktop
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);