Skip to content

Commit 65a322c

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 9d2dc9d commit 65a322c

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
match cbor_response.status_code {
151164
CtapError::Ok => (),
152165
error => return Err(Error::Ctap(error)),
@@ -163,8 +176,7 @@ where
163176
debug!("CTAP2 Authenticator Selection request");
164177
let cbor_request = CborRequest::new(Ctap2CommandCode::AuthenticatorSelection);
165178

166-
self.cbor_send(&cbor_request, timeout).await?;
167-
let cbor_response = self.cbor_recv(timeout).await?;
179+
let cbor_response = cbor_send_recv(self, &cbor_request, timeout).await?;
168180
match cbor_response.status_code {
169181
CtapError::Ok => {
170182
return Ok(());
@@ -183,8 +195,7 @@ where
183195
timeout: Duration,
184196
) -> Result<Ctap2ClientPinResponse, Error> {
185197
trace!(?request);
186-
self.cbor_send(&request.try_into()?, timeout).await?;
187-
let cbor_response = self.cbor_recv(timeout).await?;
198+
let cbor_response = cbor_send_recv(self, &request.try_into()?, timeout).await?;
188199
match cbor_response.status_code {
189200
CtapError::Ok => (),
190201
error => return Err(Error::Ctap(error)),
@@ -209,8 +220,7 @@ where
209220
timeout: Duration,
210221
) -> Result<(), Error> {
211222
trace!(?request);
212-
self.cbor_send(&request.try_into()?, timeout).await?;
213-
let cbor_response = self.cbor_recv(timeout).await?;
223+
let cbor_response = cbor_send_recv(self, &request.try_into()?, timeout).await?;
214224
match cbor_response.status_code {
215225
CtapError::Ok => {
216226
return Ok(());
@@ -232,8 +242,7 @@ where
232242
timeout: Duration,
233243
) -> Result<Ctap2BioEnrollmentResponse, Error> {
234244
trace!(?request);
235-
self.cbor_send(&request.try_into()?, timeout).await?;
236-
let cbor_response = self.cbor_recv(timeout).await?;
245+
let cbor_response = cbor_send_recv(self, &request.try_into()?, timeout).await?;
237246
match cbor_response.status_code {
238247
CtapError::Ok => (),
239248
error => return Err(Error::Ctap(error)),
@@ -258,8 +267,7 @@ where
258267
timeout: Duration,
259268
) -> Result<Ctap2CredentialManagementResponse, Error> {
260269
trace!(?request);
261-
self.cbor_send(&request.try_into()?, timeout).await?;
262-
let cbor_response = self.cbor_recv(timeout).await?;
270+
let cbor_response = cbor_send_recv(self, &request.try_into()?, timeout).await?;
263271
match cbor_response.status_code {
264272
CtapError::Ok => (),
265273
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);
@@ -387,7 +394,11 @@ impl<'d> HidChannel<'d> {
387394
debug!("Ignoring HID keep-alive");
388395
continue;
389396
}
390-
Err(Error::Platform(PlatformError::Cancelled)) => {
397+
Err(Error::Platform(PlatformError::Cancelled))
398+
| Err(Error::Transport(TransportError::Timeout)) => {
399+
// CTAP 2.2 §11.2.9.1.5: send CTAPHID_CANCEL when the
400+
// platform gives up (caller cancelled or wall-clock
401+
// budget exhausted).
391402
let _ = self.hid_cancel().await;
392403
break response;
393404
}
@@ -402,16 +413,37 @@ impl<'d> HidChannel<'d> {
402413
timeout: Duration,
403414
) -> Result<HidMessage, Error> {
404415
let mut parser = HidMessageParser::new();
416+
let deadline = Instant::now().checked_add(timeout);
405417
loop {
406418
if !matches!(cancel_rx.try_recv(), Err(TryRecvError::Empty)) {
407419
return Err(Error::Platform(PlatformError::Cancelled));
408420
}
409421

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

0 commit comments

Comments
 (0)