From d60303ec7595e661fd0dedb1dec95004e1a260f6 Mon Sep 17 00:00:00 2001 From: Alfie Fresta Date: Sun, 10 May 2026 21:28:34 +0100 Subject: [PATCH 1/5] 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 4da3fff1278cbe9a4ef8ed63727c8048fc7559de Mon Sep 17 00:00:00 2001 From: Alfie Fresta Date: Sun, 10 May 2026 21:30:32 +0100 Subject: [PATCH 2/5] 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 99cb6f44ea807f05e17428fc6d43a758de9695e2 Mon Sep 17 00:00:00 2001 From: Alfie Fresta Date: Sun, 10 May 2026 21:31:07 +0100 Subject: [PATCH 3/5] 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 62e75ca9e57d7f5082e2ce4bdd4825b11f972eb7 Mon Sep 17 00:00:00 2001 From: Alfie Fresta Date: Sun, 10 May 2026 21:31:58 +0100 Subject: [PATCH 4/5] 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 9236103f19eb7a0138f3a5a17ef946ba5d95521c Mon Sep 17 00:00:00 2001 From: Alfie Fresta Date: Sun, 10 May 2026 21:32:21 +0100 Subject: [PATCH 5/5] 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: //