kyle
7755be49e9
pad_message wrote 0x00 into the last byte of the current block and deferred the 0x80 pad byte to the next block when mbi == 63, producing incorrect digests for every message whose length is 63 mod 64 (and correspondingly wrong HMAC tags). Always write the pad byte in the current block first, then flush an extra block only when the 8-byte length field no longer fits. Adds regression tests for SHA-256 boundary lengths 55/56/63/64/65 and HMAC with a 63-byte message. Bumps to 1.0.4 for crates.io publication. Ref: docs/LAGUNA-AUDIT.md
73 lines
5.2 KiB
Markdown
73 lines
5.2 KiB
Markdown
# Claude audit: local SHA-256 padding fix (1.0.4)
|
||
|
||
**Date:** 2026-09-17
|
||
**Mode:** local uncommitted changes vs `HEAD`
|
||
**Scope:** `Cargo.toml`, `Cargo.lock`, `src/sha256.rs`, `tests/sha256.rs`, `tests/hmac.rs`
|
||
**Diff:** 5 files, +63 / −18
|
||
**Passes:** security review, cloud multi-agent review, code review (high effort)
|
||
**Verdict:** The padding rewrite is correct. No vulnerabilities and no correctness bugs. One latent panic closed with a `debug_assert!`.
|
||
|
||
## Summary
|
||
|
||
Three independent review passes examined the 1.0.4 padding fix. Two found nothing; the third found one unreachable panic and one intentional packaging change. All three confirmed the rewrite is correct SHA-256 padding and that it repairs a real digest bug.
|
||
|
||
The old `pad_message` mishandled `mbi == 63` (message length ≡ 63 mod 64): it wrote `0x00` into the last byte of the current block and deferred the `0x80` pad byte to index 0 of the next block. Every such message hashed wrong, and HMAC tags over those lengths were nonstandard. The new path writes the pad byte into the current block unconditionally and starts an extra block only when the 8-byte length field no longer fits (`mbi > MB_SIZE - 8`).
|
||
|
||
All three passes are scoped to the diff. None of them audited the unchanged source.
|
||
|
||
## Passes
|
||
|
||
### 1 — security review (`/security-review`)
|
||
|
||
Sub-agent review of the diff for newly introduced vulnerabilities, with false-positive filtering. **No findings.**
|
||
|
||
It walked `pad_message` for every entry value of `mbi` in `0..=63`, confirming the `0x80` byte lands directly after the message and the length occupies the final 8 bytes of the last block in each case. It established that no stale buffer bytes can reach a compressed block — in the two-block path every byte of the final block is rewritten, and in the one-block path everything after the `0x80` is zeroed — so padding stays injective and distinct messages cannot collide through it. It confirmed `src/hmac.rs` is unchanged, uses only `update`/`finalize`/`reset`, and gains no forgery or length-extension avenue.
|
||
|
||
The pass reported comparing 2107 SHA-256 and HMAC-SHA-256 digests against Python `hashlib`/`hmac` with no mismatches, covering message lengths 0–300 across six `update` split points and HMAC key lengths 0–130.
|
||
|
||
### 2 — cloud multi-agent review (ultrareview)
|
||
|
||
Multi-agent review of the same 5-file diff, run in the cloud. **No findings.**
|
||
|
||
### 3 — code review (`/code-review high`)
|
||
|
||
High-effort correctness and quality review of the diff. **Two low findings**, both below.
|
||
|
||
It confirmed the old-code bug directly, verified all six new test vectors against Python `hashlib`/`hmac`, and reported an exhaustive comparison against `hashlib` for every input length 0–299. `cargo clippy --all-targets` was clean.
|
||
|
||
## Issues
|
||
|
||
### 1 — low — resolved — `src/sha256.rs:174`
|
||
|
||
`pad_message` wrote `self.mb[self.mbi] = pc` unconditionally, resting on the invariant that `mbi < MB_SIZE` on entry. `update` has one hole in that invariant: the byte store and `mbi += 1` happen before `self.add_length(8)?`, while the block flush that resets `mbi` happens after it. If `add_length` returns `InputTooLong` on the byte that just filled the block, `mbi` is left at 64 with `hresult` still `OK` and `complete` still false, so a later `finalize` reaches `pad_message` and indexes out of bounds.
|
||
|
||
Reaching this requires `mlen` to overflow `u64` — roughly 2^61 bytes through a single context — and the consequence is a panic, not a wrong digest. The old code happened to survive the state because its `update_message_block` call reset `mbi` first, making this a narrow regression rather than a new class of problem.
|
||
|
||
**Resolved:** `debug_assert!(self.mbi < MB_SIZE)` added at the top of `pad_message`, documenting the invariant the comment already asserted. The flush ordering in `update` was left as-is.
|
||
|
||
### 2 — not a defect — `Cargo.toml:8`
|
||
|
||
Removing `publish = ["kellnr"]` changes what a bare `cargo publish` does: with the key present it errors out unless `--registry kellnr` is given, and without it the crate defaults to publishable and 1.0.4 goes to crates.io. Both the code review and the security review raised it for confirmation.
|
||
|
||
**Confirmed intentional** — 1.0.4 is meant for crates.io.
|
||
|
||
## Verification
|
||
|
||
Checked directly rather than taken from a review pass:
|
||
|
||
- The six new test vectors match `sha256sum` and Python `hmac` exactly — SHA-256 at 55/56/63/64/65 bytes, and HMAC-SHA-256 over a 63-byte message with the 20-byte `0x0b` key.
|
||
- `cargo test` passes: 11 tests across the unit, integration, and doc suites, including the new boundary cases.
|
||
|
||
## Counts
|
||
|
||
| Severity | Count | State |
|
||
|------------|-------|------------------|
|
||
| high | 0 | — |
|
||
| medium | 0 | — |
|
||
| low | 1 | resolved |
|
||
| packaging | 1 | confirmed intent |
|
||
|
||
## Coverage
|
||
|
||
Every pass here reviewed the diff, not the repository. Reviewers read surrounding files such as `src/hmac.rs`, `src/lib.rs`, and `src/common.rs` for context, but findings were anchored to changed lines and unchanged code was not itself audited. A full-repository scan was offered and declined during this session, so the crate outside this diff remains unreviewed by these passes.
|