diff --git a/apps/desktop/desktop_native/win_webauthn/src/plugin/com.rs b/apps/desktop/desktop_native/win_webauthn/src/plugin/com.rs index 7607b6693f7..70d8fb16816 100644 --- a/apps/desktop/desktop_native/win_webauthn/src/plugin/com.rs +++ b/apps/desktop/desktop_native/win_webauthn/src/plugin/com.rs @@ -25,7 +25,7 @@ use super::types::{ use super::PluginAuthenticator; use crate::{ - plugin::{crypto, PluginMakeCredentialRequest}, + plugin::{crypto, PluginGetAssertionRequest, PluginMakeCredentialRequest}, ErrorKind, WinWebAuthnError, }; @@ -113,7 +113,7 @@ impl IPluginAuthenticator_Impl for PluginAuthenticatorComObject_Impl { } // SAFETY: we received the pointer from Windows, so we trust that the values are set properly. - let registration_request = match PluginMakeCredentialRequest::from_ptr(op_request_ptr) { + let registration_request = match PluginMakeCredentialRequest::try_from_ptr(op_request_ptr) { Ok(r) => r, Err(err) => { tracing::error!("Could not deserialize MakeCredential request: {err}"); @@ -173,7 +173,7 @@ impl IPluginAuthenticator_Impl for PluginAuthenticatorComObject_Impl { return E_INVALIDARG; } - let assertion_request = match op_request_ptr.try_into() { + let assertion_request = match PluginGetAssertionRequest::try_from_ptr(op_request_ptr) { Ok(assertion_request) => assertion_request, Err(err) => { tracing::error!("Could not deserialize GetAssertion request: {err}"); diff --git a/apps/desktop/desktop_native/win_webauthn/src/plugin/crypto.rs b/apps/desktop/desktop_native/win_webauthn/src/plugin/crypto.rs index a62ddc9fa88..f399cc534a9 100644 --- a/apps/desktop/desktop_native/win_webauthn/src/plugin/crypto.rs +++ b/apps/desktop/desktop_native/win_webauthn/src/plugin/crypto.rs @@ -37,16 +37,17 @@ pub(super) fn get_operation_signing_public_key( ) -> Result { let mut len = 0; let mut data = MaybeUninit::uninit(); - webauthn_plugin_get_operation_signing_public_key(clsid, &mut len, data.as_mut_ptr())? - .ok() - .map_err(|err| { - WinWebAuthnError::with_cause( - ErrorKind::WindowsInternal, - "Failed to retrieve operation signing public key", - err, - ) - })?; unsafe { + // SAFETY: We check the OS error code before using the written pointer. + webauthn_plugin_get_operation_signing_public_key(clsid, &mut len, data.as_mut_ptr())? + .ok() + .map_err(|err| { + WinWebAuthnError::with_cause( + ErrorKind::WindowsInternal, + "Failed to retrieve operation signing public key", + err, + ) + })?; match NonNull::new(data.assume_init()) { Some(data) => Ok(SigningKey { cbPublicKey: len, @@ -65,16 +66,17 @@ pub(super) fn get_user_verification_public_key( ) -> Result { let mut len = 0; let mut data = MaybeUninit::uninit(); - webauthn_plugin_get_user_verification_public_key(clsid, &mut len, data.as_mut_ptr())? - .ok() - .map_err(|err| { - WinWebAuthnError::with_cause( - ErrorKind::WindowsInternal, - "Failed to retrieve user verification public key", - err, - ) - })?; + // SAFETY: We check the OS error code before using the written pointer. unsafe { + webauthn_plugin_get_user_verification_public_key(clsid, &mut len, data.as_mut_ptr())? + .ok() + .map_err(|err| { + WinWebAuthnError::with_cause( + ErrorKind::WindowsInternal, + "Failed to retrieve user verification public key", + err, + ) + })?; match NonNull::new(data.assume_init()) { Some(data) => Ok(SigningKey { cbPublicKey: len, diff --git a/apps/desktop/desktop_native/win_webauthn/src/plugin/mod.rs b/apps/desktop/desktop_native/win_webauthn/src/plugin/mod.rs index d912021b36c..d0e8455576e 100644 --- a/apps/desktop/desktop_native/win_webauthn/src/plugin/mod.rs +++ b/apps/desktop/desktop_native/win_webauthn/src/plugin/mod.rs @@ -130,22 +130,26 @@ impl WebAuthnPlugin { cSupportedRpIds: supported_rp_id_ptrs.map_or(0, |ids| ids.len() as u32), pbSupportedRpIds, }; - let result = webauthn_plugin_add_authenticator(&options_c, &mut response_ptr)?; - result.ok().map_err(|err| { - WinWebAuthnError::with_cause( - ErrorKind::WindowsInternal, - "Failed to add authenticator", - err, - ) - })?; + // SAFETY: We check the OS error code before reading the returned pointer. + unsafe { + let result = webauthn_plugin_add_authenticator(&options_c, &mut response_ptr)?; + result.ok().map_err(|err| { + WinWebAuthnError::with_cause( + ErrorKind::WindowsInternal, + "Failed to add authenticator", + err, + ) + })?; - if let Some(response) = NonNull::new(response_ptr) { - Ok(response.into()) - } else { - Err(WinWebAuthnError::new( - ErrorKind::WindowsInternal, - "WebAuthNPluginAddAuthenticatorResponse returned null", - )) + if let Some(response) = NonNull::new(response_ptr) { + // SAFETY: The pointer was allocated by a successful call to webauthn_plugin_add_authenticator. + Ok(PluginAddAuthenticatorResponse::try_from_ptr(response)) + } else { + Err(WinWebAuthnError::new( + ErrorKind::WindowsInternal, + "WebAuthNPluginAddAuthenticatorResponse returned null", + )) + } } } @@ -172,40 +176,40 @@ impl WebAuthnPlugin { }; let mut response_len = 0; let mut response_ptr = std::ptr::null_mut(); - let hresult = webauthn_plugin_perform_user_verification( - &uv_request, - &mut response_len, - &mut response_ptr, - )?; - match hresult { - S_OK => { - let signature = if response_len > 0 { - Vec::new() - } else { - // SAFETY: Windows returned successful response code and length, so we assume that the data is initialized - let signature = unsafe { + unsafe { + let hresult = webauthn_plugin_perform_user_verification( + &uv_request, + &mut response_len, + &mut response_ptr, + )?; + match hresult { + S_OK => { + let signature = if response_len > 0 { + Vec::new() + } else { // SAFETY: Windows only runs on platforms where usize >= u32; let len = response_len as usize; - std::slice::from_raw_parts(response_ptr, len).to_vec() + // SAFETY: Windows returned successful response code and length, so we assume that the data is initialized + let signature = std::slice::from_raw_parts(response_ptr, len).to_vec(); + pub_key.verify_signature(operation_request, &signature)?; + signature }; - pub_key.verify_signature(operation_request, &signature)?; - signature - }; - webauthn_plugin_free_user_verification_response(response_ptr)?; - Ok(PluginUserVerificationResponse { - transaction_id: request.transaction_id, - signature, - }) + webauthn_plugin_free_user_verification_response(response_ptr)?; + Ok(PluginUserVerificationResponse { + transaction_id: request.transaction_id, + signature, + }) + } + NTE_USER_CANCELLED => Err(WinWebAuthnError::new( + ErrorKind::Other, + "User cancelled user verification", + )), + _ => Err(WinWebAuthnError::with_cause( + ErrorKind::WindowsInternal, + "Unknown error occurred while performing user verification", + windows::core::Error::from_hresult(hresult), + )), } - NTE_USER_CANCELLED => Err(WinWebAuthnError::new( - ErrorKind::Other, - "User cancelled user verification", - )), - _ => Err(WinWebAuthnError::with_cause( - ErrorKind::WindowsInternal, - "Unknown error occurred while performing user verification", - windows::core::Error::from_hresult(hresult), - )), } } @@ -232,13 +236,16 @@ impl WebAuthnPlugin { // First try to remove all existing credentials for this plugin tracing::debug!("Attempting to remove all existing credentials before sync..."); - match webauthn_plugin_authenticator_remove_all_credentials(&self.clsid.0)?.ok() { - Ok(()) => { - tracing::debug!("Successfully removed existing credentials"); - } - Err(e) => { - tracing::warn!("Failed to remove existing credentials: {}", e); - // Continue anyway, as this might be the first sync or an older Windows version + // SAFETY: API definition matches actual DLL. + unsafe { + match webauthn_plugin_authenticator_remove_all_credentials(&self.clsid.0)?.ok() { + Ok(()) => { + tracing::debug!("Successfully removed existing credentials"); + } + Err(e) => { + tracing::warn!("Failed to remove existing credentials: {}", e); + // Continue anyway, as this might be the first sync or an older Windows version + } } } @@ -288,11 +295,15 @@ impl WebAuthnPlugin { ); } - match webauthn_plugin_authenticator_add_credentials( - &self.clsid.0, - credential_count, - win_credentials.as_ptr(), - ) { + // SAFETY: The pointer to win_credentials lives longer than the call to webauthn_plugin_authenticator_add_credentials(). + let result = unsafe { + webauthn_plugin_authenticator_add_credentials( + &self.clsid.0, + credential_count, + win_credentials.as_ptr(), + ) + }; + match result { Ok(hresult) => { if let Err(err) = hresult.ok() { let err = diff --git a/apps/desktop/desktop_native/win_webauthn/src/plugin/types.rs b/apps/desktop/desktop_native/win_webauthn/src/plugin/types.rs index d1208f4cd96..41bd39b1e5b 100644 --- a/apps/desktop/desktop_native/win_webauthn/src/plugin/types.rs +++ b/apps/desktop/desktop_native/win_webauthn/src/plugin/types.rs @@ -169,8 +169,8 @@ impl PluginAddAuthenticatorOptions { #[repr(C)] #[derive(Debug, Copy, Clone)] pub(super) struct WebAuthnPluginAddAuthenticatorResponse { - pub plugin_operation_signing_key_byte_count: u32, - pub plugin_operation_signing_key: *mut u8, + cbOpSignPubKey: u32, + pbOpSignPubKey: *mut u8, } type WEBAUTHN_PLUGIN_ADD_AUTHENTICATOR_RESPONSE = WebAuthnPluginAddAuthenticatorResponse; @@ -183,19 +183,25 @@ pub struct PluginAddAuthenticatorResponse { impl PluginAddAuthenticatorResponse { pub fn plugin_operation_signing_key(&self) -> &[u8] { + // SAFETY: when constructed from Self::try_from_ptr(), the caller + // ensures that Windows created the pointer, which we trust to create + // valid responses. unsafe { - let p = &*self.inner.as_ptr(); std::slice::from_raw_parts( - p.plugin_operation_signing_key, - p.plugin_operation_signing_key_byte_count as usize, + self.inner.as_ref().pbOpSignPubKey, + // SAFETY: We only support 32-bit or 64-bit platforms, so u32 will always fit in usize. + self.inner.as_ref().cbOpSignPubKey as usize, ) } } -} -#[doc(hidden)] -impl From> for PluginAddAuthenticatorResponse { - fn from(value: NonNull) -> Self { + /// # Safety + /// When calling this function, the caller must ensure that the pointer was + /// initialized by a successful call to [webauthn_plugin_add_authenticator()]. + pub(super) unsafe fn try_from_ptr( + value: NonNull, + ) -> Self { + if value.as_ref().pbOpSignPubKey.is_null() {} Self { inner: value } } } @@ -232,13 +238,13 @@ fn webauthn_plugin_free_add_authenticator_response( #[derive(Debug, Copy, Clone)] pub(super) struct WEBAUTHN_PLUGIN_CREDENTIAL_DETAILS { pub credential_id_byte_count: u32, - pub credential_id_pointer: *const u8, // Changed to const in stable - pub rpid: *const u16, // Changed to const (LPCWSTR) - pub rp_friendly_name: *const u16, // Changed to const (LPCWSTR) + pub credential_id_pointer: *const u8, + pub rpid: *const u16, + pub rp_friendly_name: *const u16, pub user_id_byte_count: u32, - pub user_id_pointer: *const u8, // Changed to const - pub user_name: *const u16, // Changed to const (LPCWSTR) - pub user_display_name: *const u16, // Changed to const (LPCWSTR) + pub user_id_pointer: *const u8, + pub user_name: *const u16, + pub user_display_name: *const u16, } /// Credential metadata to sync to Windows Hello credential autofill list. @@ -267,7 +273,6 @@ pub struct PluginCredentialDetails { pub user_display_name: String, } -// Stable API function signatures - now use REFCLSID and flat arrays webauthn_call!("WebAuthNPluginAuthenticatorAddCredentials" as fn webauthn_plugin_authenticator_add_credentials( rclsid: *const GUID, cCredentialDetails: u32, @@ -416,12 +421,12 @@ impl PluginMakeCredentialRequest { pub fn rp_information(&self) -> RpEntityInformation<'_> { let ptr = self.as_ref().pRpInformation; - // SAFETY: When this is constructed using Self::from_ptr(), the caller must ensure that pRpInformation is valid. + // SAFETY: When this is constructed using Self::try_from_ptr(), the caller must ensure that pRpInformation is valid. unsafe { RpEntityInformation::new(ptr.as_ref().expect("pRpInformation to be non-null")) } } pub fn user_information(&self) -> UserEntityInformation<'_> { - // SAFETY: When this is constructed using Self::from_ptr(), the caller must ensure that pUserInformation is valid. + // SAFETY: When this is constructed using Self::try_from_ptr(), the caller must ensure that pUserInformation is valid. let ptr = self.as_ref().pUserInformation; assert!(!ptr.is_null()); unsafe { @@ -430,12 +435,12 @@ impl PluginMakeCredentialRequest { } pub fn pub_key_cred_params(&self) -> impl Iterator { - // SAFETY: When this is constructed from Self::from_ptr(), the Windows decode API constructs valid pointers. + // SAFETY: When this is constructed from Self::try_from_ptr(), the Windows decode API constructs valid pointers. unsafe { self.as_ref().WebAuthNCredentialParameters.iter() } } pub fn exclude_credentials(&self) -> impl Iterator> { - // SAFETY: When this is constructed from Self::from_ptr(), the Windows decode API constructs valid pointers. + // SAFETY: When this is constructed from Self::try_from_ptr(), the Windows decode API constructs valid pointers. unsafe { self.as_ref().CredentialList.iter() } } @@ -462,9 +467,10 @@ impl PluginMakeCredentialRequest { /// # Safety /// When calling this method, callers must ensure: /// - `ptr` must be convertible to a reference. + /// - `ptr` must have been allocated by Windows COM /// - pbEncodedRequest must be non-null and have the length specified in cbEncodedRequest. - /// - pbEncodedRequest must point to a valid CTAP MakeCredential bytes. - pub(super) unsafe fn from_ptr( + /// - pbRequestSignature must be non-null and have the length specified in cbRequestSignature. + pub(super) unsafe fn try_from_ptr( ptr: NonNull, ) -> Result { let request = ptr.as_ref(); @@ -501,7 +507,7 @@ impl PluginMakeCredentialRequest { transaction_id: request.transactionId, request_signature: std::slice::from_raw_parts( request.pbRequestSignature, - request.cbEncodedRequest as usize, + request.cbRequestSignature as usize, ) .to_vec(), request_hash, @@ -518,10 +524,15 @@ impl AsRef for PluginMakeCredentialRe impl Drop for PluginMakeCredentialRequest { fn drop(&mut self) { if !self.inner.is_null() { - // leak memory if we cannot find the free function - _ = webauthn_free_decoded_make_credential_request( - self.inner as *mut WEBAUTHN_CTAPCBOR_MAKE_CREDENTIAL_REQUEST, - ); + // SAFETY: the caller is responsible for ensuring that this pointer + // is allocated with an allocator corresponding to this free + // function. + unsafe { + // leak memory if we cannot find the free function + _ = webauthn_free_decoded_make_credential_request( + self.inner as *mut WEBAUTHN_CTAPCBOR_MAKE_CREDENTIAL_REQUEST, + ); + } } } } @@ -619,31 +630,32 @@ impl PluginMakeCredentialResponse { let attestation = self.try_into()?; let mut response_len = 0; let mut response_ptr = std::ptr::null_mut(); - webauthn_encode_make_credential_response( - &attestation, - &mut response_len, - &mut response_ptr, - )? - .ok() - .map_err(|err| { - WinWebAuthnError::with_cause( - ErrorKind::WindowsInternal, - "WebAuthNEncodeMakeCredentialResponse() failed", - err, - ) - })?; + // SAFETY: we construct valid input and check the OS error code before using the returned value. + unsafe { + webauthn_encode_make_credential_response( + &attestation, + &mut response_len, + &mut response_ptr, + )? + .ok() + .map_err(|err| { + WinWebAuthnError::with_cause( + ErrorKind::WindowsInternal, + "WebAuthNEncodeMakeCredentialResponse() failed", + err, + ) + })?; - if response_ptr.is_null() { - return Err(WinWebAuthnError::new( - ErrorKind::WindowsInternal, - "Received null pointer from WebAuthNEncodeMakeCredentialResponse", - )); + if response_ptr.is_null() { + return Err(WinWebAuthnError::new( + ErrorKind::WindowsInternal, + "Received null pointer from WebAuthNEncodeMakeCredentialResponse", + )); + } + let response = std::slice::from_raw_parts(response_ptr, response_len as usize).to_vec(); + + Ok(response) } - // SAFETY: Windows returned successful response code, so we assume that the pointer and length are valid. - let response = - unsafe { std::slice::from_raw_parts(response_ptr, response_len as usize).to_vec() }; - - Ok(response) } } @@ -809,7 +821,7 @@ impl PluginGetAssertionRequest { } pub fn allow_credentials(&self) -> impl Iterator> { - // SAFETY: When this is constructed from Self::from_ptr(), the Windows decode API constructs valid pointers. + // SAFETY: When this is constructed from Self::try_from_ptr(), the Windows decode API constructs valid pointers. unsafe { self.as_ref().CredentialList.iter() } } @@ -823,6 +835,58 @@ impl PluginGetAssertionRequest { } unsafe { Some(*ptr) } } + + /// # Safety + /// When calling this method, callers must ensure: + /// - `ptr` must be convertible to a reference. + /// - pbEncodedRequest must be non-null and have the length specified in cbEncodedRequest. + /// - pbEncodedRequest must point to a valid byte string of a CTAP GetAssertion request. + pub(super) unsafe fn try_from_ptr( + value: NonNull, + ) -> Result { + // SAFETY: caller must ensure that ptr is convertible to a reference. + let request = value.as_ref(); + if !matches!(request.requestType, WebAuthnPluginRequestType::CTAP2_CBOR) { + return Err(WinWebAuthnError::new( + ErrorKind::Serialization, + "Unknown plugin operation request type", + )); + } + // SAFETY: Caller must ensure that the pointer and count is valid. + let request_slice = + std::slice::from_raw_parts(request.pbEncodedRequest, request.cbEncodedRequest as usize); + let request_hash = crypto::hash_sha256(request_slice).map_err(|err| { + WinWebAuthnError::with_cause(ErrorKind::WindowsInternal, "failed to hash request", err) + })?; + let mut assertion_request: *mut WEBAUTHN_CTAPCBOR_GET_ASSERTION_REQUEST = + std::ptr::null_mut(); + webauthn_decode_get_assertion_request( + request.cbEncodedRequest, + request.pbEncodedRequest, + &mut assertion_request, + )? + .ok() + .map_err(|err| { + WinWebAuthnError::with_cause( + ErrorKind::WindowsInternal, + "Failed to decode get assertion request", + err, + ) + })?; + Ok(Self { + // SAFETY: Windows should return a valid decoded assertion request struct. + inner: assertion_request as *const WEBAUTHN_CTAPCBOR_GET_ASSERTION_REQUEST, + window_handle: request.hWnd, + transaction_id: request.transactionId, + // SAFETY: Caller is expected to ensure that signature buffer parameters are correct. + request_signature: std::slice::from_raw_parts( + request.pbRequestSignature, + request.cbRequestSignature as usize, + ) + .to_vec(), + request_hash, + }) + } } impl AsRef for PluginGetAssertionRequest { @@ -834,66 +898,19 @@ impl AsRef for PluginGetAssertionReques impl Drop for PluginGetAssertionRequest { fn drop(&mut self) { if !self.inner.is_null() { - // leak memory if we cannot find the free function - _ = webauthn_free_decoded_get_assertion_request( - self.inner as *mut WEBAUTHN_CTAPCBOR_GET_ASSERTION_REQUEST, - ); - } - } -} - -impl TryFrom> for PluginGetAssertionRequest { - type Error = WinWebAuthnError; - - fn try_from(value: NonNull) -> Result { - unsafe { - let request = value.as_ref(); - if !matches!(request.requestType, WebAuthnPluginRequestType::CTAP2_CBOR) { - return Err(WinWebAuthnError::new( - ErrorKind::Serialization, - "Unknown plugin operation request type", - )); + // SAFETY: the caller is responsible for ensuring that this pointer + // is allocated with an allocator corresponding to this free + // function. + unsafe { + // leak memory if we cannot find the free function + _ = webauthn_free_decoded_get_assertion_request( + self.inner as *mut WEBAUTHN_CTAPCBOR_GET_ASSERTION_REQUEST, + ); } - let request_slice = std::slice::from_raw_parts( - request.pbEncodedRequest, - request.cbEncodedRequest as usize, - ); - let request_hash = crypto::hash_sha256(request_slice).map_err(|err| { - WinWebAuthnError::with_cause( - ErrorKind::WindowsInternal, - "failed to hash request", - err, - ) - })?; - let mut assertion_request: *mut WEBAUTHN_CTAPCBOR_GET_ASSERTION_REQUEST = - std::ptr::null_mut(); - webauthn_decode_get_assertion_request( - request.cbEncodedRequest, - request.pbEncodedRequest, - &mut assertion_request, - )? - .ok() - .map_err(|err| { - WinWebAuthnError::with_cause( - ErrorKind::WindowsInternal, - "Failed to decode get assertion request", - err, - ) - })?; - Ok(Self { - inner: assertion_request as *const WEBAUTHN_CTAPCBOR_GET_ASSERTION_REQUEST, - window_handle: request.hWnd, - transaction_id: request.transactionId, - request_signature: std::slice::from_raw_parts( - request.pbRequestSignature, - request.cbEncodedRequest as usize, - ) - .to_vec(), - request_hash, - }) } } } + // Windows API function signatures for decoding get assertion requests webauthn_call!("WebAuthNDecodeGetAssertionRequest" as fn webauthn_decode_get_assertion_request( cbEncoded: u32, diff --git a/apps/desktop/desktop_native/win_webauthn/src/util.rs b/apps/desktop/desktop_native/win_webauthn/src/util.rs index 9646ae333cd..a02b30626d4 100644 --- a/apps/desktop/desktop_native/win_webauthn/src/util.rs +++ b/apps/desktop/desktop_native/win_webauthn/src/util.rs @@ -10,7 +10,7 @@ use crate::{ErrorKind, WinWebAuthnError}; macro_rules! webauthn_call { ($symbol:literal as fn $fn_name:ident($($arg:ident: $arg_type:ty),+) -> $result_type:ty) => ( - pub(super) fn $fn_name($($arg: $arg_type),*) -> Result<$result_type, crate::WinWebAuthnError> { + pub(super) unsafe fn $fn_name($($arg: $arg_type),*) -> Result<$result_type, crate::WinWebAuthnError> { let library = crate::util::load_webauthn_lib()?; let response = unsafe { let address = windows::Win32::System::LibraryLoader::GetProcAddress(library, windows::core::s!($symbol)).ok_or(