crypto: cleanup root certificates and skip PEM deserialization #56999
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,6 +74,10 @@ static const char system_cert_path[] = NODE_OPENSSL_SYSTEM_CERT_PATH; | |
|
|
||
| static std::string extra_root_certs_file; // NOLINT(runtime/string) | ||
|
|
||
| static std::atomic<bool> has_cached_bundled_root_certs{false}; | ||
| static std::atomic<bool> has_cached_system_root_certs{false}; | ||
| static std::atomic<bool> has_cached_extra_root_certs{false}; | ||
|
|
||
| X509_STORE* GetOrCreateRootCertStore() { | ||
| // Guaranteed thread-safe by standard, just don't use -fno-threadsafe-statics. | ||
| static X509_STORE* store = NewRootCertStore(); | ||
|
|
@@ -261,35 +265,6 @@ bool isSelfIssued(X509* cert) { | |
| return X509_NAME_cmp(subject, issuer) == 0; | ||
| } | ||
|
|
||
| // The following code is loosely based on | ||
| // https://github.com/chromium/chromium/blob/54bd8e3/net/cert/internal/trust_store_mac.cc | ||
| // and | ||
|
|
@@ -482,7 +457,7 @@ bool IsCertificateTrustedForPolicy(X509* cert, SecCertificateRef ref) { | |
| } | ||
|
|
||
| void ReadMacOSKeychainCertificates( | ||
| std::vector<X509*>* system_root_certificates_X509) { | ||
| CFTypeRef search_keys[] = {kSecClass, kSecMatchLimit, kSecReturnRef}; | ||
| CFTypeRef search_values[] = { | ||
| kSecClassCertificate, kSecMatchLimitAll, kCFBooleanTrue}; | ||
|
|
@@ -504,7 +479,6 @@ void ReadMacOSKeychainCertificates( | |
|
|
||
| CFIndex count = CFArrayGetCount(curr_anchors); | ||
|
|
||
| for (int i = 0; i < count; ++i) { | ||
| SecCertificateRef cert_ref = reinterpret_cast<SecCertificateRef>( | ||
| const_cast<void*>(CFArrayGetValueAtIndex(curr_anchors, i))); | ||
|
|
@@ -521,13 +495,10 @@ void ReadMacOSKeychainCertificates( | |
| CFRelease(der_data); | ||
| bool is_valid = IsCertificateTrustedForPolicy(cert, cert_ref); | ||
| if (is_valid) { | ||
| system_root_certificates_X509->emplace_back(cert); | ||
| } | ||
| } | ||
| CFRelease(curr_anchors); | ||
| } | ||
| #endif // __APPLE__ | ||
|
|
||
|
|
@@ -596,7 +567,7 @@ bool IsCertTrustedForServerAuth(PCCERT_CONTEXT cert) { | |
| return false; | ||
| } | ||
|
|
||
| void GatherCertsForLocation(std::vector<X509*>* vector, | ||
| DWORD location, | ||
| LPCWSTR store_name) { | ||
| if (!(location == CERT_SYSTEM_STORE_LOCAL_MACHINE || | ||
|
|
@@ -639,114 +610,142 @@ void GatherCertsForLocation(std::vector<X509Pointer>* vector, | |
| } | ||
|
|
||
| void ReadWindowsCertificates( | ||
| std::vector<X509*>* system_root_certificates_X509) { | ||
| // TODO(joyeecheung): match Chromium's policy, collect more certificates | ||
| // from user-added CAs and support disallowed (revoked) certificates. | ||
|
|
||
| // Grab the user-added roots. | ||
| GatherCertsForLocation( | ||
| system_root_certificates_X509, CERT_SYSTEM_STORE_LOCAL_MACHINE, L"ROOT"); | ||
| GatherCertsForLocation(system_root_certificates_X509, | ||
| CERT_SYSTEM_STORE_LOCAL_MACHINE_GROUP_POLICY, | ||
| L"ROOT"); | ||
| GatherCertsForLocation(system_root_certificates_X509, | ||
| CERT_SYSTEM_STORE_LOCAL_MACHINE_ENTERPRISE, | ||
| L"ROOT"); | ||
| GatherCertsForLocation( | ||
| system_root_certificates_X509, CERT_SYSTEM_STORE_CURRENT_USER, L"ROOT"); | ||
| GatherCertsForLocation(system_root_certificates_X509, | ||
| CERT_SYSTEM_STORE_CURRENT_USER_GROUP_POLICY, | ||
| L"ROOT"); | ||
|
|
||
| // Grab the user-added trusted server certs. Trusted end-entity certs are | ||
| // only allowed for server auth in the "local machine" store, but not in the | ||
| // "current user" store. | ||
| GatherCertsForLocation(system_root_certificates_X509, | ||
| CERT_SYSTEM_STORE_LOCAL_MACHINE, | ||
| L"TrustedPeople"); | ||
| GatherCertsForLocation(system_root_certificates_X509, | ||
| CERT_SYSTEM_STORE_LOCAL_MACHINE_GROUP_POLICY, | ||
| L"TrustedPeople"); | ||
| GatherCertsForLocation(system_root_certificates_X509, | ||
| CERT_SYSTEM_STORE_LOCAL_MACHINE_ENTERPRISE, | ||
| L"TrustedPeople"); | ||
| } | ||
| #endif | ||
|
|
||
| static std::vector<X509*> InitializeBundledRootCertificates() { | ||
| // Read the bundled certificates in node_root_certs.h into | ||
| // bundled_root_certs_vector. | ||
| std::vector<X509*> bundled_root_certs; | ||
joyeecheung marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| size_t bundled_root_cert_count = arraysize(root_certs); | ||
| for (size_t i = 0; i < bundled_root_cert_count; i++) { | ||
| X509* x509 = PEM_read_bio_X509( | ||
| NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i])).get(), | ||
|
There was a problem hiding this comment. Can we avoid strlen() from this function somehow? There was a problem hiding this comment. I don't think so, not without changing the perl script that generates the root certificates. Note that this code is not new, it just reverts the copying added by #56599 (which still leads to something like strlen in the string constructor). It has been using strlen since Node.js v0.x days or probably ever since Node.js started implementing TLS..(so > 10 years) |
||
| nullptr, // no re-use of X509 structure | ||
| NoPasswordCallback, | ||
| nullptr); // no callback data | ||
|
|
||
| // Parse errors from the built-in roots are fatal. | ||
| CHECK_NOT_NULL(x509); | ||
|
|
||
| bundled_root_certs.push_back(x509); | ||
| } | ||
| return bundled_root_certs; | ||
| } | ||
|
|
||
|
|
||
| // TODO(joyeecheung): it is a bit excessive to do this PEM -> X509 | ||
| // dance when we could've just pass everything around in binary. Change the | ||
| // root_certs to be embedded as DER so that we can save the serialization | ||
| // and deserialization. | ||
| static std::vector<X509*>& GetBundledRootCertificates() { | ||
| // Use function-local static to guarantee thread safety. | ||
| static std::vector<X509*> bundled_root_certs = | ||
| InitializeBundledRootCertificates(); | ||
| has_cached_bundled_root_certs.store(true); | ||
| return bundled_root_certs; | ||
| } | ||
|
|
||
| static std::vector<X509*> InitializeSystemStoreCertificates() { | ||
| std::vector<X509*> system_store_certs; | ||
| #ifdef __APPLE__ | ||
| ReadMacOSKeychainCertificates(&system_store_certs); | ||
| #endif | ||
| #ifdef _WIN32 | ||
| ReadWindowsCertificates(&system_store_certs); | ||
| #endif | ||
| return system_store_certs; | ||
| } | ||
|
|
||
| static std::vector<X509*>& GetSystemStoreRootCertificates() { | ||
| // Use function-local static to guarantee thread safety. | ||
| static std::vector<X509*> system_store_certs = | ||
| InitializeSystemStoreCertificates(); | ||
| has_cached_system_root_certs.store(true); | ||
| return system_store_certs; | ||
| } | ||
|
|
||
| static std::vector<X509*> InitializeExtraCACertificates() { | ||
| std::vector<X509*> extra_certs; | ||
| unsigned long err = LoadCertsFromFile( // NOLINT(runtime/int) | ||
| &extra_certs, | ||
| extra_root_certs_file.c_str()); | ||
| if (err) { | ||
| char buf[256]; | ||
| ERR_error_string_n(err, buf, sizeof(buf)); | ||
| fprintf(stderr, | ||
| "Warning: Ignoring extra certs from `%s`, load failed: %s\n", | ||
| extra_root_certs_file.c_str(), | ||
| buf); | ||
| } | ||
| return extra_certs; | ||
| } | ||
|
|
||
| static std::vector<X509*>& GetExtraCACertificates() { | ||
| // Use function-local static to guarantee thread safety. | ||
| static std::vector<X509*> extra_certs = InitializeExtraCACertificates(); | ||
| has_cached_extra_root_certs.store(true); | ||
| return extra_certs; | ||
| } | ||
|
|
||
| // Due to historical reasons the various options of CA certificates | ||
| // may invalid one another. The current rule is: | ||
| // 1. If the configure-time option --openssl-use-def-ca-store is NOT used | ||
| // (default): | ||
| // a. If the runtime option --use-openssl-ca is used, load the | ||
| // CA certificates from the default locations respected by OpenSSL. | ||
| // b. Otherwise, --use-bundled-ca is assumed to be the default, and we | ||
| // use the bundled CA certificates. | ||
| // 2. If the configure-time option --openssl-use-def-ca-store IS used, | ||
| // --use-openssl-ca is assumed to be the default, with the default | ||
| // location set to the path specified by the configure-time option. | ||
| // 3. --use-openssl-ca and --use-bundled-ca are mutually exclusive. | ||
| // 4. --use-openssl-ca and --use-system-ca are mutually exclusive. | ||
| // 5. --use-bundled-ca and --use-system-ca can be used together. | ||
| // The certificates can be combined. | ||
| // 6. Independent of all other flags, NODE_EXTRA_CA_CERTS always | ||
| // adds extra certificates from the specified path, so it works | ||
| // with all the other flags. | ||
| // 7. Certificates from --use-bundled-ca, --use-system-ca and | ||
| // NODE_EXTRA_CA_CERTS are cached after first load. Certificates | ||
| // from --use-system-ca are not cached and always reloaded from | ||
| // disk. | ||
| // TODO(joyeecheung): maybe these rules need a bit of consolidation? | ||
| X509_STORE* NewRootCertStore() { | ||
| X509_STORE* store = X509_STORE_new(); | ||
| CHECK_NOT_NULL(store); | ||
|
|
||
| if (*system_cert_path != '\0') { | ||
| ERR_set_mark(); | ||
| X509_STORE_load_locations(store, system_cert_path, nullptr); | ||
|
|
@@ -756,15 +755,45 @@ X509_STORE* NewRootCertStore() { | |
| Mutex::ScopedLock cli_lock(node::per_process::cli_options_mutex); | ||
| if (per_process::cli_options->ssl_openssl_cert_store) { | ||
| CHECK_EQ(1, X509_STORE_set_default_paths(store)); | ||
| } else { | ||
| for (X509* cert : GetBundledRootCertificates()) { | ||
| CHECK_EQ(1, X509_STORE_add_cert(store, cert)); | ||
| } | ||
| if (per_process::cli_options->use_system_ca) { | ||
| for (X509* cert : GetSystemStoreRootCertificates()) { | ||
| CHECK_EQ(1, X509_STORE_add_cert(store, cert)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!extra_root_certs_file.empty()) { | ||
| for (X509* cert : GetExtraCACertificates()) { | ||
| CHECK_EQ(1, X509_STORE_add_cert(store, cert)); | ||
| } | ||
| } | ||
|
|
||
| return store; | ||
| } | ||
|
|
||
| void CleanupCachedRootCertificates() { | ||
| if (has_cached_bundled_root_certs.load()) { | ||
| for (X509* cert : GetBundledRootCertificates()) { | ||
| X509_free(cert); | ||
| } | ||
| } | ||
| if (has_cached_system_root_certs.load()) { | ||
| for (X509* cert : GetSystemStoreRootCertificates()) { | ||
| X509_free(cert); | ||
| } | ||
| } | ||
|
|
||
| if (has_cached_system_root_certs.load()) { | ||
| for (X509* cert : GetExtraCACertificates()) { | ||
| X509_free(cert); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void GetRootCertificates(const FunctionCallbackInfo<Value>& args) { | ||
| Environment* env = Environment::GetCurrent(args); | ||
| Local<Value> result[arraysize(root_certs)]; | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need thread safety here? Wouldn't the bundled root certs be only cached once on the initial boot, or am I missing something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are initialized when the root store is first used e.g. when a TLS connection is first made. Not when e.g. the program is started to read a file and does not need to use TLS at all. If multiple workers are starting TLS connection at the same time then there can be a race in initialization.