From 09a223e61d5dbaeb15e793841206015988518ebf Mon Sep 17 00:00:00 2001 From: Alfie Fresta Date: Sun, 10 May 2026 21:28:34 +0100 Subject: [PATCH 01/10] fix(pin): validate peer COSE EC2 key x/y length in PIN/UV ECDH MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A malicious or buggy authenticator can return an `EcdhEsHkdf256PublicKey` whose x or y coordinate is shorter than 32 bytes. `cosey` accepts any length up to 32, but `EncodedPoint::from_affine_coordinates` requires exactly 32 bytes per coordinate; the `.into()` calls invoke `GenericArray::from_slice` which panics on length mismatch. CTAP 2.2 §6.5.6 requires x and y to be 32 bytes (P-256 field-element size). Validate explicitly via `try_into` and return `Error::Ctap(CtapError::Other)` on mismatch. Add regression tests for short and empty x, and short y. --- libwebauthn/src/pin.rs | 68 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 63 insertions(+), 5 deletions(-) diff --git a/libwebauthn/src/pin.rs b/libwebauthn/src/pin.rs index d72d47d..bdb28d4 100644 --- a/libwebauthn/src/pin.rs +++ b/libwebauthn/src/pin.rs @@ -175,11 +175,23 @@ where ); return Err(Error::Ctap(CtapError::Other)); }; - let encoded_point = EncodedPoint::from_affine_coordinates( - peer_public_key.x.as_bytes().into(), - peer_public_key.y.as_bytes().into(), - false, - ); + // x and y must be exactly 32 bytes (P-256 field size). `cosey` accepts + // any length up to 32; validate before converting to `&FieldBytes`. + let x: &[u8; 32] = peer_public_key.x.as_bytes().try_into().map_err(|_| { + error!( + x_len = peer_public_key.x.as_bytes().len(), + "Peer public key x coordinate is not 32 bytes" + ); + Error::Ctap(CtapError::Other) + })?; + let y: &[u8; 32] = peer_public_key.y.as_bytes().try_into().map_err(|_| { + error!( + y_len = peer_public_key.y.as_bytes().len(), + "Peer public key y coordinate is not 32 bytes" + ); + Error::Ctap(CtapError::Other) + })?; + let encoded_point = EncodedPoint::from_affine_coordinates(x.into(), y.into(), false); let Some(peer_public_key) = P256PublicKey::from_encoded_point(&encoded_point).into() else { error!("Failed to parse public key."); return Err(Error::Ctap(CtapError::Other)); @@ -578,3 +590,49 @@ where .await } } + +#[cfg(test)] +mod tests { + use super::*; + use cosey::{Bytes, EcdhEsHkdf256PublicKey}; + + fn make_peer_key(x: &[u8], y: &[u8]) -> cose::PublicKey { + cose::PublicKey::EcdhEsHkdf256Key(EcdhEsHkdf256PublicKey { + x: Bytes::from_slice(x).unwrap(), + y: Bytes::from_slice(y).unwrap(), + }) + } + + #[test] + fn ecdh_rejects_short_x() { + let proto = PinUvAuthProtocolOne::new(); + let x = vec![0x01u8; 31]; + let y = vec![0x02u8; 32]; + let key = make_peer_key(&x, &y); + + let result = PinUvAuthProtocol::encapsulate(&proto, &key); + assert!(matches!(result, Err(Error::Ctap(CtapError::Other)))); + } + + #[test] + fn ecdh_rejects_empty_x() { + let proto = PinUvAuthProtocolOne::new(); + let x: Vec = Vec::new(); + let y = vec![0x02u8; 32]; + let key = make_peer_key(&x, &y); + + let result = PinUvAuthProtocol::encapsulate(&proto, &key); + assert!(matches!(result, Err(Error::Ctap(CtapError::Other)))); + } + + #[test] + fn ecdh_rejects_short_y() { + let proto = PinUvAuthProtocolTwo::new(); + let x = vec![0x01u8; 32]; + let y = vec![0x02u8; 16]; + let key = make_peer_key(&x, &y); + + let result = PinUvAuthProtocol::encapsulate(&proto, &key); + assert!(matches!(result, Err(Error::Ctap(CtapError::Other)))); + } +} From 7ac168a0eba67278357dcb6b893c7e4074ca5ed7 Mon Sep 17 00:00:00 2001 From: Alfie Fresta Date: Sun, 10 May 2026 21:30:32 +0100 Subject: [PATCH 02/10] fix(pin): bounds-checked slicing in PIN/UV protocol 2 primitives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PinUvAuthProtocolTwo::{encrypt, decrypt}` use `&key[32..]` to discard the HMAC-key portion of the shared secret, and `authenticate` uses `&key[..32]`. Both panic with an out-of-bounds slice index if the key is shorter than 32 bytes. This is reachable from device-controlled data: in `user_verification`, `uv_proto.decrypt(&shared_secret, &encrypted_pin_uv_auth_token)?` yields a pinUvAuthToken of `encrypted_pin_uv_auth_token.len() - 16` bytes; a malicious authenticator returning a 16-byte IV-only ciphertext decrypts to an empty token, which then panics `PinUvAuthProtocolTwo::authenticate(token, clientDataHash)`. Replace raw slice indexing with `.get(..32).ok_or(...)` / `.get(32..).ok_or(...)`, returning `Error::Ctap(CtapError::Other)` on short keys. Validate decrypted pinUvAuthToken length at the boundary (16 bytes for PUAP1 per CTAP 2.2 §6.5.5.7 step 3.7, 32 bytes for PUAP2). Update PUAP1 mocks to use a spec-correct 16-byte token. Add regression tests for empty and 16-byte keys. --- libwebauthn/src/pin.rs | 57 ++++++++++++++++++- libwebauthn/src/webauthn/pin_uv_auth_token.rs | 22 ++++++- 2 files changed, 73 insertions(+), 6 deletions(-) diff --git a/libwebauthn/src/pin.rs b/libwebauthn/src/pin.rs index bdb28d4..00844f4 100644 --- a/libwebauthn/src/pin.rs +++ b/libwebauthn/src/pin.rs @@ -362,7 +362,13 @@ impl PinUvAuthProtocol for PinUvAuthProtocolTwo { fn encrypt(&self, key: &[u8], plaintext: &[u8]) -> Result, Error> { // Discard the first 32 bytes of key. (This selects the AES-key portion of the shared secret.) - let key = &key[32..]; + let key = key.get(32..).ok_or_else(|| { + error!( + key_len = key.len(), + "key shorter than 32 bytes; cannot select AES-key portion" + ); + Error::Ctap(CtapError::Other) + })?; // Let iv be a 16-byte, random bytestring. let iv: [u8; 16] = thread_rng().gen(); @@ -383,7 +389,13 @@ impl PinUvAuthProtocol for PinUvAuthProtocolTwo { fn decrypt(&self, key: &[u8], ciphertext: &[u8]) -> Result, Error> { // Discard the first 32 bytes of key. (This selects the AES-key portion of the shared secret.) - let key = &key[32..]; + let key = key.get(32..).ok_or_else(|| { + error!( + key_len = key.len(), + "key shorter than 32 bytes; cannot select AES-key portion" + ); + Error::Ctap(CtapError::Other) + })?; // If demPlaintext is less than 16 bytes in length, return an error if ciphertext.len() < 16 { @@ -409,7 +421,13 @@ impl PinUvAuthProtocol for PinUvAuthProtocolTwo { fn authenticate(&self, key: &[u8], message: &[u8]) -> Result, Error> { // If key is longer than 32 bytes, discard the excess. (This selects the HMAC-key portion of the shared secret. // When key is the pinUvAuthToken, it is exactly 32 bytes long and thus this step has no effect.) - let key = &key[..32]; + let key = key.get(..32).ok_or_else(|| { + error!( + key_len = key.len(), + "key shorter than 32 bytes; cannot select HMAC-key portion" + ); + Error::Ctap(CtapError::Other) + })?; // Return the result of computing HMAC-SHA-256 on key and message. hmac_sha256(key, message) @@ -635,4 +653,37 @@ mod tests { let result = PinUvAuthProtocol::encapsulate(&proto, &key); assert!(matches!(result, Err(Error::Ctap(CtapError::Other)))); } + + #[test] + fn proto_two_authenticate_rejects_empty_key() { + let proto = PinUvAuthProtocolTwo::new(); + let result = proto.authenticate(&[], b"clientDataHash"); + assert!(matches!(result, Err(Error::Ctap(CtapError::Other)))); + } + + #[test] + fn proto_two_authenticate_rejects_short_key() { + let proto = PinUvAuthProtocolTwo::new(); + let short_key = [0u8; 16]; + let result = proto.authenticate(&short_key, b"hello"); + assert!(matches!(result, Err(Error::Ctap(CtapError::Other)))); + } + + #[test] + fn proto_two_encrypt_rejects_short_key() { + let proto = PinUvAuthProtocolTwo::new(); + let short_key = [0u8; 16]; + let plaintext = [0u8; 16]; + let result = proto.encrypt(&short_key, &plaintext); + assert!(matches!(result, Err(Error::Ctap(CtapError::Other)))); + } + + #[test] + fn proto_two_decrypt_rejects_short_key() { + let proto = PinUvAuthProtocolTwo::new(); + let short_key = [0u8; 16]; + let ct = [0u8; 32]; + let result = proto.decrypt(&short_key, &ct); + assert!(matches!(result, Err(Error::Ctap(CtapError::Other)))); + } } diff --git a/libwebauthn/src/webauthn/pin_uv_auth_token.rs b/libwebauthn/src/webauthn/pin_uv_auth_token.rs index 5748dd1..c9547ca 100644 --- a/libwebauthn/src/webauthn/pin_uv_auth_token.rs +++ b/libwebauthn/src/webauthn/pin_uv_auth_token.rs @@ -359,6 +359,22 @@ where let uv_auth_token = uv_proto.decrypt(&shared_secret, &encrypted_pin_uv_auth_token)?; + // pinUvAuthToken is 16 bytes for PUAP1 and 32 bytes for PUAP2. + // Reject a shorter token before it is used as a key downstream. + let min_token_len = match uv_proto.version() { + Ctap2PinUvAuthProtocol::One => 16, + Ctap2PinUvAuthProtocol::Two => 32, + }; + if uv_auth_token.len() < min_token_len { + error!( + protocol = ?uv_proto.version(), + token_len = uv_auth_token.len(), + min_expected = min_token_len, + "Decrypted pinUvAuthToken is shorter than required" + ); + return Err(Error::Ctap(CtapError::Other)); + } + let token_identifier = Ctap2AuthTokenPermission::new( uv_proto.version(), ctap2_request.permissions(), @@ -838,7 +854,7 @@ mod test { .unwrap(); // We do here what the device would need to do, i.e. generate a new random // pinUvAuthToken (here all 5's), then encrypt it using the shared_secret. - let token = [5; 32]; + let token = [5; 16]; let encrypted_token = pin_protocol.encrypt(&shared_secret, &token).unwrap(); let pin_resp = CborResponse::new_success_from_slice( to_vec(&Ctap2ClientPinResponse { @@ -1182,7 +1198,7 @@ mod test { .unwrap(); // We do here what the device would need to do, i.e. generate a new random // pinUvAuthToken (here all 5's), then encrypt it using the shared_secret. - let token = [5; 32]; + let token = [5; 16]; let encrypted_token = pin_protocol.encrypt(&shared_secret, &token).unwrap(); let pin_resp = CborResponse::new_success_from_slice( to_vec(&Ctap2ClientPinResponse { @@ -1322,7 +1338,7 @@ mod test { .unwrap(); // We do here what the device would need to do, i.e. generate a new random // pinUvAuthToken (here all 5's), then encrypt it using the shared_secret. - let token = [5; 32]; + let token = [5; 16]; let encrypted_token = pin_protocol.encrypt(&shared_secret, &token).unwrap(); let pin_resp = CborResponse::new_success_from_slice( to_vec(&Ctap2ClientPinResponse { From a00302833925ff15a5d8ad032e0c587bf05980da Mon Sep 17 00:00:00 2001 From: Alfie Fresta Date: Sun, 10 May 2026 21:31:07 +0100 Subject: [PATCH 03/10] fix(get_info): graceful early-return in Ctap2GetInfoResponse::uv_operation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A malicious or buggy authenticator can advertise `pinUvAuthToken=true` without `clientPin` set (or supported). CTAP 2.2 §6.4 makes these options independent and the platform must tolerate any combination. The current `assert!(self.option_enabled("clientPin"))` panics on this path, taking down the host process via every `make_credential` / `get_assertion` / `change_pin` flow that calls into `select_uv_proto`. Replace the assertion with a debug log + early return of `Ctap2UserVerificationOperation::OnlyForSharedSecret`, matching the existing handling for "pinUvAuthToken supported but PIN not set". The caller can still establish a shared secret (e.g., for hmac-secret), while not attempting a PIN-token ceremony that the device cannot service. Add a regression test. --- libwebauthn/src/proto/ctap2/model/get_info.rs | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/libwebauthn/src/proto/ctap2/model/get_info.rs b/libwebauthn/src/proto/ctap2/model/get_info.rs index ab38609..ebd701c 100644 --- a/libwebauthn/src/proto/ctap2/model/get_info.rs +++ b/libwebauthn/src/proto/ctap2/model/get_info.rs @@ -246,7 +246,16 @@ impl Ctap2GetInfoResponse { // If we do have a PIN, check if we need to use legacy getPinToken or new getPinUvAuthToken..-command if self.option_enabled("pinUvAuthToken") { - assert!(self.option_enabled("clientPin")); + // `pinUvAuthToken` and `clientPin` are independent options + // (CTAP 2.2 §6.4). Tolerate the combination without `clientPin` + // and fall back to a shared-secret-only flow. + if !self.option_enabled("clientPin") { + debug!( + "Device advertises pinUvAuthToken without clientPin; \ + falling back to OnlyForSharedSecret" + ); + return Some(Ctap2UserVerificationOperation::OnlyForSharedSecret); + } debug!("getPinUvAuthTokenUsingPinWithPermissions"); Some(Ctap2UserVerificationOperation::GetPinUvAuthTokenUsingPinWithPermissions) } else if self.option_enabled("clientPin") { @@ -574,4 +583,17 @@ mod test { Some(Ctap2UserVerificationOperation::OnlyForSharedSecret) ); } + + #[test] + fn device_pin_uv_auth_token_without_client_pin_does_not_panic() { + let info = create_info(&[("pinUvAuthToken", true)]); + assert_eq!( + info.uv_operation(false), + Some(Ctap2UserVerificationOperation::OnlyForSharedSecret) + ); + assert_eq!( + info.uv_operation(true), + Some(Ctap2UserVerificationOperation::OnlyForSharedSecret) + ); + } } From f99a329b7deecd27a9876c5a76e96664d69c2804 Mon Sep 17 00:00:00 2001 From: Alfie Fresta Date: Sun, 10 May 2026 21:31:58 +0100 Subject: [PATCH 04/10] fix(cable): bound decrypt_frame indexing on malformed Noise plaintext After Noise transport decryption, the last byte of the plaintext is read as a 0..=255 padding length and the frame is truncated by `padding_len + 1`. Two crash inputs are reachable from any legitimate-but-malicious paired peer: 1. Empty plaintext (Noise transport accepts a 16-byte AEAD-tag-only ciphertext, decrypting to 0 bytes): reading `frame[len - 1]` underflows to `usize::MAX` and panics with `index out of bounds`. 2. Under-padded plaintext (e.g., `[0x05]`): `1 - 6` panics in debug builds and silently wraps in release, so the subsequent `truncate(huge)` no-ops and the malformed plaintext is parsed downstream. Extract the padding-stripping into a `strip_frame_padding` helper that uses `.last()` and `.checked_sub`, returning `Error::Transport(TransportError::InvalidFraming)` on either edge case. Add regression tests for the empty and overlong-padding inputs plus a happy-path check. --- libwebauthn/src/transport/cable/tunnel.rs | 56 ++++++++++++++++++++++- 1 file changed, 54 insertions(+), 2 deletions(-) diff --git a/libwebauthn/src/transport/cable/tunnel.rs b/libwebauthn/src/transport/cable/tunnel.rs index 622a814..1a4f929 100644 --- a/libwebauthn/src/transport/cable/tunnel.rs +++ b/libwebauthn/src/transport/cable/tunnel.rs @@ -523,6 +523,31 @@ async fn connection_recv_binary_frame(message: Message) -> Result } } +/// Strip the trailing padding-length byte and `padding_len` bytes of padding +/// from a decrypted Noise transport frame, returning `InvalidFraming` on an +/// empty plaintext or a declared padding length that exceeds the frame. +fn strip_frame_padding(mut decrypted_frame: Vec) -> Result, Error> { + let padding_len = match decrypted_frame.last() { + Some(&b) => b as usize, + None => { + error!("Decrypted frame is empty; cannot read padding length"); + return Err(Error::Transport(TransportError::InvalidFraming)); + } + }; + let new_len = decrypted_frame + .len() + .checked_sub(padding_len + 1) + .ok_or_else(|| { + error!( + frame_len = decrypted_frame.len(), + padding_len, "Padding length exceeds frame length" + ); + Error::Transport(TransportError::InvalidFraming) + })?; + decrypted_frame.truncate(new_len); + Ok(decrypted_frame) +} + async fn decrypt_frame( encrypted_frame: Vec, noise_state: &mut TunnelNoiseState, @@ -543,8 +568,7 @@ async fn decrypt_frame( } } - let padding_len = decrypted_frame[decrypted_frame.len() - 1] as usize; - decrypted_frame.truncate(decrypted_frame.len() - (padding_len + 1)); + let decrypted_frame = strip_frame_padding(decrypted_frame)?; trace!( ?decrypted_frame, decrypted_frame_len = decrypted_frame.len(), @@ -795,4 +819,32 @@ mod tests { } // TODO: test the non-known case + + #[test] + fn strip_frame_padding_rejects_empty() { + let result = strip_frame_padding(Vec::new()); + assert!(matches!( + result, + Err(Error::Transport(TransportError::InvalidFraming)) + )); + } + + #[test] + fn strip_frame_padding_rejects_overlong_padding() { + // Length 1 + declared padding of 5 -> would require subtracting 6 from 1. + let frame = vec![0x05u8]; + let result = strip_frame_padding(frame); + assert!(matches!( + result, + Err(Error::Transport(TransportError::InvalidFraming)) + )); + } + + #[test] + fn strip_frame_padding_strips_normal_padding() { + // 4 bytes of payload, 3 bytes of zero padding, then padding-length 3. + let frame = vec![0xAA, 0xBB, 0xCC, 0xDD, 0x00, 0x00, 0x00, 0x03]; + let stripped = strip_frame_padding(frame).unwrap(); + assert_eq!(stripped, vec![0xAA, 0xBB, 0xCC, 0xDD]); + } } From 23e6a5ef5f03369c3fc65ca4c9de88a817c0378d Mon Sep 17 00:00:00 2001 From: Alfie Fresta Date: Sun, 10 May 2026 21:32:21 +0100 Subject: [PATCH 05/10] fix(u2f): replace production assert! in U2F register response upgrade `RegisterResponse::try_upgrade` asserts that the canonical CBOR encoding of the synthesized COSE P-256 key is exactly 77 bytes. The 77-byte assumption holds for current `cosey 0.3` output, but is implementation-defined: a future `cosey` revision adding an optional field (e.g., `kid`) would round-trip to a slightly different size and panic the host process. The recent panic-removal pass (commit 5df814b) missed this site because `clippy::panic` does not lint `assert!`. Replace the assertion with a typed length check that returns `Error::Platform(PlatformError::CryptoError(...))` on mismatch. --- libwebauthn/src/ops/u2f.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/libwebauthn/src/ops/u2f.rs b/libwebauthn/src/ops/u2f.rs index ba40c6f..91f36e3 100644 --- a/libwebauthn/src/ops/u2f.rs +++ b/libwebauthn/src/ops/u2f.rs @@ -89,7 +89,18 @@ impl UpgradableResponse for Regis y: y.into(), }); let cose_encoded_public_key = cbor::to_vec(&cose_public_key)?; - assert!(cose_encoded_public_key.len() == 77); + // Canonical CBOR encoding of the COSE P-256 key is 77 bytes for the + // fields we set; return a typed error if a future encoder change + // produces a different length rather than `assert!`-panicking. + if cose_encoded_public_key.len() != 77 { + error!( + len = cose_encoded_public_key.len(), + "COSE-encoded P-256 public key is not 77 bytes" + ); + return Err(Error::Platform(PlatformError::CryptoError( + "unexpected COSE-encoded public key length".into(), + ))); + } // Let attestedCredData be a byte string with following structure: // From ccb3877f4a059271e8edfec951faf8d7e31b6b0f Mon Sep 17 00:00:00 2001 From: Alfie Fresta Date: Sun, 10 May 2026 21:06:37 +0100 Subject: [PATCH 06/10] chore(clippy): enable indexing_slicing and unwrap_in_result lints `clippy::indexing_slicing` flags any `&v[i]` / `&v[a..b]` that could panic at runtime, complementing the existing `deny(clippy::panic)` which does not see slice-index panics from transitive crates. `clippy::unwrap_in_result` flags `.unwrap()` / `.expect()` inside functions that return `Result`, where bubbling the error is almost always preferable. Enable both for non-test code; tests retain the latitude they already have via the existing cfg_attr pattern. This commit is intentionally lint-failing: subsequent commits in this PR fix the call sites that the new lints surface. --- libwebauthn/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libwebauthn/src/lib.rs b/libwebauthn/src/lib.rs index 1c18e2b..7972f91 100644 --- a/libwebauthn/src/lib.rs +++ b/libwebauthn/src/lib.rs @@ -1,10 +1,11 @@ -// Deny panic-inducing patterns in production code. // Tests and the virt test-utility feature are allowed to use unwrap/expect/panic for convenience. #![cfg_attr(not(any(test, feature = "virt")), deny(clippy::unwrap_used))] #![cfg_attr(not(any(test, feature = "virt")), deny(clippy::expect_used))] #![cfg_attr(not(any(test, feature = "virt")), deny(clippy::panic))] #![cfg_attr(not(any(test, feature = "virt")), deny(clippy::todo))] #![cfg_attr(not(any(test, feature = "virt")), deny(clippy::unreachable))] +#![cfg_attr(not(any(test, feature = "virt")), deny(clippy::indexing_slicing))] +#![cfg_attr(not(any(test, feature = "virt")), deny(clippy::unwrap_in_result))] #[cfg(all( feature = "nfc", From 061e7570a406b1e2a851e4ca29f2c30c581b8250 Mon Sep 17 00:00:00 2001 From: Alfie Fresta Date: Sun, 10 May 2026 21:13:10 +0100 Subject: [PATCH 07/10] fix(proto): bounds-check slicing flagged by clippy::indexing_slicing Replace direct slice / index access with `.get()`, `split_first`, `split_at`, or iterator-based equivalents in CTAP message parsing and PIN/UV helpers. Functions affected: - `fido::AuthenticatorData::deserialize`: bound the trailing-data check. - `pin::PinUvAuthProtocolOne::authenticate`: handle the (impossible in practice) case of an HMAC output shorter than 16 bytes. - `pin::pin_hash`: use iterator `take(16)` on the SHA-256 output. - `proto::ctap1::apdu::ApduResponse::try_from`: rewrite using `checked_sub` and `split_at` so an empty / 1-byte packet errors out. - `proto::ctap1::model`: bound the U2F register-response signature split via `checked_sub`. - `proto::ctap2::cbor::CborResponse::try_from`: rewrite using `split_first`. - `proto::ctap2::model::get_assertion`: convert SHA-256 outputs to `[u8; 32]` via `GenericArray::into` and use `first()` on the allowList shortcut. --- libwebauthn/src/fido.rs | 6 ++++- libwebauthn/src/pin.rs | 14 ++++++++--- libwebauthn/src/proto/ctap1/apdu/response.rs | 23 ++++++++++++------- libwebauthn/src/proto/ctap1/model.rs | 13 ++++++++++- libwebauthn/src/proto/ctap2/cbor/response.rs | 20 ++++++++-------- .../src/proto/ctap2/model/get_assertion.rs | 13 +++++------ 6 files changed, 59 insertions(+), 30 deletions(-) diff --git a/libwebauthn/src/fido.rs b/libwebauthn/src/fido.rs index 5d22778..f21b46d 100644 --- a/libwebauthn/src/fido.rs +++ b/libwebauthn/src/fido.rs @@ -249,7 +249,11 @@ impl<'de, T: DeserializeOwned> Deserialize<'de> for AuthenticatorData { }; // Check if we have trailing data - if !&data[cursor.position() as usize..].is_empty() { + let pos = cursor.position() as usize; + let trailing = data.get(pos..).ok_or_else(|| { + DesError::custom("cursor advanced past end of authenticator data") + })?; + if !trailing.is_empty() { return Err(DesError::invalid_length(data.len(), &"trailing data")); } diff --git a/libwebauthn/src/pin.rs b/libwebauthn/src/pin.rs index 00844f4..3c022fa 100644 --- a/libwebauthn/src/pin.rs +++ b/libwebauthn/src/pin.rs @@ -262,7 +262,14 @@ impl PinUvAuthProtocol for PinUvAuthProtocolOne { fn authenticate(&self, key: &[u8], message: &[u8]) -> Result, Error> { // Return the first 16 bytes of the result of computing HMAC-SHA-256 with the given key and message. let hmac = hmac_sha256(key, message)?; - Ok(Vec::from(&hmac[..16])) + // HMAC-SHA-256 produces 32 bytes, so this slice is always valid. + let truncated = hmac.get(..16).ok_or_else(|| { + error!(len = hmac.len(), "HMAC output shorter than 16 bytes"); + Error::Platform(PlatformError::CryptoError( + "HMAC output shorter than 16 bytes".into(), + )) + })?; + Ok(Vec::from(truncated)) } #[instrument(skip_all)] @@ -438,8 +445,9 @@ impl PinUvAuthProtocol for PinUvAuthProtocolTwo { pub fn pin_hash(pin: &[u8]) -> Vec { let mut hasher = Sha256::default(); hasher.update(pin); - let hashed = hasher.finalize().to_vec(); - Vec::from(&hashed[..16]) + let hashed = hasher.finalize(); + // SHA-256 output is fixed at 32 bytes; keep only the first 16 per the spec. + hashed.into_iter().take(16).collect() } pub fn hmac_sha256(key: &[u8], message: &[u8]) -> Result, Error> { diff --git a/libwebauthn/src/proto/ctap1/apdu/response.rs b/libwebauthn/src/proto/ctap1/apdu/response.rs index 95a9289..e6c77e5 100644 --- a/libwebauthn/src/proto/ctap1/apdu/response.rs +++ b/libwebauthn/src/proto/ctap1/apdu/response.rs @@ -45,19 +45,26 @@ impl ApduResponse { impl TryFrom<&Vec> for ApduResponse { type Error = IOError; fn try_from(packet: &Vec) -> Result { - if packet.len() < 2 { - return Err(IOError::new( + let split_at = packet.len().checked_sub(2).ok_or_else(|| { + IOError::new( IOErrorKind::InvalidData, "Apdu response packets must contain at least 2 bytes.", - )); - } + ) + })?; + let (body, status) = packet.split_at(split_at); + // `status` is guaranteed to have exactly 2 elements by construction above. + let sw1 = *status + .first() + .ok_or_else(|| IOError::new(IOErrorKind::InvalidData, "Missing APDU status byte 1"))?; + let sw2 = *status + .get(1) + .ok_or_else(|| IOError::new(IOErrorKind::InvalidData, "Missing APDU status byte 2"))?; - let data = if packet.len() > 2 { - Some(Vec::from(&packet[0..packet.len() - 2])) - } else { + let data = if body.is_empty() { None + } else { + Some(Vec::from(body)) }; - let (sw1, sw2) = (packet[packet.len() - 2], packet[packet.len() - 1]); Ok(Self { data, sw1, sw2 }) } diff --git a/libwebauthn/src/proto/ctap1/model.rs b/libwebauthn/src/proto/ctap1/model.rs index 9a6a192..1bc1af5 100644 --- a/libwebauthn/src/proto/ctap1/model.rs +++ b/libwebauthn/src/proto/ctap1/model.rs @@ -142,7 +142,18 @@ impl TryFrom for Ctap1RegisterResponse { "Failed to parse X509 attestation data", )))?; let signature = Vec::from(signature); - let attestation = Vec::from(&remaining[0..remaining.len() - signature.len()]); + // `signature` is a suffix of `remaining` returned by `X509Certificate::from_der`, + // so the split index is always within bounds in practice; bound it explicitly. + let split_at = remaining + .len() + .checked_sub(signature.len()) + .ok_or_else(|| { + IOError::new( + IOErrorKind::InvalidData, + "Signature longer than remaining U2F register response data", + ) + })?; + let attestation = Vec::from(remaining.get(..split_at).unwrap_or(&[])); Ok(Ctap1RegisterResponse { version: Ctap1Version::U2fV2, diff --git a/libwebauthn/src/proto/ctap2/cbor/response.rs b/libwebauthn/src/proto/ctap2/cbor/response.rs index fc1a457..e52970e 100644 --- a/libwebauthn/src/proto/ctap2/cbor/response.rs +++ b/libwebauthn/src/proto/ctap2/cbor/response.rs @@ -25,25 +25,25 @@ impl CborResponse { impl TryFrom<&Vec> for CborResponse { type Error = IOError; fn try_from(packet: &Vec) -> Result { - if packet.is_empty() { - return Err(IOError::new( + let (status_byte, body) = packet.split_first().ok_or_else(|| { + IOError::new( IOErrorKind::InvalidData, "Cbor response packets must contain at least 1 byte.", - )); - } + ) + })?; - let Ok(status_code) = packet[0].try_into() else { - error!({ code = ?packet[0] }, "Invalid CTAP error code"); + let Ok(status_code) = (*status_byte).try_into() else { + error!({ code = ?*status_byte }, "Invalid CTAP error code"); return Err(IOError::new( IOErrorKind::InvalidData, - format!("Invalid CTAP error code: {:x}", packet[0]), + format!("Invalid CTAP error code: {:x}", status_byte), )); }; - let data = if packet.len() > 1 { - Some(Vec::from(&packet[1..])) - } else { + let data = if body.is_empty() { None + } else { + Some(Vec::from(body)) }; Ok(CborResponse { status_code, data }) } diff --git a/libwebauthn/src/proto/ctap2/model/get_assertion.rs b/libwebauthn/src/proto/ctap2/model/get_assertion.rs index dd61a2a..10923f4 100644 --- a/libwebauthn/src/proto/ctap2/model/get_assertion.rs +++ b/libwebauthn/src/proto/ctap2/model/get_assertion.rs @@ -332,8 +332,9 @@ impl Ctap2GetAssertionRequestExtensions { let mut hasher = Sha256::default(); hasher.update(salt1_input); - let salt1_hash = hasher.finalize().to_vec(); - input.salt1.copy_from_slice(&salt1_hash[..32]); + // SHA-256 produces a fixed 32-byte output, which lines up with salt1. + let salt1_hash: [u8; 32] = hasher.finalize().into(); + input.salt1.copy_from_slice(&salt1_hash); // 5.2 If ev.second is present, let salt2 be the value of SHA-256(UTF8Encode("WebAuthn PRF") || 0x00 || ev.second). if let Some(second) = ev.second { @@ -341,10 +342,8 @@ impl Ctap2GetAssertionRequestExtensions { salt2_input.extend(second); let mut hasher = Sha256::default(); hasher.update(salt2_input); - let salt2_hash = hasher.finalize().to_vec(); - let mut salt2 = [0u8; 32]; - salt2.copy_from_slice(&salt2_hash[..32]); - input.salt2 = Some(salt2); + let salt2_hash: [u8; 32] = hasher.finalize().into(); + input.salt2 = Some(salt2_hash); }; Ok(Some(input)) @@ -481,7 +480,7 @@ impl Ctap2GetAssertionResponse { // We always return it, for convenience. let credential_id = self.credential_id.or_else(|| { if request.allow.len() == 1 { - Some(request.allow[0].clone()) + request.allow.first().cloned() } else { None } From 9f888c0443be6ecbb751db55cffd4fe3b290290f Mon Sep 17 00:00:00 2001 From: Alfie Fresta Date: Sun, 10 May 2026 21:13:19 +0100 Subject: [PATCH 08/10] fix(cable): bounds-check slicing flagged by clippy::indexing_slicing - `crypto::trial_decrypt_advert`: switch to `.get()` for all EID-key and candidate-advert subslices, returning `None` on length mismatch (the up-front length checks make this dead in practice, but the lint requires explicit bounded access). - `crypto::reserved_bits_are_zero`: use `.first().copied()`. - `digit_encode`: rewrite to iterate `chunks(CHUNK_SIZE)` over the input and use `.get()` on the zero-padding lookup; behaviour is preserved (verified by the existing unit test). - `tunnel::CableTunnelMessage::from_slice`: use `split_first` instead of indexing `slice[0]` after a manual length check. - `tunnel::decode_tunnel_server_domain`: use `KNOWN_TUNNEL_DOMAINS.get()`, convert the SHA-256 digest into a fixed `[u8; 32]`, and use `BASE32_CHARS.get()` / `TLDS.get()`. - `tunnel::connect` (initial msg buffer) and the trace log: bound the buffer slice via `.get()`. - `tunnel::send_cbor` padding-length byte: use `last_mut`. --- libwebauthn/src/transport/cable/crypto.rs | 19 +++++--- .../src/transport/cable/digit_encode.rs | 27 ++++++------ libwebauthn/src/transport/cable/tunnel.rs | 44 +++++++++++-------- 3 files changed, 52 insertions(+), 38 deletions(-) diff --git a/libwebauthn/src/transport/cable/crypto.rs b/libwebauthn/src/transport/cable/crypto.rs index ee9f6d7..be9a992 100644 --- a/libwebauthn/src/transport/cable/crypto.rs +++ b/libwebauthn/src/transport/cable/crypto.rs @@ -26,11 +26,13 @@ pub fn derive(secret: &[u8], salt: Option<&[u8]>, purpose: KeyPurpose) -> Result } fn reserved_bits_are_zero(plaintext: &[u8]) -> bool { - plaintext[0] == 0 + plaintext.first().copied() == Some(0) } #[instrument] pub fn trial_decrypt_advert(eid_key: &[u8], candidate_advert: &[u8]) -> Option<[u8; 16]> { + // Both lengths are checked up front so the subsequent slicing is in bounds; + // use `.get(..)` regardless so the clippy::indexing_slicing lint is satisfied. if candidate_advert.len() != 20 { warn!("candidate advert is not 20 bytes"); return None; @@ -41,15 +43,20 @@ pub fn trial_decrypt_advert(eid_key: &[u8], candidate_advert: &[u8]) -> Option<[ return None; } - let expected_tag = hmac_sha256(&eid_key[32..], &candidate_advert[..16]).ok()?; - if expected_tag[..4] != candidate_advert[16..] { - warn!({ expected = ?expected_tag[..4], actual = ?candidate_advert[16..] }, + let mac_key = eid_key.get(32..)?; + let advert_body = candidate_advert.get(..16)?; + let advert_tag = candidate_advert.get(16..)?; + let expected_tag = hmac_sha256(mac_key, advert_body).ok()?; + let expected_tag_truncated = expected_tag.get(..4)?; + if expected_tag_truncated != advert_tag { + warn!({ expected = ?expected_tag_truncated, actual = ?advert_tag }, "candidate advert HMAC tag does not match"); return None; } - let cipher = Aes256::new(GenericArray::from_slice(&eid_key[..32])); - let mut block = Block::clone_from_slice(&candidate_advert[..16]); + let aes_key = eid_key.get(..32)?; + let cipher = Aes256::new(GenericArray::from_slice(aes_key)); + let mut block = Block::clone_from_slice(advert_body); cipher.decrypt_block(&mut block); if !reserved_bits_are_zero(&block) { diff --git a/libwebauthn/src/transport/cable/digit_encode.rs b/libwebauthn/src/transport/cable/digit_encode.rs index 8a40bca..a7f96bb 100644 --- a/libwebauthn/src/transport/cable/digit_encode.rs +++ b/libwebauthn/src/transport/cable/digit_encode.rs @@ -8,23 +8,22 @@ const PARTIAL_CHUNK_DIGITS: usize = 0x0fda8530; pub fn digit_encode(input: &[u8]) -> String { let mut output = String::new(); - let mut input = input; - while input.len() >= CHUNK_SIZE { + for chunk_slice in input.chunks(CHUNK_SIZE) { let mut chunk = [0u8; 8]; - chunk[..CHUNK_SIZE].copy_from_slice(&input[..CHUNK_SIZE]); + let (head, _) = chunk.split_at_mut(chunk_slice.len()); + head.copy_from_slice(chunk_slice); let v = u64::from_le_bytes(chunk); let v = v.to_string(); - output.push_str(&ZEROS[..CHUNK_DIGITS - v.len()]); - output.push_str(&v); - input = &input[CHUNK_SIZE..]; - } - if !input.is_empty() { - let digits = 0x0F & (PARTIAL_CHUNK_DIGITS >> (4 * input.len())); - let mut chunk = [0u8; 8]; - chunk[..input.len()].copy_from_slice(input); - let v = u64::from_le_bytes(chunk); - let v = v.to_string(); - output.push_str(&ZEROS[..digits - v.len()]); + let digits = if chunk_slice.len() == CHUNK_SIZE { + CHUNK_DIGITS + } else { + 0x0F & (PARTIAL_CHUNK_DIGITS >> (4 * chunk_slice.len())) + }; + // ZEROS is 17 chars (CHUNK_DIGITS); slice within bounds. + let pad_len = digits.saturating_sub(v.len()); + if let Some(pad) = ZEROS.get(..pad_len) { + output.push_str(pad); + } output.push_str(&v); } output diff --git a/libwebauthn/src/transport/cable/tunnel.rs b/libwebauthn/src/transport/cable/tunnel.rs index 1a4f929..35afae0 100644 --- a/libwebauthn/src/transport/cable/tunnel.rs +++ b/libwebauthn/src/transport/cable/tunnel.rs @@ -61,11 +61,14 @@ impl CableTunnelMessage { } } pub fn from_slice(slice: &[u8]) -> Result { - if slice.len() < 2 { + let (type_byte, payload) = slice + .split_first() + .ok_or(Error::Transport(TransportError::InvalidFraming))?; + if payload.is_empty() { return Err(Error::Transport(TransportError::InvalidFraming)); } - let message_type = match slice[0] { + let message_type = match *type_byte { 0 => CableTunnelMessageType::Shutdown, 1 => CableTunnelMessageType::Ctap, 2 => CableTunnelMessageType::Update, @@ -76,7 +79,7 @@ impl CableTunnelMessage { Ok(Self { message_type, - payload: ByteBuf::from(slice[1..].to_vec()), + payload: ByteBuf::from(payload.to_vec()), }) } @@ -131,10 +134,9 @@ enum CableTunnelMessageType { pub fn decode_tunnel_server_domain(encoded: u16) -> Option { if encoded < 256 { - if encoded as usize >= KNOWN_TUNNEL_DOMAINS.len() { - return None; - } - return Some(KNOWN_TUNNEL_DOMAINS[encoded as usize].to_string()); + return KNOWN_TUNNEL_DOMAINS + .get(encoded as usize) + .map(|s| (*s).to_string()); } let mut sha_input = SHA_INPUT.to_vec(); @@ -143,21 +145,22 @@ pub fn decode_tunnel_server_domain(encoded: u16) -> Option { sha_input.push(0); let mut hasher = Sha256::default(); hasher.update(&sha_input); - let digest = hasher.finalize(); - - let mut v = u64::from_le_bytes([ - digest[0], digest[1], digest[2], digest[3], digest[4], digest[5], digest[6], digest[7], - ]); - let tld_index = v & 3; + // SHA-256 produces 32 bytes, so the first 8 bytes are always present. + let digest: [u8; 32] = hasher.finalize().into(); + let mut digest_head = [0u8; 8]; + digest_head.copy_from_slice(digest.get(..8)?); + let mut v = u64::from_le_bytes(digest_head); + let tld_index = (v & 3) as usize; v >>= 2; let mut ret = String::from("cable."); while v != 0 { - ret.push(BASE32_CHARS[(v & 31) as usize] as char); + let ch = *BASE32_CHARS.get((v & 31) as usize)?; + ret.push(ch as char); v >>= 5; } - ret.push_str(TLDS[tld_index as usize]); + ret.push_str(TLDS.get(tld_index)?); Some(ret) } @@ -310,7 +313,10 @@ pub(crate) async fn do_handshake( } }; - let initial_msg: Vec = initial_msg_buffer[..initial_msg_len].into(); + let initial_msg: Vec = initial_msg_buffer + .get(..initial_msg_len) + .map(<[u8]>::to_vec) + .ok_or(TransportError::ConnectionFailed)?; trace!( { handshake = ?initial_msg }, "Sending initial handshake message" @@ -366,7 +372,7 @@ pub(crate) async fn do_handshake( }; debug!( - { handshake = ?payload[..payload_len] }, + { handshake = ?payload.get(..payload_len) }, "Received handshake response" ); @@ -466,7 +472,9 @@ async fn connection_send( let mut padded_cbor_request = cbor_request.clone(); padded_cbor_request.resize(padded_len, 0u8); - padded_cbor_request[padded_len - 1] = (extra_bytes - 1) as u8; + if let Some(last) = padded_cbor_request.last_mut() { + *last = (extra_bytes - 1) as u8; + } let frame = CableTunnelMessage::new(CableTunnelMessageType::Ctap, &padded_cbor_request); let frame_serialized = frame.to_vec(); From 86975174da95b2ac505e4f04794ce47b702d2af2 Mon Sep 17 00:00:00 2001 From: Alfie Fresta Date: Sun, 10 May 2026 21:13:25 +0100 Subject: [PATCH 09/10] fix(transport): bounds-check slicing in BLE and HID framing In `transport::ble::framing` and `transport::hid::framing`, rewrite `frame()` / `message()`, `expected_bytes()`, and length helpers to use `split_first`, `.get()`, and `saturating_sub` instead of direct indexing. The behaviour is preserved (verified by the existing unit tests). In `transport::hid::channel::open` the INIT nonce comparison now uses `response.payload.get(..INIT_NONCE_LEN)` rather than the previously panic-prone slice index; the explicit length check on the line above makes this safe in practice but the lint requires bounded access. --- libwebauthn/src/transport/ble/framing.rs | 44 +++++++++++++++--------- libwebauthn/src/transport/hid/channel.rs | 6 +++- libwebauthn/src/transport/hid/framing.rs | 35 +++++++++++-------- 3 files changed, 53 insertions(+), 32 deletions(-) diff --git a/libwebauthn/src/transport/ble/framing.rs b/libwebauthn/src/transport/ble/framing.rs index a8b0326..9f92bdb 100644 --- a/libwebauthn/src/transport/ble/framing.rs +++ b/libwebauthn/src/transport/ble/framing.rs @@ -126,16 +126,25 @@ impl BleFrameParser { )); } - let cmd = self.fragments[0][0].try_into().or(Err(IOError::new( + let (initial, continuations) = self + .fragments + .split_first() + .ok_or_else(|| IOError::new(IOErrorKind::InvalidData, "Frame has no fragments"))?; + let cmd_byte = *initial + .first() + .ok_or_else(|| IOError::new(IOErrorKind::InvalidData, "Initial fragment is empty"))?; + let cmd = cmd_byte.try_into().or(Err(IOError::new( IOErrorKind::InvalidData, - format!("Invalid BLE frame command: {:x}", self.fragments[0][0]), + format!("Invalid BLE frame command: {:x}", cmd_byte), )))?; let mut data = vec![]; - data.extend(&self.fragments[0][INITIAL_FRAGMENT_HEADER_LENGTH..self.fragments[0].len()]); - for cont_fragment in &self.fragments[1..self.fragments.len()] { - data.extend_from_slice( - &cont_fragment[CONT_FRAGMENT_HEADER_LENGTH..cont_fragment.len()], - ); + if let Some(initial_data) = initial.get(INITIAL_FRAGMENT_HEADER_LENGTH..) { + data.extend(initial_data); + } + for cont_fragment in continuations { + if let Some(cont_data) = cont_fragment.get(CONT_FRAGMENT_HEADER_LENGTH..) { + data.extend_from_slice(cont_data); + } } Ok(BleFrame::new(cmd, &data)) @@ -154,22 +163,23 @@ impl BleFrameParser { } fn expected_bytes(&self) -> Option { - if self.fragments.is_empty() { - return None; - } - - let mut cursor = IOCursor::new(vec![self.fragments[0][1], self.fragments[0][2]]); + let initial = self.fragments.first()?; + let b1 = *initial.get(1)?; + let b2 = *initial.get(2)?; + let mut cursor = IOCursor::new(vec![b1, b2]); Some(cursor.read_u16::().ok()? as usize) } fn data_len(&self) -> usize { - if self.fragments.is_empty() { + let Some((initial, continuations)) = self.fragments.split_first() else { return 0; - } + }; - let mut data_len = self.fragments[0].len() - INITIAL_FRAGMENT_HEADER_LENGTH; - for cont_fragment in &self.fragments[1..self.fragments.len()] { - data_len += cont_fragment.len() - CONT_FRAGMENT_HEADER_LENGTH; + let mut data_len = initial.len().saturating_sub(INITIAL_FRAGMENT_HEADER_LENGTH); + for cont_fragment in continuations { + data_len += cont_fragment + .len() + .saturating_sub(CONT_FRAGMENT_HEADER_LENGTH); } data_len } diff --git a/libwebauthn/src/transport/hid/channel.rs b/libwebauthn/src/transport/hid/channel.rs index e56f7aa..cdcb300 100644 --- a/libwebauthn/src/transport/hid/channel.rs +++ b/libwebauthn/src/transport/hid/channel.rs @@ -189,7 +189,11 @@ impl<'d> HidChannel<'d> { return Err(Error::Transport(TransportError::InvalidEndpoint)); } - if response.payload[0..INIT_NONCE_LEN] != nonce[0..INIT_NONCE_LEN] { + let payload_nonce = response + .payload + .get(..INIT_NONCE_LEN) + .ok_or(Error::Transport(TransportError::InvalidEndpoint))?; + if payload_nonce != nonce.as_slice() { warn!("INIT nonce mismatch. Terminating."); return Err(Error::Transport(TransportError::InvalidEndpoint)); } diff --git a/libwebauthn/src/transport/hid/framing.rs b/libwebauthn/src/transport/hid/framing.rs index 9785100..69da48a 100644 --- a/libwebauthn/src/transport/hid/framing.rs +++ b/libwebauthn/src/transport/hid/framing.rs @@ -147,22 +147,21 @@ impl HidMessageParser { } fn expected_bytes(&self) -> Option { - if self.packets.is_empty() { - return None; - } - - let mut cursor = IOCursor::new(vec![self.packets[0][5], self.packets[0][6]]); + let initial = self.packets.first()?; + let b5 = *initial.get(5)?; + let b6 = *initial.get(6)?; + let mut cursor = IOCursor::new(vec![b5, b6]); Some(cursor.read_u16::().ok()? as usize) } fn payload_len(&self) -> usize { - if self.packets.is_empty() { + let Some((initial, continuations)) = self.packets.split_first() else { return 0; - } + }; - let mut payload_len = self.packets[0].len() - PACKET_INITIAL_HEADER_SIZE; - for cont_packet in &self.packets[1..self.packets.len()] { - payload_len += cont_packet.len() - PACKET_CONT_HEADER_SIZE; + let mut payload_len = initial.len().saturating_sub(PACKET_INITIAL_HEADER_SIZE); + for cont_packet in continuations { + payload_len += cont_packet.len().saturating_sub(PACKET_CONT_HEADER_SIZE); } payload_len } @@ -175,7 +174,11 @@ impl HidMessageParser { )); } - let mut cursor = IOCursor::new(&self.packets[0]); + let (initial, continuations) = self + .packets + .split_first() + .ok_or_else(|| IOError::new(IOErrorKind::InvalidData, "Message has no packets"))?; + let mut cursor = IOCursor::new(initial); let cid = cursor.read_u32::()?; let cmd = cursor.read_u8()? ^ PACKET_INITIAL_CMD_MASK; let Ok(cmd) = cmd.try_into() else { @@ -188,9 +191,13 @@ impl HidMessageParser { let expected_size = cursor.read_u16::()?; let mut payload = vec![]; - payload.extend(&self.packets[0][PACKET_INITIAL_HEADER_SIZE..]); - for cont_packet in &self.packets[1..] { - payload.extend_from_slice(&cont_packet[PACKET_CONT_HEADER_SIZE..]); + if let Some(initial_payload) = initial.get(PACKET_INITIAL_HEADER_SIZE..) { + payload.extend(initial_payload); + } + for cont_packet in continuations { + if let Some(cont_payload) = cont_packet.get(PACKET_CONT_HEADER_SIZE..) { + payload.extend_from_slice(cont_payload); + } } payload.truncate(expected_size as usize); From 0a7c0908eb1c97f7ffff7b3031a67b8bc96c3a38 Mon Sep 17 00:00:00 2001 From: Alfie Fresta Date: Sun, 10 May 2026 21:39:34 +0100 Subject: [PATCH 10/10] fix(nfc): bounds-check slicing flagged by clippy::indexing_slicing The NFC backend (gated behind the `nfc`/`pcsc`/`libnfc` features) was not exercised by the default `cargo build`, so the lint enabled in the previous commits did not surface there. CI's `cargo clippy --all-targets --all-features` flags 5 sites: - `channel::NfcChannel::handle`: replace `&buf[..len]` (returned by `handle_in_ctx` into a fixed 1024-byte buffer) with `buf.get(..len)` and surface `HandleError::NotEnoughBuffer` if a backend overruns. - `channel::NfcChannel::cbor_send`: replace the `&rest[..250]` / `&rest[250..]` chunking with `rest.split_at(250)`; the `rest.len() > 250` loop predicate keeps this panic-safe. - `libnfc::Channel::connect_to_target`: replace `&modulations[modulations.len() - 1]` with `.last()`, returning `TransportUnavailable` if the device reports no supported baud rates. --- libwebauthn/src/transport/nfc/channel.rs | 10 ++++++---- libwebauthn/src/transport/nfc/libnfc/mod.rs | 4 +++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/libwebauthn/src/transport/nfc/channel.rs b/libwebauthn/src/transport/nfc/channel.rs index c6b15fb..3e66649 100644 --- a/libwebauthn/src/transport/nfc/channel.rs +++ b/libwebauthn/src/transport/nfc/channel.rs @@ -196,7 +196,8 @@ where let mut rapdu = Vec::new(); let len = self.handle_in_ctx(ctx, &command_buf, &mut buf)?; - let mut resp = Response::from(&buf[..len]); + let resp_bytes = buf.get(..len).ok_or(HandleError::NotEnoughBuffer(len))?; + let mut resp = Response::from(resp_bytes); let (mut sw1, mut sw2) = resp.trailer; rapdu.extend_from_slice(resp.payload); @@ -205,7 +206,8 @@ where let get_response_cmd = command_get_response(0x00, 0x00, sw2); let get_response_buf = Vec::from(get_response_cmd); let len = self.handle_in_ctx(ctx, &get_response_buf, &mut buf)?; - resp = Response::from(&buf[..len]); + let resp_bytes = buf.get(..len).ok_or(HandleError::NotEnoughBuffer(len))?; + resp = Response::from(resp_bytes); (sw1, sw2) = resp.trailer; rapdu.extend_from_slice(resp.payload); } @@ -272,8 +274,8 @@ where let mut rest: &[u8] = data; while rest.len() > 250 { - let to_send = &rest[..250]; - rest = &rest[250..]; + let (to_send, remaining) = rest.split_at(250); + rest = remaining; let ctap_msg = command_ctap_msg(true, to_send); let resp = self.handle(self.ctx, ctap_msg)?; trace!("cbor_send has_more {:?} {:?}", to_send, resp); diff --git a/libwebauthn/src/transport/nfc/libnfc/mod.rs b/libwebauthn/src/transport/nfc/libnfc/mod.rs index f77d718..d47d0ea 100644 --- a/libwebauthn/src/transport/nfc/libnfc/mod.rs +++ b/libwebauthn/src/transport/nfc/libnfc/mod.rs @@ -150,7 +150,9 @@ impl Channel { baud_rate: *baud_rate, }) .collect::>(); - let modulation = &modulations[modulations.len() - 1]; + let modulation = modulations + .last() + .ok_or(Error::Transport(TransportError::TransportUnavailable))?; let is_one_rate = modulations.len() == 1; for i in 0..2 { if i > 0 {