fix(security): bound panics on hostile authenticator inputs#203
Open
AlfioEmanueleFresta wants to merge 5 commits into
Open
fix(security): bound panics on hostile authenticator inputs#203AlfioEmanueleFresta wants to merge 5 commits into
AlfioEmanueleFresta wants to merge 5 commits into
Conversation
AlfioEmanueleFresta
force-pushed
the
fix/security-input-hardening
branch
from
2488fa7 to
6a8e588
Compare
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.
AlfioEmanueleFresta
force-pushed
the
fix/security-input-hardening
branch
from
6a8e588 to
9236103
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
1 participant
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