From 3b8395f265fc4c96fcac822ddd10be2b9fcd3a15 Mon Sep 17 00:00:00 2001 From: Peter Brodsky <5933299+p-c-h-b@users.noreply.github.com> Date: Wed, 23 Sep 2026 02:24:50 -0400 Subject: [PATCH 1/3] fix(cable): send the tunnel Shutdown message on close `CableChannel::close()` was a TODO, and dropping the channel aborts the connection task, so the authenticator never received the caBLE Shutdown control message: it saw the tunnel drop instead. Google Play services then ends even a successful hybrid ceremony on a "Something went wrong" screen. Close the session the way Chromium's `FidoTunnelDevice` does (it encrypts and sends a one-byte `kShutdown` message, then waits for the peer to close): - `close()` signals the connection task once the tunnel is connected, then waits up to 3 s for it to finish; before the tunnel is up there is nothing to shut down and it returns at once. - The connection task sends Shutdown (type byte 0, padded and encrypted through the same path as CTAP frames), then waits up to 2 s for the peer to close its side. - Inbound, a bare Shutdown (the type byte with no payload, as Chromium and phones send it) is accepted instead of failing as `InvalidFraming`. Tests cover the bare-Shutdown parse, the Shutdown frame on the wire (decrypts to exactly `[0x00]`), and CTAP frames through the shared path. --- libwebauthn/src/transport/cable/channel.rs | 33 +++- .../src/transport/cable/connection_stages.rs | 6 +- .../src/transport/cable/known_devices.rs | 5 +- libwebauthn/src/transport/cable/protocol.rs | 152 +++++++++++++++++- .../src/transport/cable/qr_code_device.rs | 5 +- 5 files changed, 188 insertions(+), 13 deletions(-) diff --git a/libwebauthn/src/transport/cable/channel.rs b/libwebauthn/src/transport/cable/channel.rs index 5bc1cacb..a415b6c3 100644 --- a/libwebauthn/src/transport/cable/channel.rs +++ b/libwebauthn/src/transport/cable/channel.rs @@ -3,9 +3,9 @@ use std::sync::Arc; use std::time::Duration; use async_trait::async_trait; -use tokio::sync::{broadcast, mpsc, watch}; +use tokio::sync::{broadcast, mpsc, oneshot, watch}; use tokio::{task, time}; -use tracing::error; +use tracing::{debug, error}; use crate::pin::persistent_token::PersistentTokenStore; use crate::proto::{ @@ -48,8 +48,17 @@ pub struct CableChannel { pub(crate) ux_update_sender: broadcast::Sender, pub(crate) connection_state_receiver: watch::Receiver, pub(crate) persistent_token_store: Option>, + /// Asks the connection task to send the tunnel Shutdown message and + /// close. Taken by [`Channel::close`]. + pub(crate) shutdown_sender: Option>, } +/// How long [`Channel::close`] waits for the connection task to deliver the +/// Shutdown message and see the peer close. Chromium waits up to three minutes +/// for the peer; a short bound keeps `close()` from stalling a caller that is +/// about to exit, and phones close within a round trip of receiving Shutdown. +pub(crate) const CLOSE_TIMEOUT: Duration = Duration::from_secs(3); + impl CableChannel { async fn wait_for_connection(&self) -> Result<(), CableError> { let mut rx = self.connection_state_receiver.clone(); @@ -140,8 +149,26 @@ impl Channel for CableChannel { } } + /// Send the caBLE tunnel Shutdown message and close the connection, the + /// way Chromium's `FidoTunnelDevice` does. Without it the phone sees the + /// tunnel drop and shows an error even after a successful ceremony. async fn close(&mut self) { - // TODO Send CableTunnelMessageType#Shutdown and drop the connection + // Nothing to say Shutdown on before the tunnel is up: the task is + // still in the handshake, and dropping the channel aborts it. + if *self.connection_state_receiver.borrow() != ConnectionState::Connected { + self.shutdown_sender.take(); + return; + } + let Some(shutdown) = self.shutdown_sender.take() else { + return; + }; + if shutdown.send(()).is_ok() + && time::timeout(CLOSE_TIMEOUT, &mut self.handle_connection) + .await + .is_err() + { + debug!("caBLE connection did not finish closing in time"); + } } async fn apdu_send( diff --git a/libwebauthn/src/transport/cable/connection_stages.rs b/libwebauthn/src/transport/cable/connection_stages.rs index 740c64bc..4fe3cc1a 100644 --- a/libwebauthn/src/transport/cable/connection_stages.rs +++ b/libwebauthn/src/transport/cable/connection_stages.rs @@ -1,6 +1,6 @@ use ::btleplug::api::{AddressType, BDAddr}; use async_trait::async_trait; -use tokio::sync::{broadcast, mpsc, watch}; +use tokio::sync::{broadcast, mpsc, oneshot, watch}; use tracing::{debug, error, info, instrument, trace, warn}; use super::advertisement::{await_advertisement, DecryptedAdvert}; @@ -201,6 +201,8 @@ pub(crate) struct TunnelConnectionInput { pub noise_state: TunnelNoiseState, pub cbor_tx_recv: mpsc::Receiver, pub cbor_rx_send: mpsc::Sender, + /// Fires when the channel is closed. + pub shutdown_recv: oneshot::Receiver<()>, } impl TunnelConnectionInput { @@ -209,6 +211,7 @@ impl TunnelConnectionInput { known_device_store: Option>, cbor_tx_recv: mpsc::Receiver, cbor_rx_send: mpsc::Sender, + shutdown_recv: oneshot::Receiver<()>, ) -> Self { Self { connection_type: handshake_output.connection_type, @@ -218,6 +221,7 @@ impl TunnelConnectionInput { noise_state: handshake_output.noise_state, cbor_tx_recv, cbor_rx_send, + shutdown_recv, } } } diff --git a/libwebauthn/src/transport/cable/known_devices.rs b/libwebauthn/src/transport/cable/known_devices.rs index 01d42cdc..b8d97d87 100644 --- a/libwebauthn/src/transport/cable/known_devices.rs +++ b/libwebauthn/src/transport/cable/known_devices.rs @@ -19,7 +19,7 @@ use futures::lock::Mutex; use serde::Serialize; use serde_bytes::ByteBuf; use serde_indexed::SerializeIndexed; -use tokio::sync::{broadcast, mpsc, watch}; +use tokio::sync::{broadcast, mpsc, oneshot, watch}; use tokio::task; use tracing::{debug, instrument, trace}; @@ -201,6 +201,7 @@ impl<'d> Device<'d, Cable, CableChannel> for CableKnownDevice { let (ux_update_sender, _) = broadcast::channel(16); let (cbor_tx_send, cbor_tx_recv) = mpsc::channel(16); let (cbor_rx_send, cbor_rx_recv) = mpsc::channel(16); + let (shutdown_sender, shutdown_recv) = oneshot::channel(); let (connection_state_sender, connection_state_receiver) = watch::channel(ConnectionState::Connecting); @@ -224,6 +225,7 @@ impl<'d> Device<'d, Cable, CableChannel> for CableKnownDevice { Some(known_device.store), cbor_tx_recv, cbor_rx_send, + shutdown_recv, ); match protocol::connection(tunnel_input).await { @@ -246,6 +248,7 @@ impl<'d> Device<'d, Cable, CableChannel> for CableKnownDevice { ux_update_sender, connection_state_receiver, persistent_token_store: settings.persistent_token_store, + shutdown_sender: Some(shutdown_sender), }) } } diff --git a/libwebauthn/src/transport/cable/protocol.rs b/libwebauthn/src/transport/cable/protocol.rs index 73f68bb8..d663776b 100644 --- a/libwebauthn/src/transport/cable/protocol.rs +++ b/libwebauthn/src/transport/cable/protocol.rs @@ -27,6 +27,8 @@ use crate::transport::cable::known_devices::CableKnownDeviceId; const P256_X962_LENGTH: usize = 65; const MAX_CBOR_SIZE: usize = 1024 * 1024; const PADDING_GRANULARITY: usize = 32; +/// After sending Shutdown, how long to wait for the peer to close its side. +const PEER_CLOSE_GRACE: std::time::Duration = std::time::Duration::from_secs(2); const CABLE_PROLOGUE_STATE_ASSISTED: &[u8] = &[0u8]; const CABLE_PROLOGUE_QR_INITIATED: &[u8] = &[1u8]; @@ -46,7 +48,9 @@ impl CableTunnelMessage { } pub fn from_slice(slice: &[u8]) -> Result { let (type_byte, payload) = slice.split_first().ok_or(CableError::InvalidFraming)?; - if payload.is_empty() { + // Shutdown is the type byte alone (Chromium sends exactly that); + // every other message carries a payload. + if payload.is_empty() && *type_byte != 0 { return Err(CableError::InvalidFraming); } @@ -330,6 +334,28 @@ pub(crate) async fn connection(mut input: TunnelConnectionInput) -> Result<(), C } } } + // `CableChannel::close` asked to end the session. Send + // Shutdown, as Chromium does, so the authenticator ends in its + // success state, then give the peer a moment to close first. + _ = &mut input.shutdown_recv => { + debug!("Sending Shutdown control message"); + if let Err(e) = send_tunnel_message( + CableTunnelMessageType::Shutdown, + &[], + &mut *input.data_channel, + &mut input.noise_state, + ) + .await + { + debug!(?e, "Could not send Shutdown; the tunnel is already gone"); + return Ok(()); + } + let _ = tokio::time::timeout(PEER_CLOSE_GRACE, async { + while let Ok(Some(_)) = input.data_channel.recv().await {} + }) + .await; + return Ok(()); + } Some(request) = input.cbor_tx_recv.recv() => { match request.command { // Optimisation: respond to GetInfo requests immediately with the cached response @@ -378,16 +404,33 @@ async fn connection_send( } trace!(?cbor_request, cbor_request_len = cbor_request.len()); - let extra_bytes = PADDING_GRANULARITY - (cbor_request.len() % PADDING_GRANULARITY); - let padded_len = cbor_request.len() + extra_bytes; + send_tunnel_message( + CableTunnelMessageType::Ctap, + &cbor_request, + data_channel, + noise_state, + ) + .await +} + +/// Pad, frame, encrypt and send one tunnel message. Shared by CTAP requests +/// and the Shutdown control message. +async fn send_tunnel_message( + message_type: CableTunnelMessageType, + payload: &[u8], + data_channel: &mut dyn CableDataChannel, + noise_state: &mut TunnelNoiseState, +) -> Result<(), CableError> { + let extra_bytes = PADDING_GRANULARITY - (payload.len() % PADDING_GRANULARITY); + let padded_len = payload.len() + extra_bytes; - let mut padded_cbor_request = cbor_request.clone(); - padded_cbor_request.resize(padded_len, 0u8); - if let Some(last) = padded_cbor_request.last_mut() { + let mut padded_payload = payload.to_vec(); + padded_payload.resize(padded_len, 0u8); + if let Some(last) = padded_payload.last_mut() { *last = (extra_bytes - 1) as u8; } - let frame = CableTunnelMessage::new(CableTunnelMessageType::Ctap, &padded_cbor_request); + let frame = CableTunnelMessage::new(message_type, &padded_payload); let frame_serialized = frame.to_vec(); trace!(?frame_serialized); @@ -773,6 +816,101 @@ mod tests { ); } + /// A connected pair of Noise transport states, standing in for the + /// desktop and the phone after the handshake. + fn noise_pair() -> (TunnelNoiseState, TunnelNoiseState) { + let params: snow::params::NoiseParams = "Noise_NN_P256_AESGCM_SHA256".parse().unwrap(); + let mut initiator = Builder::new(params.clone()).build_initiator().unwrap(); + let mut responder = Builder::new(params).build_responder().unwrap(); + let (mut buf, mut out) = ([0u8; 1024], [0u8; 1024]); + let len = initiator.write_message(&[], &mut buf).unwrap(); + responder.read_message(&buf[..len], &mut out).unwrap(); + let len = responder.write_message(&[], &mut buf).unwrap(); + initiator.read_message(&buf[..len], &mut out).unwrap(); + let state = |hs: snow::HandshakeState| TunnelNoiseState { + handshake_hash: hs.get_handshake_hash().to_vec(), + transport_state: hs.into_transport_mode().unwrap(), + }; + (state(initiator), state(responder)) + } + + #[derive(Default)] + struct RecordingChannel { + sent: Vec>, + } + + #[async_trait] + impl CableDataChannel for RecordingChannel { + async fn send(&mut self, message: &[u8]) -> Result<(), CableError> { + self.sent.push(message.to_vec()); + Ok(()) + } + async fn recv(&mut self) -> Result>, CableError> { + Ok(None) + } + } + + #[test] + fn a_bare_shutdown_parses_and_other_empty_messages_do_not() { + let message = CableTunnelMessage::from_slice(&[0]).unwrap(); + assert!(matches!( + message.message_type, + CableTunnelMessageType::Shutdown + )); + assert!(matches!( + CableTunnelMessage::from_slice(&[1]), + Err(CableError::InvalidFraming) + )); + assert!(matches!( + CableTunnelMessage::from_slice(&[2]), + Err(CableError::InvalidFraming) + )); + } + + #[tokio::test] + async fn shutdown_goes_on_the_wire_as_an_encrypted_padded_type_byte() { + let (mut desktop, mut phone) = noise_pair(); + let mut channel = RecordingChannel::default(); + send_tunnel_message( + CableTunnelMessageType::Shutdown, + &[], + &mut channel, + &mut desktop, + ) + .await + .unwrap(); + assert_eq!(channel.sent.len(), 1, "exactly one frame"); + let plaintext = decrypt_frame(channel.sent.remove(0), &mut phone) + .await + .unwrap(); + assert_eq!(plaintext, vec![0u8], "Shutdown is the type byte alone"); + let message = CableTunnelMessage::from_slice(&plaintext).unwrap(); + assert!(matches!( + message.message_type, + CableTunnelMessageType::Shutdown + )); + } + + #[tokio::test] + async fn ctap_frames_still_round_trip_through_the_shared_path() { + let (mut desktop, mut phone) = noise_pair(); + let mut channel = RecordingChannel::default(); + let payload = vec![0xA1, 0x01, 0x02]; + send_tunnel_message( + CableTunnelMessageType::Ctap, + &payload, + &mut channel, + &mut desktop, + ) + .await + .unwrap(); + let plaintext = decrypt_frame(channel.sent.remove(0), &mut phone) + .await + .unwrap(); + assert_eq!(plaintext[0], CableTunnelMessageType::Ctap as u8); + assert_eq!(&plaintext[1..], payload.as_slice()); + } + #[test] fn strip_frame_padding_rejects_empty() { let result = strip_frame_padding(Vec::new()); diff --git a/libwebauthn/src/transport/cable/qr_code_device.rs b/libwebauthn/src/transport/cable/qr_code_device.rs index 5f85e8a3..7ab0e46c 100644 --- a/libwebauthn/src/transport/cable/qr_code_device.rs +++ b/libwebauthn/src/transport/cable/qr_code_device.rs @@ -11,7 +11,7 @@ use serde::Serialize; use serde_bytes::ByteArray; use serde_indexed::SerializeIndexed; use serde_repr::Serialize_repr; -use tokio::sync::{broadcast, mpsc, watch}; +use tokio::sync::{broadcast, mpsc, oneshot, watch}; use tokio::task; use tracing::instrument; @@ -247,6 +247,7 @@ impl<'d> Device<'d, Cable, CableChannel> for CableQrCodeDevice { let (ux_update_sender, _) = broadcast::channel(16); let (cbor_tx_send, cbor_tx_recv) = mpsc::channel(16); let (cbor_rx_send, cbor_rx_recv) = mpsc::channel(16); + let (shutdown_sender, shutdown_recv) = oneshot::channel(); let (connection_state_sender, connection_state_receiver) = watch::channel(ConnectionState::Connecting); @@ -270,6 +271,7 @@ impl<'d> Device<'d, Cable, CableChannel> for CableQrCodeDevice { qr_device.store, cbor_tx_recv, cbor_rx_send, + shutdown_recv, ); match protocol::connection(tunnel_input).await { Ok(()) => { @@ -291,6 +293,7 @@ impl<'d> Device<'d, Cable, CableChannel> for CableQrCodeDevice { ux_update_sender, connection_state_receiver, persistent_token_store: settings.persistent_token_store, + shutdown_sender: Some(shutdown_sender), }) } From f74edec4bdae7777ae47517704f0aec8d8c7e2fb Mon Sep 17 00:00:00 2001 From: Peter Brodsky <5933299+p-c-h-b@users.noreply.github.com> Date: Wed, 23 Sep 2026 02:34:53 -0400 Subject: [PATCH 2/3] test(cable): closing with a request in flight sends one Shutdown Drives the connection loop with a scripted peer: post-handshake message, a pending getAssertion, then close. The only frame after the request is an encrypted Shutdown, and the task ends cleanly once the peer closes its side. --- libwebauthn/src/transport/cable/protocol.rs | 93 +++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/libwebauthn/src/transport/cable/protocol.rs b/libwebauthn/src/transport/cable/protocol.rs index d663776b..e9b86028 100644 --- a/libwebauthn/src/transport/cable/protocol.rs +++ b/libwebauthn/src/transport/cable/protocol.rs @@ -850,6 +850,99 @@ mod tests { } } + /// A data channel the test scripts: inbound messages come from `inbox` + /// (closing it is the peer closing), outbound ones are recorded. + struct ScriptedChannel { + inbox: tokio::sync::mpsc::UnboundedReceiver>, + sent: Arc>>>, + } + + #[async_trait] + impl CableDataChannel for ScriptedChannel { + async fn send(&mut self, message: &[u8]) -> Result<(), CableError> { + self.sent.lock().unwrap().push(message.to_vec()); + Ok(()) + } + async fn recv(&mut self) -> Result>, CableError> { + Ok(self.inbox.recv().await) + } + } + + /// The phone's post-handshake message: `{1: getInfo}` with a minimal + /// getInfo (`{1: ["FIDO_2_0"], 3: aaguid}`), plus the zero-padding marker. + fn initial_message_plaintext() -> Vec { + let mut get_info = vec![0xA2, 0x01, 0x81, 0x68]; + get_info.extend_from_slice(b"FIDO_2_0"); + get_info.extend_from_slice(&[0x03, 0x50]); + get_info.extend_from_slice(&[0u8; 16]); + let mut initial = vec![0xA1, 0x01, 0x58, get_info.len() as u8]; + initial.extend_from_slice(&get_info); + initial.push(0x00); + initial + } + + #[tokio::test] + async fn closing_while_a_request_is_pending_sends_one_shutdown_and_ends_when_the_peer_closes() { + let (desktop, mut phone) = noise_pair(); + let (inbox_tx, inbox) = tokio::sync::mpsc::unbounded_channel(); + let sent = Arc::new(Mutex::new(Vec::new())); + let (cbor_tx, cbor_tx_recv) = tokio::sync::mpsc::channel(4); + let (cbor_rx_send, _cbor_rx_recv) = tokio::sync::mpsc::channel(4); + let (shutdown, shutdown_recv) = tokio::sync::oneshot::channel(); + let input = TunnelConnectionInput { + connection_type: CableTunnelConnectionType::QrCode { + routing_id: "000000".into(), + tunnel_id: "00".repeat(16), + private_key: NonZeroScalar::random(&mut OsRng), + }, + tunnel_domain: "cable.example.com".into(), + known_device_store: None, + data_channel: Box::new(ScriptedChannel { + inbox, + sent: Arc::clone(&sent), + }), + noise_state: desktop, + cbor_tx_recv, + cbor_rx_send, + shutdown_recv, + }; + let task = tokio::spawn(connection(input)); + + // The phone says hello, the desktop sends a request, and the phone + // is still working on it (a chooser or a fingerprint prompt). + let mut frame = vec![0u8; 1024]; + let len = phone + .transport_state + .write_message(&initial_message_plaintext(), &mut frame) + .unwrap(); + inbox_tx.send(frame[..len].to_vec()).unwrap(); + cbor_tx + .send(CborRequest::new( + Ctap2CommandCode::AuthenticatorGetAssertion, + )) + .await + .unwrap(); + while sent.lock().unwrap().len() < 1 { + tokio::task::yield_now().await; + } + + // The person cancels on the desktop. + shutdown.send(()).unwrap(); + while sent.lock().unwrap().len() < 2 { + tokio::task::yield_now().await; + } + assert!(!task.is_finished(), "waits for the phone to close first"); + drop(inbox_tx); // the phone closes its side + assert!(task.await.unwrap().is_ok(), "a clean close, not an error"); + + let frames = sent.lock().unwrap().clone(); + assert_eq!(frames.len(), 2, "the request, then exactly one more frame"); + let request = decrypt_frame(frames[0].clone(), &mut phone).await.unwrap(); + assert_eq!(request[0], CableTunnelMessageType::Ctap as u8); + let shutdown = decrypt_frame(frames[1].clone(), &mut phone).await.unwrap(); + assert_eq!(shutdown, vec![0u8], "the second frame is Shutdown"); + } + #[test] fn a_bare_shutdown_parses_and_other_empty_messages_do_not() { let message = CableTunnelMessage::from_slice(&[0]).unwrap(); From a90f2ff74d3eb45f336bf9cdc9f74c349a519942 Mon Sep 17 00:00:00 2001 From: Peter Brodsky <5933299+p-c-h-b@users.noreply.github.com> Date: Wed, 23 Sep 2026 02:35:20 -0400 Subject: [PATCH 3/3] test(cable): use is_empty in the pending-request test (clippy) --- libwebauthn/src/transport/cable/protocol.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libwebauthn/src/transport/cable/protocol.rs b/libwebauthn/src/transport/cable/protocol.rs index e9b86028..9d1cc1c3 100644 --- a/libwebauthn/src/transport/cable/protocol.rs +++ b/libwebauthn/src/transport/cable/protocol.rs @@ -922,7 +922,7 @@ mod tests { )) .await .unwrap(); - while sent.lock().unwrap().len() < 1 { + while sent.lock().unwrap().is_empty() { tokio::task::yield_now().await; }