diff --git a/libwebauthn/src/fido.rs b/libwebauthn/src/fido.rs index 5d227786..f21b46db 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/lib.rs b/libwebauthn/src/lib.rs index 1c18e2be..7972f919 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", diff --git a/libwebauthn/src/ops/u2f.rs b/libwebauthn/src/ops/u2f.rs index ba40c6f4..91f36e37 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 d72d47d8..3c022fa2 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)); @@ -250,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)] @@ -350,7 +369,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 +396,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 +428,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) @@ -408,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> { @@ -578,3 +616,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/ctap1/apdu/response.rs b/libwebauthn/src/proto/ctap1/apdu/response.rs index 95a9289d..e6c77e54 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 9a6a192a..1bc1af55 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 fc1a4572..e52970e0 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 dd61a2a6..10923f42 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 } diff --git a/libwebauthn/src/proto/ctap2/model/get_info.rs b/libwebauthn/src/proto/ctap2/model/get_info.rs index ab38609f..ebd701c9 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/ble/framing.rs b/libwebauthn/src/transport/ble/framing.rs index a8b03267..9f92bdb4 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/cable/crypto.rs b/libwebauthn/src/transport/cable/crypto.rs index ee9f6d7f..be9a9927 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 8a40bcad..a7f96bb7 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 622a814e..35afae05 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(); @@ -523,6 +531,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 +576,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 +827,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/transport/hid/channel.rs b/libwebauthn/src/transport/hid/channel.rs index e56f7aab..cdcb3000 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 97851004..69da48ad 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); diff --git a/libwebauthn/src/transport/nfc/channel.rs b/libwebauthn/src/transport/nfc/channel.rs index c6b15fb3..3e66649e 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 f77d7188..d47d0ea8 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 { diff --git a/libwebauthn/src/webauthn/pin_uv_auth_token.rs b/libwebauthn/src/webauthn/pin_uv_auth_token.rs index 5748dd1e..c9547ca2 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 {