block / block/buzz

buzz-audit: field-boundary collision in compute_hash lets a detail edit survive verify_chain

Open
#4,173 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
32.7k
Forks
4.3k
Avg merge
1d 13h
Merged PRs (30d)
253

Description

## Summary

`crates/buzz-audit/src/hash.rs::compute_hash` concatenates its fields into the
SHA-256 preimage with no length prefixes or delimiters. Two of those fields are
adjacent and both variable-length:

```rust
match &entry.object_id {
Some(id) => { hasher.update([1u8]); hasher.update(id.as_bytes()); }
None => hasher.update([0u8]),
}
hasher.update(canonical_json(&entry.detail)?.as_bytes());
```

`object_id` is `Option` (free text) and `detail` is an arbitrary
`serde_json::Value`, which may be a bare scalar. So the boundary between them
can be shifted, and two distinct entries can share one digest.

This defeats the property `entry.rs` documents for `detail`:

> *"Arbitrary JSON context. **Included in the hash** (serialized with sorted keys
> for determinism) so tampering with it is detectable."*

An entry can be substituted for a different one and `verify_chain` still passes.

## Reproduction

Both of these hash identically:

| | `object_id` | `detail` | concatenated tail |
|---|---|---|---|
| A | `"x"` | `12` | `x` ‖ `12` = `x12` |
| B | `"x1"` | `2` | `x1` ‖ `2` = `x12` |

Confirmed by reimplementing the exact field order (all other fields held equal —
`community_id`, `seq`, `created_at`, `action`, `actor_pubkey`, `prev_hash`):

```
A (object_id='x', detail=12) = b69cc4a59d5feb1eea4cdb5ea3d65924025bca46250701e0f86e6f8e0b017abb
B (object_id='x1', detail=2 ) = b69cc4a59d5feb1eea4cdb5ea3d65924025bca46250701e0f86e6f8e0b017abb
COLLISION: True
```

A Rust test in the style of the existing `sensitive_to_each_field` should
reproduce it directly.

## Severity

Low as shipped, and we want to be precise about that rather than overstate it.
`object_id` is relay-resolved (event id hex, channel UUID, media sha256) and
`detail` is written by relay code, so we found no path where a client controls
both today. `NewAuditEntry` is deliberately not `Deserialize`, which helps.

It becomes live the first time an audit action logs a caller-supplied
`object_id` alongside a scalar `detail` — a natural future change that would
silently reintroduce forgeability with no test failing.

## Suggested fix

Make the encoding unambiguous. Any one of:

1. **Length-prefix the variable-length fields** — `update(len.to_be_bytes())`
before each of `created_at`, `action`, `actor_pubkey`, `object_id`, and the
canonical JSON. Most robust; invalidates existing chains.
2. **Move a fixed-width field between them** — e.g. hash `prev_hash` between
`object_id` and `detail`. Cheapest structurally, still a chain-invalidating
field-order change.
3. **Hash the canonical JSON of the whole entry** as one object rather than a
field concatenation.

Any of these changes the digest, so it needs the same migration treatment as any
other change to the fixed field order.

## Context / disclosure

We adapted the hashing design (not the code) for a Python project, with
attribution per Apache-2.0, and found this while writing our own regression
tests. Our port happens not to be vulnerable — we place the fixed-width 32-byte
`prev_hash` between the sequence number and the variable-length payload, and our
payload is always a JSON object, so its canonical form always begins with `{`.
That was luck of field ordering rather than design, which is why we went looking.

Happy to open a PR with a failing test if useful.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.