Skip to content

mcf: validate the field pushed by PasswordHash::push_base64 - #2498

Open
choksi2212 wants to merge 1 commit into
RustCrypto:masterfrom
choksi2212:fix/mcf-push-base64-empty-field-2471
Open

choksi2212 wants to merge 1 commit into
RustCrypto:masterfrom
choksi2212:fix/mcf-push-base64-empty-field-2471

Conversation

@choksi2212

Copy link
Copy Markdown
Contributor

PasswordHash is documented to always hold a valid MCF hash, but push_base64 was the one push taking unvalidated input that neither validated nor reported: it appended the $ delimiter plus the base64-encoding of its input unconditionally. Empty input therefore produced a hash string that no longer parses, exactly as diagnosed in #2471 (credit to @yuxi-liu-wired for the analysis and the reproduced scenarios — this implements their option 1):

let mut h = PasswordHash::from_id("6").unwrap();
h.push_base64(b"", Base64::Crypt);
h.as_str()                      // "$6$"
PasswordHash::new("$6$")        // Err(DelimiterInvalid)  <- no longer parses
h.push_str("tail").unwrap();    // "$6$$tail"
PasswordHash::new("$6$$tail")   // Err(FieldInvalid), fields() = ["", "tail"]

reproduced empirically on pristine master, while push_str("") and push_displayable("") already reject empty with Err(FieldInvalid) — so the infallible push_base64 was the one remaining unvalidated push.

Change

push_base64 now validates the encoded field and returns Result, mirroring the adjacent push_displayable (#2115/#2118): the pre-push length is cached, and on validation failure the delimiter is rolled back and the Field error is returned, so a rejected push leaves the hash exactly as it was — subsequent pushes still build a valid hash (pinned by the second regression test via the salt-then-hash push sequence the pbkdf2/scrypt MCF hashers use).

Non-empty input is unaffected: every Base64 alphabet encodes into the [A-Za-z0-9./+=,\-] field charset, so the only newly-rejected input is empty.

Breaking-change note

The signature changes () -> Result<()>, i.e. a breaking change for the next mcf release (0.7.0). No other crate in this repo calls it; the known downstream callers are the MCF hashers in RustCrypto/password-hashes, all inside functions that already return Result:

  • pbkdf2/src/mcf.rs:56,57
  • scrypt/src/mcf.rs:69
  • sha-crypt/src/mcf.rs:79,83
  • yescrypt/src/mcf.rs:54,57

each a mechanical .unwrap()/? addition once mcf 0.7 is published. I'm happy to prep those PRs once this lands. If you'd rather avoid the break, the alternatives considered in #2471 were a fallible try_push_base64 alongside the current method (leaves the invariant breakable via push_base64 in release builds) or panicking (counter to the direction #2118 took for push_displayable); happy to rework if you prefer a different shape.

Verification

rustc 1.99.0, Windows/MSVC:

  • new regression tests push_base64_empty + push_base64_empty_leaves_hash_usable: green with the fix; negative control — with the validation removed but the Result signature kept, both fail with Ok(()) != Err(FieldInvalid)
  • cargo test -p mcf --all-features --lib --tests: 9/9 (default features 9/9, --no-default-features --features alloc 6/6)
  • cargo build --workspace: clean
  • cargo clippy -p mcf --all-features --all-targets: 0 warnings
  • cargo fmt -p mcf -- --check: clean
  • mcf/CHANGELOG.md: Unreleased entry added

Fixes #2471

`PasswordHash` is documented to always hold a valid MCF hash, but
`push_base64` was the one push taking unvalidated input that neither
validated nor reported: it appended the `$` delimiter plus the
base64-encoding of its input unconditionally, so empty input produced
a hash string that no longer parses (the `$6$` shapes from RustCrypto#2471).

Make `push_base64` validate the encoded field and return `Result`,
mirroring the adjacent `push_displayable`: the pre-push length is
cached and on validation failure the hash is rolled back to its prior
contents, so a rejected push leaves the hash exactly as it was.
Empty input now fails with the same `FieldInvalid` error `push_str`
and `push_displayable` already return for empty fields.

Fixes RustCrypto#2471

Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>

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.

mcf: PasswordHash::push_base64 with empty input produces an invalid hash

1 participant