From 57b89a94688e5147ecd47e178d40150a83c82dbb Mon Sep 17 00:00:00 2001 From: ComplexSpaces Date: Fri, 23 Aug 2024 19:56:32 -0500 Subject: [PATCH] Port from winapi to windows-sys --- Cargo.lock | 39 +--- rustls-platform-verifier/Cargo.toml | 2 +- rustls-platform-verifier/src/lib.rs | 3 - .../src/verification/mod.rs | 7 +- .../src/verification/windows.rs | 169 +++++++++++------- rustls-platform-verifier/src/windows.rs | 38 ---- 6 files changed, 113 insertions(+), 145 deletions(-) delete mode 100644 rustls-platform-verifier/src/windows.rs diff --git a/Cargo.lock b/Cargo.lock index f146773d..8abfc98d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -255,7 +255,7 @@ dependencies = [ "libc", "spin", "untrusted", - "windows-sys 0.52.0", + "windows-sys", ] [[package]] @@ -319,7 +319,7 @@ dependencies = [ "security-framework", "security-framework-sys", "webpki-roots", - "winapi", + "windows-sys", ] [[package]] @@ -352,7 +352,7 @@ version = "0.1.23" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fbc91545643bcf3a0bbb6569265615222618bdf33ce4ffbbd13c4bbd4c093534" dependencies = [ - "windows-sys 0.52.0", + "windows-sys", ] [[package]] @@ -459,37 +459,15 @@ dependencies = [ "rustls-pki-types", ] -[[package]] -name = "winapi" -version = "0.3.9" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5c839a674fcd7a98952e593242ea400abe93992746761e38641405d28b00f419" -dependencies = [ - "winapi-i686-pc-windows-gnu", - "winapi-x86_64-pc-windows-gnu", -] - -[[package]] -name = "winapi-i686-pc-windows-gnu" -version = "0.4.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6" - [[package]] name = "winapi-util" version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cf221c93e13a30d793f7645a0e7762c55d169dbb0a49671918a2319d289b10bb" dependencies = [ - "windows-sys 0.59.0", + "windows-sys", ] -[[package]] -name = "winapi-x86_64-pc-windows-gnu" -version = "0.4.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f" - [[package]] name = "windows-sys" version = "0.52.0" @@ -499,15 +477,6 @@ dependencies = [ "windows-targets", ] -[[package]] -name = "windows-sys" -version = "0.59.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1e38bc4d79ed67fd075bcc251a1c39b32a1776bbe92e5bef1f0bf1f8c531853b" -dependencies = [ - "windows-targets", -] - [[package]] name = "windows-targets" version = "0.52.6" diff --git a/rustls-platform-verifier/Cargo.toml b/rustls-platform-verifier/Cargo.toml index 7a071a62..b9bd7d35 100644 --- a/rustls-platform-verifier/Cargo.toml +++ b/rustls-platform-verifier/Cargo.toml @@ -59,7 +59,7 @@ security-framework = { version = "2.10", features = ["OSX_10_14"] } security-framework-sys = { version = "2.10", features = ["OSX_10_14"] } [target.'cfg(windows)'.dependencies] -winapi = { version = "0.3", features = ["wincrypt", "winerror"] } +windows-sys = { version = "0.52", default-features = false, features = ["Win32_Foundation", "Win32_Security_Cryptography"] } [dev-dependencies] rustls = { version = "0.23", default-features = false, features = ["ring"] } diff --git a/rustls-platform-verifier/src/lib.rs b/rustls-platform-verifier/src/lib.rs index 23a13e46..975ad0dc 100644 --- a/rustls-platform-verifier/src/lib.rs +++ b/rustls-platform-verifier/src/lib.rs @@ -15,9 +15,6 @@ pub use verification::Verifier; #[cfg_attr(docsrs, doc(cfg(target_os = "android")))] pub mod android; -#[cfg(windows)] -mod windows; - /// Fixures and data to support testing the server /// certificate verifier. #[cfg(any(test, feature = "ffi-testing"))] diff --git a/rustls-platform-verifier/src/verification/mod.rs b/rustls-platform-verifier/src/verification/mod.rs index 56387fe5..e55d71d6 100644 --- a/rustls-platform-verifier/src/verification/mod.rs +++ b/rustls-platform-verifier/src/verification/mod.rs @@ -82,11 +82,10 @@ fn invalid_certificate(reason: impl Into) -> rustls::Error { /// - id-kp-serverAuth // TODO: Chromium also allows for `OID_ANY_EKU` on Android. #[cfg(target_os = "windows")] -// XXX: Windows requires that we NUL terminate EKU strings and we want to make sure that only the -// data part of the `&str` pointer (using `.as_ptr()`), not all of its metadata. -// This can be cleaned up when our MSRV is increased to 1.77 and C-string literals are available. +// XXX: Windows requires that we NUL terminate EKU strings. // See https://github.com/rustls/rustls-platform-verifier/issues/126#issuecomment-2306232794. -const ALLOWED_EKUS: &[*mut u8] = &["1.3.6.1.5.5.7.3.1\0".as_ptr() as *mut u8]; +const ALLOWED_EKUS: &[windows_sys::core::PCSTR] = + &[windows_sys::Win32::Security::Cryptography::szOID_PKIX_KP_SERVER_AUTH]; #[cfg(target_os = "android")] pub const ALLOWED_EKUS: &[&str] = &["1.3.6.1.5.5.7.3.1"]; diff --git a/rustls-platform-verifier/src/verification/windows.rs b/rustls-platform-verifier/src/verification/windows.rs index 19d5ff9a..730e57ef 100644 --- a/rustls-platform-verifier/src/verification/windows.rs +++ b/rustls-platform-verifier/src/verification/windows.rs @@ -19,72 +19,84 @@ //! [Microsoft's Example]: use super::{log_server_cert, ALLOWED_EKUS}; -use crate::windows::{ - c_void_from_ref, c_void_from_ref_mut, nonnull_from_const_ptr, ZeroedWithSize, -}; use once_cell::sync::OnceCell; use rustls::client::danger::{HandshakeSignatureValid, ServerCertVerifier}; use rustls::crypto::{verify_tls12_signature, verify_tls13_signature, CryptoProvider}; use rustls::pki_types; -use rustls::{CertificateError, DigitallySignedStruct, Error as TlsError, SignatureScheme}; -use winapi::{ - shared::{ - minwindef::{FILETIME, TRUE}, - ntdef::{LPSTR, VOID}, - winerror::{ - CERT_E_CN_NO_MATCH, CERT_E_EXPIRED, CERT_E_INVALID_NAME, CERT_E_UNTRUSTEDROOT, - CERT_E_WRONG_USAGE, CRYPT_E_REVOKED, - }, +use rustls::{ + CertificateError, DigitallySignedStruct, Error as TlsError, Error::InvalidCertificate, + SignatureScheme, +}; +use std::{ + convert::TryInto, + mem::{self, MaybeUninit}, + os::raw::c_void, + ptr::{self, NonNull}, + sync::Arc, +}; +use windows_sys::Win32::{ + Foundation::{ + BOOL, CERT_E_CN_NO_MATCH, CERT_E_EXPIRED, CERT_E_INVALID_NAME, CERT_E_UNTRUSTEDROOT, + CERT_E_WRONG_USAGE, CRYPT_E_REVOKED, FILETIME, TRUE, }, - um::wincrypt::{ + Security::Cryptography::{ CertAddEncodedCertificateToStore, CertCloseStore, CertFreeCertificateChain, CertFreeCertificateChainEngine, CertFreeCertificateContext, CertGetCertificateChain, CertOpenStore, CertSetCertificateContextProperty, CertVerifyCertificateChainPolicy, - AUTHTYPE_SERVER, CERT_CHAIN_CACHE_END_CERT, CERT_CHAIN_CONTEXT, CERT_CHAIN_PARA, + HTTPSPolicyCallbackData, AUTHTYPE_SERVER, CERT_CHAIN_CACHE_END_CERT, CERT_CHAIN_CONTEXT, 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_USAGE_MATCH, CRYPT_DATA_BLOB, CTL_USAGE, SSL_EXTRA_CERT_CHAIN_POLICY_PARA, + CERT_STRONG_SIGN_PARA, CERT_USAGE_MATCH, CRYPT_INTEGER_BLOB, CTL_USAGE, USAGE_MATCH_TYPE_AND, X509_ASN_ENCODING, }, }; -use rustls::Error::InvalidCertificate; -use std::{ - convert::TryInto, - mem::{self, MaybeUninit}, - ptr::{self, NonNull}, - sync::Arc, -}; +// XXX: The `windows-sys` definition for `CERT_CHAIN_PARA` is missing the optional fields. +// See https://github.com/microsoft/win32metadata/commit/185c3ef016663ab8201878da7efab0a2cff0bcc7. +// +// However: by defining it ourselves we can keep better (hypothetical) OS backwards compat. +// When its available in `windows-sys`, a compile-time size assertion can be added to help it stay in sync. +#[allow(non_camel_case_types, non_snake_case)] +#[repr(C)] +struct CERT_CHAIN_PARA { + pub cbSize: u32, + pub RequestedUsage: CERT_USAGE_MATCH, + pub RequestedIssuancePolicy: CERT_USAGE_MATCH, + pub dwUrlRetrievalTimeout: u32, + pub fCheckRevocationFreshnessTime: BOOL, + pub dwRevocationFreshnessTime: u32, + pub pftCacheResync: *mut FILETIME, + // XXX: `dwStrongSignFlags` might or might not be defined on the current system. It started + // being available in Windows 8. See https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/ns-wincrypt-cert_chain_para + #[cfg(not(target_vendor = "win7"))] + pub pStrongSignPara: *const CERT_STRONG_SIGN_PARA, + pub dwStrongSignFlags: u32, +} use crate::verification::invalid_certificate; #[cfg(any(test, feature = "ffi-testing", feature = "dbg"))] -use winapi::um::wincrypt::CERT_CHAIN_ENGINE_CONFIG; +use windows_sys::Win32::Security::Cryptography::CERT_CHAIN_ENGINE_CONFIG; // SAFETY: see method implementation unsafe impl ZeroedWithSize for CERT_CHAIN_PARA { fn zeroed_with_size() -> Self { - // This must be zeroed and not constructed since `dwStrongSignFlags` might or might not be defined on - // the current system. - // https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/ns-wincrypt-cert_chain_para // SAFETY: `CERT_CHAIN_PARA` only contains pointers and integers, which are safe to zero. + // Additionally, MSDN states you *MUST* zero all unused fields. let mut new: Self = unsafe { mem::zeroed() }; - new.cbSize = size_of_struct(&new); + new.cbSize = Self::SIZE; new } } // SAFETY: see method implementation -unsafe impl ZeroedWithSize for SSL_EXTRA_CERT_CHAIN_POLICY_PARA { +unsafe impl ZeroedWithSize for HTTPSPolicyCallbackData { fn zeroed_with_size() -> Self { // SAFETY: zeroed is needed here since it contains a union. let mut new: Self = unsafe { mem::zeroed() }; - let size = size_of_struct(&new); - // SAFETY: Its safe to write to to a union field that is `Copy`. - // https://doc.rust-lang.org/reference/items/unions.html#reading-and-writing-union-fields - *(unsafe { new.u.cbSize_mut() }) = size; + new.Anonymous.cbSize = Self::SIZE; new } } @@ -94,7 +106,7 @@ unsafe impl ZeroedWithSize for CERT_CHAIN_POLICY_PARA { fn zeroed_with_size() -> Self { // SAFETY: This structure only contains integers and pointers. let mut new: Self = unsafe { mem::zeroed() }; - new.cbSize = size_of_struct(&new); + new.cbSize = Self::SIZE; new } } @@ -105,7 +117,7 @@ unsafe impl ZeroedWithSize for CERT_CHAIN_ENGINE_CONFIG { fn zeroed_with_size() -> Self { // SAFETY: This structure only contains integers and pointers. let mut new: Self = unsafe { mem::zeroed() }; - new.cbSize = size_of_struct(&new); + new.cbSize = Self::SIZE; new } } @@ -119,7 +131,7 @@ impl CertChain { &self, mut server_null_terminated: Vec, ) -> Result { - let mut extra_params = SSL_EXTRA_CERT_CHAIN_POLICY_PARA::zeroed_with_size(); + let mut extra_params = HTTPSPolicyCallbackData::zeroed_with_size(); extra_params.dwAuthType = AUTHTYPE_SERVER; // `server_null_terminated` outlives `extra_params`. extra_params.pwszServerName = server_null_terminated.as_mut_ptr(); @@ -129,7 +141,7 @@ impl CertChain { // 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`. - params.pvExtraPolicyPara = c_void_from_ref_mut(&mut extra_params); + params.pvExtraPolicyPara = NonNull::from(&mut extra_params).cast::().as_ptr(); let mut status: MaybeUninit = MaybeUninit::uninit(); @@ -138,7 +150,7 @@ impl CertChain { CertVerifyCertificateChainPolicy( CERT_CHAIN_POLICY_SSL, self.inner.as_ptr(), - &mut params, + ¶ms, status.as_mut_ptr(), ) }; @@ -181,7 +193,7 @@ impl Certificate { unsafe fn set_property( &mut self, prop_id: u32, - prop_data: *const VOID, + prop_data: *const c_void, ) -> Result<(), TlsError> { // SAFETY: `cert` points to a valid certificate context and the OCSP data is valid to read. call_with_last_error(|| { @@ -211,19 +223,20 @@ impl Drop for Certificate { /// `CertificateStore`. This is only safe to do if the certificate store is /// constructed with `CERT_STORE_DEFER_CLOSE_UNTIL_LAST_FREE_FLAG`. struct CertificateStore { - inner: NonNull, // HCERTSTORE + inner: NonNull, // HCERTSTORE // In production code, this is always `None`. // // 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) { - if let Some(engine) = self.engine.take() { + 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.as_ptr()) }; + unsafe { CertFreeCertificateChainEngine(engine_ptr) }; } // SAFETY: See the `CertificateStore` documentation. @@ -256,13 +269,14 @@ impl CertificateStore { }) } - fn engine_ptr(&self) -> *mut VOID { - self.engine.map(|e| e.as_ptr()).unwrap_or(ptr::null_mut()) + 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 winapi::um::wincrypt::{ + use windows_sys::Win32::Security::Cryptography::{ CertCreateCertificateChainEngine, CERT_CHAIN_CACHE_ONLY_URL_RETRIEVAL, CERT_CHAIN_ENABLE_CACHE_AUTO_UPDATE, }; @@ -285,11 +299,12 @@ impl CertificateStore { config.dwFlags = CERT_CHAIN_CACHE_ONLY_URL_RETRIEVAL | CERT_CHAIN_ENABLE_CACHE_AUTO_UPDATE; config.hExclusiveRoot = root_store.inner.as_ptr(); - let mut engine = ptr::null_mut(); + let mut engine = 0; // SAFETY: `engine` is valid to be written to and the config is valid to be read. - let res = unsafe { CertCreateCertificateChainEngine(&mut config, &mut engine) }; + let res = unsafe { CertCreateCertificateChainEngine(&config, &mut engine) }; - let engine = call_with_last_error(|| match nonnull_from_const_ptr(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, })?; @@ -304,7 +319,7 @@ impl CertificateStore { /// /// Errors if the certificate was malformed and couldn't be added. fn add_cert(&mut self, cert: &[u8]) -> Result { - let mut cert_context: *const CERT_CONTEXT = ptr::null_mut(); + let mut cert_context: *mut CERT_CONTEXT = ptr::null_mut(); // SAFETY: `inner` is a valid certificate store, and `cert` is a valid a byte array valid // for reads, the correct length is being provided, and `cert_context` is valid to write to. @@ -323,7 +338,7 @@ impl CertificateStore { // SAFETY: Constructing a `Certificate` is only safe if the store was // created with the right flags; see the `CertificateStore` docs. - match (res, nonnull_from_const_ptr(cert_context)) { + match (res, NonNull::new(cert_context)) { (TRUE, Some(cert)) => Ok(Certificate { inner: cert }), _ => Err(InvalidCertificate(CertificateError::BadEncoding)), } @@ -334,7 +349,7 @@ impl CertificateStore { certificate: &Certificate, now: pki_types::UnixTime, ) -> Result { - let mut cert_chain = ptr::null(); + let mut cert_chain = ptr::null_mut(); let mut parameters = CERT_CHAIN_PARA::zeroed_with_size(); @@ -344,13 +359,13 @@ impl CertificateStore { dwType: USAGE_MATCH_TYPE_AND, Usage: CTL_USAGE { cUsageIdentifier: ALLOWED_EKUS.len() as u32, - rgpszUsageIdentifier: ALLOWED_EKUS.as_ptr() as *mut LPSTR, + rgpszUsageIdentifier: ALLOWED_EKUS.as_ptr() as *mut windows_sys::core::PSTR, }, }; parameters.RequestedUsage = usage; #[allow(clippy::as_conversions)] - let mut time = { + let time = { /// Seconds between Jan 1st, 1601 and Jan 1, 1970. const UNIX_ADJUSTMENT: std::time::Duration = std::time::Duration::from_secs(11_644_473_600); @@ -384,12 +399,18 @@ impl CertificateStore { // SAFETY: `cert` points to a valid certificate context, parameters is valid for reads, `cert_chain` is valid // for writes, and the certificate store is valid and initialized. let res = unsafe { + // XXX: Due to the redefinition of `CERT_CHAIN_PARA`, we need to do pointer casts + // in order to pass our expanded structure into `CertGetCertificateChain`. + // This is safe because the OS uses `cbSize` to know if the extra parameters + // are present or not. As we set `cbSize` correctly, the fields can be read from correctly. + let parameters = NonNull::from(¶meters).cast().as_ptr(); + CertGetCertificateChain( self.engine_ptr(), certificate.inner.as_ptr(), - &mut time, + &time, self.inner.as_ptr(), - &mut parameters, + parameters, FLAGS, ptr::null_mut(), &mut cert_chain, @@ -398,7 +419,7 @@ impl CertificateStore { // XXX: Windows will internally map the chain's `TrustStatus.dwErrorStatus` to a `dwError` when // a chain policy is verified, so we only check for errors there. - call_with_last_error(|| match nonnull_from_const_ptr(cert_chain) { + call_with_last_error(|| match NonNull::new(cert_chain) { Some(c) if res == TRUE => Some(CertChain { inner: c }), _ => None, }) @@ -478,7 +499,7 @@ impl Verifier { if let Some(ocsp_data) = ocsp_data { #[allow(clippy::as_conversions)] - let data = CRYPT_DATA_BLOB { + let data = CRYPT_INTEGER_BLOB { cbData: ocsp_data.len().try_into().map_err(|_| { invalid_certificate("Malformed OCSP response stapled to server certificate") })?, @@ -487,7 +508,10 @@ impl Verifier { // SAFETY: `data` is a valid pointer and matches the property ID. unsafe { - primary_cert.set_property(CERT_OCSP_RESPONSE_PROP_ID, c_void_from_ref(&data))?; + primary_cert.set_property( + CERT_OCSP_RESPONSE_PROP_ID, + NonNull::from(&data).cast::().as_ptr(), + )?; } } @@ -526,12 +550,6 @@ impl Verifier { } } -fn size_of_struct(val: &T) -> u32 { - mem::size_of_val(val) - .try_into() - .expect("size of struct can't exceed u32") -} - impl ServerCertVerifier for Verifier { fn verify_server_cert( &self, @@ -610,3 +628,26 @@ impl Default for Verifier { Self::new() } } + +/// A trait to represent an object that can be safely created with all zero values +/// and have a size assigned to it. +/// +/// # Safety +/// +/// This has the same safety requirements as [std::mem::zeroed]. +unsafe trait ZeroedWithSize: Sized { + const SIZE: u32 = { + let size = core::mem::size_of::(); + + // NB: `TryInto` isn't stable in const yet. + #[allow(clippy::as_conversions)] + if size <= u32::MAX as usize { + size as u32 + } else { + panic!("structure was larger then DWORD") + } + }; + + /// Returns a zeroed structure with its structure size (`cbSize`) field set to the correct value. + fn zeroed_with_size() -> Self; +} diff --git a/rustls-platform-verifier/src/windows.rs b/rustls-platform-verifier/src/windows.rs deleted file mode 100644 index d32130ca..00000000 --- a/rustls-platform-verifier/src/windows.rs +++ /dev/null @@ -1,38 +0,0 @@ -//! Utilities to deal with weird aspects of interfacing with Windows through `winapi`. -#![allow(clippy::as_conversions)] - -use std::ptr::NonNull; - -/// A trait to represent an object that can be safely created with all zero values -/// and have a size assigned to it. -/// -/// # Safety -/// -/// This has the same safety requirements as [std::mem::zeroed]. -pub(crate) unsafe trait ZeroedWithSize { - /// Returns a zeroed structure with its structure size (`cbsize`) field set to the correct value. - fn zeroed_with_size() -> Self; -} - -/// Returns `p` as a `*const c_void`. -/// -/// The conversion is done in the most type-safe way possible. -pub(crate) fn c_void_from_ref(p: &T) -> *const winapi::ctypes::c_void { - let p: *const T = p; - p as *const winapi::ctypes::c_void -} - -/// Returns `p` as a `*mut c_void`. -/// -/// The conversion is done in the most type-safe way possible. -pub(crate) fn c_void_from_ref_mut(p: &mut T) -> *mut winapi::ctypes::c_void { - let p: *mut T = p; - p as *mut winapi::ctypes::c_void -} - -/// Returns `p` as a `NonNull`, erasing its `const`-ness. -/// -/// The conversion is done in the most type-safe way possible. -pub(crate) fn nonnull_from_const_ptr(p: *const T) -> Option> { - NonNull::new(p as *mut T) -}