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: // diff --git a/libwebauthn/src/pin.rs b/libwebauthn/src/pin.rs index d72d47d..00844f4 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)); @@ -350,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(); @@ -371,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 { @@ -397,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) @@ -578,3 +608,82 @@ 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)))); + } + + #[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/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) + ); + } } 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]); + } } 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 {