Skip to content

Commit fb6269e

Browse files
AlfioEmanueleFrestaAlfioEmanueleFresta
committed
fix(hid): enforce wall-clock timeout on HID receive and CTAP2 exchanges
hidapi `read_timeout` returns `Ok(0)` on timeout (not an error) and the leftover zero-initialised buffer used to be silently dropped by the parser, so a stalled or unplugged device could hang `hid_recv_hidapi` indefinitely. Additionally, the CTAP2 protocol layer did not wrap the `cbor_send`/`cbor_recv` pair in any wall-clock timeout (unlike CTAP1). Changes: - `hid_recv_hidapi` now caps each blocking read to a short poll interval, tracks the overall deadline across iterations, treats `Ok(0)` byte counts as per-iteration timeouts to retry against the remaining budget, and returns `TransportError::Timeout` once the budget is exhausted. - `hid_recv` sends `CTAPHID_CANCEL` on `TransportError::Timeout` in addition to `PlatformError::Cancelled`, per CTAP 2.2 §11.2.9.1.5. - A new `cbor_send_recv` helper wraps the CTAP2 send/recv pair in `tokio::time::timeout` and is used by every `ctap2_*` method on the `Ctap2` trait, mirroring `send_apdu_request_wait_uv` in the CTAP1 protocol module. Spec refs: - CTAP 2.2 §11.2 USB HID transport - CTAP 2.2 §11.2.9.1.5 CTAPHID_CANCEL
1 parent 5c205ce commit fb6269e

2 files changed

Lines changed: 64 additions & 24 deletions

File tree

‎libwebauthn/src/proto/ctap2/protocol.rs‎

Lines changed: 27 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
use std::time::Duration;
22

33
use async_trait::async_trait;
4+
use tokio::time::timeout as tokio_timeout;
45
use tracing::{debug, instrument, trace, warn};
56

