docs: security audit of 2b42864 (report only)

Evidence-backed audit at docs/SECURITY_AUDIT.md. No src/ changes.

docs/SECURITY_AUDIT.md

@@ -0,0 +1,194 @@
+# OnionWire Security Audit
+Commit: 2b42864ebaaef2aab64e66055ee482c525180f63 (`2b42864`)
+Baseline: `cargo test --locked` **pass** (100 passed, 3 ignored), `cargo clippy --locked --all-targets -- -D warnings` **pass**
+Auditor: rust-dev (no access to running hidden services / live Monero wallet)
+Tree: worktree `wt/t_d85060fb` at `/home/lancelot/Projects/onionwire/.worktrees/t_d85060fb`
+Remote: `origin/main` = same SHA (`https://forgejo.siriusdevops.com/sirius/onionwire.git`)
+
+Ignored tests (`needs live Tor network`): `rotate_hs`, `tor_hs`, `two_node`. Not re-run.
+
+## Severity key
+Critical = remote key compromise or plaintext disclosure
+High     = local key/plaintext disclosure, authn bypass, or payment forgery
+Medium   = DoS, nonce/IV weakness, metadata leak
+Low      = hygiene, error-path leakage, docs mismatch
+Info     = observation
+
+## Findings
+
+### F1 — Incoming receipt `verified=1` on unrelated wallet history  [High]
+Location: `src/wallet.rs:142-148`, used at `src/node.rs:667-683`
+
+Evidence: confirmation is not “this txid paid this amount to this address”.
+
+```142:148:src/wallet.rs
+pub fn transfers_match(rows: &[TransferRow], txid: &str, amount: &str, address: &str) -> bool {
+    rows.iter().any(|r| {
+        r.txid == txid
+            || (!address.is_empty()
+                && r.address == address
+                && (amount.is_empty() || r.amount == amount))
+    })
+}
+```
+
+`ingest_receipt` verifies the **ed25519** on the `rcp` frame (so the *friend* signed it), inserts `verified: false`, then flips `verified` if `transfers_match` is true (`src/node.rs:671-682`). Replica of that predicate against two wallet rows `{txid:aaa111, amount:1000, addr:4AAA…}` and `{txid:bbb222, amount:5, addr:8BBB…}`:
+
+```
+txid-only match (wrong amount+addr): True
+addr+amount match (wrong txid): True
+honest miss: False
+```
+
+Impact: a friend who completed Noise IK can send a signed receipt for an arbitrary amount/address and get the TUI line `[receipt] N XMR` (verified) if *either* (a) `txid` appears anywhere in `get_transfers` `in`/`pending`, or (b) some inbound row already has that address and amount. That is payment forgery against the local “verified” bit. It is not a third-party wire injection: the frame still has to decrypt under the pinned session. `docs/THREAT_MODEL.md:33` says “Never trust a `rcp` frame without RPC confirmation (`verified` stays 0)” — the code *does* promote `verified`, and the RPC check does not bind amount+address+txid together. Tests encode the store default (`tests/pay.rs` `incoming_receipt_is_not_verified`) but never exercise `transfers_match` against mismatched amount.
+
+Fix: require `txid == row.txid && amount == row.amount && address == row.address` (and reject empty fields). Do not OR. Keep `verified=0` if RPC is down.
+
+### F2 — Chat AEAD has empty AAD; ciphertext rows are interchangeable  [Medium]
+Location: `src/backup.rs:98-108` (`aead_encrypt`), `src/store.rs:649` / `670`
+
+Evidence: bodies are `nonce || ChaCha20-Poly1305(key, nonce, pt)` with no associated data. The same 32-byte `msg_key` wraps every row. A DB writer who cannot open the passphrase can still swap `messages.plaintext` blobs. Throwaway against this tree (`/tmp/ow-audit-repro`, `CARGO_TARGET_DIR` = this worktree `target`):
+
+```
+SWAP: alice sees "secret-for-bob"
+SWAP: bob sees "secret-for-alice"
+```
+
+Both `list_messages` calls returned `Ok`; Poly1305 verified. `dir` / `friend_id` / `id` / `created_at` are plaintext columns and are not in the MAC.
+
+Impact: anyone with write access to `onionwire.db` (same uid, stolen unlocked file, or a bug that writes sqlite) can reattribute ciphertext across friends and in/out without the passphrase. This is *not* remote plaintext disclosure. Identity secret keys are already plaintext in `self` (threat model says so); this is extra: the body encryption does not bind a row to its owner. Nonces are 96-bit random per `generate_nonce` — reuse across restarts/`wipe-all` (new key) / first-unlock rewrap is not the failure mode here.
+
+Fix: encrypt as `Aead::encrypt` with AAD = `friend_id || dir || row_id` (or a committed header), or include those fields in the plaintext that is MACed. Reject decrypt if AAD does not match the row.
+
+### F3 — Invite `sig` is over concatenated strings, not length-prefixed fields  [Medium]
+Location: `src/qr.rs:67-72` (`sign_msg`), `src/qr.rs:36-64` (`decode`)
+
+Evidence: `sig` covers `k.as_bytes() || onion.as_bytes() || spk.as_bytes()` with no delimiters or lengths. `k` is 64 hex chars after the 32-byte check, so it cannot shift. `o` and `spk` can. Encode a real v3 onion + 32-byte `spk`, then move the first 8 hex chars of `spk` onto `o`, keep `k` and `sig`. `qr::decode` **accepts** the mutant:
+
+```
+CONCAT: mutated invite accepted
+CONCAT: onion_changed=true
+CONCAT: onion=abcdefghijklmnopqrstuvwxyz234567abcdefghijklmnopqrstuvwxyz234567.onionabababab
+CONCAT: spk_len=28 (want 32)
+CONCAT: pubkey_unchanged=true
+```
+
+`decode` does not require `signed_prekey.len()==32` or a v3 onion. `Node::add_friend_payload` then `upsert_friend` (same `k` **replaces** locator) and `set_friend_prekey`. Duplicate/unknown fields are rejected (`src/qr.rs:85-106`); all four fields are required. Empty `sig` fails `from_hex`. This is not a classic steal-the-identity concat: `k` stays the signer.
+
+Impact: a mutated invite still verifies under the real identity key. F3-paste (or a same-`k` rescan) can poison `friends.onion` / `prekey` for that pubkey. The shifted onion is not an arbitrary attacker HS (you can only append a hex prefix of the original `spk`), so this is roster integrity / availability, not a silent MITM. Handshake then fails (`prekey` length ≠ 32 at `node.rs:388`). No panic on this path. `from_hex` has **no size cap**: a 4,000,000-char hex string decoded to 2,000,000 bytes in 0.34s in CPython; `decode` allocates that before `pubkey.len()!=32` rejects.
+
+Fix: sign a domain-separated encoding (`k` || `0x00` || `o` || `0x00` || `spk`, or length prefixes). Reject `spk` ≠ 32 bytes and onion ≠ v3. Cap invite length before `from_hex` (a few KiB).
+
+### F4 — Monero address check is prefix+length, not checksum  [Medium]
+Location: `src/pay.rs:38-48`; tests *require* the junk form to pass (`tests/pay.rs:15-50`)
+
+Evidence:
+
+```38:48:src/pay.rs
+pub fn check_address(addr: &str) -> Result<()> {
+    let ok = match addr.as_bytes().first() {
+        Some(b'4') if addr.len() == 95 || addr.len() == 106 => true,
+        Some(b'8') if addr.len() == 95 => true,
+        _ => false,
+    };
+    if ok && !addr.contains('\n') {
+        Ok(())
+    } else {
+        Err(Error("invalid Monero address".into()))
+    }
+}
+```
+
+`4` + `'A' * 94` is accepted (len 95). No network byte, no Keccak checksum, no alphabet check. `profile::check_fields` (`src/profile.rs:118-128`) does not call `check_address` at all — `xmr_addr` may be `"4abc"` (`tests/profile.rs`). `/tip` does call `check_address` on the stored profile address (`src/node.rs:331`). `/pay` invoices are signed over that address (`src/node.rs:290`).
+
+Impact: OnionWire will persist and sign invoices/receipts for strings no Monero wallet should pay. A live `monero-wallet-rpc` will usually reject checksum failures on `transfer`, so this is not by itself silent theft. It *is* a local fail-open: garbage becomes a signed `inv`/`rcp` payload and a payments row. Combined with F1, a verified receipt can cite such an address.
+
+Fix: decode base58, check network prefix + checksum. Empty profile `xmr_addr` stays allowed; non-empty must pass the same check.
+
+### F5 — Wallet RPC is unauthenticated HTTP and can mark receipts from its full transfer list  [Medium]
+Location: `src/wallet.rs:151-171`, `223-237`, `117-138`; README documents `ONIONWIRE_WALLET_RPC=http://127.0.0.1:18083`
+
+Evidence: URL parser requires `http://` (no TLS), host must be loopback or `.onion` (`allowed_host`, tested in `tests/wallet.rs` `refuse_non_loopback_non_onion_host`). There is no `user:pass` / Digest / header. `http_post` is raw `TcpStream` + `read_to_end` with a 5s timeout, no body size cap. `.onion` hosts are allowed but `TcpStream::connect((onion, port))` does **not** go through Arti — so an onion RPC URL fail-closes at connect (not examined live). `eprintln!("ONIONWIRE_WALLET_RPC: {e}")` prints the parse error, not the URL, on bad env.
+
+Impact: the documented operator setup is “HTTP to loopback, no login”. Any local process that can reach that port can `transfer` (spend) and `get_transfers` (the same list F1 trusts). OnionWire never holds spend keys (threat model — true); it also never authenticates to the process that does. This is local, not remote, if the operator actually bound loopback. Code cannot express `--rpc-login`.
+
+Fix: require digest (or a unix socket). Refuse URLs without credentials. Cap RPC read size. If onion RPC is a goal, dial it through the Arti client, not `TcpStream`.
+
+### F6 — `/wipe` does not touch payments; sqlite is not `secure_delete`  [Low]
+Location: `src/store.rs:677-685`, `122-125`
+
+Evidence: `wipe_messages` overwrites `messages.plaintext` with `zeroblob(length)`, `DELETE FROM messages`, `VACUUM`. No `PRAGMA secure_delete`. No `DELETE FROM payments`. WAL is required (`journal_mode = WAL` fail-closed). Threat model (`docs/THREAT_MODEL.md:21`) says “`/wipe` overwrites message bodies and vacuums” — that part matches. It does not say payments go away; they do not. Identity / friend pubkeys / onions stay plaintext (disclosed, not a finding). `onionwire.db` itself is never `chmod 0600`; the home and `arti/` dirs are `0700` after `create_dir_all` (`src/store.rs:753-758`, `tests/store.rs:61-62`). Backup files *are* `0o600` at create (`src/node.rs:145-150`).
+
+Impact: `/wipe` is not a forensic erase (SSD wear-leveling, WAL snapshots, payments table, roster). A seized disk after `/wipe` still has who you pay and who you talk to. `/wipe-all` is `remove_dir_all` — same disk caveat.
+
+Fix: if `/wipe` should mean “chat history gone”, also drop `payments` (and checkpoint WAL). Document that `/wipe` is not crypto-shred. Optional `secure_delete` is still not a guarantee on flash.
+
+### F7 — HS publish logs the onion; `dangerously_trust_everyone` is Arti-only  [Low]
+Location: `src/hs.rs:31-36`, `src/hs.rs:95`, `src/node.rs:95`
+
+Evidence: `wait_until_published(..., &onion, &onion)` uses the unredacted onion as `label`. `eprintln!("{label} hs status: {state:?}")` and probe lines go to stderr. `onion_string` uses `display_unredacted` (`src/hs.rs:70`) — required to persist the locator; the leak is the log. `client_config` calls `builder.storage().permissions().dangerously_trust_everyone()` after `create_dir_all` on the Arti state/cache paths. That API is fs-mistrust for **Arti’s** directories, not sqlite. `Store::open_at_with_passphrase` `mkdir_700`s `home` and `home/arti` first; `home/cache` is created by Arti’s `create_dir_all` without `0700`. `cbtmintimeout` / `cbtinitialtimeout` = 20s is a circuit-build floor (perf / publish reliability), not an auth bypass.
+
+Impact: journald/script logs contain the current v3 locator. Arti state/cache may be created `0755` until something else tightens them; sqlite lives under the `0700` home. A world-readable Arti cache is descriptor/consensus metadata, not chat bodies.
+
+Fix: log a redacted onion (safelog). `mkdir_700` the cache dir before `client_config`. Keep `dangerously_trust_everyone` scoped to Arti storage; do not reuse it for `onionwire.db`.
+
+### F8 — Threat model overstates receipt verification and omits F1–F3  [Low]
+Location: `docs/THREAT_MODEL.md:17-21`, `:33`, `:39-41`
+
+Evidence: TM correctly describes live-only send, loc rules, identity-vs-locator, passphrase-wrapped message key, plaintext identity/roster, experimental Arti, global 30/60s burst-10 token bucket, backup = identity. It claims RPC confirmation keeps `verified` at 0 unless the chain view agrees — F1 shows the matcher is not that. It does not mention empty AEAD AAD, invite concat, shape-only XMR addresses, or env passphrase (`ONIONWIRE_STORE_PASSPHRASE` in `src/store.rs:745-750`, visible in `/proc/<pid>/environ`).
+
+Impact: an operator who treats TM as the capability list will believe “verified receipt ⇒ wallet saw that payment”.
+
+Fix: either implement F1’s conjunctive match or change the sentence to “incoming `rcp` is displayed; `verified` is best-effort and must not be trusted in v0.2”.
+
+## Verified correct
+
+- Noise pattern is actually `Noise_IK_25519_ChaChaPoly_BLAKE2s` with prologue `onionwire-v1` (`src/session.rs:8-9, 111-122`). Initiator sets `remote_public_key` to the QR/roster x25519 prekey (`src/session.rs:189`). Responder takes remote static from snow (`get_remote_static`) and looks up the friend (`src/session.rs:255-260`, `src/node.rs:550-555`).
+- Mutual identity proofs are `ed25519_sign(handshake_hash)` with the 32-byte pubkey prefix; `verify_proof` calls `VerifyingKey::verify` (`src/session.rs:289-310`). Initiator compares proof prefix to the **pinned** identity (`src/session.rs:219-222`); responder to the roster id (`src/session.rs:273-276`). Mismatch is `Error::mismatch` → send hard-fails (`src/node.rs:490`).
+- Transport nonces are snow `TransportState` counters (Noise spec: increment, reject reuse). Application code does not set ChaCha nonces on the wire. Session keys include ephemeral DH → compromise of long-term static does not decrypt **past** transport; it does allow impersonation **forward**. That is IK, not a bug.
+- Loc frames: `apply_loc` verifies ed25519 against the **session** `peer_identity`, requires the pubkey already in `friends`, and requires `ts > onion_updated_at` (`src/store.rs:452-480`, `src/loc.rs:38-55`, `src/node.rs:561-568`). A peer cannot silently move you to an onion they control unless they hold that identity key (Noise proof + loc sig). An old loc with smaller `ts` cannot rewind after a later rotate. (F3 invite paste can still overwrite locator; that is out-of-band.)
+- Unknown typed prefixes (`xyz `) are `Kind::Drop`, not fatal (`src/dispatch.rs:30-38`). Unparseable loc/inv/rcp are ignored (`src/node.rs:562`, `610-611`, `643-644`). Handshake/frame errors print `incoming: {e}` and the rend task ends; the accept loop continues (`src/node.rs:723-729`).
+- Frames are length-prefixed, `MAX_FRAME = 65535`, checked **before** allocating the body (`src/frame.rs:5, 63-67`).
+- Token bucket is **one global** `TokenBucket::default()` = 30 tokens / 60s, burst 10 (`src/ratelimit.rs:41-44`, `src/node.rs:86, 712-720`). Matches TM; one flood can starve every friend (availability, not auth).
+- Backup: Argon2id v0x13, `m=19456` KiB, `t=2`, `p=1` (`src/backup.rs:52-53`) = OWASP 2023 minimum. Per-export 16-byte salt + 12-byte random nonce in the file. `open` AEAD-fails with a single error before `replace_identity_keys` (`src/backup.rs:91-95`, `src/node.rs:155-168`). Onion is not in the blob (`tests/backup.rs`). Wrong passphrase does not write keys.
+- First-run message key: 32 random bytes, wrapped with the same KDF/AEAD, stored in `store_meta` (`src/store.rs:281-293`). Empty passphrase refused. Integrity check fail-closed (`src/store.rs:191-194`).
+- Amounts on the pay path are decimal **integer piconero** (`src/pay.rs:51-59`); `xmr_to_atomic` pads a ≤12-digit fraction without float (`src/pay.rs:62-85`). `/tip` then `parse`s to `u64` for RPC (`src/node.rs:332-334`).
+- Incoming `inv`/`rcp` are verified against **session peer identity**, not a field inside the frame (`src/node.rs:613`, `646`).
+- Locked product decisions (no server/XMPP/MAM/DHT, live-only send, identity=pubkey, onion=locator, no dual-host grace) match the code. Not findings.
+
+## Not examined / out of scope
+
+- Live hidden-service reachability, IPT/HsDir, and two-node Tor tests (ignored; no HS from this auditor).
+- Live `monero-wallet-rpc` (auth defaults, `get_transfers` JSON shape vs `json_amount`, unlock/spend confirm).
+- snow 0.10 internals beyond the `Builder`/`TransportState` API used here (constant-time, rekey at 2^64).
+- Arti keystore encryption at rest, fs-mistrust semantics of `dangerously_trust_everyone` beyond “it is called on Arti storage”.
+- Timing of Argon2 / ed25519 verify (failed backup passphrase is one error string; KDF still runs).
+- TUI rendering of hostile chat (ratatui text; no HTML).
+- Traffic analysis / HS existence (TM already declines that).
+- `cargo audit`: **not installed** (`which cargo-audit` empty). Lockfile inspected by hand; no RustSec lookup was executed against this `Cargo.lock`.
+
+## Dependencies (Cargo.lock)
+
+| Crate | Lock version | Cargo.toml |
+|---|---|---|
+| snow | 0.10.0 | `0.10` |
+| chacha20poly1305 | 0.10.1 | `0.10` |
+| argon2 | 0.5.3 | `0.5` |
+| ed25519-dalek | 2.2.0 | `2` |
+| x25519-dalek | 2.0.1 | `2` |
+| rusqlite | 0.36.0 | `0.36` (bundled) |
+| arti-client | 0.46.0 | `0.46` + `onion-service-client` + `onion-service-service` |
+| tor-hsservice | 0.46.0 | `0.46` |
+
+Caret reqs are not `=`; `cargo update` can move 0.10.x / 0.46.x without a Cargo.toml edit. `--locked` CI is the real pin. Arti onion services are still experimental upstream (TM + onionwire skill); this tree fail-closes, no C-tor fallback (`src/hs.rs:55`).
+
+## Open questions for Lance
+
+- Is a “verified” receipt allowed to mean anything in v0.2, or should the UI only ever show “unverified” until F1 is conjunctive and covered by a test?
+- Invite encoding: length-prefix / `0x00` separators now, or wait for `onionwire:v2` (v1 strings stay in the wild)?
+- Store passphrase: keep `ONIONWIRE_STORE_PASSPHRASE` (proc-visible) or prompt / kernel keyring?
+- Wallet RPC: document “loopback + `--rpc-login` you type into a wrapper”, or teach OnionWire digest?
+
+## Stop / go (auditor, not a ship decision)
+
+Go for **friends-only chat** under the written seizure model (identity keys plaintext; bodies encrypted; Tor relays are not a server). **Do not** treat `verified` receipts as money moved. **Do not** treat F3-paste of a string you did not copy yourself as an integrity-checked locator. No Critical remote key/plaintext bug found in this revision.