Skip to content

Commit f069246

Browse files
bnoordhuisjuanarbol
authored andcommitted
src: fix tls certificate root store data race
OpenSSL internally synchronizes access to the X509_STORE. Creation of the global root store in Node was not properly synchronized, however, introducing the possibility of data races when multiple threads try to create it concurrently. This commit coincidentally removes the last call to the thread-unsafe ERR_error_string() function. Fixes: #45743 PR-URL: #45767 Reviewed-By: Anna Henningsen <anna@addaleax.net>
1 parent d94dba9 commit f069246

File tree

1 file changed

+23
-29
lines changed

1 file changed

+23
-29
lines changed

src/crypto/crypto_context.cc

+23-29
Original file line numberDiff line numberDiff line change
@@ -47,10 +47,14 @@ static const char* const root_certs[] = {
4747

4848
static const char system_cert_path[] = NODE_OPENSSL_SYSTEM_CERT_PATH;
4949

50-
static X509_STORE* root_cert_store;
51-
5250
static bool extra_root_certs_loaded = false;
5351

52+
inline X509_STORE* GetOrCreateRootCertStore() {
53+
// Guaranteed thread-safe by standard, just don't use -fno-threadsafe-statics.
54+
static X509_STORE* store = NewRootCertStore();
55+
return store;
56+
}
57+
5458
// Takes a string or buffer and loads it into a BIO.
5559
// Caller responsible for BIO_free_all-ing the returned object.
5660
BIOPointer LoadBIO(Environment* env, Local<Value> v) {
@@ -701,7 +705,7 @@ void SecureContext::AddCACert(const FunctionCallbackInfo<Value>& args) {
701705
X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_.get());
702706
while (X509Pointer x509 = X509Pointer(PEM_read_bio_X509_AUX(
703707
bio.get(), nullptr, NoPasswordCallback, nullptr))) {
704-
if (cert_store == root_cert_store) {
708+
if (cert_store == GetOrCreateRootCertStore()) {
705709
cert_store = NewRootCertStore();
706710
SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store);
707711
}
@@ -731,7 +735,7 @@ void SecureContext::AddCRL(const FunctionCallbackInfo<Value>& args) {
731735
return THROW_ERR_CRYPTO_OPERATION_FAILED(env, "Failed to parse CRL");
732736

733737
X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_.get());
734-
if (cert_store == root_cert_store) {
738+
if (cert_store == GetOrCreateRootCertStore()) {
735739
cert_store = NewRootCertStore();
736740
SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store);
737741
}
@@ -745,14 +749,10 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args) {
745749
SecureContext* sc;
746750
ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder());
747751
ClearErrorOnReturn clear_error_on_return;
748-
749-
if (root_cert_store == nullptr) {
750-
root_cert_store = NewRootCertStore();
751-
}
752-
752+
X509_STORE* store = GetOrCreateRootCertStore();
753753
// Increment reference count so global store is not deleted along with CTX.
754-
X509_STORE_up_ref(root_cert_store);
755-
SSL_CTX_set_cert_store(sc->ctx_.get(), root_cert_store);
754+
X509_STORE_up_ref(store);
755+
SSL_CTX_set_cert_store(sc->ctx_.get(), store);
756756
}
757757

758758
void SecureContext::SetCipherSuites(const FunctionCallbackInfo<Value>& args) {
@@ -1025,7 +1025,7 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
10251025
for (int i = 0; i < sk_X509_num(extra_certs.get()); i++) {
10261026
X509* ca = sk_X509_value(extra_certs.get(), i);
10271027

1028-
if (cert_store == root_cert_store) {
1028+
if (cert_store == GetOrCreateRootCertStore()) {
10291029
cert_store = NewRootCertStore();
10301030
SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store);
10311031
}
@@ -1328,24 +1328,18 @@ unsigned long AddCertsFromFile( // NOLINT(runtime/int)
13281328

13291329
// UseExtraCaCerts is called only once at the start of the Node.js process.
13301330
void UseExtraCaCerts(const std::string& file) {
1331+
if (file.empty()) return;
13311332
ClearErrorOnReturn clear_error_on_return;
1332-
1333-
if (root_cert_store == nullptr) {
1334-
root_cert_store = NewRootCertStore();
1335-
1336-
if (!file.empty()) {
1337-
unsigned long err = AddCertsFromFile( // NOLINT(runtime/int)
1338-
root_cert_store,
1339-
file.c_str());
1340-
if (err) {
1341-
fprintf(stderr,
1342-
"Warning: Ignoring extra certs from `%s`, load failed: %s\n",
1343-
file.c_str(),
1344-
ERR_error_string(err, nullptr));
1345-
} else {
1346-
extra_root_certs_loaded = true;
1347-
}
1348-
}
1333+
X509_STORE* store = GetOrCreateRootCertStore();
1334+
if (auto err = AddCertsFromFile(store, file.c_str())) {
1335+
char buf[256];
1336+
ERR_error_string_n(err, buf, sizeof(buf));
1337+
fprintf(stderr,
1338+
"Warning: Ignoring extra certs from `%s`, load failed: %s\n",
1339+
file.c_str(),
1340+
buf);
1341+
} else {
1342+
extra_root_certs_loaded = true;
13491343
}
13501344
}
13511345

0 commit comments

Comments
 (0)