6-
use crate::proto::ctap2::cbor::{self, CborRequest};
7+
use crate::proto::ctap2::cbor::{self, CborRequest, CborResponse};
78
use crate::proto::ctap2::{Ctap2BioEnrollmentResponse, Ctap2CommandCode};
9+
use crate::transport::error::TransportError;
810
use crate::transport::Channel;
911
use crate::unwrap_field;
1012
use crate::webauthn::error::{CtapError, Error, PlatformError};
@@ -19,6 +21,21 @@ use super::{
1921

2022
const TIMEOUT_GET_INFO: Duration = Duration::from_millis(250);
2123

24+
/// CBOR send + recv with a wall-clock timeout over the pair. Mirrors
25+
/// `send_apdu_request_wait_uv` in the CTAP1 module.
26+
async fn cbor_send_recv<C: Channel + ?Sized>(
27+
channel: &mut C,
28+
request: &CborRequest,
29+
timeout: Duration,
30+
) -> Result<CborResponse, Error> {
31+
tokio_timeout(timeout, async {
32+
channel.cbor_send(request, timeout).await?;
33+
channel.cbor_recv(timeout).await
34+
})
35+
.await
36+
.map_err(|_| Error::Transport(TransportError::Timeout))?
37+
}
38+
2239
macro_rules! parse_cbor {
2340
($type:ty, $data:expr) => {{
2441
match cbor::from_slice::<$type>($data) {
@@ -83,8 +100,7 @@ where
83100
#[instrument(skip_all)]
84101
async fn ctap2_get_info(&mut self) -> Result<Ctap2GetInfoResponse, Error> {
85102
let cbor_request = CborRequest::new(Ctap2CommandCode::AuthenticatorGetInfo);
86-
self.cbor_send(&cbor_request, TIMEOUT_GET_INFO).await?;
87-
let cbor_response = self.cbor_recv(TIMEOUT_GET_INFO).await?;
103+
let cbor_response = cbor_send_recv(self, &cbor_request, TIMEOUT_GET_INFO).await?;
88104
match cbor_response.status_code {
89105
CtapError::Ok => (),
90106
error => return Err(Error::Ctap(error)),
@@ -103,8 +119,7 @@ where
103119
timeout: Duration,
104120
) -> Result<Ctap2MakeCredentialResponse, Error> {
105121
trace!(?request);
106-
self.cbor_send(&request.try_into()?, timeout).await?;
107-
let cbor_response = self.cbor_recv(timeout).await?;
122+
let cbor_response = cbor_send_recv(self, &request.try_into()?, timeout).await?;
108123
match cbor_response.status_code {
109124
CtapError::Ok => (),
110125
error => return Err(Error::Ctap(error)),
@@ -124,8 +139,7 @@ where
124139
timeout: Duration,
125140
) -> Result<Ctap2GetAssertionResponse, Error> {
126141
trace!(?request);
127-
self.cbor_send(&request.try_into()?, timeout).await?;
128-
let cbor_response = self.cbor_recv(timeout).await?;
142+
let cbor_response = cbor_send_recv(self, &request.try_into()?, timeout).await?;
129143
match cbor_response.status_code {
130144
CtapError::Ok => (),
131145
error => return Err(Error::Ctap(error)),
@@ -145,8 +159,7 @@ where
145159
) -> Result<Ctap2GetAssertionResponse, Error> {
146160
debug!("CTAP2 GetNextAssertion request");
147161
let cbor_request = CborRequest::new(Ctap2CommandCode::AuthenticatorGetNextAssertion);
148-
self.cbor_send(&cbor_request, timeout).await?;
149-
let cbor_response = self.cbor_recv(timeout).await?;
162+
let cbor_response = cbor_send_recv(self, &cbor_request, timeout).await?;
150163
let data = unwrap_field!(cbor_response.data);
151164
let ctap_response = parse_cbor!(Ctap2GetAssertionResponse, &data);
152165
debug!("CTAP2 GetNextAssertion successful");
@@ -159,8 +172,7 @@ where
159172
debug!("CTAP2 Authenticator Selection request");
160173
let cbor_request = CborRequest::new(Ctap2CommandCode::AuthenticatorSelection);
161174

162-
self.cbor_send(&cbor_request, timeout).await?;
163-
let cbor_response = self.cbor_recv(timeout).await?;
175+
let cbor_response = cbor_send_recv(self, &cbor_request, timeout).await?;
164176
match cbor_response.status_code {
165177
CtapError::Ok => {
166178
return Ok(());
@@ -179,8 +191,7 @@ where
179191
timeout: Duration,
180192
) -> Result<Ctap2ClientPinResponse, Error> {
181193
trace!(?request);
182-
self.cbor_send(&request.try_into()?, timeout).await?;
183-
let cbor_response = self.cbor_recv(timeout).await?;
194+
let cbor_response = cbor_send_recv(self, &request.try_into()?, timeout).await?;
184195
match cbor_response.status_code {
185196
CtapError::Ok => (),
186197
error => return Err(Error::Ctap(error)),
@@ -205,8 +216,7 @@ where
205216
timeout: Duration,
206217
) -> Result<(), Error> {
207218
trace!(?request);
208-
self.cbor_send(&request.try_into()?, timeout).await?;
209-
let cbor_response = self.cbor_recv(timeout).await?;
219+
let cbor_response = cbor_send_recv(self, &request.try_into()?, timeout).await?;
210220
match cbor_response.status_code {
211221
CtapError::Ok => {
212222
return Ok(());
@@ -228,8 +238,7 @@ where
228238
timeout: Duration,
229239
) -> Result<Ctap2BioEnrollmentResponse, Error> {
230240
trace!(?request);
231-
self.cbor_send(&request.try_into()?, timeout).await?;
232-
let cbor_response = self.cbor_recv(timeout).await?;
241+
let cbor_response = cbor_send_recv(self, &request.try_into()?, timeout).await?;
233242
match cbor_response.status_code {
234243
CtapError::Ok => (),
235244
error => return Err(Error::Ctap(error)),
@@ -254,8 +263,7 @@ where
254263
timeout: Duration,
255264
) -> Result<Ctap2CredentialManagementResponse, Error> {
256265
trace!(?request);
257-
self.cbor_send(&request.try_into()?, timeout).await?;
258-
let cbor_response = self.cbor_recv(timeout).await?;
266+
let cbor_response = cbor_send_recv(self, &request.try_into()?, timeout).await?;
259267
match cbor_response.status_code {
260268
CtapError::Ok => (),
261269
error => return Err(Error::Ctap(error)),

‎libwebauthn/src/transport/hid/channel.rs‎

Lines changed: 37 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ use std::fmt::{Debug, Display, Formatter};
33
use std::io::{Cursor as IOCursor, Seek, SeekFrom};
44
use std::ops::DerefMut;
55
use std::sync::{Arc, Mutex};
6-
use std::time::Duration;
6+
use std::time::{Duration, Instant};
77

88
use async_trait::async_trait;
99
use byteorder::{BigEndian, ReadBytesExt};
@@ -44,6 +44,13 @@ const INIT_TIMEOUT: Duration = Duration::from_millis(200);
4444
const PACKET_SIZE: usize = 64;
4545
const REPORT_ID: u8 = 0x00;
4646

47+
// Per-iteration cap on hidapi::read_timeout. `read_timeout` returns as soon
48+
// as the device delivers a report, so this does NOT add latency to normal
49+
// responses; it only bounds how quickly the loop wakes up to re-check the
50+
// wall-clock deadline and the cancel signal. 100ms is a small fraction of
51+
// any realistic CTAP timeout and gives ~10 wakeups/sec per active channel.
52+
const HID_READ_POLL_INTERVAL: Duration = Duration::from_millis(100);
53+
4754
// Some devices fail when sending a WINK command followed immediately
4855
// by a CBOR command, so we want to ensure we wait some time after winking.
4956
const WINK_MIN_WAIT: Duration = Duration::from_secs(2);
@@ -384,7 +391,11 @@ impl<'d> HidChannel<'d> {
384391
debug!("Ignoring HID keep-alive");
385392
continue;
386393
}
387-
Err(Error::Platform(PlatformError::Cancelled)) => {
394+
Err(Error::Platform(PlatformError::Cancelled))
395+
| Err(Error::Transport(TransportError::Timeout)) => {
396+
// CTAP 2.2 §11.2.9.1.5: send CTAPHID_CANCEL when the
397+
// platform gives up (caller cancelled or wall-clock
398+
// budget exhausted).
388399
let _ = self.hid_cancel().await;
389400
break response;
390401
}
@@ -399,16 +410,37 @@ impl<'d> HidChannel<'d> {
399410
timeout: Duration,
400411
) -> Result<HidMessage, Error> {
401412
let mut parser = HidMessageParser::new();
413+
let deadline = Instant::now().checked_add(timeout);
402414
loop {
403415
if !matches!(cancel_rx.try_recv(), Err(TryRecvError::Empty)) {
404416
return Err(Error::Platform(PlatformError::Cancelled));
405417
}
406418

419+
// Cap each read at HID_READ_POLL_INTERVAL so we re-check the
420+
// cancel channel and remaining budget; a stalled device cannot
421+
// hang the caller past `timeout`.
422+
let remaining = match deadline {
423+
Some(d) => d.saturating_duration_since(Instant::now()),
424+
None => timeout,
425+
};
426+
if remaining.is_zero() {
427+
warn!("HID receive timed out before any data was read");
428+
return Err(Error::Transport(TransportError::Timeout));
429+
}
430+
let read_for = remaining.min(HID_READ_POLL_INTERVAL);
431+
407432
let mut report = [0; PACKET_SIZE];
408-
device
409-
.read_timeout(&mut report, timeout.as_millis() as i32)
433+
let bytes_read = device
434+
.read_timeout(&mut report, read_for.as_millis() as i32)
410435
.or(Err(Error::Transport(TransportError::ConnectionLost)))?;
411-
debug!({ len = report.len() }, "Received HID report");
436+
if bytes_read == 0 {
437+
// hidapi signals per-iteration timeout as Ok(0); retry
438+
// against the remaining budget rather than passing the
439+
// zero-initialised buffer to the parser.
440+
trace!("hidapi read_timeout returned 0 bytes, continuing");
441+
continue;
442+
}
443+
debug!({ len = bytes_read }, "Received HID report");
412444
trace!(?report);
413445
if let HidMessageParserState::Done = parser
414446
.update(&report)

0 commit comments

Comments
 (0)