Some checks failed
CI / lint (push) Failing after 49s
CI / test-python (push) Has been cancelled
CI / test (push) Has been cancelled
CI / miri (push) Has been cancelled
CI / test-zig (push) Has been cancelled
CI / test-wasm (push) Has been cancelled
CI / build (push) Has been cancelled
CI / publish (push) Has been cancelled
CI / publish-wasm (push) Has been cancelled
CI / fuzz-regression (push) Has been cancelled
CI / publish-python (push) Has been cancelled
109 lines
6.8 KiB
Markdown
109 lines
6.8 KiB
Markdown
# 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.
|