diff --git a/security/manager/ssl/NSSSocketControl.cpp b/security/manager/ssl/NSSSocketControl.cpp index 4698e0b15ee5..599044b89c1d 100644 --- a/security/manager/ssl/NSSSocketControl.cpp +++ b/security/manager/ssl/NSSSocketControl.cpp @@ -14,6 +14,7 @@ #include "mozilla/dom/Promise.h" #include "mozilla/glean/SecurityManagerSslMetrics.h" #include "nsNSSCallbacks.h" +#include "nsNSSComponent.h" #include "nsProxyRelease.h" using namespace mozilla; @@ -460,6 +461,9 @@ void NSSSocketControl::ClientAuthCertificateSelected( const_cast(certBytes.Elements()), static_cast(certBytes.Length()), }; + // Ensure that osclientcerts (or ipcclientcerts, in the socket process) will + // populate its list of certificates and keys. + AutoSearchingForClientAuthCertificates _; UniqueCERTCertificate cert(CERT_NewTempCertificate( CERT_GetDefaultCertDB(), &certItem, nullptr, false, true)); UniqueSECKEYPrivateKey key; diff --git a/security/manager/ssl/TLSClientAuthCertSelection.cpp b/security/manager/ssl/TLSClientAuthCertSelection.cpp index d6e845d4f7ad..0a168b2d93dc 100644 --- a/security/manager/ssl/TLSClientAuthCertSelection.cpp +++ b/security/manager/ssl/TLSClientAuthCertSelection.cpp @@ -771,16 +771,6 @@ SECStatus SSLGetClientAuthDataHook(void* arg, PRFileDesc* socket, nsTArray> caNames(CollectCANames(caNamesDecoded)); - // Currently, the IPC client certs module only refreshes its view of - // available certificates and keys if the platform issues a search for all - // certificates or keys. In the socket process, such a search may not have - // happened, so this ensures it has. - // Additionally, instantiating certificates in NSS is not thread-safe and has - // performance implications, so search for them here (on the socket thread) - // when not in the socket process. - UniqueCERTCertList potentialClientCertificates( - FindClientCertificatesWithPrivateKeys()); - RefPtr continuation( new ClientAuthCertificateSelected(info)); // If this is the socket process, dispatch an IPC call to select a client @@ -866,6 +856,11 @@ SECStatus SSLGetClientAuthDataHook(void* arg, PRFileDesc* socket, return SECWouldBlock; } + // Instantiating certificates in NSS is not thread-safe and has performance + // implications, so search for them here (on the socket thread). + UniqueCERTCertList potentialClientCertificates( + FindClientCertificatesWithPrivateKeys()); + nsTArray>> potentialClientCertificateChains; FilterPotentialClientCertificatesByCANames(potentialClientCertificates, caNames, enterpriseCertificates, diff --git a/security/manager/ssl/nsNSSCertificateDB.cpp b/security/manager/ssl/nsNSSCertificateDB.cpp index 84faba6a3472..8a31c85b587d 100644 --- a/security/manager/ssl/nsNSSCertificateDB.cpp +++ b/security/manager/ssl/nsNSSCertificateDB.cpp @@ -1185,6 +1185,8 @@ NS_IMETHODIMP nsNSSCertificateDB::AsPKCS7Blob( NS_IMETHODIMP nsNSSCertificateDB::GetCerts(nsTArray>& _retval) { + AutoSearchingForClientAuthCertificates _; + nsresult rv = BlockUntilLoadableCertsLoaded(); if (NS_FAILED(rv)) { return rv; diff --git a/security/manager/ssl/nsNSSComponent.cpp b/security/manager/ssl/nsNSSComponent.cpp index 9209fd450e12..bf6ee61d3043 100644 --- a/security/manager/ssl/nsNSSComponent.cpp +++ b/security/manager/ssl/nsNSSComponent.cpp @@ -4,6 +4,8 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +#include + #include "nsNSSComponent.h" #include "BinaryPath.h" @@ -1931,6 +1933,25 @@ nsNSSComponent::AsyncClearSSLExternalAndInternalSessionCache( return NS_OK; } +std::atomic sSearchingForClientAuthCertificates{false}; + +extern "C" { + +bool IsGeckoSearchingForClientAuthCertificates() { + return sSearchingForClientAuthCertificates; +} +} + +AutoSearchingForClientAuthCertificates:: + AutoSearchingForClientAuthCertificates() { + sSearchingForClientAuthCertificates = true; +} + +AutoSearchingForClientAuthCertificates:: + ~AutoSearchingForClientAuthCertificates() { + sSearchingForClientAuthCertificates = false; +} + namespace mozilla { namespace psm { @@ -1978,6 +1999,7 @@ static inline void CopyCertificatesTo(UniqueCERTCertList& from, UniqueCERTCertList FindClientCertificatesWithPrivateKeys() { MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("FindClientCertificatesWithPrivateKeys")); + AutoSearchingForClientAuthCertificates _; (void)BlockUntilLoadableCertsLoaded(); (void)CheckForSmartCardChanges(); diff --git a/security/manager/ssl/nsNSSComponent.h b/security/manager/ssl/nsNSSComponent.h index 8ddf39bc742c..783dbf9fc760 100644 --- a/security/manager/ssl/nsNSSComponent.h +++ b/security/manager/ssl/nsNSSComponent.h @@ -51,6 +51,26 @@ bool HandleTLSPrefChange(const nsCString& aPref); void SetValidationOptionsCommon(); void PrepareForShutdownInSocketProcess(); +// RAII helper class to indicate that gecko is searching for client auth +// certificates. Will automatically stop indicating that a search is happening +// when it goes out of scope. +// osclientcerts (or ipcclientcerts, in the socket process) will call +// IsGeckoSearchingForClientAuthCertificates() to determine if gecko is +// searching for client auth certificates. If so, the module knows to refresh +// its list of certificates and keys (which can be costly). +// In theory, two separate threads could both create a +// AutoSearchingForClientAuthCertificates at overlapping times. If one goes out +// of scope sooner than the other, IsGeckoSearchingForClientAuthCertificates() +// could potentially incorrectly return false for the slower thread. However, +// as long as the faster thread has ensured that osclientcerts/ipcclientcerts +// has updated its list of known certificates, a second search would be +// redundant anyway, so it doesn't matter. +class AutoSearchingForClientAuthCertificates { + public: + AutoSearchingForClientAuthCertificates(); + ~AutoSearchingForClientAuthCertificates(); +}; + // Implementation of the PSM component interface. class nsNSSComponent final : public nsINSSComponent, public nsIObserver { public: diff --git a/security/manager/ssl/rsclientcerts/src/manager.rs b/security/manager/ssl/rsclientcerts/src/manager.rs index 507bed3a8308..c92e4dbd2684 100644 --- a/security/manager/ssl/rsclientcerts/src/manager.rs +++ b/security/manager/ssl/rsclientcerts/src/manager.rs @@ -12,7 +12,10 @@ use std::time::{Duration, Instant}; use crate::error::{Error, ErrorType}; use crate::error_here; -use crate::util::*; + +extern "C" { + fn IsGeckoSearchingForClientAuthCertificates() -> bool; +} /// Helper enum to differentiate between sessions on the modern slot and sessions on the legacy /// slot. The former is for EC keys and RSA keys that can be used with RSA-PSS whereas the latter is @@ -321,37 +324,6 @@ impl ManagerProxy { } } -// Determines if the attributes of a given search correspond to NSS looking for all certificates or -// private keys. Returns true if so, and false otherwise. -// These searches are of the form: -// { { type: CKA_TOKEN, value: [1] }, -// { type: CKA_CLASS, value: [CKO_CERTIFICATE or CKO_PRIVATE_KEY, as serialized bytes] } } -// (although not necessarily in that order - see nssToken_TraverseCertificates and -// nssToken_FindPrivateKeys) -fn search_is_for_all_certificates_or_keys( - attrs: &[(CK_ATTRIBUTE_TYPE, Vec)], -) -> Result { - if attrs.len() != 2 { - return Ok(false); - } - let token_bytes = vec![1_u8]; - let mut found_token = false; - let cko_certificate_bytes = serialize_uint(CKO_CERTIFICATE)?; - let cko_private_key_bytes = serialize_uint(CKO_PRIVATE_KEY)?; - let mut found_certificate_or_private_key = false; - for (attr_type, attr_value) in attrs.iter() { - if attr_type == &CKA_TOKEN && attr_value == &token_bytes { - found_token = true; - } - if attr_type == &CKA_CLASS - && (attr_value == &cko_certificate_bytes || attr_value == &cko_private_key_bytes) - { - found_certificate_or_private_key = true; - } - } - Ok(found_token && found_certificate_or_private_key) -} - const SUPPORTED_ATTRIBUTES: &[CK_ATTRIBUTE_TYPE] = &[ CKA_CLASS, CKA_TOKEN, @@ -439,7 +411,7 @@ pub struct Manager { /// The next object handle to hand out. next_handle: CK_OBJECT_HANDLE, /// The last time the implementation looked for new objects in the backend. - /// The implementation does this search no more than once every 3 seconds. + /// The implementation does this search no more than once every 2 seconds. last_scan_time: Option, backend: B, } @@ -460,14 +432,14 @@ impl Manager { } } - /// When a new search session is opened (provided at least 3 seconds have elapsed since the + /// When a new search session is opened (provided at least 2 seconds have elapsed since the /// last session was opened), this searches for certificates and keys to expose. We /// de-duplicate previously-found certificates and keys by keeping track of their IDs. fn maybe_find_new_objects(&mut self) -> Result<(), Error> { let now = Instant::now(); match self.last_scan_time { Some(last_scan_time) => { - if now.duration_since(last_scan_time) < Duration::new(3, 0) { + if now.duration_since(last_scan_time) < Duration::new(2, 0) { return Ok(()); } } @@ -552,12 +524,11 @@ impl Manager { return Ok(()); } } - // When NSS wants to find all certificates or all private keys, it will perform a search - // with a particular set of attributes. This implementation uses these searches as an - // indication for the backend to re-scan for new objects from tokens that may have been - // inserted or certificates that may have been imported into the OS. Since these searches - // are relatively rare, this minimizes the impact of doing these re-scans. - if search_is_for_all_certificates_or_keys(&attrs)? { + // Only search for new objects when gecko has indicated that it is looking for client + // authentication certificates (or all certificates). + // Since these searches are relatively rare, this minimizes the impact of doing these + // re-scans. + if unsafe { IsGeckoSearchingForClientAuthCertificates() } { self.maybe_find_new_objects()?; } let mut handles = Vec::new();