From ac14ed9ee0e97aa4a6203fb424438c88029e0f87 Mon Sep 17 00:00:00 2001 From: John Schanck Date: Wed, 9 Aug 2023 20:47:55 +0000 Subject: [PATCH] Bug 1844136 - avoid the AuthenticatorService and Manager types from auth-rs. r=keeler Depends on D183056 Differential Revision: https://phabricator.services.mozilla.com/D183893 --- Cargo.lock | 1 - dom/webauthn/authrs_bridge/Cargo.toml | 1 - dom/webauthn/authrs_bridge/src/lib.rs | 100 ++++++--- dom/webauthn/authrs_bridge/src/test_token.rs | 205 +++++-------------- 4 files changed, 118 insertions(+), 189 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3b5c814abacd..7d26d9f2fc00 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -331,7 +331,6 @@ dependencies = [ "nserror", "nsstring", "rand", - "runloop", "serde_cbor", "static_prefs", "thin-vec", diff --git a/dom/webauthn/authrs_bridge/Cargo.toml b/dom/webauthn/authrs_bridge/Cargo.toml index 18490d45f332..3cd4c3321524 100644 --- a/dom/webauthn/authrs_bridge/Cargo.toml +++ b/dom/webauthn/authrs_bridge/Cargo.toml @@ -11,7 +11,6 @@ moz_task = { path = "../../../xpcom/rust/moz_task" } nserror = { path = "../../../xpcom/rust/nserror" } nsstring = { path = "../../../xpcom/rust/nsstring" } rand = "0.8" -runloop = "0.1.0" serde_cbor = "0.11" static_prefs = { path = "../../../modules/libpref/init/static_prefs" } thin-vec = { version = "0.2.1", features = ["gecko-ffi"] } diff --git a/dom/webauthn/authrs_bridge/src/lib.rs b/dom/webauthn/authrs_bridge/src/lib.rs index 2c44a2bd18e0..c29594a2cc91 100644 --- a/dom/webauthn/authrs_bridge/src/lib.rs +++ b/dom/webauthn/authrs_bridge/src/lib.rs @@ -9,7 +9,7 @@ extern crate log; extern crate xpcom; use authenticator::{ - authenticatorservice::{AuthenticatorService, RegisterArgs, SignArgs}, + authenticatorservice::{RegisterArgs, SignArgs}, ctap2::attestation::AttestationStatement, ctap2::commands::get_info::AuthenticatorVersion, ctap2::server::{ @@ -18,7 +18,7 @@ use authenticator::{ }, errors::{AuthenticatorError, PinError, U2FTokenError}, statecallback::StateCallback, - Assertion, Pin, RegisterResult, SignResult, StatusPinUv, StatusUpdate, + Assertion, Pin, RegisterResult, SignResult, StateMachine, StatusPinUv, StatusUpdate, }; use moz_task::RunnableBuilder; use nserror::{ @@ -40,7 +40,7 @@ use xpcom::interfaces::{ use xpcom::{xpcom_method, RefPtr}; mod test_token; -use test_token::{TestTokenManager, TestTokenManagerState, TestTokenTransport}; +use test_token::TestTokenManager; fn make_prompt(action: &str, tid: u64, origin: &str, browsing_context_id: u64) -> String { format!( @@ -396,10 +396,10 @@ fn status_callback( // 2) a channel through which to receive a pin callback. #[xpcom(implement(nsIWebAuthnTransport), atomic)] pub struct AuthrsTransport { - auth_service: RefCell, // interior mutable for use in XPCOM methods + usb_token_manager: RefCell, // interior mutable for use in XPCOM methods + test_token_manager: TestTokenManager, controller: Controller, pin_receiver: Arc>, - test_token_manager: TestTokenManager, } impl AuthrsTransport { @@ -408,6 +408,10 @@ impl AuthrsTransport { Err(NS_ERROR_NOT_IMPLEMENTED) } + // # Safety + // + // This will mutably borrow the controller pointer through a RefCell. The caller must ensure + // that at most one WebAuthn transaction is active at any given time. xpcom_method!(set_controller => SetController(aController: *const nsIWebAuthnController)); fn set_controller(&self, controller: *const nsIWebAuthnController) -> Result<(), nsresult> { self.controller.init(controller) @@ -428,6 +432,10 @@ impl AuthrsTransport { } } + // # Safety + // + // This will mutably borrow usb_token_manager through a RefCell. The caller must ensure that at + // most one WebAuthn transaction is active at any given time. xpcom_method!(make_credential => MakeCredential(aTid: u64, aBrowsingContextId: u64, aArgs: *const nsICtapRegisterArgs)); fn make_credential( &self, @@ -604,12 +612,35 @@ impl AuthrsTransport { }), ); - self.auth_service - .borrow_mut() - .register(timeout_ms as u64, info.into(), status_tx, state_callback) - .or(Err(NS_ERROR_FAILURE)) + // The authenticator crate provides an `AuthenticatorService` which can dispatch a request + // in parallel to any number of transports. We only support the USB transport in production + // configurations, so we do not need the full generality of `AuthenticatorService` here. + // We disable the USB transport in tests that use virtual devices. + if static_prefs::pref!("security.webauth.webauthn_enable_usbtoken") { + self.usb_token_manager.borrow_mut().register( + timeout_ms as u64, + info.into(), + status_tx, + state_callback, + ); + } else if static_prefs::pref!("security.webauth.webauthn_enable_softtoken") { + self.test_token_manager.register( + timeout_ms as u64, + info.into(), + status_tx, + state_callback, + ); + } else { + return Err(NS_ERROR_FAILURE); + } + + Ok(()) } + // # Safety + // + // This will mutably borrow usb_token_manager through a RefCell. The caller must ensure that at + // most one WebAuthn transaction is active at any given time. xpcom_method!(get_assertion => GetAssertion(aTid: u64, aBrowsingContextId: u64, aArgs: *const nsICtapSignArgs)); fn get_assertion( &self, @@ -731,22 +762,37 @@ impl AuthrsTransport { use_ctap1_fallback, }; - self.auth_service - .borrow_mut() - .sign(timeout_ms as u64, info.into(), status_tx, state_callback) - .or(Err(NS_ERROR_FAILURE)) + // As in `register`, we are intentionally avoiding `AuthenticatorService` here. + if static_prefs::pref!("security.webauth.webauthn_enable_usbtoken") { + self.usb_token_manager.borrow_mut().sign( + timeout_ms as u64, + info.into(), + status_tx, + state_callback, + ); + } else if static_prefs::pref!("security.webauth.webauthn_enable_softtoken") { + self.test_token_manager + .sign(timeout_ms as u64, info.into(), status_tx, state_callback); + } else { + return Err(NS_ERROR_FAILURE); + } + + Ok(()) } + // # Safety + // + // This will mutably borrow usb_token_manager through a RefCell. The caller must ensure that at + // most one WebAuthn transaction is active at any given time. xpcom_method!(cancel => Cancel()); - fn cancel(&self) -> Result { + fn cancel(&self) -> Result<(), nsresult> { // We may be waiting for a pin. Drop the channel to release the // state machine from `ask_user_for_pin`. drop(self.pin_receiver.lock().or(Err(NS_ERROR_FAILURE))?.take()); - match &self.auth_service.borrow_mut().cancel() { - Ok(_) => Ok(NS_OK), - Err(e) => Err(authrs_to_nserror(e)), - } + self.usb_token_manager.borrow_mut().cancel(); + + Ok(()) } xpcom_method!( @@ -862,25 +908,11 @@ impl AuthrsTransport { pub extern "C" fn authrs_transport_constructor( result: *mut *const nsIWebAuthnTransport, ) -> nsresult { - let mut auth_service = match AuthenticatorService::new() { - Ok(auth_service) => auth_service, - _ => return NS_ERROR_FAILURE, - }; - - let enable_usb_transports = static_prefs::pref!("security.webauth.webauthn_enable_usbtoken"); - if enable_usb_transports { - auth_service.add_u2f_usb_hid_platform_transports(); - } - - let test_token_manager_state = TestTokenManagerState::new(); - let transport = TestTokenTransport::new(Arc::clone(&test_token_manager_state)); - auth_service.add_transport(Box::new(transport)); - let wrapper = AuthrsTransport::allocate(InitAuthrsTransport { - auth_service: RefCell::new(auth_service), + usb_token_manager: RefCell::new(StateMachine::new()), + test_token_manager: TestTokenManager::new(), controller: Controller(RefCell::new(std::ptr::null())), pin_receiver: Arc::new(Mutex::new(None)), - test_token_manager: TestTokenManager::new(test_token_manager_state), }); unsafe { RefPtr::new(wrapper.coerce::()).forget(&mut *result); diff --git a/dom/webauthn/authrs_bridge/src/test_token.rs b/dom/webauthn/authrs_bridge/src/test_token.rs index 8479a02433b5..54b771935e4c 100644 --- a/dom/webauthn/authrs_bridge/src/test_token.rs +++ b/dom/webauthn/authrs_bridge/src/test_token.rs @@ -2,7 +2,7 @@ * 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/. */ -use authenticator::authenticatorservice::{AuthenticatorTransport, RegisterArgs, SignArgs}; +use authenticator::authenticatorservice::{RegisterArgs, SignArgs}; use authenticator::crypto::{ecdsa_p256_sha256_sign_raw, COSEAlgorithm, COSEKey, SharedSecret}; use authenticator::ctap2::{ attestation::{ @@ -11,7 +11,7 @@ use authenticator::ctap2::{ }, client_data::ClientDataHash, commands::{ - client_pin::{ClientPIN, ClientPinResponse, PINSubcommand, Pin}, + client_pin::{ClientPIN, ClientPinResponse, PINSubcommand}, get_assertion::{Assertion, GetAssertion, GetAssertionResponse, GetAssertionResult}, get_info::{AuthenticatorInfo, AuthenticatorOptions, AuthenticatorVersion}, get_version::{GetVersion, U2FInfo}, @@ -30,13 +30,12 @@ use authenticator::{RegisterResult, SignResult, StatusUpdate}; use nserror::{nsresult, NS_ERROR_FAILURE, NS_ERROR_INVALID_ARG, NS_ERROR_NOT_IMPLEMENTED, NS_OK}; use nsstring::{nsACString, nsCString}; use rand::{thread_rng, RngCore}; -use runloop::RunLoop; use std::cell::{Ref, RefCell}; use std::collections::{hash_map::Entry, HashMap}; use std::ops::{Deref, DerefMut}; use std::sync::atomic::{AtomicU32, Ordering}; use std::sync::mpsc::Sender; -use std::sync::{Arc, Mutex}; +use std::sync::Mutex; use thin_vec::ThinVec; use xpcom::interfaces::nsICredentialParameters; use xpcom::{xpcom_method, RefPtr}; @@ -549,18 +548,6 @@ impl VirtualFidoDevice for TestToken { } } -pub(crate) struct TestTokenManagerState { - tokens: HashMap, -} - -impl TestTokenManagerState { - pub fn new() -> Arc> { - Arc::new(Mutex::new(TestTokenManagerState { - tokens: HashMap::new(), - })) - } -} - #[xpcom(implement(nsICredentialParameters), atomic)] struct CredentialParameters { credential_id: Vec, @@ -603,13 +590,14 @@ impl CredentialParameters { } } +#[derive(Default)] pub(crate) struct TestTokenManager { - state: Arc>, + state: Mutex>, } impl TestTokenManager { - pub fn new(state: Arc>) -> Self { - Self { state } + pub fn new() -> Self { + Default::default() } pub fn add_virtual_authenticator( @@ -630,7 +618,7 @@ impl TestTokenManager { ); loop { let id = rand::random::() & 0x1f_ffff_ffff_ffffu64; // Make the id safe for JS (53 bits) - match guard.deref_mut().tokens.entry(id) { + match guard.deref_mut().entry(id) { Entry::Occupied(_) => continue, Entry::Vacant(v) => { v.insert(token); @@ -644,7 +632,6 @@ impl TestTokenManager { let mut guard = self.state.lock().map_err(|_| NS_ERROR_FAILURE)?; guard .deref_mut() - .tokens .remove(&authenticator_id) .ok_or(NS_ERROR_INVALID_ARG)?; Ok(()) @@ -663,7 +650,6 @@ impl TestTokenManager { let mut guard = self.state.lock().map_err(|_| NS_ERROR_FAILURE)?; let token = guard .deref_mut() - .tokens .get_mut(&authenticator_id) .ok_or(NS_ERROR_INVALID_ARG)?; let rp = RelyingParty { @@ -688,7 +674,6 @@ impl TestTokenManager { ) -> Result>>, nsresult> { let mut guard = self.state.lock().map_err(|_| NS_ERROR_FAILURE)?; let token = guard - .tokens .get_mut(&authenticator_id) .ok_or(NS_ERROR_INVALID_ARG)?; let credentials = token.get_credentials(); @@ -719,7 +704,6 @@ impl TestTokenManager { let mut guard = self.state.lock().map_err(|_| NS_ERROR_FAILURE)?; let token = guard .deref_mut() - .tokens .get_mut(&authenticator_id) .ok_or(NS_ERROR_INVALID_ARG)?; if token.delete_credential(id) { @@ -733,7 +717,6 @@ impl TestTokenManager { let mut guard = self.state.lock().map_err(|_| NS_ERROR_FAILURE)?; let token = guard .deref_mut() - .tokens .get_mut(&authenticator_id) .ok_or(NS_ERROR_INVALID_ARG)?; token.delete_all_credentials(); @@ -746,158 +729,74 @@ impl TestTokenManager { is_user_verified: bool, ) -> Result<(), nsresult> { let mut guard = self.state.lock().map_err(|_| NS_ERROR_FAILURE)?; - let mut token = guard + let token = guard .deref_mut() - .tokens .get_mut(&authenticator_id) .ok_or(NS_ERROR_INVALID_ARG)?; token.is_user_verified = is_user_verified; Ok(()) } -} -pub(crate) struct TestTokenTransport { - state: Arc>, - queue: Option, -} - -impl TestTokenTransport { - pub fn new(state: Arc>) -> Self { - Self { state, queue: None } - } -} - -impl Drop for TestTokenTransport { - fn drop(&mut self) { - if let Some(queue) = self.queue.take() { - queue.cancel(); - } - } -} - -impl AuthenticatorTransport for TestTokenTransport { - fn register( - &mut self, - timeout: u64, + pub fn register( + &self, + _timeout: u64, ctap_args: RegisterArgs, status: Sender, callback: StateCallback>, - ) -> Result<(), AuthenticatorError> { - if let Some(queue) = self.queue.take() { - queue.cancel(); - } - + ) { if !static_prefs::pref!("security.webauth.webauthn_enable_softtoken") { - return Ok(()); + return; } - let state = self.state.clone(); - let queue = RunLoop::new_with_timeout( - move |alive| { - let mut state_obj = state.lock().unwrap(); + let mut state_obj = self.state.lock().unwrap(); - for token in state_obj.tokens.values_mut() { - let _ = token.init(); - if ctap2::register( - token, - ctap_args.clone(), - status.clone(), - callback.clone(), - alive, - ) { - // callback was called - return; - } - } - // Send an error, if the callback wasn't called already. - callback.call(Err(AuthenticatorError::U2FToken(U2FTokenError::NotAllowed))); - }, - timeout, - ) - .map_err(|_| AuthenticatorError::Platform)?; + // We query the tokens sequentially since the register operation will not block. + for token in state_obj.values_mut() { + let _ = token.init(); + if ctap2::register( + token, + ctap_args.clone(), + status.clone(), + callback.clone(), + &|| true, + ) { + // callback was called + return; + } + } - self.queue = Some(queue); - - Ok(()) + // Send an error, if the callback wasn't called already. + callback.call(Err(AuthenticatorError::U2FToken(U2FTokenError::NotAllowed))); } - fn sign( - &mut self, - timeout: u64, + pub fn sign( + &self, + _timeout: u64, ctap_args: SignArgs, status: Sender, callback: StateCallback>, - ) -> Result<(), AuthenticatorError> { - if let Some(queue) = self.queue.take() { - queue.cancel(); - } - + ) { if !static_prefs::pref!("security.webauth.webauthn_enable_softtoken") { - return Ok(()); + return; } - let state = self.state.clone(); - let queue = RunLoop::new_with_timeout( - move |alive| { - let mut state_obj = state.lock().unwrap(); + let mut state_obj = self.state.lock().unwrap(); - for token in state_obj.tokens.values_mut() { - let _ = token.init(); - if ctap2::sign( - token, - ctap_args.clone(), - status.clone(), - callback.clone(), - alive, - ) { - // callback was called - return; - } - } - // Send an error, if the callback wasn't called already. - callback.call(Err(AuthenticatorError::U2FToken(U2FTokenError::NotAllowed))); - }, - timeout, - ) - .map_err(|_| AuthenticatorError::Platform)?; - - self.queue = Some(queue); - - Ok(()) - } - - fn cancel(&mut self) -> Result<(), AuthenticatorError> { - if let Some(r) = self.queue.take() { - r.cancel(); + // We query the tokens sequentially since the sign operation will not block. + for token in state_obj.values_mut() { + let _ = token.init(); + if ctap2::sign( + token, + ctap_args.clone(), + status.clone(), + callback.clone(), + &|| true, + ) { + // callback was called + return; + } } - Ok(()) - } - - fn reset( - &mut self, - _timeout: u64, - _status: Sender, - _callback: StateCallback>, - ) -> Result<(), AuthenticatorError> { - unimplemented!(); - } - - fn set_pin( - &mut self, - _timeout: u64, - _new_pin: Pin, - _status: Sender, - _callback: StateCallback>, - ) -> Result<(), AuthenticatorError> { - unimplemented!(); - } - - fn manage( - &mut self, - _timeout: u64, - _status: Sender, - _callback: StateCallback>, - ) -> Result<(), AuthenticatorError> { - unimplemented!(); + // Send an error, if the callback wasn't called already. + callback.call(Err(AuthenticatorError::U2FToken(U2FTokenError::NotAllowed))); } }