Files

T

kyle 7755be49e9 Fix SHA-256 padding bug for messages of length 63 mod 64

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

2026-09-17 12:25:23 -07:00

2.9 KiB

Raw Permalink Blame History

Grok audit: local SHA-256 padding fix (1.0.4)

Date: 2026-09-17
Mode: local uncommitted changes (staged + unstaged) vs HEAD
Scope: Cargo.toml, Cargo.lock, src/sha256.rs, tests/sha256.rs, tests/hmac.rs
Diff: 5 files, +62 / −18
Verdict: Correctness of the padding rewrite is sound. No bugs. Two open suggestions.

Summary

This change fixes SHA-256 padding when the current block index mbi is 63 (message length ≡ 63 mod 64), bumps the crate from 1.0.3 to 1.0.4, and adds regression vectors for SHA-256 lengths 55/56/63/64/65 and for HMAC with a 63-byte message on a short key.

The old pad_message path treated mbi == MB_SIZE - 1 as a special case: it zero-filled the last byte of the current block and wrote 0x80 at the start of the next block. The new path always writes the pad byte into the current block (safe because update flushes a full block, so mbi is always < MB_SIZE on entry) and only starts an extra block when the 8-byte length field no longer fits (mbi > MB_SIZE - 8).

Independent SHA-256 and HMAC-SHA-256 vectors match the new tests. Residual issues are comments that restate control flow or bug history. Removing publish = ["kellnr"] is intentional so 1.0.4 can be published to crates.io.

What was reviewed as correct

Writing pc at self.mb[self.mbi] without the mbi < MB_SIZE - 1 branch is valid: update processes a block as soon as it fills, so pad_message never sees mbi == 64.
The extra-block condition mbi > (MB_SIZE - 8) covers both “pad byte left fewer than 8 bytes” (lengths 56–62 mod 64) and the previously broken mbi == 63 case (length 63 mod 64).
SHA-256 boundary tests at 55, 56, 63, 64, and 65 bytes exercise the pad-fits, extra-block, last-byte, full-block, and overflow-into-next-block cases.
HMAC test_hmac_pad_boundary is the inner-hash instance of the same bug: 64-byte ipad + 63-byte message → 127 bytes → mbi == 63 at pad time.
Dropping publish = ["kellnr"] is intentional so cargo publish targets crates.io.

Issues

1 — suggestion — `src/sha256.rs:177`

The comment on the extra-block branch restates the if body (zero-fill, process, start a new block) instead of only explaining why the extra block is needed.

The first sentence already states the invariant (“fewer than 8 bytes remain, so the length field cannot fit”). Drop the second sentence, or keep a single WHY line.

2 — suggestion — `tests/hmac.rs:63`

The comment explains the 63-byte inner-hash case, then narrates the bug (“the length that triggered the padding bug”) rather than stating the invariant the test protects.

Drop the history clause. Say that 64-byte ipad + 63-byte message leaves mbi == 63, so padding must place 0x80 as the last byte of the current block.

Counts

Severity	Count
bug	0
suggestion	2
nit	0

All issues remain open.

2.9 KiB Raw Permalink Blame History Unescape Escape