From 17d2e848f666b342be43c1b92e3cb55ff3a91365 Mon Sep 17 00:00:00 2001 From: Isaiah Inuwa Date: Wed, 17 Dec 2025 15:39:35 -0600 Subject: [PATCH] Document safety for credential metadata iterators --- .../win_webauthn/src/plugin/types.rs | 34 +++-- .../win_webauthn/src/types/mod.rs | 139 ++++++++---------- .../desktop_native/win_webauthn/src/util.rs | 3 +- .../src/assert.rs | 4 +- .../windows_plugin_authenticator/src/main.rs | 3 +- .../src/make_credential.rs | 5 +- 6 files changed, 91 insertions(+), 97 deletions(-) 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 da60043312b..d1208f4cd96 100644 --- a/apps/desktop/desktop_native/win_webauthn/src/plugin/types.rs +++ b/apps/desktop/desktop_native/win_webauthn/src/plugin/types.rs @@ -14,16 +14,18 @@ use windows::{ use crate::{ plugin::crypto, - types::{RpEntityInformation, UserEntityInformation, UserId}, + types::{ + CredentialEx, RpEntityInformation, UserEntityInformation, UserId, + WEBAUTHN_COSE_CREDENTIAL_PARAMETER, + }, util::{webauthn_call, WindowsString}, CredentialId, ErrorKind, WinWebAuthnError, }; use crate::types::{ - AuthenticatorInfo, CredentialList, CtapTransport, HmacSecretSalt, - WebAuthnExtensionMakeCredentialOutput, WEBAUTHN_COSE_CREDENTIAL_PARAMETERS, - WEBAUTHN_CREDENTIAL_ATTESTATION, WEBAUTHN_CREDENTIAL_LIST, WEBAUTHN_EXTENSIONS, - WEBAUTHN_RP_ENTITY_INFORMATION, WEBAUTHN_USER_ENTITY_INFORMATION, + AuthenticatorInfo, CtapTransport, HmacSecretSalt, WebAuthnExtensionMakeCredentialOutput, + WEBAUTHN_COSE_CREDENTIAL_PARAMETERS, WEBAUTHN_CREDENTIAL_ATTESTATION, WEBAUTHN_CREDENTIAL_LIST, + WEBAUTHN_EXTENSIONS, WEBAUTHN_RP_ENTITY_INFORMATION, WEBAUTHN_USER_ENTITY_INFORMATION, }; use super::Clsid; @@ -378,7 +380,7 @@ pub(super) struct WEBAUTHN_CTAPCBOR_MAKE_CREDENTIAL_REQUEST { pub pbClientDataHash: *const u8, pub pRpInformation: *const WEBAUTHN_RP_ENTITY_INFORMATION, pub pUserInformation: *const WEBAUTHN_USER_ENTITY_INFORMATION, - pub WebAuthNCredentialParameters: WEBAUTHN_COSE_CREDENTIAL_PARAMETERS, // Matches C++ sample + pub WebAuthNCredentialParameters: WEBAUTHN_COSE_CREDENTIAL_PARAMETERS, pub CredentialList: WEBAUTHN_CREDENTIAL_LIST, pub cbCborExtensionsMap: u32, pub pbCborExtensionsMap: *const u8, @@ -414,12 +416,12 @@ impl PluginMakeCredentialRequest { pub fn rp_information(&self) -> RpEntityInformation<'_> { let ptr = self.as_ref().pRpInformation; - // SAFETY: if this is constructed using Self::from_ptr(), the caller must ensure that pRpInformation is valid. + // SAFETY: When this is constructed using Self::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: if this is constructed using Self::from_ptr(), the caller must ensure that pUserInformation is valid. + // SAFETY: When this is constructed using Self::from_ptr(), the caller must ensure that pUserInformation is valid. let ptr = self.as_ref().pUserInformation; assert!(!ptr.is_null()); unsafe { @@ -427,12 +429,14 @@ impl PluginMakeCredentialRequest { } } - pub fn pub_key_cred_params(&self) -> WEBAUTHN_COSE_CREDENTIAL_PARAMETERS { - self.as_ref().WebAuthNCredentialParameters + 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. + unsafe { self.as_ref().WebAuthNCredentialParameters.iter() } } - pub fn exclude_credentials(&self) -> CredentialList { - self.as_ref().CredentialList + pub fn exclude_credentials(&self) -> impl Iterator> { + // SAFETY: When this is constructed from Self::from_ptr(), the Windows decode API constructs valid pointers. + unsafe { self.as_ref().CredentialList.iter() } } /// CTAP CBOR extensions map @@ -459,6 +463,7 @@ impl PluginMakeCredentialRequest { /// 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 CTAP MakeCredential bytes. pub(super) unsafe fn from_ptr( ptr: NonNull, ) -> Result { @@ -803,8 +808,9 @@ impl PluginGetAssertionRequest { } } - pub fn allow_credentials(&self) -> CredentialList { - self.as_ref().CredentialList + pub fn allow_credentials(&self) -> impl Iterator> { + // SAFETY: When this is constructed from Self::from_ptr(), the Windows decode API constructs valid pointers. + unsafe { self.as_ref().CredentialList.iter() } } // TODO: Support extensions diff --git a/apps/desktop/desktop_native/win_webauthn/src/types/mod.rs b/apps/desktop/desktop_native/win_webauthn/src/types/mod.rs index 70bfebeb05f..392c5ce21e9 100644 --- a/apps/desktop/desktop_native/win_webauthn/src/types/mod.rs +++ b/apps/desktop/desktop_native/win_webauthn/src/types/mod.rs @@ -324,27 +324,23 @@ impl UserEntityInformation<'_> { #[repr(C)] #[derive(Debug, Copy, Clone)] pub struct WEBAUTHN_COSE_CREDENTIAL_PARAMETER { - pub dwVersion: u32, - pub pwszCredentialType: *const u16, // LPCWSTR - pub lAlg: i32, // LONG - COSE algorithm identifier + dwVersion: u32, + pwszCredentialType: NonNull, // LPCWSTR + lAlg: i32, // LONG - COSE algorithm identifier } impl WEBAUTHN_COSE_CREDENTIAL_PARAMETER { pub fn credential_type(&self) -> Result { - if self.pwszCredentialType.is_null() { - return Err(WinWebAuthnError::new( - ErrorKind::WindowsInternal, - "Invalid credential type", - )); - } unsafe { - PCWSTR(self.pwszCredentialType).to_string().map_err(|err| { - WinWebAuthnError::with_cause( - ErrorKind::WindowsInternal, - "Invalid credential type", - err, - ) - }) + PCWSTR(self.pwszCredentialType.as_ptr()) + .to_string() + .map_err(|err| { + WinWebAuthnError::with_cause( + ErrorKind::WindowsInternal, + "Invalid credential type", + err, + ) + }) } } pub fn alg(&self) -> i32 { @@ -354,19 +350,21 @@ impl WEBAUTHN_COSE_CREDENTIAL_PARAMETER { #[repr(C)] #[derive(Debug, Copy, Clone)] -pub struct WEBAUTHN_COSE_CREDENTIAL_PARAMETERS { +pub(crate) struct WEBAUTHN_COSE_CREDENTIAL_PARAMETERS { cCredentialParameters: u32, pCredentialParameters: *const WEBAUTHN_COSE_CREDENTIAL_PARAMETER, } impl WEBAUTHN_COSE_CREDENTIAL_PARAMETERS { - pub fn iter(&self) -> ArrayPointerIterator<'_, WEBAUTHN_COSE_CREDENTIAL_PARAMETER> { - unsafe { - ArrayPointerIterator::new( - self.pCredentialParameters, - self.cCredentialParameters as usize, - ) - } + /// # Safety + /// The caller must ensure that the pCredentialParameters is either null or + /// marks the beginning of a list whose count is represented accurately by + /// cCredentialParameters. + pub unsafe fn iter(&self) -> ArrayPointerIterator<'_, WEBAUTHN_COSE_CREDENTIAL_PARAMETER> { + ArrayPointerIterator::new( + self.pCredentialParameters, + self.cCredentialParameters as usize, + ) } } @@ -614,7 +612,7 @@ impl TryFrom> for CredentialId { #[repr(C)] #[derive(Debug, Copy, Clone)] -pub struct WEBAUTHN_CREDENTIAL_EX { +pub(crate) struct WEBAUTHN_CREDENTIAL_EX { dwVersion: u32, cbId: u32, pbId: *const u8, @@ -622,36 +620,47 @@ pub struct WEBAUTHN_CREDENTIAL_EX { dwTransports: u32, } -impl WEBAUTHN_CREDENTIAL_EX { +pub struct CredentialEx<'a> { + inner: &'a WEBAUTHN_CREDENTIAL_EX, +} + +impl CredentialEx<'_> { pub fn credential_id(&self) -> Option<&[u8]> { - if self.cbId == 0 || self.pbId.is_null() { + if self.inner.cbId == 0 || self.inner.pbId.is_null() { None } else { - unsafe { Some(std::slice::from_raw_parts(self.pbId, self.cbId as usize)) } + unsafe { + Some(std::slice::from_raw_parts( + self.inner.pbId, + self.inner.cbId as usize, + )) + } } } pub fn credential_type(&self) -> Result { - if self.pwszCredentialType.is_null() { + if self.inner.pwszCredentialType.is_null() { return Err(WinWebAuthnError::new( ErrorKind::WindowsInternal, "Received invalid credential ID", )); } unsafe { - PCWSTR(self.pwszCredentialType).to_string().map_err(|err| { - WinWebAuthnError::with_cause( - ErrorKind::WindowsInternal, - "Invalid credential ID", - err, - ) - }) + PCWSTR(self.inner.pwszCredentialType) + .to_string() + .map_err(|err| { + WinWebAuthnError::with_cause( + ErrorKind::WindowsInternal, + "Invalid credential ID", + err, + ) + }) } } pub fn transports(&self) -> Vec { let mut transports = Vec::new(); - let mut t = self.dwTransports; + let mut t = self.inner.dwTransports; if t == 0 { return transports; }; @@ -677,23 +686,6 @@ impl WEBAUTHN_CREDENTIAL_EX { } } -pub struct CredentialEx { - inner: NonNull, -} - -impl AsRef for CredentialEx { - fn as_ref(&self) -> &WEBAUTHN_CREDENTIAL_EX { - // SAFETY: We initialize memory manually in constructors. - unsafe { self.inner.as_ref() } - } -} - -impl From> for CredentialEx { - fn from(value: NonNull) -> Self { - Self { inner: value } - } -} - #[repr(u32)] #[derive(Clone, Copy)] pub enum CtapTransport { @@ -708,30 +700,13 @@ pub enum CtapTransport { #[repr(C)] #[derive(Debug, Copy, Clone)] -pub struct CredentialList { +pub(crate) struct WEBAUTHN_CREDENTIAL_LIST { pub cCredentials: u32, pub ppCredentials: *const *const WEBAUTHN_CREDENTIAL_EX, } -pub(crate) type WEBAUTHN_CREDENTIAL_LIST = CredentialList; -pub struct CredentialListIterator<'a> { - inner: ArrayPointerIterator<'a, *const WEBAUTHN_CREDENTIAL_EX>, -} - -impl<'a> Iterator for CredentialListIterator<'a> { - type Item = &'a WEBAUTHN_CREDENTIAL_EX; - - fn next(&mut self) -> Option { - let item = self.inner.next()?; - // SAFETY: This type can only be constructed from this library using - // responses from Windows APIs, and we trust that the pointer and length - // of each inner item of the array is valid. - unsafe { item.as_ref() } - } -} - -impl CredentialList { - pub fn iter(&self) -> CredentialListIterator<'_> { +impl WEBAUTHN_CREDENTIAL_LIST { + pub unsafe fn iter(&self) -> CredentialListIterator<'_> { // SAFETY: This type can only be constructed from this library using // responses from Windows APIs. The pointer is checked for null safety // on construction. @@ -743,6 +718,22 @@ impl CredentialList { } } +pub struct CredentialListIterator<'a> { + inner: ArrayPointerIterator<'a, *const WEBAUTHN_CREDENTIAL_EX>, +} + +impl<'a> Iterator for CredentialListIterator<'a> { + type Item = CredentialEx<'a>; + + fn next(&mut self) -> Option { + let item = self.inner.next()?; + // SAFETY: This type can only be constructed from this library using + // responses from Windows APIs, and we trust that the pointer and length + // of each inner item of the array is valid. + unsafe { item.as_ref().map(|inner| CredentialEx { inner }) } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/apps/desktop/desktop_native/win_webauthn/src/util.rs b/apps/desktop/desktop_native/win_webauthn/src/util.rs index 57ba84dcde9..9646ae333cd 100644 --- a/apps/desktop/desktop_native/win_webauthn/src/util.rs +++ b/apps/desktop/desktop_native/win_webauthn/src/util.rs @@ -72,7 +72,8 @@ pub struct ArrayPointerIterator<'a, T> { } impl ArrayPointerIterator<'_, T> { - /// Safety constraints: The caller must ensure that the pointer and length is + /// # Safety + /// The caller must ensure that the pointer and length is /// valid. A null pointer returns an empty iterator. pub unsafe fn new(data: *const T, len: usize) -> Self { let slice = if !data.is_null() { diff --git a/apps/desktop/desktop_native/windows_plugin_authenticator/src/assert.rs b/apps/desktop/desktop_native/windows_plugin_authenticator/src/assert.rs index 497fef34436..cfee6b7d0cd 100644 --- a/apps/desktop/desktop_native/windows_plugin_authenticator/src/assert.rs +++ b/apps/desktop/desktop_native/windows_plugin_authenticator/src/assert.rs @@ -39,9 +39,7 @@ pub fn get_assertion( // Extract allowed credentials from credential list let allowed_credential_ids: Vec> = request .allow_credentials() - .iter() - .filter_map(|cred| cred.credential_id()) - .map(|id| id.to_vec()) + .filter_map(|cred| cred.credential_id().map(|id| id.to_vec())) .collect(); let client_window_handle = request.window_handle.0.addr().to_le_bytes().to_vec(); diff --git a/apps/desktop/desktop_native/windows_plugin_authenticator/src/main.rs b/apps/desktop/desktop_native/windows_plugin_authenticator/src/main.rs index f95d38e5abe..ff88052dcdc 100644 --- a/apps/desktop/desktop_native/windows_plugin_authenticator/src/main.rs +++ b/apps/desktop/desktop_native/windows_plugin_authenticator/src/main.rs @@ -302,7 +302,8 @@ impl PluginAuthenticator for BitwardenPluginAuthenticator { let is_unlocked = get_lock_status(&client).map_or(false, |response| response.is_unlocked); // Don't mess with the window unless we're going to need it: if the // vault is locked or if we need to show credential selection dialog. - let needs_ui = !is_unlocked || request.allow_credentials().cCredentials != 1; + + let needs_ui = !is_unlocked || request.allow_credentials().count() != 1; if needs_ui { unsafe { let plugin_window = get_window_details(&client)?; diff --git a/apps/desktop/desktop_native/windows_plugin_authenticator/src/make_credential.rs b/apps/desktop/desktop_native/windows_plugin_authenticator/src/make_credential.rs index b98afbf179f..d2243864c3d 100644 --- a/apps/desktop/desktop_native/windows_plugin_authenticator/src/make_credential.rs +++ b/apps/desktop/desktop_native/windows_plugin_authenticator/src/make_credential.rs @@ -49,7 +49,6 @@ pub fn make_credential( // Extract supported algorithms let supported_algorithms: Vec = request .pub_key_cred_params() - .iter() .map(|params| params.alg()) .collect(); @@ -66,9 +65,7 @@ pub fn make_credential( // Extract excluded credentials from credential list let excluded_credentials: Vec> = request .exclude_credentials() - .iter() - .filter_map(|cred| cred.credential_id()) - .map(|id| id.to_vec()) + .filter_map(|cred| cred.credential_id().map(|id| id.to_vec())) .collect(); if !excluded_credentials.is_empty() { tracing::debug!(