libsoliton/REVIEW-2026-09-08.md
Kamal Tufekcic 35c5179e12
Some checks failed
CI / lint (push) Failing after 39s
CI / test-python (push) Has been cancelled
CI / test (push) Has been cancelled
CI / test-zig (push) Has been cancelled
CI / miri (push) Has been cancelled
CI / fuzz-regression (push) Has been cancelled
CI / build (push) Has been cancelled
CI / publish-python (push) Has been cancelled
CI / publish-wasm (push) Has been cancelled
CI / test-wasm (push) Has been cancelled
CI / publish (push) Has been cancelled
version bumps, MAX_RECV_SEEN patch, FIPS interop, doc updates
2026-09-09 00:46:40 +03:00

109 lines
6.8 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# libsoliton core review — 2026-09-08 (single-reviewer trial)
## Overall assessment
libsoliton is shippable as reviewed. Across the full primitive stack, the key
exchange, the double ratchet, streaming/storage AEAD, call keys, auth, and
verification, I found no defect at medium severity or above. The areas the brief
flagged as highest-risk are handled correctly and defensively: every AEAD nonce
is provably unique within its key's lifetime (ratchet counter-mode nonces are a
strict function of a monotonic `send_count` that aborts at `u32::MAX` before it
can wrap; the first-message key reuses the epoch key only at counter 0 with a
random nonce and a distinct AAD; streaming nonces are injective over
`(index, tag_byte)`; storage uses fresh random nonces). Secret-dependent
comparisons all use `subtle`/constant-time paths (MAC/token in `auth_verify`,
all-zero key/secret guards, fingerprint checks against untrusted wire data,
ratchet-pk epoch identification, shared-secret zero-check in X25519 DH). Key
material is wrapped in `Zeroizing`/`ZeroizeOnDrop` and every keygen/DH/KEM path
explicitly zeroizes the `Copy`-type stack staging buffers before returning. The
double-ratchet state machine is the strongest single piece: `encrypt` is
atomic-by-ordering (all fallible ops precede any mutation), `decrypt` takes a
full snapshot and rolls back on any error, duplicate detection is deferred past
AEAD to avoid a timing oracle, the seen-sets and epoch keys rotate correctly
across direction changes, and `from_bytes` has an extensive battery of
structural/co-presence/all-zero guards.
The weakest areas are documentation-vs-code drift and a heavy reliance on
documented caller obligations, neither of which is a code defect. Two footguns
that could produce catastrophic nonce reuse — double-`from_bytes` and reusing a
streaming `(index, is_last)` pair — are real, but both are deprecated/
`&self`-random-access APIs with explicit in-code warnings and safe alternatives
(`from_bytes_with_min_epoch`, sequential `encrypt_chunk`), so they fall under the
brief's "documented decision" exclusion. The one concrete mismatch worth fixing
is a one-off in the `recv_seen` cap (code is stricter than the docs claim; safe
direction). I would ship it; the notes below are hygiene, not blockers.
## Findings (medium and above)
None.
## Notes below medium
- **low — `recv_seen`/`prev_recv_seen` cap is off-by-one vs the documentation.**
`soliton/src/ratchet/mod.rs:582` (and `:573`) reject with `ChainExhausted` when
`self.recv_seen.len() + 1 >= MAX_RECV_SEEN` (65536), i.e. the set can hold at
most 65535 entries and the 65536th *distinct*-counter decrypt is rejected. The
CHEATSHEET ("the 65536th distinct-counter decrypt succeeds … the 65537th
returns `ChainExhausted`") and the `MAX_RECV_SEEN` comment in
`soliton/src/constants.rs:83` both state the limit one message higher. The code
is the safe/conservative side (it keeps the live set always serializable,
matching the `>= MAX_RECV_SEEN` reject in `to_bytes` at `:1060`/`:1074` and
`from_bytes` at `:1292`/`:1316`), so this is a doc/reimplementor-guidance error,
not a security defect. No test would catch it without a 65k-message epoch.
- **info — contradictory comments about HKDF PRK zeroization.**
`soliton/src/primitives/hkdf.rs:19-22` states "The `hkdf` crate's internal PRK
is **not** zeroized on drop (`hkdf 0.13.0-rc.5` has no `zeroize` feature)",
while `soliton/soliton/Cargo.toml:26-29` explains that `sha3` 0.11-rc + the
`hmac` `zeroize` feature were chosen specifically so the keyed SHA3 state used
by HMAC/HKDF is `ZeroizeOnDrop`. Since `Hkdf<Sha3_256>` stores its PRK as an
`Hmac<Sha3_256>` field, that field's drop should in fact zeroize the PRK. The
two comments can't both be right; reconcile them so maintainers don't reason
from the pessimistic one. Either way the PRK lifetime is a short-lived local,
so the practical exposure is minimal.
- **info — streaming compressed-empty non-final chunk skips the size check.**
`soliton/src/streaming.rs:527` gates decompression on
`self.compressed && !decrypted.is_empty()`; a compressed *non-final* chunk that
AEAD-decrypts to zero bytes would return `(empty, is_last=false)` without the
`decompressed.len() != STREAM_CHUNK_SIZE` guard at `:533`. Unreachable in
practice: producing a valid AEAD chunk requires the stream key, and the
legitimate encryptor never emits an empty non-final chunk (non-final plaintext
must be exactly `STREAM_CHUNK_SIZE`, which compresses to non-empty). Latent
only.
- **info — `Copy`-typed key material leaves brief stack residue by design.**
`random::random_array`, `hmac_sha3_256`, and the various `[u8; 32]` deref-copies
documented throughout return `Copy` values the callee can't zeroize. Every
in-library use zeroizes the staging copy (e.g. `x25519.rs:62`, `mlkem.rs:162`,
`call.rs:312`, `ratchet.rs:174`), and the hazard is called out in the
CHEATSHEET for callers. Noted for completeness, not a defect.
- **info — nonce-reuse footguns are deprecated/documented, not defects.**
Double-`from_bytes` (`ratchet/mod.rs:1114`, `#[deprecated]`, with an explicit
double-deserialization warning and the `from_bytes_with_min_epoch` safe path)
and reusing a streaming `(index, is_last)` pair via `encrypt_chunk_at`
(`streaming.rs:281`, explicit SECURITY warning) both cause catastrophic nonce
reuse if misused, but each ships with a safe alternative and in-code warnings.
## Coverage
Read in full (to the test boundary given in the brief), in dependency order:
`README.md`; `CHEATSHEET.md` (all 2354 lines); `soliton/Cargo.toml`;
`src/lib.rs`; `src/error.rs`; `src/constants.rs` (1–163); `src/primitives/mod.rs`;
`src/primitives/random.rs` (1–34); `src/primitives/sha3_256.rs` (1–28);
`src/primitives/hmac.rs` (1–40); `src/primitives/hkdf.rs` (1–44);
`src/primitives/aead.rs` (1–109); `src/primitives/ed25519.rs` (1–58);
`src/primitives/x25519.rs` (1–105); `src/primitives/mlkem.rs` (1–314);
`src/primitives/mldsa.rs` (1–436); `src/primitives/xwing.rs` (1–395);
`src/primitives/argon2.rs` (1–145); `src/identity/mod.rs` (1–349);
`src/auth/mod.rs` (1–102); `src/kex/mod.rs` (1–924); `src/verification.rs`
(1–125); `src/ratchet/mod.rs` (1–1606); `src/streaming.rs` (1–549);
`src/storage/mod.rs` (1–593); `src/call.rs` (1–318). No file was skimmed; every
allowed source file was read line-by-line. Test modules, `tests/`, `benches/`,
`fuzz/`, `build.rs`, `Specification.md`, `Abstract.md`, and all non-core crates
were not opened, per the brief. No build or scratch experiment was needed — the
notes were settled by reading. Constants in `constants.rs` were cross-checked
against the standard sizes (ML-KEM-768 1184/2400/1088, ML-DSA-65 1952/3309/32
seed, X25519/Ed25519 32/64, X-Wing 1216/2432/1120, composite 3200/2496, hybrid
sig 3373) and against every use site; all agree.