From 9ba8ca92cdc33d716d62c6f1a0efaab335a2d285 Mon Sep 17 00:00:00 2001 From: John Schanck Date: Thu, 21 Sep 2023 16:07:45 +0000 Subject: [PATCH] Bug 1854016 - move webauthn signature selection logic to authrs_bridge. r=keeler Differential Revision: https://phabricator.services.mozilla.com/D188639 --- Cargo.lock | 1 + browser/base/content/browser.js | 2 +- dom/webauthn/WebAuthnController.cpp | 188 ++++++------------ dom/webauthn/WebAuthnController.h | 4 +- dom/webauthn/authrs_bridge/Cargo.toml | 1 + dom/webauthn/authrs_bridge/src/lib.rs | 176 ++++++++-------- dom/webauthn/authrs_bridge/src/test_token.rs | 87 ++++---- dom/webauthn/nsIWebAuthnController.idl | 3 +- .../browser/browser_fido_appid_extension.js | 4 +- .../browser/browser_webauthn_ipaddress.js | 2 +- .../tests/browser/browser_webauthn_prompts.js | 51 ++++- dom/webauthn/tests/browser/head.js | 24 ++- 12 files changed, 277 insertions(+), 266 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a07bbe9b2545..d358d047b2e5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -333,6 +333,7 @@ dependencies = [ "nsstring", "rand", "serde_cbor", + "serde_json", "static_prefs", "thin-vec", "xpcom", diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js index 3c365c4892bc..7f43f7bdf61d 100644 --- a/browser/base/content/browser.js +++ b/browser/base/content/browser.js @@ -7655,7 +7655,7 @@ var WebAuthnPromptHelper = { let secondaryActions = []; for (let i = 0; i < usernames.length; i++) { secondaryActions.push({ - label: unescape(decodeURIComponent(usernames[i])), + label: usernames[i], accessKey: i.toString(), callback(aState) { mgr.signatureSelectionCallback(tid, i); diff --git a/dom/webauthn/WebAuthnController.cpp b/dom/webauthn/WebAuthnController.cpp index 879720d0ffdd..a099ebf79074 100644 --- a/dom/webauthn/WebAuthnController.cpp +++ b/dom/webauthn/WebAuthnController.cpp @@ -51,9 +51,6 @@ static const char16_t kRegisterDirectPromptNotification[] = u"\"origin\":\"%s\",\"browsingContextId\":%llu}"; static const char16_t kCancelPromptNotification[] = u"{\"action\":\"cancel\",\"tid\":%llu}"; -static const char16_t kSelectSignResultNotification[] = - u"{\"action\":\"select-sign-result\",\"tid\":%llu," - u"\"origin\":\"%s\",\"browsingContextId\":%llu,\"usernames\":[%s]}"; /*********************************************************************** * U2FManager Implementation @@ -123,7 +120,6 @@ void WebAuthnController::ClearTransaction(bool cancel_prompt) { // Forget any pending registration. mPendingRegisterInfo.reset(); mPendingSignInfo.reset(); - mPendingSignResults.Clear(); mTransaction.reset(); } @@ -509,17 +505,13 @@ void WebAuthnController::Sign(PWebAuthnTransactionParent* aTransactionParent, } NS_IMETHODIMP -WebAuthnController::FinishSign( - uint64_t aTransactionId, - const nsTArray>& aResult) { +WebAuthnController::FinishSign(uint64_t aTransactionId, + nsICtapSignResult* aResult) { MOZ_ASSERT(XRE_IsParentProcess()); - nsTArray> ownedResult = aResult.Clone(); - nsCOMPtr r( - NewRunnableMethod>>( + NewRunnableMethod>( "WebAuthnController::RunFinishSign", this, - &WebAuthnController::RunFinishSign, aTransactionId, - std::move(ownedResult))); + &WebAuthnController::RunFinishSign, aTransactionId, aResult)); if (!gWebAuthnBackgroundThread) { return NS_ERROR_FAILURE; @@ -531,82 +523,78 @@ WebAuthnController::FinishSign( } void WebAuthnController::RunFinishSign( - uint64_t aTransactionId, - const nsTArray>& aResult) { + uint64_t aTransactionId, const RefPtr& aResult) { mozilla::ipc::AssertIsOnBackgroundThread(); if (mTransaction.isNothing() || aTransactionId != mTransaction.ref().mTransactionId) { return; } - if (aResult.Length() == 0) { + nsresult status; + nsresult rv = aResult->GetStatus(&status); + if (NS_WARN_IF(NS_FAILED(rv))) { + AbortTransaction(aTransactionId, NS_ERROR_DOM_NOT_ALLOWED_ERR, true); + return; + } + if (NS_FAILED(status)) { + bool shouldCancelActiveDialog = true; + if (status == NS_ERROR_DOM_INVALID_STATE_ERR) { + // PIN-related errors, e.g. blocked token. Let the dialog show to inform + // the user + shouldCancelActiveDialog = false; + } Telemetry::ScalarAdd(Telemetry::ScalarID::SECURITY_WEBAUTHN_USED, u"CTAPSignAbort"_ns, 1); + AbortTransaction(aTransactionId, NS_ERROR_DOM_NOT_ALLOWED_ERR, + shouldCancelActiveDialog); + return; + } + + nsTArray credentialId; + rv = aResult->GetCredentialId(credentialId); + if (NS_WARN_IF(NS_FAILED(rv))) { AbortTransaction(aTransactionId, NS_ERROR_DOM_NOT_ALLOWED_ERR, true); return; } - if (aResult.Length() == 1) { - nsresult status; - nsresult rv = aResult[0]->GetStatus(&status); - if (NS_WARN_IF(NS_FAILED(rv))) { - AbortTransaction(aTransactionId, NS_ERROR_DOM_NOT_ALLOWED_ERR, true); - return; - } - if (NS_FAILED(status)) { - bool shouldCancelActiveDialog = true; - if (status == NS_ERROR_DOM_INVALID_STATE_ERR) { - // PIN-related errors, e.g. blocked token. Let the dialog show to inform - // the user - shouldCancelActiveDialog = false; - } - Telemetry::ScalarAdd(Telemetry::ScalarID::SECURITY_WEBAUTHN_USED, - u"CTAPSignAbort"_ns, 1); - AbortTransaction(aTransactionId, NS_ERROR_DOM_NOT_ALLOWED_ERR, - shouldCancelActiveDialog); - return; - } - mPendingSignResults = aResult.Clone(); - RunResumeWithSelectedSignResult(aTransactionId, 0); + nsTArray signature; + rv = aResult->GetSignature(signature); + if (NS_WARN_IF(NS_FAILED(rv))) { + AbortTransaction(aTransactionId, NS_ERROR_DOM_NOT_ALLOWED_ERR, true); return; } - // If we more than one assertion, all of them should have OK status. - for (const auto& assertion : aResult) { - nsresult status; - nsresult rv = assertion->GetStatus(&status); - if (NS_WARN_IF(NS_FAILED(rv))) { - AbortTransaction(aTransactionId, NS_ERROR_DOM_NOT_ALLOWED_ERR, true); - return; - } - if (NS_WARN_IF(NS_FAILED(status))) { - Telemetry::ScalarAdd(Telemetry::ScalarID::SECURITY_WEBAUTHN_USED, - u"CTAPSignAbort"_ns, 1); - AbortTransaction(aTransactionId, NS_ERROR_DOM_NOT_ALLOWED_ERR, true); - return; - } + nsTArray authenticatorData; + rv = aResult->GetAuthenticatorData(authenticatorData); + if (NS_WARN_IF(NS_FAILED(rv))) { + AbortTransaction(aTransactionId, NS_ERROR_DOM_NOT_ALLOWED_ERR, true); + return; } - nsCString usernames; - StringJoinAppend( - usernames, ","_ns, aResult, - [](nsACString& dst, const RefPtr& assertion) { - nsCString username; - nsresult rv = assertion->GetUserName(username); - if (NS_FAILED(rv)) { - username.Assign(""); - } - nsCString escaped_username; - NS_Escape(username, escaped_username, url_XAlphas); - dst.Append("\""_ns + escaped_username + "\""_ns); - }); + nsTArray rpIdHash; + rv = aResult->GetRpIdHash(rpIdHash); + if (NS_WARN_IF(NS_FAILED(rv))) { + AbortTransaction(aTransactionId, NS_ERROR_DOM_NOT_ALLOWED_ERR, true); + return; + } - mPendingSignResults = aResult.Clone(); - NS_ConvertUTF16toUTF8 origin(mPendingSignInfo.ref().Origin()); - SendPromptNotification(kSelectSignResultNotification, - mTransaction.ref().mTransactionId, origin.get(), - mPendingSignInfo.ref().BrowsingContextId(), - usernames.get()); + nsTArray userHandle; + Unused << aResult->GetUserHandle(userHandle); // optional + + nsTArray extensions; + if (mTransaction.ref().mAppIdHash.isSome()) { + bool usedAppId = (rpIdHash == mTransaction.ref().mAppIdHash.ref()); + extensions.AppendElement(WebAuthnExtensionResultAppId(usedAppId)); + } + + WebAuthnGetAssertionResult result(mTransaction.ref().mClientDataJSON, + credentialId, signature, authenticatorData, + extensions, userHandle); + + Telemetry::ScalarAdd(Telemetry::ScalarID::SECURITY_WEBAUTHN_USED, + u"CTAPSignFinish"_ns, 1); + Unused << mTransactionParent->SendConfirmSign(aTransactionId, result); + ClearTransaction(true); } NS_IMETHODIMP @@ -630,64 +618,12 @@ WebAuthnController::SignatureSelectionCallback(uint64_t aTransactionId, } void WebAuthnController::RunResumeWithSelectedSignResult( - uint64_t aTransactionId, uint64_t idx) { + uint64_t aTransactionId, uint64_t aIndex) { mozilla::ipc::AssertIsOnBackgroundThread(); - if (mTransaction.isNothing() || - mTransaction.ref().mTransactionId != aTransactionId) { - return; + + if (mTransportImpl) { + mTransportImpl->SelectionCallback(aTransactionId, aIndex); } - - if (NS_WARN_IF(mPendingSignResults.Length() <= idx)) { - return; - } - - RefPtr& selected = mPendingSignResults[idx]; - - nsTArray credentialId; - nsresult rv = selected->GetCredentialId(credentialId); - if (NS_WARN_IF(NS_FAILED(rv))) { - AbortTransaction(aTransactionId, NS_ERROR_DOM_NOT_ALLOWED_ERR, true); - return; - } - - nsTArray signature; - rv = selected->GetSignature(signature); - if (NS_WARN_IF(NS_FAILED(rv))) { - AbortTransaction(aTransactionId, NS_ERROR_DOM_NOT_ALLOWED_ERR, true); - return; - } - - nsTArray authenticatorData; - rv = selected->GetAuthenticatorData(authenticatorData); - if (NS_WARN_IF(NS_FAILED(rv))) { - AbortTransaction(aTransactionId, NS_ERROR_DOM_NOT_ALLOWED_ERR, true); - return; - } - - nsTArray rpIdHash; - rv = selected->GetRpIdHash(rpIdHash); - if (NS_WARN_IF(NS_FAILED(rv))) { - AbortTransaction(aTransactionId, NS_ERROR_DOM_NOT_ALLOWED_ERR, true); - return; - } - - nsTArray userHandle; - Unused << selected->GetUserHandle(userHandle); // optional - - nsTArray extensions; - if (mTransaction.ref().mAppIdHash.isSome()) { - bool usedAppId = (rpIdHash == mTransaction.ref().mAppIdHash.ref()); - extensions.AppendElement(WebAuthnExtensionResultAppId(usedAppId)); - } - - WebAuthnGetAssertionResult result(mTransaction.ref().mClientDataJSON, - credentialId, signature, authenticatorData, - extensions, userHandle); - - Telemetry::ScalarAdd(Telemetry::ScalarID::SECURITY_WEBAUTHN_USED, - u"CTAPSignFinish"_ns, 1); - Unused << mTransactionParent->SendConfirmSign(aTransactionId, result); - ClearTransaction(true); } NS_IMETHODIMP diff --git a/dom/webauthn/WebAuthnController.h b/dom/webauthn/WebAuthnController.h index 7e8594feeef0..c78a65e7fa8e 100644 --- a/dom/webauthn/WebAuthnController.h +++ b/dom/webauthn/WebAuthnController.h @@ -74,7 +74,7 @@ class WebAuthnController final : public nsIWebAuthnController { void RunFinishRegister(uint64_t aTransactionId, const RefPtr& aResult); void RunFinishSign(uint64_t aTransactionId, - const nsTArray>& aResult); + const RefPtr& aResult); // The main thread runnable function for "nsIU2FTokenManager.ResumeRegister". void RunResumeRegister(uint64_t aTransactionId, bool aForceNoneAttestation); @@ -98,8 +98,6 @@ class WebAuthnController final : public nsIWebAuthnController { // Pending registration info while we wait for user input. Maybe mPendingSignInfo; - nsTArray> mPendingSignResults; - class Transaction { public: Transaction(uint64_t aTransactionId, const nsTArray& aRpIdHash, diff --git a/dom/webauthn/authrs_bridge/Cargo.toml b/dom/webauthn/authrs_bridge/Cargo.toml index d99caf5e3337..8fe32decc1cb 100644 --- a/dom/webauthn/authrs_bridge/Cargo.toml +++ b/dom/webauthn/authrs_bridge/Cargo.toml @@ -13,6 +13,7 @@ nserror = { path = "../../../xpcom/rust/nserror" } nsstring = { path = "../../../xpcom/rust/nsstring" } rand = "0.8" serde_cbor = "0.11" +serde_json = "1.0" static_prefs = { path = "../../../modules/libpref/init/static_prefs" } thin-vec = { version = "0.2.1", features = ["gecko-ffi"] } xpcom = { path = "../../../xpcom/rust/xpcom" } diff --git a/dom/webauthn/authrs_bridge/src/lib.rs b/dom/webauthn/authrs_bridge/src/lib.rs index 52a25b188757..f38a607561f8 100644 --- a/dom/webauthn/authrs_bridge/src/lib.rs +++ b/dom/webauthn/authrs_bridge/src/lib.rs @@ -19,7 +19,7 @@ use authenticator::{ }, errors::{AuthenticatorError, PinError, U2FTokenError}, statecallback::StateCallback, - Assertion, Pin, RegisterResult, SignResult, StateMachine, StatusPinUv, StatusUpdate, + Pin, RegisterResult, SignResult, StateMachine, StatusPinUv, StatusUpdate, }; use base64::Engine; use moz_task::RunnableBuilder; @@ -31,6 +31,7 @@ use nserror::{ }; use nsstring::{nsACString, nsCString, nsString}; use serde_cbor; +use serde_json::json; use std::cell::RefCell; use std::sync::mpsc::{channel, Receiver, RecvError, Sender}; use std::sync::{Arc, Mutex}; @@ -73,6 +74,29 @@ fn make_pin_required_prompt( ) } +fn make_user_selection_prompt( + tid: u64, + origin: &str, + browsing_context_id: u64, + user_entities: &[PublicKeyCredentialUserEntity], +) -> String { + // Bug 1854280: "Unknown username" should be a localized string here. + let usernames: Vec = user_entities + .iter() + .map(|entity| { + entity + .name + .clone() + .unwrap_or("".to_string()) + }) + .collect(); + let usernames_json = json!(usernames); + let out = format!( + r#"{{"action":"select-sign-result","tid":{tid},"origin":"{origin}","browsingContextId":{browsing_context_id},"usernames":{usernames_json}}}"#, + ); + out +} + fn authrs_to_nserror(e: &AuthenticatorError) -> nsresult { match e { AuthenticatorError::U2FToken(U2FTokenError::NotSupported) => NS_ERROR_DOM_NOT_SUPPORTED_ERR, @@ -190,73 +214,51 @@ impl WebAuthnAttObj { #[xpcom(implement(nsICtapSignResult), atomic)] pub struct CtapSignResult { - result: Result, + result: Result, } impl CtapSignResult { xpcom_method!(get_credential_id => GetCredentialId() -> ThinVec); fn get_credential_id(&self) -> Result, nsresult> { - let mut out = ThinVec::new(); - if let Ok(assertion) = &self.result { - if let Some(cred) = &assertion.credentials { - out.extend_from_slice(&cred.id); - return Ok(out); - } - } - Err(NS_ERROR_FAILURE) + let rv = NS_ERROR_FAILURE; + let inner = self.result.as_ref().or(Err(rv))?; + let cred = inner.assertion.credentials.as_ref().ok_or(rv)?; + Ok(cred.id.as_slice().into()) } xpcom_method!(get_signature => GetSignature() -> ThinVec); fn get_signature(&self) -> Result, nsresult> { - let mut out = ThinVec::new(); - if let Ok(assertion) = &self.result { - out.extend_from_slice(&assertion.signature); - return Ok(out); - } - Err(NS_ERROR_FAILURE) + let inner = self.result.as_ref().or(Err(NS_ERROR_FAILURE))?; + Ok(inner.assertion.signature.as_slice().into()) } xpcom_method!(get_authenticator_data => GetAuthenticatorData() -> ThinVec); fn get_authenticator_data(&self) -> Result, nsresult> { - self.result - .as_ref() - .map(|assertion| assertion.auth_data.to_vec().into()) - .or(Err(NS_ERROR_FAILURE)) + let inner = self.result.as_ref().or(Err(NS_ERROR_FAILURE))?; + Ok(inner.assertion.auth_data.to_vec().into()) } xpcom_method!(get_user_handle => GetUserHandle() -> ThinVec); fn get_user_handle(&self) -> Result, nsresult> { - let mut out = ThinVec::new(); - if let Ok(assertion) = &self.result { - if let Some(user) = &assertion.user { - out.extend_from_slice(&user.id); - return Ok(out); - } - } - Err(NS_ERROR_FAILURE) + let rv = NS_ERROR_NOT_AVAILABLE; + let inner = self.result.as_ref().or(Err(rv))?; + let user = &inner.assertion.user.as_ref().ok_or(rv)?; + Ok(user.id.as_slice().into()) } xpcom_method!(get_user_name => GetUserName() -> nsACString); fn get_user_name(&self) -> Result { - if let Ok(assertion) = &self.result { - if let Some(user) = &assertion.user { - if let Some(name) = &user.name { - return Ok(nsCString::from(name)); - } - } - } - Err(NS_ERROR_NOT_AVAILABLE) + let rv = NS_ERROR_NOT_AVAILABLE; + let inner = self.result.as_ref().or(Err(rv))?; + let user = inner.assertion.user.as_ref().ok_or(rv)?; + let name = user.name.as_ref().ok_or(rv)?; + Ok(nsCString::from(name)) } xpcom_method!(get_rp_id_hash => GetRpIdHash() -> ThinVec); fn get_rp_id_hash(&self) -> Result, nsresult> { - // assertion.auth_data.rp_id_hash - let mut out = ThinVec::new(); - if let Ok(assertion) = &self.result { - out.extend_from_slice(&assertion.auth_data.rp_id_hash.0); - return Ok(out); - } - Err(NS_ERROR_FAILURE) + let inner = self.result.as_ref().or(Err(NS_ERROR_FAILURE))?; + Ok(inner.assertion.auth_data.rp_id_hash.0.into()) } xpcom_method!(get_status => GetStatus() -> nsresult); @@ -321,38 +323,22 @@ impl Controller { if (*self.0.borrow()).is_null() { return Err(NS_ERROR_FAILURE); } - - // If result is an error, we return a single CtapSignResult that has its status field set - // to an error. Otherwise we convert the entries of SignResult (= Vec) into - // CtapSignResults with OK statuses. - let mut assertions: ThinVec>> = ThinVec::new(); - match result { - Err(e) => assertions.push( - CtapSignResult::allocate(InitCtapSignResult { result: Err(e) }) - .query_interface::(), - ), - Ok(result) => { - assertions.push( - CtapSignResult::allocate(InitCtapSignResult { - result: Ok(result.assertion), - }) - .query_interface::(), - ); - } - } - + let wrapped_result = CtapSignResult::allocate(InitCtapSignResult { result }) + .query_interface::() + .ok_or(NS_ERROR_FAILURE)?; unsafe { - (**(self.0.borrow())).FinishSign(tid, &mut assertions); + (**(self.0.borrow())).FinishSign(tid, wrapped_result.coerce()); } Ok(()) } } -// The state machine creates a Sender/Receiver channel in ask_user_for_pin. It passes the -// Sender through status_callback, which stores the Sender in the pin_receiver field of an -// AuthrsTransport. The u64 in PinReceiver is a transaction ID, which the AuthrsTransport uses the -// transaction ID as a consistency check. +// A transaction may create a channel to ask a user for additional input, e.g. a PIN. The Sender +// component of this channel is sent to an AuthrsTransport in a StatusUpdate. AuthrsTransport +// caches the sender along with the expected (u64) transaction ID, which is used as a consistency +// check in callbacks. type PinReceiver = Option<(u64, Sender)>; +type SelectionReceiver = Option<(u64, Sender>)>; fn status_callback( status_rx: Receiver, @@ -361,6 +347,7 @@ fn status_callback( browsing_context_id: u64, controller: Controller, pin_receiver: Arc>, /* Shared with an AuthrsTransport */ + selection_receiver: Arc>, /* Shared with an AuthrsTransport */ ) { loop { match status_rx.recv() { @@ -376,23 +363,13 @@ fn status_callback( controller.send_prompt(tid, ¬ification_str); } Ok(StatusUpdate::PinUvError(StatusPinUv::PinRequired(sender))) => { - let guard = pin_receiver.lock(); - if let Ok(mut entry) = guard { - entry.replace((tid, sender)); - } else { - return; - } + pin_receiver.lock().unwrap().replace((tid, sender)); let notification_str = make_pin_required_prompt(tid, origin, browsing_context_id, false, -1); controller.send_prompt(tid, ¬ification_str); } Ok(StatusUpdate::PinUvError(StatusPinUv::InvalidPin(sender, attempts))) => { - let guard = pin_receiver.lock(); - if let Ok(mut entry) = guard { - entry.replace((tid, sender)); - } else { - return; - } + pin_receiver.lock().unwrap().replace((tid, sender)); let notification_str = make_pin_required_prompt( tid, origin, @@ -437,8 +414,12 @@ fn status_callback( Ok(StatusUpdate::InteractiveManagement(_)) => { debug!("STATUS: interactive management"); } - Ok(StatusUpdate::SelectResultNotice(_, _)) => { - // The selection prompt will be added in Bug 1854016 + Ok(StatusUpdate::SelectResultNotice(sender, choices)) => { + debug!("STATUS: select result notice"); + selection_receiver.lock().unwrap().replace((tid, sender)); + let notification_str = + make_user_selection_prompt(tid, origin, browsing_context_id, &choices); + controller.send_prompt(tid, ¬ification_str); } Err(RecvError) => { debug!("STATUS: end"); @@ -459,6 +440,7 @@ pub struct AuthrsTransport { test_token_manager: TestTokenManager, controller: Controller, pin_receiver: Arc>, + selection_receiver: Arc>, } impl AuthrsTransport { @@ -480,7 +462,6 @@ impl AuthrsTransport { fn pin_callback(&self, transaction_id: u64, pin: &nsACString) -> Result<(), nsresult> { let mut guard = self.pin_receiver.lock().or(Err(NS_ERROR_FAILURE))?; match guard.take() { - // The pin_receiver is single-use. Some((tid, channel)) if tid == transaction_id => channel .send(Pin::new(&pin.to_string())) .or(Err(NS_ERROR_FAILURE)), @@ -491,6 +472,20 @@ impl AuthrsTransport { } } + xpcom_method!(selection_callback => SelectionCallback(aTransactionId: u64, aSelection: u64)); + fn selection_callback(&self, transaction_id: u64, selection: u64) -> Result<(), nsresult> { + let mut guard = self.selection_receiver.lock().or(Err(NS_ERROR_FAILURE))?; + match guard.take() { + Some((tid, channel)) if tid == transaction_id => channel + .send(Some(selection as usize)) + .or(Err(NS_ERROR_FAILURE)), + // Either we weren't expecting a selection, or the controller is confused + // about which transaction is active. Neither is recoverable, so it's + // OK to drop the SelectionReceiver here. + _ => Err(NS_ERROR_FAILURE), + } + } + // # Safety // // This will mutably borrow usb_token_manager through a RefCell. The caller must ensure that at @@ -624,6 +619,7 @@ impl AuthrsTransport { let (status_tx, status_rx) = channel::(); let pin_receiver = self.pin_receiver.clone(); + let selection_receiver = self.selection_receiver.clone(); let controller = self.controller.clone(); let status_origin = origin.to_string(); RunnableBuilder::new( @@ -636,6 +632,7 @@ impl AuthrsTransport { browsing_context_id, controller, pin_receiver, + selection_receiver, ) }, ) @@ -755,6 +752,7 @@ impl AuthrsTransport { let (status_tx, status_rx) = channel::(); let pin_receiver = self.pin_receiver.clone(); + let selection_receiver = self.selection_receiver.clone(); let controller = self.controller.clone(); let status_origin = origin.to_string(); RunnableBuilder::new("AuthrsTransport::GetAssertion::StatusReceiver", move || { @@ -765,6 +763,7 @@ impl AuthrsTransport { browsing_context_id, controller, pin_receiver, + selection_receiver, ) }) .may_block(true) @@ -830,9 +829,15 @@ impl AuthrsTransport { // most one WebAuthn transaction is active at any given time. xpcom_method!(cancel => Cancel()); 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`. + // The transaction thread may be waiting for user input. Dropping the associated channel + // will cause the transaction to error out with a "CancelledByUser" result. drop(self.pin_receiver.lock().or(Err(NS_ERROR_FAILURE))?.take()); + drop( + self.selection_receiver + .lock() + .or(Err(NS_ERROR_FAILURE))? + .take(), + ); self.usb_token_manager.borrow_mut().cancel(); @@ -969,6 +974,7 @@ pub extern "C" fn authrs_transport_constructor( test_token_manager: TestTokenManager::new(), controller: Controller(RefCell::new(std::ptr::null())), pin_receiver: Arc::new(Mutex::new(None)), + selection_receiver: Arc::new(Mutex::new(None)), }); #[cfg(feature = "fuzzing")] diff --git a/dom/webauthn/authrs_bridge/src/test_token.rs b/dom/webauthn/authrs_bridge/src/test_token.rs index 8b2c8990363f..fe934eabe4cb 100644 --- a/dom/webauthn/authrs_bridge/src/test_token.rs +++ b/dom/webauthn/authrs_bridge/src/test_token.rs @@ -31,6 +31,7 @@ use authenticator::{ctap2, statecallback::StateCallback}; use authenticator::{FidoDevice, FidoDeviceIO, FidoProtocol, VirtualFidoDevice}; use authenticator::{RegisterResult, SignResult, StatusUpdate}; use base64::Engine; +use moz_task::RunnableBuilder; 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}; @@ -39,7 +40,7 @@ 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::Mutex; +use std::sync::{Arc, Mutex}; use thin_vec::ThinVec; use xpcom::interfaces::nsICredentialParameters; use xpcom::{xpcom_method, RefPtr}; @@ -626,7 +627,7 @@ impl CredentialParameters { #[derive(Default)] pub(crate) struct TestTokenManager { - state: Mutex>, + state: Arc>>, } impl TestTokenManager { @@ -772,7 +773,7 @@ impl TestTokenManager { pub fn register( &self, - _timeout: u64, + _timeout_ms: u64, ctap_args: RegisterArgs, status: Sender, callback: StateCallback>, @@ -781,30 +782,37 @@ impl TestTokenManager { return; } - let mut state_obj = self.state.lock().unwrap(); + let state_obj = self.state.clone(); - // 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; + // Registration doesn't currently block, but it might in a future version, so we run it on + // a background thread. + let _ = RunnableBuilder::new("TestTokenManager::register", move || { + // TODO(Bug 1854278) We should actually run one thread per token here + // and attempt to fulfill this request in parallel. + for token in state_obj.lock().unwrap().values_mut() { + let _ = token.init(); + if ctap2::register( + token, + ctap_args.clone(), + status.clone(), + callback.clone(), + &|| true, + ) { + // callback was called + return; + } } - } - // Send an error, if the callback wasn't called already. - callback.call(Err(AuthenticatorError::U2FToken(U2FTokenError::NotAllowed))); + // Send an error, if the callback wasn't called already. + callback.call(Err(AuthenticatorError::U2FToken(U2FTokenError::NotAllowed))); + }) + .may_block(true) + .dispatch_background_task(); } pub fn sign( &self, - _timeout: u64, + _timeout_ms: u64, ctap_args: SignArgs, status: Sender, callback: StateCallback>, @@ -813,23 +821,30 @@ impl TestTokenManager { return; } - let mut state_obj = self.state.lock().unwrap(); + let state_obj = self.state.clone(); - // 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; + // Signing can block during signature selection, so we need to run it on a background thread. + let _ = RunnableBuilder::new("TestTokenManager::sign", move || { + // TODO(Bug 1854278) We should actually run one thread per token here + // and attempt to fulfill this request in parallel. + for token in state_obj.lock().unwrap().values_mut() { + let _ = token.init(); + if ctap2::sign( + token, + ctap_args.clone(), + status.clone(), + callback.clone(), + &|| true, + ) { + // callback was called + return; + } } - } - // Send an error, if the callback wasn't called already. - callback.call(Err(AuthenticatorError::U2FToken(U2FTokenError::NotAllowed))); + + // Send an error, if the callback wasn't called already. + callback.call(Err(AuthenticatorError::U2FToken(U2FTokenError::NotAllowed))); + }) + .may_block(true) + .dispatch_background_task(); } } diff --git a/dom/webauthn/nsIWebAuthnController.idl b/dom/webauthn/nsIWebAuthnController.idl index c483c5fa56df..2331d074142f 100644 --- a/dom/webauthn/nsIWebAuthnController.idl +++ b/dom/webauthn/nsIWebAuthnController.idl @@ -194,7 +194,7 @@ interface nsIWebAuthnController : nsISupports // Authenticator callbacks [noscript] void sendPromptNotificationPreformatted(in uint64_t aTransactionId, in ACString aJSON); [noscript] void finishRegister(in uint64_t aTransactionId, in nsICtapRegisterResult aResult); - [noscript] void finishSign(in uint64_t aTransactionId, in Array aResult); + [noscript] void finishSign(in uint64_t aTransactionId, in nsICtapSignResult aResult); }; [scriptable, uuid(6c4ecd9f-57c0-4d7d-8080-bf6e4d499f8f)] @@ -263,6 +263,7 @@ interface nsIWebAuthnTransport : nsISupports // These are prompt callbacks but they're not intended to be called directly from // JavaScript---they are proxied through the nsIWebAuthnController first. + [noscript] void selectionCallback(in uint64_t aTransactionId, in uint64_t aIndex); [noscript] void pinCallback(in uint64_t aTransactionId, in ACString aPin); [noscript] void cancel(); }; diff --git a/dom/webauthn/tests/browser/browser_fido_appid_extension.js b/dom/webauthn/tests/browser/browser_fido_appid_extension.js index 5b62fadf53a1..fca4443074aa 100644 --- a/dom/webauthn/tests/browser/browser_fido_appid_extension.js +++ b/dom/webauthn/tests/browser/browser_fido_appid_extension.js @@ -19,7 +19,9 @@ add_task(async function test_appid() { let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_URL); // The FIDO AppId extension can't be used for MakeCredential. - await promiseWebAuthnMakeCredential(tab, "none", { appid: gAppId }) + await promiseWebAuthnMakeCredential(tab, "none", "discouraged", { + appid: gAppId, + }) .then(arrivingHereIsBad) .catch(expectNotSupportedError); diff --git a/dom/webauthn/tests/browser/browser_webauthn_ipaddress.js b/dom/webauthn/tests/browser/browser_webauthn_ipaddress.js index 49ec10145052..6658ae5f02ca 100644 --- a/dom/webauthn/tests/browser/browser_webauthn_ipaddress.js +++ b/dom/webauthn/tests/browser/browser_webauthn_ipaddress.js @@ -21,7 +21,7 @@ add_task(async function test_appid() { // Open a new tab. let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_URL); - await promiseWebAuthnMakeCredential(tab, "none", {}) + await promiseWebAuthnMakeCredential(tab) .then(arrivingHereIsBad) .catch(expectSecurityError); diff --git a/dom/webauthn/tests/browser/browser_webauthn_prompts.js b/dom/webauthn/tests/browser/browser_webauthn_prompts.js index 3f5613568797..bbc2a19b3f14 100644 --- a/dom/webauthn/tests/browser/browser_webauthn_prompts.js +++ b/dom/webauthn/tests/browser/browser_webauthn_prompts.js @@ -11,6 +11,7 @@ XPCOMUtils.defineLazyScriptGetter( ); const TEST_URL = "https://example.com/"; +var gAuthenticatorId; add_task(async function test_setup_usbtoken() { return SpecialPowers.pushPrefEnv({ @@ -39,7 +40,7 @@ add_task(async function test_setup_fullscreen() { add_task(test_fullscreen_show_nav_toolbar); add_task(test_no_fullscreen_dom); add_task(async function test_setup_softtoken() { - add_virtual_authenticator(); + gAuthenticatorId = add_virtual_authenticator(); return SpecialPowers.pushPrefEnv({ set: [ ["security.webauth.webauthn_enable_softtoken", true], @@ -49,6 +50,7 @@ add_task(async function test_setup_softtoken() { }); add_task(test_register_direct_proceed); add_task(test_register_direct_proceed_anon); +add_task(test_select_sign_result); function promiseNotification(id) { return new Promise(resolve => { @@ -123,7 +125,7 @@ async function test_register() { // Request a new credential and wait for the prompt. let active = true; - let request = promiseWebAuthnMakeCredential(tab, "none", {}) + let request = promiseWebAuthnMakeCredential(tab) .then(arrivingHereIsBad) .catch(expectNotAllowedError) .then(() => (active = false)); @@ -144,7 +146,7 @@ async function test_register_escape() { // Request a new credential and wait for the prompt. let active = true; - let request = promiseWebAuthnMakeCredential(tab, "none", {}) + let request = promiseWebAuthnMakeCredential(tab) .then(arrivingHereIsBad) .catch(expectNotAllowedError) .then(() => (active = false)); @@ -207,7 +209,7 @@ async function test_register_direct_cancel() { // Request a new credential with direct attestation and wait for the prompt. let active = true; - let promise = promiseWebAuthnMakeCredential(tab, "direct", {}) + let promise = promiseWebAuthnMakeCredential(tab, "direct") .then(arrivingHereIsBad) .catch(expectNotAllowedError) .then(() => (active = false)); @@ -230,7 +232,7 @@ async function test_tab_switching() { // Request a new credential and wait for the prompt. let active = true; - let request = promiseWebAuthnMakeCredential(tab_one, "none", {}) + let request = promiseWebAuthnMakeCredential(tab_one) .then(arrivingHereIsBad) .catch(expectNotAllowedError) .then(() => (active = false)); @@ -276,7 +278,7 @@ async function test_window_switching() { // Request a new credential and wait for the prompt. let active = true; - let request = promiseWebAuthnMakeCredential(tab, "none", {}) + let request = promiseWebAuthnMakeCredential(tab) .then(arrivingHereIsBad) .catch(expectNotAllowedError) .then(() => (active = false)); @@ -324,7 +326,7 @@ async function test_register_direct_proceed() { let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_URL); // Request a new credential with direct attestation and wait for the prompt. - let request = promiseWebAuthnMakeCredential(tab, "direct", {}); + let request = promiseWebAuthnMakeCredential(tab, "direct"); await promiseNotification("webauthn-prompt-register-direct"); // Proceed. @@ -342,7 +344,7 @@ async function test_register_direct_proceed_anon() { let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_URL); // Request a new credential with direct attestation and wait for the prompt. - let request = promiseWebAuthnMakeCredential(tab, "direct", {}); + let request = promiseWebAuthnMakeCredential(tab, "direct"); await promiseNotification("webauthn-prompt-register-direct"); // Check "anonymize anyway" and proceed. @@ -356,6 +358,35 @@ async function test_register_direct_proceed_anon() { await BrowserTestUtils.removeTab(tab); } +async function test_select_sign_result() { + // Open a new tab. + let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_URL); + + // Make two discoverable credentials for the same RP ID so that + // the user has to select one to return. + let cred1 = await addCredential(gAuthenticatorId, "example.com"); + let cred2 = await addCredential(gAuthenticatorId, "example.com"); + + let active = true; + let request = promiseWebAuthnGetAssertionDiscoverable(tab) + .then(arrivingHereIsBad) + .catch(expectNotAllowedError) + .then(() => (active = false)); + + // Ensure the selection prompt is shown + await promiseNotification("webauthn-prompt-select-sign-result"); + + ok(active, "request is active"); + + // Cancel the request + PopupNotifications.panel.firstElementChild.button.click(); + await request; + + await removeCredential(gAuthenticatorId, cred1); + await removeCredential(gAuthenticatorId, cred2); + await BrowserTestUtils.removeTab(tab); +} + async function test_fullscreen_show_nav_toolbar() { let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, TEST_URL); @@ -375,7 +406,7 @@ async function test_fullscreen_show_nav_toolbar() { let navToolboxShownPromise = promiseNavToolboxStatus("shown"); let active = true; - let requestPromise = promiseWebAuthnMakeCredential(tab, "direct", {}) + let requestPromise = promiseWebAuthnMakeCredential(tab, "direct") .then(arrivingHereIsBad) .catch(expectNotAllowedError) .then(() => (active = false)); @@ -412,7 +443,7 @@ async function test_no_fullscreen_dom() { fullScreenPaintPromise = promiseFullScreenPaint(); let active = true; - let requestPromise = promiseWebAuthnMakeCredential(tab, "direct", {}) + let requestPromise = promiseWebAuthnMakeCredential(tab, "direct") .then(arrivingHereIsBad) .catch(expectNotAllowedError) .then(() => (active = false)); diff --git a/dom/webauthn/tests/browser/head.js b/dom/webauthn/tests/browser/head.js index 58e3eaec3e0a..2bdb486645df 100644 --- a/dom/webauthn/tests/browser/head.js +++ b/dom/webauthn/tests/browser/head.js @@ -121,12 +121,13 @@ function expectError(aType) { function promiseWebAuthnMakeCredential( tab, attestation = "none", + residentKey = "discouraged", extensions = {} ) { return ContentTask.spawn( tab.linkedBrowser, - [attestation, extensions], - ([attestation, extensions]) => { + [attestation, residentKey, extensions], + ([attestation, residentKey, extensions]) => { const cose_alg_ECDSA_w_SHA256 = -7; let challenge = content.crypto.getRandomValues(new Uint8Array(16)); @@ -146,6 +147,10 @@ function promiseWebAuthnMakeCredential( displayName: "none", }, pubKeyCredParams, + authenticatorSelection: { + authenticatorAttachment: "cross-platform", + residentKey, + }, extensions, attestation, challenge, @@ -201,6 +206,21 @@ function promiseWebAuthnGetAssertion(tab, key_handle = null, extensions = {}) { ); } +function promiseWebAuthnGetAssertionDiscoverable(tab, extensions = {}) { + return ContentTask.spawn(tab.linkedBrowser, [extensions], ([extensions]) => { + let challenge = content.crypto.getRandomValues(new Uint8Array(16)); + + let publicKey = { + challenge, + extensions, + rpId: content.document.domain, + allowCredentials: [], + }; + + return content.navigator.credentials.get({ publicKey }); + }); +} + function checkRpIdHash(rpIdHash, hostname) { return crypto.subtle .digest("SHA-256", string2buffer(hostname))