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
This commit is contained in:
Generated
+1
-1
@@ -4,4 +4,4 @@ version = 4
|
|||||||
|
|
||||||
[[package]]
|
[[package]]
|
||||||
name = "emsha"
|
name = "emsha"
|
||||||
version = "1.0.3"
|
version = "1.0.4"
|
||||||
|
|||||||
+1
-2
@@ -5,8 +5,7 @@ repository = "https://git.wntrmute.dev/wntrmute/emsha-rs"
|
|||||||
categories = ["cryptography", "no-std", "embedded"]
|
categories = ["cryptography", "no-std", "embedded"]
|
||||||
keywords = ["sha256", "hmac", "hash", "embedded", "no_std"]
|
keywords = ["sha256", "hmac", "hash", "embedded", "no_std"]
|
||||||
license-file = "LICENSE"
|
license-file = "LICENSE"
|
||||||
version = "1.0.3"
|
version = "1.0.4"
|
||||||
edition = "2024"
|
edition = "2024"
|
||||||
publish = ["kellnr"]
|
|
||||||
|
|
||||||
[dependencies]
|
[dependencies]
|
||||||
|
|||||||
@@ -0,0 +1,72 @@
|
|||||||
|
# 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.
|
||||||
@@ -0,0 +1,47 @@
|
|||||||
|
# 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**.
|
||||||
@@ -0,0 +1,73 @@
|
|||||||
|
# Laguna 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, +74 / −21
|
||||||
|
**Verdict:** Correct. No bugs. Two minor suggestions addressed.
|
||||||
|
|
||||||
|
## Summary
|
||||||
|
|
||||||
|
This change fixes a SHA-256 padding bug for messages whose length is 63 mod 64,
|
||||||
|
bumps the crate from 1.0.3 to 1.0.4, adds regression tests, and removes the
|
||||||
|
`publish = ["kellnr"]` restriction for crates.io publication.
|
||||||
|
|
||||||
|
The old `pad_message` in `src/sha256.rs` used the condition `self.mbi < MB_SIZE - 1`
|
||||||
|
to decide whether to write the `0x80` pad byte in the current block before
|
||||||
|
zero-fill and block processing. When `mbi == 63` (i.e. the message is 63 mod 64
|
||||||
|
bytes), that condition was false, so position 63 was zero-filled instead of
|
||||||
|
receiving `0x80`. The block was then processed with `message || 0x00`, and `0x80`
|
||||||
|
was written at index 0 of the next block — producing a digest of
|
||||||
|
`message || 0x00 || 0x80 || ...` instead of the correct `message || 0x80 || 0x00 || ...`.
|
||||||
|
|
||||||
|
The new code always writes the pad byte at `self.mb[self.mbi]` first (safe because
|
||||||
|
`update` flushes full blocks, so `mbi` is always `< MB_SIZE` on entry), then
|
||||||
|
zero-fills and processes the current block only when the 8-byte length field no
|
||||||
|
longer fits (`mbi > MB_SIZE - 8`).
|
||||||
|
|
||||||
|
HMAC-SHA-256 was affected too, since its inner hash processes 64-byte ipad +
|
||||||
|
message bytes; a 63-byte message makes the inner length 127 (63 mod 64).
|
||||||
|
|
||||||
|
## Verification
|
||||||
|
|
||||||
|
- The six new test vectors match `sha256sum` and Python `hmac`/`hashlib` exactly:
|
||||||
|
SHA-256 at lengths 55, 56, 63, 64, 65 bytes (all `a`), and HMAC-SHA-256 over
|
||||||
|
a 63-byte message with the 20-byte `0x0b` key.
|
||||||
|
- `cargo test` passes: 11 tests across unit, integration, and doc suites.
|
||||||
|
- `cargo clippy --all-targets` is clean.
|
||||||
|
|
||||||
|
## Issues
|
||||||
|
|
||||||
|
### 1 — resolved — comment refinement: `src/sha256.rs:177`
|
||||||
|
|
||||||
|
The branch comment originally restated the code ("Zero-fill and process it, then
|
||||||
|
start a fresh block for the length"). Trimmed to state the *why*:
|
||||||
|
|
||||||
|
```
|
||||||
|
// Fewer than 8 bytes remain; flush this block and start
|
||||||
|
// a fresh block for the length field.
|
||||||
|
```
|
||||||
|
|
||||||
|
### 2 — resolved — comment refinement: `tests/hmac.rs:63`
|
||||||
|
|
||||||
|
The test comment referenced "the length that triggered the padding bug" (bug
|
||||||
|
history) rather than the invariant. Reworded to state the invariant:
|
||||||
|
|
||||||
|
```
|
||||||
|
// A 63-byte message makes the inner SHA-256 process 127 bytes
|
||||||
|
// (64 key-pad + 63 message), which is 63 mod 64 — the boundary
|
||||||
|
// where the pad byte must start a new block.
|
||||||
|
```
|
||||||
|
|
||||||
|
## Packaging change
|
||||||
|
|
||||||
|
Removing `publish = ["kellnr"]` from `Cargo.toml` is intentional so `cargo
|
||||||
|
publish` targets crates.io for the 1.0.4 release.
|
||||||
|
|
||||||
|
## Counts
|
||||||
|
|
||||||
|
| Severity | Count | State |
|
||||||
|
|----------------|-------|------------|
|
||||||
|
| bug | 0 | — |
|
||||||
|
| suggestion | 2 | resolved |
|
||||||
|
| packaging | 1 | intentional |
|
||||||
+6
-13
@@ -169,28 +169,21 @@ impl SHA256 {
|
|||||||
}
|
}
|
||||||
|
|
||||||
fn pad_message(&mut self, pc: u8) -> Result<()> {
|
fn pad_message(&mut self, pc: u8) -> Result<()> {
|
||||||
if self.mbi < (MB_SIZE - 8) {
|
// After update, mbi is always below MB_SIZE, so the pad byte
|
||||||
|
// always fits in the current block.
|
||||||
|
debug_assert!(self.mbi < MB_SIZE);
|
||||||
self.mb[self.mbi] = pc;
|
self.mb[self.mbi] = pc;
|
||||||
self.mbi += 1;
|
self.mbi += 1;
|
||||||
} else {
|
|
||||||
let mut pc_add = false;
|
|
||||||
|
|
||||||
if self.mbi < MB_SIZE - 1 {
|
|
||||||
self.mb[self.mbi] = pc;
|
|
||||||
self.mbi += 1;
|
|
||||||
pc_add = true;
|
|
||||||
}
|
|
||||||
|
|
||||||
|
// Fewer than 8 bytes remain; flush this block and start
|
||||||
|
// a fresh block for the length field.
|
||||||
|
if self.mbi > (MB_SIZE - 8) {
|
||||||
while self.mbi < MB_SIZE {
|
while self.mbi < MB_SIZE {
|
||||||
self.mb[self.mbi] = 0;
|
self.mb[self.mbi] = 0;
|
||||||
self.mbi += 1;
|
self.mbi += 1;
|
||||||
}
|
}
|
||||||
|
|
||||||
self.update_message_block();
|
self.update_message_block();
|
||||||
if !pc_add {
|
|
||||||
self.mb[self.mbi] = pc;
|
|
||||||
self.mbi += 1;
|
|
||||||
}
|
|
||||||
|
|
||||||
// Assumption: updating the message block has not left the
|
// Assumption: updating the message block has not left the
|
||||||
// context in a corrupted state.
|
// context in a corrupted state.
|
||||||
|
|||||||
@@ -58,6 +58,29 @@ fn test_hmac_02() -> Result<()> {
|
|||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_hmac_pad_boundary() -> Result<()> {
|
||||||
|
// A 63-byte message makes the inner SHA-256 process 127 bytes
|
||||||
|
// (64 key-pad + 63 message), which is 63 mod 64 — the boundary
|
||||||
|
// where the pad byte must start a new block.
|
||||||
|
let k: [u8; 20] = [
|
||||||
|
0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b,
|
||||||
|
0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b,
|
||||||
|
];
|
||||||
|
let msg = [0x61u8; 63];
|
||||||
|
let output = b"2396ff2784cd6b8bdf0ac13df75a30de92e3f15374065b9f21ac81a268a93904";
|
||||||
|
let mut digest: [u8; sha256::SIZE] = [0; sha256::SIZE];
|
||||||
|
let mut hdigest: [u8; 64] = [0; 64];
|
||||||
|
|
||||||
|
let mut h = hmac::HMAC_SHA256::new(&k)?;
|
||||||
|
h.update(&msg)?;
|
||||||
|
h.finalize(&mut digest)?;
|
||||||
|
to_hex(&digest, &mut hdigest);
|
||||||
|
|
||||||
|
assert_eq!(&hdigest, output);
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn test_hmac_03() -> Result<()> {
|
fn test_hmac_03() -> Result<()> {
|
||||||
let k: [u8; 25] = [
|
let k: [u8; 25] = [
|
||||||
|
|||||||
@@ -20,6 +20,35 @@ fn test_self_test() -> Result<()> {
|
|||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_padding_boundaries() -> Result<()> {
|
||||||
|
let tests: &[(usize, &[u8])] = &[
|
||||||
|
(55, b"9f4390f8d30c2dd92ec9f095b65e2b9ae9b0a925a5258e241c9f1e910f734318"),
|
||||||
|
(56, b"b35439a4ac6f0948b6d6f9e3c6af0f5f590ce20f1bde7090ef7970686ec6738a"),
|
||||||
|
(63, b"7d3e74a05d7db15bce4ad9ec0658ea98e3f06eeecf16b4c6fff2da457ddc2f34"),
|
||||||
|
(64, b"ffe054fe7ae0cb6dc65c3af9b61d5209f439851db43d0ba5997337df154668eb"),
|
||||||
|
(65, b"635361c48bb9eab14198e76ea8ab7f1a41685d6ad62aa9146d301d4f17eb0ae0"),
|
||||||
|
];
|
||||||
|
|
||||||
|
let mut h = sha256::SHA256::default();
|
||||||
|
let mut d: [u8; 32] = [0; 32];
|
||||||
|
let mut s: [u8; 64] = [0; 64];
|
||||||
|
let input_buf = [0x61u8; 65];
|
||||||
|
|
||||||
|
let mut i: usize = 0;
|
||||||
|
while i < tests.len() {
|
||||||
|
let (len, expected) = tests[i];
|
||||||
|
h.update(&input_buf[..len])?;
|
||||||
|
h.finalize(&mut d)?;
|
||||||
|
to_hex(&d, &mut s);
|
||||||
|
assert_eq!(&s, expected, "length {}", len);
|
||||||
|
h.reset()?;
|
||||||
|
i += 1;
|
||||||
|
}
|
||||||
|
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn test_golden_tests() -> Result<()> {
|
fn test_golden_tests() -> Result<()> {
|
||||||
let golden_tests: &[HashTest] = &[
|
let golden_tests: &[HashTest] = &[
|
||||||
|
|||||||
Reference in New Issue
Block a user