Harden passkey/WebAuthn registration against malformed input - #3076
Harden passkey/WebAuthn registration against malformed input#3076jeremy wants to merge 2 commits into
Conversation
A HackerOne researcher fuzzed POST /:slug/my/passkeys and produced five distinct uncaught 500s in the new ActionPack::WebAuthn / ActionPack::Passkey stack. Every leg fails closed (no auth bypass, replay, or origin/RP-ID confusion), but validation failures and malformed input surfaced as 500s instead of a clean rejection, and two legs are genuine defects that also affect legitimate authenticators. Root fixes: - Give the five WebAuthn error classes a common ActionPack::WebAuthn::Error superclass so a single rescue turns any parse/verification failure into a 4xx. My::PasskeysController#create now redirects with an alert on WebAuthn::Error, and re-registering an existing credential (RecordNotUnique on the global unique index) returns a friendly "already registered" instead of a 500. Passkey#authenticate rescues the superclass on the assertion path. - Serialize public keys with public_to_der instead of to_der. Ed25519 keys decode to a generic OpenSSL::PKey::PKey with no #to_der, which crashed registration for EdDSA credentials; public_to_der works for EC/RSA/Ed25519 and round-trips back through OpenSSL::PKey.read. - Widen action_pack_passkeys.sign_count from a signed INT4 to bigint. WebAuthn sign counts are unsigned 32-bit; a spec-legal high counter overflowed the column with a RangeError and broke the replay/clone check at the boundary. - Reject malformed ceremony input at the decode boundary as InvalidResponseError rather than letting TypeError/NoMethodError escape: client data must be a JSON object, the challenge must be a string, tokenBinding must be an object, the attestation object must be a CBOR map with binary authData, and COSE key coordinates must be byte strings. - Bound CBOR definite-length arrays/maps by the remaining input and build arrays incrementally instead of pre-sizing to a declared count. A five-byte input could otherwise name a multi-billion-element array and drive an out-of-memory pre-allocation; nested containers could amplify further. - Validate the actual RSA modulus bit length (not the encoded byte length, which a leading-zero pad defeats) and reject degenerate public exponents such as e=1.
There was a problem hiding this comment.
Pull request overview
Hardens WebAuthn passkey registration against malformed input, unsupported credentials, serialization failures, and oversized counters.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Changes:
- Adds unified WebAuthn error handling and friendly registration failures.
- Strengthens CBOR, attestation, COSE key, and RSA validation.
- Supports Ed25519 serialization and full uint32 signature counters.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
app/controllers/my/passkeys_controller.rb |
Handles registration and duplicate errors. |
lib/action_pack/passkey.rb |
Rescues all WebAuthn ceremony errors. |
lib/action_pack/web_authn.rb |
Introduces the common error superclass. |
lib/action_pack/web_authn/authenticator/attestation.rb |
Validates decoded attestation structure. |
lib/action_pack/web_authn/authenticator/response.rb |
Adds client-data type guards. |
lib/action_pack/web_authn/cbor_decoder.rb |
Prevents declared-length preallocation DoS. |
lib/action_pack/web_authn/cose_key.rb |
Strengthens coordinate and RSA validation. |
lib/action_pack/web_authn/public_key_credential.rb |
Uses portable public-key DER serialization. |
db/migrate/20260825120000_widen_action_pack_passkeys_sign_count_to_bigint.rb |
Widens signature counters. |
db/schema.rb |
Records the MySQL schema change. |
db/schema_sqlite.rb |
Records the SQLite schema change. |
test/controllers/my/passkeys_controller_test.rb |
Covers malformed and duplicate registration. |
test/lib/action_pack/passkey_test.rb |
Covers uint32-max counter persistence. |
test/lib/action_pack/web_authn/authenticator/attestation_response_test.rb |
Covers malformed registration responses. |
test/lib/action_pack/web_authn/cbor_decoder_test.rb |
Covers declared-length bounds. |
test/lib/action_pack/web_authn/cose_key_test.rb |
Covers COSE and RSA validation. |
test/lib/action_pack/web_authn/public_key_credential_test.rb |
Covers Ed25519 and EC serialization. |
test/lib/action_pack/web_authn_test.rb |
Verifies the error hierarchy. |
test/test_helpers/webauthn_test_helper.rb |
Supports deterministic duplicate credentials. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58db449087
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -113,7 +128,9 @@ def must_not_be_cross_origin | |||
| end | |||
There was a problem hiding this comment.
When clientDataJSON supplies tokenBinding as a scalar or array, this guard silently skips validation, allowing an otherwise valid registration or authentication response to pass with schema-invalid token-binding data. The previous dig call failed closed, and this hardening should convert that failure into an InvalidResponseError, not acceptance; add an error when the field is present but is not a hash before checking its status.
Useful? React with 👍 / 👎.
Address review: several fuzzing vectors still escaped as TypeError/NoMethodError rather than the WebAuthn::Error the controller rescues, so they remained 500s. Rather than guard each decode site, enforce type at the input chokepoints: - Response/AttestationResponse/AssertionResponse constructors require the raw ceremony inputs (client_data_json, attestation_object, authenticator_data, signature) to be strings. Strong parameters permit scalars, so a JSON request body could otherwise deliver a number/object and crash on JSON.parse or #encoding. This also closes the parallel pre-auth assertion-path crashes. - CoseKey.decode rejects a decoded COSE value that is not a map (e.g. a CBOR null public key) instead of indexing nil. - AttestationResponse requires attested credential data (the AT flag): a valid CBOR map with binary authData but no attested credential would otherwise reach persistence with a nil credential id/public key and crash on public_to_der. Also correct an overstated fixture comment (256 bytes, not 2048).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ff1760cb9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # count must not pre-size the backing store. read_length caps the count | ||
| # at the remaining bytes, and nested containers still exhaust the input | ||
| # (and hit the depth limit) before the array can grow large. | ||
| Array.new.tap { |arr| read_length.times { arr << decode } } |
There was a problem hiding this comment.
An authenticated passkey-registration request can still exhaust a worker by placing a definite-length array of millions of one-byte values (or empty containers) inside the attestation map. Because MAX_SIZE permits 10 MB and read_length only compares the count with the remaining byte count, this loop can build millions of Ruby objects/references—hundreds of MB—before the later authData type check rejects the payload. Add a substantially smaller container-element limit or a protocol-appropriate attestation size limit rather than treating every remaining byte as a safe element.
AGENTS.md reference: AGENTS.md:L6-L9
Useful? React with 👍 / 👎.
| unless attestation_object.is_a?(String) | ||
| raise ActionPack::WebAuthn::InvalidResponseError, "Attestation object is missing or malformed" |
There was a problem hiding this comment.
When a library caller passes an existing Authenticator::Attestation into AttestationResponse, this String-only guard now rejects an input that previously worked: the attestation accessor delegates to Attestation.wrap, whose documented behavior is to return an existing attestation object as-is. This breaks callers that decode once or construct responses for custom attestation verifiers; permit both serialized strings and Attestation instances rather than making the wrapper's supported branch unreachable.
Useful? React with 👍 / 👎.
A HackerOne researcher fuzzed
POST /:slug/my/passkeys(2026-08-25 21:13–21:36 UTC, ~9 events, one actor) and produced five distinct uncaught 500s in the newActionPack::WebAuthn/ActionPack::Passkeyregistration stack. On-call card: On Call #10239040047.Security read up front: the verification stack itself is sound — the signed challenge is verified, origin and RP-ID hash are compared with
ActiveSupport::SecurityUtils.secure_compare, cross-origin and token-binding responses are rejected, and each COSE algorithm is strictly matched to its key type. Every one of the five legs fails closed. None is an auth bypass, replay, origin/RP-ID confusion, or credential-swap. The defects are (a) validation failures and malformed input surfacing as 500s instead of a clean rejection, and (b) two schema/serialization bugs that also break legitimate authenticators.The five probed failures and their verdict
fmt: "packed"(no verifier registered)OpenSSL::PKey::PKeywith no#to_der, crashing serialization. Narrow (ES256 is offered first and negotiated by dual-alg authenticators).public_key.to_der→public_to_der(round-trips EC/RSA/Ed25519)sign_count = 4294967295INT4(max 2147483647) while WebAuthn counts areuint32. A spec-legal counter raisedRangeErrorrather than truncating — no bad write, but the credential was unusable.sign_counttobigintattStmtunderfmt: "none"noneverifier correctly rejects a non-empty statement (fails closed). Disposition only.credential_idRecordNotUniquewas just an unfriendly 500.RecordNotUnique→ "already registered"What the probing revealed beyond the five (fixed here)
Array.new(read_argument) { decode }pre-sized the backing store to an attacker-declared count. A five-byte input (0x9a ff ff ff ff) named a ~4.3-billion-element array and drove a ~34 GB allocation (reproduced: hangs the process). Nested containers amplified further. Fixed by bounding a definite-length container's declared count by the remaining input and building arrays incrementally so a lying count can never pre-allocate.TypeError/NoMethodError. Closed at the decode boundary: client data must be a JSON object, the challenge must be a string,tokenBindingmust be an object, the attestation object must be a CBOR map with binaryauthData, and COSE coordinates must be byte strings.e = 1(verifies a forged signature with no private key — confirmed locally). Now validates the actual modulus bit length and rejects degenerate exponents. Reachability is low (registration is authenticated; the actor would only weaken their own credential), but it is cheap correctness in the exact COSE surface under probe.Fixes
lib/action_pack/web_authn.rb— commonActionPack::WebAuthn::Errorsuperclass for the five error classes.app/controllers/my/passkeys_controller.rb—createrescuesWebAuthn::Error(→ alert) andRecordNotUnique(→ "already registered").lib/action_pack/passkey.rb—authenticaterescues the superclass on the assertion path.lib/action_pack/web_authn/public_key_credential.rb—public_to_der.lib/action_pack/web_authn/authenticator/response.rb,attestation.rb,cose_key.rb— decode-boundary type guards.lib/action_pack/web_authn/cbor_decoder.rb— declared-length bound + incremental array build.db/migrate/20260825120000_widen_action_pack_passkeys_sign_count_to_bigint.rb+db/schema.rb—sign_count→bigint.Tests
New/extended coverage for every leg: controller graceful-rejection + duplicate handling (
test/controllers/my/passkeys_controller_test.rb), Ed25519/ECto_hround-trip (public_key_credential_test.rb),uint32-max sign-count persistence (passkey_test.rb), CBOR declared-length DoS bounds (cbor_decoder_test.rb), COSE type/RSA-exponent/bit-length guards (cose_key_test.rb), malformed client-data/challenge/attestation rejection (attestation_response_test.rb), and the error-hierarchy invariant (web_authn_test.rb). Fullbin/cigreen (rubocop, brakeman, all audits, OSS + system tests).Flagged, not fixed here (design-level, tracked on the card)
Not silently patched — surfaced for follow-up: challenge tokens are authenticated + expiring but not single-use / not bound to the identity (replayable within the TTL); the sign-count check/update is an unlocked read-check-write (racy under concurrency); the
credential_idunique index uses a case-insensitive collation (utf8mb4_0900_ai_ci) while base64url IDs are case-sensitive, and thestring(255)column is narrower than spec-legal credential IDs;userVerification: requiredis not server-enforced (no live impact — Fizzy uses the defaultpreferred). These are separate from the 500 cluster and warrant their own assessment.Response contract
Registration failures redirect back with a flash alert (HTTP 302), consistent with this controller's
update/destroyactions and the Turbo form flow — not a bare 422 body. The security property is that malformed input fails closed with a clean, non-500 response. (An earlier draft of this description said "422"; the redirect is the intended UX.)