Skip to content

Commit 4b677e6

Browse files
fix(hid): wall-clock timeout and continuation-packet validation (#205)
- Validate CID, SEQ, and init bit on every continuation packet - Enforce a wall-clock timeout on HID receive and CTAP2 exchanges
1 parent e00499c commit 4b677e6

3 files changed

Lines changed: 208 additions & 28 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)

libwebauthn/src/transport/hid/framing.rs

Lines changed: 144 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,13 @@ use std::io::{Cursor as IOCursor, Error as IOError, ErrorKind as IOErrorKind};
33

44
use byteorder::{BigEndian, ReadBytesExt, WriteBytesExt};
55
use num_enum::{IntoPrimitive, TryFromPrimitive};
6-
use tracing::{debug, error};
6+
use tracing::error;
77

88
const BROADCAST_CID: u32 = 0xFFFFFFFF;
99
const PACKET_INITIAL_HEADER_SIZE: usize = 7;
1010
const PACKET_INITIAL_CMD_MASK: u8 = 0x80;
1111
const PACKET_CONT_HEADER_SIZE: usize = 5;
12+
const PACKET_CONT_SEQ_MAX: u8 = 0x7F;
1213

1314
#[derive(Debug, IntoPrimitive, TryFromPrimitive, Copy, Clone, PartialEq)]
1415
#[repr(u8)]
@@ -121,17 +122,69 @@ impl HidMessageParser {
121122
if (self.packets.is_empty() && packet.len() < PACKET_INITIAL_HEADER_SIZE)
122123
|| packet.len() < PACKET_CONT_HEADER_SIZE + 1
123124
{
124-
error!("Packet length in invalid");
125+
error!("Packet length is invalid");
125126
return Err(IOError::new(
126127
IOErrorKind::InvalidInput,
127128
"Packet length is invalid",
128129
));
129130
}
131+
132+
// CID 0x00000000 is reserved (CTAP 2.2 §11.2.4); reject all-zero frames.
130133
if packet.iter().all(|&b| b == 0) {
131-
debug!("Received unexpected packet of all zeroes, ignoring"); // ?!
134+
error!("Received all-zero packet, rejecting");
135+
return Err(IOError::new(
136+
IOErrorKind::InvalidData,
137+
"All-zero packet is not a valid CTAPHID frame",
138+
));
139+
}
140+
141+
if self.packets.is_empty() {
142+
// First packet must be an initialization packet: high bit of
143+
// byte 4 set (CTAP 2.2 §11.2.4).
144+
if packet[4] & PACKET_INITIAL_CMD_MASK == 0 {
145+
error!("First packet is not an initialization packet");
146+
return Err(IOError::new(
147+
IOErrorKind::InvalidData,
148+
"First packet must be an initialization packet",
149+
));
150+
}
132151
} else {
133-
self.packets.push(Vec::from(packet));
152+
// Continuation packets: same CID as the initial packet, SEQ has
153+
// high bit cleared, SEQ starts at 0 and increments monotonically.
154+
let initial = &self.packets[0];
155+
if packet[..4] != initial[..4] {
156+
error!("Continuation packet CID does not match initial packet");
157+
return Err(IOError::new(
158+
IOErrorKind::InvalidData,
159+
"Continuation packet CID mismatch",
160+
));
161+
}
162+
let seq = packet[4];
163+
if seq & PACKET_INITIAL_CMD_MASK != 0 {
164+
error!(seq, "Unexpected init packet during continuation");
165+
return Err(IOError::new(
166+
IOErrorKind::InvalidData,
167+
"Unexpected initialization packet during continuation",
168+
));
169+
}
170+
let expected_seq = (self.packets.len() - 1) as u8;
171+
if expected_seq > PACKET_CONT_SEQ_MAX {
172+
error!(seq, "Continuation count exceeds maximum SEQ");
173+
return Err(IOError::new(
174+
IOErrorKind::InvalidData,
175+
"Too many continuation packets",
176+
));
177+
}
178+
if seq != expected_seq {
179+
error!(seq, expected_seq, "Out-of-order continuation SEQ");
180+
return Err(IOError::new(
181+
IOErrorKind::InvalidData,
182+
"Out-of-order continuation SEQ",
183+
));
184+
}
134185
}
186+
187+
self.packets.push(Vec::from(packet));
135188
if self.more_packets_needed() {
136189
Ok(HidMessageParserState::MorePacketsExpected)
137190
} else {
@@ -299,4 +352,91 @@ mod tests {
299352
assert_eq!(msg.cmd, HidCommand::Msg);
300353
assert_eq!(msg.payload, vec![0x0A, 0x0B, 0x0C, 0x0D, 0x0E]);
301354
}
355+
356+
#[test]
357+
fn parse_continuation_with_wrong_cid_is_rejected() {
358+
let mut parser = HidMessageParser::new();
359+
assert_eq!(
360+
parser
361+
.update(&[0xC0, 0xC1, 0xC2, 0xC3, 0x83, 0x00, 0x05, 0x0A])
362+
.unwrap(),
363+
HidMessageParserState::MorePacketsExpected
364+
);
365+
// Continuation from a different channel.
366+
let err = parser
367+
.update(&[0xD0, 0xD1, 0xD2, 0xD3, 0x00, 0x0B, 0x0C])
368+
.unwrap_err();
369+
assert_eq!(err.kind(), IOErrorKind::InvalidData);
370+
}
371+
372+
#[test]
373+
fn parse_continuation_with_non_zero_first_seq_is_rejected() {
374+
let mut parser = HidMessageParser::new();
375+
assert_eq!(
376+
parser
377+
.update(&[0xC0, 0xC1, 0xC2, 0xC3, 0x83, 0x00, 0x05, 0x0A])
378+
.unwrap(),
379+
HidMessageParserState::MorePacketsExpected
380+
);
381+
// First continuation must have SEQ=0.
382+
let err = parser
383+
.update(&[0xC0, 0xC1, 0xC2, 0xC3, 0x01, 0x0B, 0x0C])
384+
.unwrap_err();
385+
assert_eq!(err.kind(), IOErrorKind::InvalidData);
386+
}
387+
388+
#[test]
389+
fn parse_continuation_with_non_monotonic_seq_is_rejected() {
390+
let mut parser = HidMessageParser::new();
391+
assert_eq!(
392+
parser
393+
.update(&[0xC0, 0xC1, 0xC2, 0xC3, 0x83, 0x00, 0x07, 0x0A])
394+
.unwrap(),
395+
HidMessageParserState::MorePacketsExpected
396+
);
397+
assert_eq!(
398+
parser
399+
.update(&[0xC0, 0xC1, 0xC2, 0xC3, 0x00, 0x0B, 0x0C])
400+
.unwrap(),
401+
HidMessageParserState::MorePacketsExpected
402+
);
403+
// Skipping SEQ=1 and jumping to SEQ=2 is not allowed.
404+
let err = parser
405+
.update(&[0xC0, 0xC1, 0xC2, 0xC3, 0x02, 0x0D, 0x0E])
406+
.unwrap_err();
407+
assert_eq!(err.kind(), IOErrorKind::InvalidData);
408+
}
409+
410+
#[test]
411+
fn parse_init_packet_after_init_is_rejected() {
412+
let mut parser = HidMessageParser::new();
413+
assert_eq!(
414+
parser
415+
.update(&[0xC0, 0xC1, 0xC2, 0xC3, 0x83, 0x00, 0x05, 0x0A])
416+
.unwrap(),
417+
HidMessageParserState::MorePacketsExpected
418+
);
419+
// Another init packet (high bit set on byte 4) for a new transaction.
420+
let err = parser
421+
.update(&[0xC0, 0xC1, 0xC2, 0xC3, 0x83, 0x00, 0x05, 0x0B])
422+
.unwrap_err();
423+
assert_eq!(err.kind(), IOErrorKind::InvalidData);
424+
}
425+
426+
#[test]
427+
fn parse_all_zero_packet_is_rejected() {
428+
let mut parser = HidMessageParser::new();
429+
let err = parser.update(&[0u8; 64]).unwrap_err();
430+
assert_eq!(err.kind(), IOErrorKind::InvalidData);
431+
}
432+
433+
#[test]
434+
fn parse_first_packet_must_be_init_packet() {
435+
// High bit of byte 4 cleared means continuation; not allowed first.
436+
let mut parser = HidMessageParser::new();
437+
let err = parser
438+
.update(&[0xC0, 0xC1, 0xC2, 0xC3, 0x00, 0x00, 0x05, 0x0A])
439+
.unwrap_err();
440+
assert_eq!(err.kind(), IOErrorKind::InvalidData);
441+
}
302442
}

0 commit comments

Comments
 (0)