Repository navigation
Reject Relying Party IDs that are empty or longer than 253 bytes - #805
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a maximum length constraint of 253 bytes for the Relying Party ID (rp_id) in CTAP2 make credential parameters, aligning with WebAuthn level 3 and RFC 1035 specifications. It also adds corresponding unit tests to verify this boundary. The review feedback suggests extending this validation to also reject empty Relying Party IDs, as they are similarly invalid, and adding a unit test to cover this case.
| if rp.rp_id.len() > MAX_RP_ID_LENGTH { | ||
| return Err(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER); | ||
| } |
There was a problem hiding this comment.
An empty Relying Party ID (rp_id) is also not a valid domain string according to the WebAuthn and CTAP2 specifications. We should reject empty RP IDs early, similar to how we reject empty user handles, to prevent potential issues or undefined behavior when creating credentials.
if rp.rp_id.is_empty() || rp.rp_id.len() > MAX_RP_ID_LENGTH {
return Err(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER);
}
35fb3a6 to
831388d
Compare
17krishna8
changed the title
|
Coverage is 97.425% — 17krishna8:fix/reject-long-rp-id into google:develop. No base build found for |
WebAuthn requires the RP ID to be a valid domain string: non-empty, and capped at 253 bytes in presentation format by RFC 1035, so browsers never send anything else. Nothing enforces that for software that talks CTAP directly over USB HID. A MakeCredential request with rk=true and an empty or longer RP ID runs the whole ceremony, including the user presence check and the key generation, and only fails at the very end when storing the discoverable credential, because the oversized RP ID pushes the serialized credential past the maximum value size of the persistent store. Reject the RP ID upfront instead, right after parsing. Rebased on develop and reuses MAX_RP_ID_LENGTH from data_formats (now pub(crate)), which was introduced for the setMinPINLength RP IDs.
831388d to
855f5ac
Compare
kaczmarczyck
left a comment
kaczmarczyck
left a comment
There was a problem hiding this comment.
Thanks for the latest batch of PRs!
WebAuthn requires the RP ID to be a valid domain string: non-empty, and capped at 253 bytes in presentation format by RFC 1035, so browsers never send anything else.
Nothing enforces that for software that talks CTAP directly over USB HID. A MakeCredential request with
rk=trueand an empty or longer RP ID runs the whole ceremony, including the user presence check and the key generation, and only fails at the very end when storing the discoverable credential, because the oversized RP ID pushes the serialized credential past the maximum value size of the persistent store. The platform then gets a confusingCTAP2_ERR_VENDOR_INTERNAL_ERRORinstead of a proper rejection of the request.This mirrors what was done for user handles in #791: check the value when parsing the parameters, so the request is rejected with
CTAP1_ERR_INVALID_PARAMETERbefore any state is touched.