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
48 lines
2.9 KiB
Markdown
48 lines
2.9 KiB
Markdown
# 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**.
|