diff --git a/rustls-platform-verifier/src/tests/verification_mock/mod.rs b/rustls-platform-verifier/src/tests/verification_mock/mod.rs index 59e322a..9fbc8ac 100644 --- a/rustls-platform-verifier/src/tests/verification_mock/mod.rs +++ b/rustls-platform-verifier/src/tests/verification_mock/mod.rs @@ -43,7 +43,7 @@ macro_rules! mock_root_test_cases { } paste::paste!{ - #[cfg(all($target, not(windows), not(target_os = "android")))] + #[cfg(all($target, not(target_os = "android")))] #[test] pub fn [<$name _extra>](){ super::[<$name _extra>]() @@ -60,7 +60,7 @@ macro_rules! mock_root_test_cases { $name, paste::paste!{ - #[cfg(all($target, not(windows), not(target_os = "android")))] + #[cfg(all($target, not(target_os = "android")))] [<$name _extra>] } @@ -77,7 +77,7 @@ macro_rules! mock_root_test_cases { } paste::paste!{ - #[cfg(all($target, not(windows), not(target_os = "android")))] + #[cfg(all($target, not(target_os = "android")))] pub(super) fn [<$name _extra>]() { test_with_mock_root(&$test_case, Roots::ExtraAndPlatform); } @@ -116,7 +116,8 @@ pub(super) fn verification_without_mock_root() { .iter() .cloned() .collect(), - ); + ) + .unwrap(); #[cfg(not(target_os = "freebsd"))] let verifier = Verifier::new(); @@ -336,8 +337,8 @@ fn test_with_mock_root( let verifier = match root_src { Roots::OnlyExtra => Verifier::new_with_fake_root(ROOT1), // TODO: time - #[cfg(all(unix, not(target_os = "android")))] - Roots::ExtraAndPlatform => Verifier::new_with_extra_roots(vec![ROOT1.into()]), + #[cfg(not(target_os = "android"))] + Roots::ExtraAndPlatform => Verifier::new_with_extra_roots(vec![ROOT1.into()]).unwrap(), }; let mut chain = test_case .chain @@ -379,6 +380,6 @@ enum Roots { /// Test with loading the extra roots and the platform trust store. /// /// Right now, not all platforms are supported. - #[cfg(all(unix, not(target_os = "android")))] + #[cfg(not(target_os = "android"))] ExtraAndPlatform, } diff --git a/rustls-platform-verifier/src/tests/verification_real_world/mod.rs b/rustls-platform-verifier/src/tests/verification_real_world/mod.rs index 532be9e..430255d 100644 --- a/rustls-platform-verifier/src/tests/verification_real_world/mod.rs +++ b/rustls-platform-verifier/src/tests/verification_real_world/mod.rs @@ -133,7 +133,8 @@ fn real_world_test(test_case: &TestCase) { .iter() .cloned() .collect(), - ); + ) + .unwrap(); #[cfg(not(target_os = "freebsd"))] let verifier = Verifier::new(); diff --git a/rustls-platform-verifier/src/verification/apple.rs b/rustls-platform-verifier/src/verification/apple.rs index 5dc3a22..07bd944 100644 --- a/rustls-platform-verifier/src/verification/apple.rs +++ b/rustls-platform-verifier/src/verification/apple.rs @@ -45,10 +45,10 @@ fn system_time_to_cfdate(time: pki_types::UnixTime) -> Result pub struct Verifier { /// Extra trust anchors to add to the verifier above and beyond those provided by /// the system-provided trust stores. - extra_roots: Vec>, + extra_roots: Vec, /// Testing only: The root CA certificate to trust. #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] - test_only_root_ca_override: Option>, + test_only_root_ca_override: Option, pub(super) crypto_provider: OnceCell>, } @@ -72,13 +72,22 @@ impl Verifier { /// facilities with the addition of extra root certificates to trust. /// /// See [Verifier::new] for the external requirements the verifier needs. - pub fn new_with_extra_roots(roots: Vec>) -> Self { - Self { - extra_roots: roots, + pub fn new_with_extra_roots( + roots: Vec>, + ) -> Result { + let extra_roots = roots + .into_iter() + .map(|root| { + SecCertificate::from_der(&root) + .map_err(|_| TlsError::InvalidCertificate(CertificateError::BadEncoding)) + }) + .collect::, _>>()?; + Ok(Self { + extra_roots, #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] test_only_root_ca_override: None, crypto_provider: OnceCell::new(), - } + }) } /// Creates a test-only TLS certificate verifier which trusts our fake root CA cert. @@ -86,7 +95,7 @@ impl Verifier { pub(crate) fn new_with_fake_root(root: &[u8]) -> Self { Self { extra_roots: Vec::new(), - test_only_root_ca_override: Some(root.into()), + test_only_root_ca_override: Some(SecCertificate::from_der(root).unwrap()), crypto_provider: OnceCell::new(), } } @@ -141,29 +150,24 @@ impl Verifier { .map_err(|e| invalid_certificate(e.to_string()))?; } - let raw_extra_roots = self.extra_roots.iter(); + #[cfg(not(any(test, feature = "ffi-testing", feature = "dbg")))] + let extra_roots = self.extra_roots.as_slice(); #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] - let extra_root = self - .test_only_root_ca_override - .as_ref() - .map(|root| pki_types::CertificateDer::from_slice(root)); - + let extra_roots: Vec<_> = self + .extra_roots + .iter() + .chain(self.test_only_root_ca_override.as_ref()) + .cloned() + .collect(); #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] - let raw_extra_roots = raw_extra_roots.chain(&extra_root).to_owned(); - - let extra_roots = raw_extra_roots - .map(|root| { - SecCertificate::from_der(root) - .map_err(|_| TlsError::InvalidCertificate(CertificateError::BadEncoding)) - }) - .collect::, _>>()?; + let extra_roots = extra_roots.as_slice(); // If any extra roots were provided by the user (or tests), provide them to the trust // evaluation regardless of their system trust settings or status. if !extra_roots.is_empty() { trust_evaluation - .set_anchor_certificates(&extra_roots) + .set_anchor_certificates(extra_roots) .map_err(|e| TlsError::Other(OtherError(Arc::new(e))))?; // We want to trust both the system-installed and the extra roots. This must be set diff --git a/rustls-platform-verifier/src/verification/others.rs b/rustls-platform-verifier/src/verification/others.rs index a145031..300e8f3 100644 --- a/rustls-platform-verifier/src/verification/others.rs +++ b/rustls-platform-verifier/src/verification/others.rs @@ -53,8 +53,10 @@ impl Verifier { /// Creates a new verifier whose certificate validation is provided by /// WebPKI, using root certificates provided by the platform and augmented by /// the provided extra root certificates. - pub fn new_with_extra_roots(roots: Vec>) -> Self { - Self { + pub fn new_with_extra_roots( + roots: Vec>, + ) -> Result { + Ok(Self { inner: OnceCell::new(), extra_roots: roots .into_iter() @@ -66,7 +68,7 @@ impl Verifier { #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] test_only_root_ca_override: None, crypto_provider: OnceCell::new(), - } + }) } /// Creates a test-only TLS certificate verifier which trusts our fake root CA cert. diff --git a/rustls-platform-verifier/src/verification/windows.rs b/rustls-platform-verifier/src/verification/windows.rs index 3ff76dd..0803082 100644 --- a/rustls-platform-verifier/src/verification/windows.rs +++ b/rustls-platform-verifier/src/verification/windows.rs @@ -40,17 +40,18 @@ use windows_sys::Win32::{ CERT_E_WRONG_USAGE, CRYPT_E_REVOKED, FILETIME, TRUE, }, Security::Cryptography::{ - CertAddEncodedCertificateToStore, CertCloseStore, CertFreeCertificateChain, - CertFreeCertificateChainEngine, CertFreeCertificateContext, CertGetCertificateChain, - CertOpenStore, CertSetCertificateContextProperty, CertVerifyCertificateChainPolicy, - HTTPSPolicyCallbackData, AUTHTYPE_SERVER, CERT_CHAIN_CACHE_END_CERT, CERT_CHAIN_CONTEXT, + CertAddEncodedCertificateToStore, CertCloseStore, CertCreateCertificateChainEngine, + CertFreeCertificateChain, CertFreeCertificateChainEngine, CertFreeCertificateContext, + CertGetCertificateChain, CertOpenStore, CertSetCertificateContextProperty, + CertVerifyCertificateChainPolicy, HTTPSPolicyCallbackData, AUTHTYPE_SERVER, + CERT_CHAIN_CACHE_END_CERT, CERT_CHAIN_CONTEXT, CERT_CHAIN_ENGINE_CONFIG, CERT_CHAIN_POLICY_IGNORE_ALL_REV_UNKNOWN_FLAGS, CERT_CHAIN_POLICY_PARA, CERT_CHAIN_POLICY_SSL, CERT_CHAIN_POLICY_STATUS, CERT_CHAIN_REVOCATION_ACCUMULATIVE_TIMEOUT, CERT_CHAIN_REVOCATION_CHECK_END_CERT, CERT_CONTEXT, CERT_OCSP_RESPONSE_PROP_ID, CERT_SET_PROPERTY_IGNORE_PERSIST_ERROR_FLAG, CERT_STORE_ADD_ALWAYS, CERT_STORE_DEFER_CLOSE_UNTIL_LAST_FREE_FLAG, CERT_STORE_PROV_MEMORY, - CERT_STRONG_SIGN_PARA, CERT_USAGE_MATCH, CRYPT_INTEGER_BLOB, CTL_USAGE, - USAGE_MATCH_TYPE_AND, X509_ASN_ENCODING, + CERT_STRONG_SIGN_PARA, CERT_TRUST_IS_PARTIAL_CHAIN, CERT_USAGE_MATCH, CRYPT_INTEGER_BLOB, + CTL_USAGE, USAGE_MATCH_TYPE_AND, X509_ASN_ENCODING, }, }; @@ -76,8 +77,6 @@ struct CERT_CHAIN_PARA { } use crate::verification::invalid_certificate; -#[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] -use windows_sys::Win32::Security::Cryptography::CERT_CHAIN_ENGINE_CONFIG; // SAFETY: see method implementation unsafe impl ZeroedWithSize for CERT_CHAIN_PARA { @@ -110,7 +109,6 @@ unsafe impl ZeroedWithSize for CERT_CHAIN_POLICY_PARA { } } -#[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] // SAFETY: see method implementation unsafe impl ZeroedWithSize for CERT_CHAIN_ENGINE_CONFIG { fn zeroed_with_size() -> Self { @@ -136,7 +134,7 @@ impl CertChain { extra_params.pwszServerName = server_null_terminated.as_mut_ptr(); let mut params = CERT_CHAIN_POLICY_PARA::zeroed_with_size(); - // Ignore any errors when trying to obtain OCSP recovcation information. + // Ignore any errors when trying to obtain OCSP revocation information. // This is also done in OpenSSL, Secure Transport from Apple, etc. params.dwFlags = CERT_CHAIN_POLICY_IGNORE_ALL_REV_UNKNOWN_FLAGS; // `extra_params` outlives `params`. @@ -214,6 +212,90 @@ impl Drop for Certificate { } } +#[derive(Debug)] +struct CertEngine { + inner: NonNull, // HCERTENGINECONTEXT +} + +impl CertEngine { + fn new_with_extra_roots( + roots: &[pki_types::CertificateDer<'static>], + ) -> Result { + let mut exclusive_store = CertificateStore::new()?; + for root in roots { + exclusive_store.add_cert(root)?; + } + + let mut config = CERT_CHAIN_ENGINE_CONFIG::zeroed_with_size(); + config.hExclusiveRoot = exclusive_store.inner.as_ptr(); + + let mut engine = 0; + // SAFETY: `engine` is valid to be written to and the config is valid to be read. + let res = unsafe { CertCreateCertificateChainEngine(&config, &mut engine) }; + + #[allow(clippy::as_conversions)] + let engine = call_with_last_error(|| match NonNull::new(engine as *mut c_void) { + Some(c) if res == TRUE => Some(c), + _ => None, + })?; + Ok(Self { inner: engine }) + } + + #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] + fn new_with_fake_root(root: &[u8]) -> Result { + use windows_sys::Win32::Security::Cryptography::{ + CERT_CHAIN_CACHE_ONLY_URL_RETRIEVAL, CERT_CHAIN_ENABLE_CACHE_AUTO_UPDATE, + }; + + let mut root_store = CertificateStore::new()?; + root_store.add_cert(root)?; + + let mut config = CERT_CHAIN_ENGINE_CONFIG::zeroed_with_size(); + // We use these flags for the following reasons: + // + // - CERT_CHAIN_CACHE_ONLY_URL_RETRIEVAL is used in an attempt to stop Windows from using the internet to + // fetch anything during the tests, regardless of what test data is used. + // + // - CERT_CHAIN_ENABLE_CACHE_AUTO_UPDATE is used as a minor performance optimization to allow Windows to reuse + // data inside of a test and avoid any extra parsing, etc, it might need to do pulling directly from the store each time. + // + // Ref: https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/ns-wincrypt-cert_chain_engine_config + config.dwFlags = CERT_CHAIN_CACHE_ONLY_URL_RETRIEVAL | CERT_CHAIN_ENABLE_CACHE_AUTO_UPDATE; + config.hExclusiveRoot = root_store.inner.as_ptr(); + + let mut engine = 0; + // SAFETY: `engine` is valid to be written to and the config is valid to be read. + let res = unsafe { CertCreateCertificateChainEngine(&config, &mut engine) }; + + #[allow(clippy::as_conversions)] + let engine = call_with_last_error(|| match NonNull::new(engine as *mut c_void) { + Some(c) if res == TRUE => Some(c), + _ => None, + })?; + + Ok(Self { inner: engine }) + } + + #[allow(clippy::as_conversions)] + fn as_ptr(&self) -> isize { + self.inner.as_ptr() as isize + } +} + +impl Drop for CertEngine { + fn drop(&mut self) { + // SAFETY: The engine pointer is guaranteed to be non-null. + unsafe { CertFreeCertificateChainEngine(self.as_ptr()) }; + } +} + +// SAFETY: We know no other threads is mutating the `CertEngine`, because it would require `unsafe`. +// Across the FFI, `CertGetCertificateChain` don't mutate it either. +unsafe impl Sync for CertEngine {} +// SAFETY: All methods of `CertEngine`, including `Drop`, are safe to be called from other +// threads, because all contained resources are owned by Windows and we only maintain reference counted handles to them. +unsafe impl Send for CertEngine {} + /// An in-memory Windows certificate store. /// /// # Safety @@ -227,17 +309,11 @@ struct CertificateStore { // // During tests, we set this to `Some` as the tests use a // custom verification engine that only uses specific roots. - engine: Option>, // HCERTENGINECONTEXT + engine: Option, // HCERTENGINECONTEXT } impl Drop for CertificateStore { fn drop(&mut self) { - let engine_ptr = self.engine_ptr(); - if self.engine.take().is_some() { - // SAFETY: The engine pointer is guaranteed to be non-null. - unsafe { CertFreeCertificateChainEngine(engine_ptr) }; - } - // SAFETY: See the `CertificateStore` documentation. unsafe { CertCloseStore(self.inner.as_ptr(), 0) }; } @@ -268,45 +344,14 @@ impl CertificateStore { }) } - fn engine_ptr(&self) -> isize { - #[allow(clippy::as_conversions)] - self.engine.map(|e| e.as_ptr() as isize).unwrap_or(0) - } - #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] fn new_with_fake_root(root: &[u8]) -> Result { - use windows_sys::Win32::Security::Cryptography::{ - CertCreateCertificateChainEngine, CERT_CHAIN_CACHE_ONLY_URL_RETRIEVAL, - CERT_CHAIN_ENABLE_CACHE_AUTO_UPDATE, - }; - let mut inner = Self::new()?; let mut root_store = CertificateStore::new()?; root_store.add_cert(root)?; - let mut config = CERT_CHAIN_ENGINE_CONFIG::zeroed_with_size(); - // We use these flags for the following reasons: - // - // - CERT_CHAIN_CACHE_ONLY_URL_RETRIEVAL is used in an attempt to stop Windows from using the internet to - // fetch anything during the tests, regardless of what test data is used. - // - // - CERT_CHAIN_ENABLE_CACHE_AUTO_UPDATE is used as a minor performance optimization to allow Windows to reuse - // data inside of a test and avoid any extra parsing, etc, it might need to do pulling directly from the store each time. - // - // Ref: https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/ns-wincrypt-cert_chain_engine_config - config.dwFlags = CERT_CHAIN_CACHE_ONLY_URL_RETRIEVAL | CERT_CHAIN_ENABLE_CACHE_AUTO_UPDATE; - config.hExclusiveRoot = root_store.inner.as_ptr(); - - let mut engine = 0; - // SAFETY: `engine` is valid to be written to and the config is valid to be read. - let res = unsafe { CertCreateCertificateChainEngine(&config, &mut engine) }; - - #[allow(clippy::as_conversions)] - let engine = call_with_last_error(|| match NonNull::new(engine as *mut c_void) { - Some(c) if res == TRUE => Some(c), - _ => None, - })?; + let engine = CertEngine::new_with_fake_root(root)?; inner.engine = Some(engine); Ok(inner) @@ -347,6 +392,7 @@ impl CertificateStore { &self, certificate: &Certificate, now: pki_types::UnixTime, + engine: Option<&CertEngine>, ) -> Result { let mut cert_chain = ptr::null_mut(); @@ -405,7 +451,7 @@ impl CertificateStore { let parameters = NonNull::from(¶meters).cast().as_ptr(); CertGetCertificateChain( - self.engine_ptr(), + engine.map(CertEngine::as_ptr).unwrap_or(0), certificate.inner.as_ptr(), &time, self.inner.as_ptr(), @@ -442,6 +488,9 @@ pub struct Verifier { #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] test_only_root_ca_override: Option>, pub(super) crypto_provider: OnceCell>, + /// Extra trust anchors to add to the verifier above and beyond those provided by + /// the system-provided trust stores. + extra_roots: Option, } impl Verifier { @@ -456,15 +505,35 @@ impl Verifier { #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] test_only_root_ca_override: None, crypto_provider: OnceCell::new(), + extra_roots: None, } } + /// Creates a new instance of a TLS certificate verifier that utilizes the + /// Windows certificate facilities and augmented by the provided extra root certificates. + /// + /// A [`CryptoProvider`] must be set with + /// [`set_provider`][Verifier::set_provider]/[`with_provider`][Verifier::with_provider] or + /// [`CryptoProvider::install_default`] before the verifier can be used. + pub fn new_with_extra_roots( + roots: Vec>, + ) -> Result { + let cert_engine = CertEngine::new_with_extra_roots(&roots)?; + Ok(Self { + #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] + test_only_root_ca_override: None, + crypto_provider: OnceCell::new(), + extra_roots: Some(cert_engine), + }) + } + /// Creates a test-only TLS certificate verifier which trusts our fake root CA cert. #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] pub(crate) fn new_with_fake_root(root: &[u8]) -> Self { Self { test_only_root_ca_override: Some(root.into()), crypto_provider: OnceCell::new(), + extra_roots: None, } } @@ -521,7 +590,27 @@ impl Verifier { .chain(Some(0)) .collect(); - let cert_chain = store.new_chain_in(&primary_cert, now)?; + let mut cert_chain = store.new_chain_in(&primary_cert, now, store.engine.as_ref())?; + + // We only use `TrustStatus` here because it hasn't had verification performed on it. + // SAFETY: The pointer is guaranteed to be non-null. + let is_partial_chain = unsafe { *cert_chain.inner.as_ptr() } + .TrustStatus + .dwErrorStatus + & CERT_TRUST_IS_PARTIAL_CHAIN + != 0; + + // If we have extra roots and building the chain gave us an error, we try to build a + // new one with the extra roots. + if is_partial_chain && self.extra_roots.is_some() { + let mut store = CertificateStore::new()?; + + for cert in intermediate_certs.iter().copied() { + store.add_cert(cert)?; + } + + cert_chain = store.new_chain_in(&primary_cert, now, self.extra_roots.as_ref())?; + } let status = cert_chain.verify_chain_policy(server)?;