fix(security): bound panics on hostile authenticator inputs#203
Merged
Conversation
AlfioEmanueleFresta
force-pushed
the
fix/security-input-hardening
branch
2 times, most recently
from
6a8e588 to
9236103
Compare
msirringhaus
requested changes
May 13, 2026
Collaborator
msirringhaus
left a comment
msirringhaus
left a comment
There was a problem hiding this comment.
I think there is a bug in the token length checks
| // 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, |
Collaborator
There was a problem hiding this comment.
I'm not entirely sure this logic is correct.
Under section PUAP1, the spec says:
pinUvAuthToken, a random, opaque byte string that MUST be either 16 or 32 bytes long.
while under PUAP2, it says:
The length of the pinUvAuthToken for PIN/UV auth protocol two MUST be 32 bytes.
This code would allow under PUAP1 a token of 20 bytes length. I think it has to check for 32 bytes equality always and optionally for 16 bytes, if we are doing PUAP1.
Member
Author
There was a problem hiding this comment.
Agreed, good catch. Fixed in 605f30e, also added a test.
| // 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]; |
Collaborator
There was a problem hiding this comment.
While puap1 would allow for 32 as well, it is probably not a bad idea to go for 16 here, just to test that both lengths works.
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.
`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.
…ation
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.
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.
`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.
The boundary check accepted any token of at least min_token_len bytes, so protocol one accepted e.g. a 20-byte token and protocol two a 33-byte one. CTAP 2.1 requires the decrypted pinUvAuthToken to be exactly 16 or 32 bytes for protocol one and exactly 32 bytes for protocol two. Replace the minimum-length check with an exact-length helper and unit-test it.
AlfioEmanueleFresta
force-pushed
the
fix/security-input-hardening
branch
from
605f30e to
14c0e5f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A misbehaving or hostile authenticator can crash the platform process through five reachable code paths, all of which trust device-supplied lengths and indices that the spec does not actually guarantee. The crate-wide
deny(clippy::panic)lint does not catch any of them, since the panics originate fromassert!macros (whichclippy::panicdoes not see) or from.into()and slice-indexing conversions inside transitive dependencies (generic-array, etc.).Changes
coseyat any length 0..=32, but the subsequentEncodedPoint::from_affine_coordinatesrequires exactly 32 each. Without this fix, a malformedgetKeyAgreementresponse (e.g. a 31-byte x coordinate) panics the very first ECDH step of every PIN/UV ceremony. The fix validates length up front.&key[..32]/&key[32..]panic when the key is shorter than 32 bytes. The relevant entry point isauthenticate(uv_auth_token, clientDataHash)immediately after the platform decrypts the device-suppliedpinUvAuthToken; an authenticator returning an IV-only ciphertext yields a zero-byte plaintext, which used to panic at the slicing step. The fix uses checked slicing and validates the token length (16 for PUAP1, 32 for PUAP2) at the decryption boundary.uv_operationassertion.assert!(self.option_enabled("clientPin"))fired when an authenticator advertisedpinUvAuthToken: truewith noclientPinand nouvoption. The new code returnsOnlyForSharedSecretfor that case rather than panicking.decrypt_frame. The final byte of a Noise-decrypted frame is the padding length; on a zero-length plaintext the subtraction underflows and panics. The fix returnsInvalidFramingon empty or under-padded frames.assert!(cose_encoded_public_key.len() == 77)panicked ifcoseyproduced a non-canonical length (a real risk on futurecoseyversions or unusual EC keys). Replaced with a typed error return.Each fix has a regression test in the same module that constructs the malformed input and asserts the error variant.
Spec references