Skip to content

Untested correctness-critical paths in tamper_log.rs: EntryTooLarge guard and VaultMutation CBOR roundtrip #249

Description

@forkwright

Finding

Two correctness-critical paths in crates/koinon/src/tamper_log.rs have no test coverage:

  1. The EntryTooLarge allocation guard in decode_entry. The function reads a 4-byte little-endian length prefix and rejects payloads exceeding MAX_ENTRY_BYTES before allocating the read buffer. No test feeds an oversized length prefix and asserts the guard fires.
  2. The CBOR roundtrip of LogEntryKind::VaultMutation. Every other LogEntryKind variant has a roundtrip test; VaultMutation — the variant recording credential vault lifecycle events — has none.

Evidence

crates/koinon/src/tamper_log.rs:228 — the allocation guard:

if payload_len > MAX_ENTRY_BYTES {
    return Err(TamperLogError::EntryTooLarge {
        size: payload_len,
        max: MAX_ENTRY_BYTES,
    });
}

The buffer is allocated from the attacker-controlled length immediately after the guard:

let mut cbor_bytes = vec![0u8; usize::try_from(payload_len).unwrap_or_default()];

crates/koinon/src/tamper_log.rs:158 — the untested variant:

VaultMutation {
    /// Human-readable credential name affected by the mutation.
    credential_name: CompactString,
    /// Mutation operation, e.g. `"add"`, `"rotate"`, `"revoke"`, or `"remove"`.
    operation: CompactString,
},

crates/koinon/src/tamper_log_tests.rs — roundtrip tests exist for five sibling variants but not VaultMutation, and no test references EntryTooLarge:

338: fn cbor_roundtrip_signal_observed() {
351: fn cbor_roundtrip_entity_created() {
363: fn cbor_roundtrip_config_changed() {
375: fn cbor_roundtrip_alert_raised() {
387: fn cbor_roundtrip_action_taken() {

A grep of the test file for EntryTooLarge, entry_too_large, VaultMutation, and vault_mutation returns no matches.

Why this matters

Under a counter-surveillance threat model the tamper log is the integrity record an adversary most wants to corrupt or weaponize:

  • The EntryTooLarge guard is the only thing standing between a crafted or corrupted length field and a pre-validation allocation. A wire frame whose 4-byte prefix is 0xFFFFFFFF would, absent the guard, drive vec![0u8; ...] to attempt a ~4 GiB allocation before the subsequent read_exact fails — a denial-of-service primitive triggerable by feeding decode_entry a single malformed frame. With no regression test, a future refactor of decode_entry could silently drop the guard and reopen this vector undetected.
  • VaultMutation records the highest-sensitivity events in the log: credential add/rotate/revoke/remove. A CBOR schema drift that breaks this variant's encode/decode roundtrip would silently corrupt the audit trail for exactly the operations whose integrity matters most, and the corruption would pass every existing test because that variant is never exercised.

Desired correction

  1. Add a test that builds a wire frame with a length prefix of 0xFFFFFFFF (4 bytes 0xFF) followed by sufficient bytes, calls decode_entry, and asserts Err(TamperLogError::EntryTooLarge { size, max }) with the expected max == MAX_ENTRY_BYTES.
  2. Add a cbor_roundtrip_vault_mutation test mirroring the existing sibling roundtrip tests: construct a LogEntry with LogEntryKind::VaultMutation, encode_entry then decode_entry, and assert equality of the decoded entry.

Done when: both tests exist and pass, removing the EntryTooLarge guard at line 228 makes the first test fail, and altering the VaultMutation CBOR shape makes the second test fail.

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions