Sitelet https://github.com/RustCrypto/formats/pull/2462
Skip to content

phc: add_b64_bytes keeps the ParamsString invariants - #2462

Open
yuxi-liu-wired wants to merge 1 commit into
RustCrypto:masterfrom
yuxi-liu-wired:fix/phc-add-b64-bytes-invariants
Open

yuxi-liu-wired wants to merge 1 commit into
RustCrypto:masterfrom
yuxi-liu-wired:fix/phc-add-b64-bytes-invariants

Conversation

@yuxi-liu-wired

Copy link
Copy Markdown
Contributor

ParamsString is meant to always hold a valid parameter string: iter() expects it with INVARIANT_VIOLATED_MSG, and FromStr validates every name and value. add_decimal and add_str go through add(), which rejects duplicates and leaves the buffer unchanged on overflow. add_b64_bytes writes straight into the buffer instead, and that breaks the invariant three ways:

let mut p = ParamsString::new();
p.add_decimal("m", 1).unwrap();
p.add_b64_bytes("m", b"x").unwrap();        // Ok: "m=1,m=eA" (add_decimal/add_str reject this)

let mut p = ParamsString::new();
p.add_b64_bytes("data", &[0xAB; 60]).unwrap(); // Ok: 80-char value, > Value::MAX_LENGTH (64)
ParamsString::from_str(p.as_str());         // Err(ParamValueTooLong)

let mut p = ParamsString::new();
p.add_decimal("a", 1).unwrap();
p.add_b64_bytes("data", &[0xAB; 90]);       // Err(Base64(InvalidLength)), but p is now "a=1,data="

End to end, a PasswordHash whose params were built with add_b64_bytes and 49–88 bytes of data (Argon2's data/keyid params are B64) serializes to a PHC string that PasswordHash::new rejects with "parameter value too long".

The fix encodes into a Value::MAX_LENGTH scratch buffer, validates the result as a Value, and adds it through add(), like the other add_* methods. Existing output is unchanged (a=AQ,b=AgM,c=BAUG).

Test: add_b64_bytes_invariants in params.rs covers the duplicate name, the 64-char boundary (48 bytes ok, 49 bytes ParamValueTooLong with nothing written), and an overflow of the total length that leaves the params unchanged. It fails on master. The phc.yml matrix passed locally: powerset tests on stable and 1.85, thumbv7em/wasm32 powerset build, fmt, clippy.

This PR was produced by AI agents (Claude) and reviewed by me before submission.

ParamsString is meant to always hold a valid parameter string
(INVARIANT_VIOLATED_MSG, used by iter()). add_b64_bytes wrote straight
into the buffer instead of going through add(), so it:

- did not reject a duplicate name ("m=1,m=eA"), unlike add_decimal and
  add_str;
- accepted values longer than Value::MAX_LENGTH (64): 49..=88 bytes
  encode to 66..=118 characters, which FromStr rejects ("parameter
  value too long"), so a PasswordHash built with such a param does not
  parse back;
- on a Base64 length error, left the delimiter and "name=" behind.

Encode into a Value::MAX_LENGTH scratch buffer and add the result
through add(), like the other add_* methods.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant