chore(clippy): enable stricter lints#207
Open
AlfioEmanueleFresta wants to merge 10 commits into
Open
Conversation
AlfioEmanueleFresta
force-pushed
the
fix/security-input-hardening
branch
from
2488fa7 to
6a8e588
Compare
AlfioEmanueleFresta
force-pushed
the
chore/clippy-stricter-lints
branch
from
774aa18 to
491c25b
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.
`clippy::indexing_slicing` flags any `&v[i]` / `&v[a..b]` that could panic at runtime, complementing the existing `deny(clippy::panic)` which does not see slice-index panics from transitive crates. `clippy::unwrap_in_result` flags `.unwrap()` / `.expect()` inside functions that return `Result`, where bubbling the error is almost always preferable. Enable both for non-test code; tests retain the latitude they already have via the existing cfg_attr pattern. This commit is intentionally lint-failing: subsequent commits in this PR fix the call sites that the new lints surface.
Replace direct slice / index access with `.get()`, `split_first`, `split_at`, or iterator-based equivalents in CTAP message parsing and PIN/UV helpers. Functions affected: - `fido::AuthenticatorData::deserialize`: bound the trailing-data check. - `pin::PinUvAuthProtocolOne::authenticate`: handle the (impossible in practice) case of an HMAC output shorter than 16 bytes. - `pin::pin_hash`: use iterator `take(16)` on the SHA-256 output. - `proto::ctap1::apdu::ApduResponse::try_from`: rewrite using `checked_sub` and `split_at` so an empty / 1-byte packet errors out. - `proto::ctap1::model`: bound the U2F register-response signature split via `checked_sub`. - `proto::ctap2::cbor::CborResponse::try_from`: rewrite using `split_first`. - `proto::ctap2::model::get_assertion`: convert SHA-256 outputs to `[u8; 32]` via `GenericArray::into` and use `first()` on the allowList shortcut.
- `crypto::trial_decrypt_advert`: switch to `.get()` for all EID-key and candidate-advert subslices, returning `None` on length mismatch (the up-front length checks make this dead in practice, but the lint requires explicit bounded access). - `crypto::reserved_bits_are_zero`: use `.first().copied()`. - `digit_encode`: rewrite to iterate `chunks(CHUNK_SIZE)` over the input and use `.get()` on the zero-padding lookup; behaviour is preserved (verified by the existing unit test). - `tunnel::CableTunnelMessage::from_slice`: use `split_first` instead of indexing `slice[0]` after a manual length check. - `tunnel::decode_tunnel_server_domain`: use `KNOWN_TUNNEL_DOMAINS.get()`, convert the SHA-256 digest into a fixed `[u8; 32]`, and use `BASE32_CHARS.get()` / `TLDS.get()`. - `tunnel::connect` (initial msg buffer) and the trace log: bound the buffer slice via `.get()`. - `tunnel::send_cbor` padding-length byte: use `last_mut`.
In `transport::ble::framing` and `transport::hid::framing`, rewrite `frame()` / `message()`, `expected_bytes()`, and length helpers to use `split_first`, `.get()`, and `saturating_sub` instead of direct indexing. The behaviour is preserved (verified by the existing unit tests). In `transport::hid::channel::open` the INIT nonce comparison now uses `response.payload.get(..INIT_NONCE_LEN)` rather than the previously panic-prone slice index; the explicit length check on the line above makes this safe in practice but the lint requires bounded access.
The NFC backend (gated behind the `nfc`/`pcsc`/`libnfc` features) was not exercised by the default `cargo build`, so the lint enabled in the previous commits did not surface there. CI's `cargo clippy --all-targets --all-features` flags 5 sites: - `channel::NfcChannel::handle`: replace `&buf[..len]` (returned by `handle_in_ctx` into a fixed 1024-byte buffer) with `buf.get(..len)` and surface `HandleError::NotEnoughBuffer` if a backend overruns. - `channel::NfcChannel::cbor_send`: replace the `&rest[..250]` / `&rest[250..]` chunking with `rest.split_at(250)`; the `rest.len() > 250` loop predicate keeps this panic-safe. - `libnfc::Channel::connect_to_target`: replace `&modulations[modulations.len() - 1]` with `.last()`, returning `TransportUnavailable` if the device reports no supported baud rates.
AlfioEmanueleFresta
force-pushed
the
fix/security-input-hardening
branch
from
6a8e588 to
9236103
Compare
AlfioEmanueleFresta
force-pushed
the
chore/clippy-stricter-lints
branch
from
491c25b to
0a7c090
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.
Follow-up to #203 (fix(security): bound panics on hostile authenticator inputs).
Enables
deny(clippy::indexing_slicing)anddeny(clippy::unwrap_in_result)for non-test code. The audit identified five reachable panics from direct slice indexing; these lints prevent regressions of that class.Commits
After the lint commit, 69 clippy errors fire across the codebase. Each subsequent commit zeroes the count for the area it touches.
The fixes replace direct slice indexing with
.get(),split_first/split_at, iterator combinators, orsaturating_sub, depending on what reads naturally at each call site. Behaviour is preserved (verified by the existing unit tests).The base of this PR is
fix/security-input-hardeningso the lint and its fixes review on top of the audit security fixes.