@@ -0,0 +1,385 @@
1+# Skrepka Security Audit — Fresh Pass (2026-07-15)
2+
3+**Scope:** Exhaustive multi-pass audit of PROTOCOL.md, server.knot, core/src/ (app.rs,
4+crypto.rs, protocol.rs, model.rs, phonemic.rs, ffi.rs), apple/SkrepkaApp/ (Effects.swift,
5+Core.swift, Views.swift, Forms.swift), install.sh. Findings below are **not** in
6+PROTOCOL-REVIEW.md or AUDIT-NEW.md, or are new angles on previously-noted areas.
7+
8+**Legend:** 🔴 critical · 🟠 high · 🟡 medium · 🔵 low · ⚪ info
9+
10+---
11+
12+## 1. Cryptographic Protocol
13+
14+### 🔴 N1. No AAD on AEAD — envelope `to` field is unauthenticated, enabling silent targeted censorship by any relay
15+
16+**Type:** Design gap (protocol + crypto.rs)
17+**Files:** `core/src/crypto.rs:384-387` (encrypt), `core/src/crypto.rs:434-438` (decrypt)
18+
19+AUDIT-NEW.md Q2 notes this but downgrades the impact to "equivalent to message loss, which is
20+an accepted limitation." That understates it. The real issue is **silent, targeted message loss
21+with zero detectability**:
22+
23+A relay holding a victim's messages can swap the `to` field on a *specific* message (from a
24+sender the relay wants to censor), while leaving other messages intact. The recipient never
25+sees the message and gets no error. The `delivery.ack` system doesn't help because the recipient
26+never received the message to ack it, and the sender's UI shows the message as "sent"
27+indefinitely — which is the same behavior as "the recipient is offline" or "the recipient read
28+it but hasn't acked yet." There is **no way for either party to distinguish "the relay ate
29+Alice's message" from "Alice didn't send anything" or "Bob is just slow to ack."**
30+
31+This is worse than the generic "relay can drop messages" case. Dropping is *eventually*
32+detectable — the sender sees their message stuck at "sent" and can investigate. But `to`-swapping
33+moves the message to a *different recipient's* mailbox, where it's undecryptable and gets
34+silently dropped on poll. The original recipient's cursor is unaffected (no events), so the
35+next poll is normal. The censorship is invisible.
36+
37+**Attack scenario:**
38+1. Alice sends a message to Bob via relay R.
39+2. R swaps `to` from Bob's pubkey to Charlie's.
40+3. Message stored under Charlie's mailbox. Charlie can't decrypt (wrong key), drops silently.
41+4. Bob polls, gets nothing. Normal poll, no error.
42+5. Alice's UI shows "sent" — same as if Bob were merely offline.
43+6. No delivery.ack ever comes. Alice can't tell if Bob received it or not.
44+
45+The relay can do this selectively — censor messages from specific senders, or suppress specific
46+content patterns (inferred from blob size), while delivering everything else normally. The
47+censorship is undetectable because the sender sees the same "sent" status they'd see for any
48+offline recipient, and the recipient sees nothing unusual.
49+
50+**Suggested fix:** Include `version_byte || ephemeral_public || nonce || recipient_ed25519_public`
51+as AAD in the AEAD. The `to` field must match `recipient_ed25519_public` in the AAD. A relay
52+swapping `to` causes the wrong recipient to fail AEAD decryption (before signature check), and
53+the blob is dropped before cursor advancement. The *original* recipient's message stays in the
54+mailbox. Wire-format change (v0x02).
55+
56+### ⚪ N2. HKDF info string and wire version byte are independent constants
57+
58+**Type:** Code hygiene (crypto.rs)
59+**Files:** `core/src/crypto.rs:28` (`HKDF_INFO = b"skrepka-v1"`), `crypto.rs:33` (`WIRE_VERSION = 1`)
60+
61+The spec says "A future revision that changes the crypto primitives should bump both the version
62+byte and the HKDF `info` string together." But there's no code-level binding between them. A
63+developer bumping `WIRE_VERSION` to `2` could forget to update `HKDF_INFO`, causing two different
64+crypto versions to derive keys with the same KDF label.
65+
66+**Suggested fix:** Derive the info string from the version byte or add a compile-time assertion
67+linking them.
68+
69+---
70+
71+## 2. Server Implementation
72+
73+### 🟡 N3. `appendMessage` dedup scan is O(n) — identical-blob replay amplifies to O(n × batch_size)
74+
75+**Type:** Implementation concern / DoS amplification (server.knot)
76+**Files:** `server.knot:919-937`
77+
78+AUDIT-NEW.md Q7 notes the O(n) dedup scan. The amplification is worse than described for
79+identical-blob replay: a batch of 100 identical blobs to the same recipient does 100 × O(|messages|)
80+scans. With |messages| = 10,000, that's 1M comparisons per request. At 60 requests/min, that's
81+60M comparisons/min. Each comparison is a string equality on 64-char hex pubkeys + 346+ char
82+hex blobs.
83+
84+**Attack:** An attacker authenticates (free keypair) and sends batches of 100 identical
85+min-size blobs to one recipient. Each request is ~35 KB but triggers 1M comparisons. Bounded
86+by the 60/min rate limit, but the CPU amplification is ~1000x per request.
87+
88+**Suggested fix:** Index `*messages` by `(toKey, encryptedBlob)` or use a hash-based dedup set.
89+
90+### 🟡 N4. `capForwards` eviction is non-deterministic when retry counts are equal
91+
92+**Type:** Implementation bug (server.knot)
93+**Files:** `server.knot:515-522` (`capForwards`)
94+
95+AUDIT-NEW.md Q9 notes this. The sort by `retries` only (no secondary sort key) means when many
96+entries share `retries: 0`, the sort is unstable and any retries-0 entry can be dropped —
97+including a fresh forward for a healthy peer that would have succeeded on the first try.
98+
99+**Impact:** Delivery delay, not permanent loss (the message stays in `*messages` and can be
100+re-forwarded on the next presence announce). But it's a real availability issue under
101+federation abuse.
102+
103+**Suggested fix:** Sort by `(retries, encryptedBlob)` for determinism, or skip entries for
104+peers in backoff before capping.
105+
106+### 🟡 N5. `handleRecvGossip` `fromServer` not verified against connecting peer's address
107+
108+**Type:** Design gap (server.knot)
109+**Files:** `server.knot:1533-1578`
110+
111+AUDIT-NEW.md Q29 notes this. The `fromServer` field is self-asserted and used to route forwarded
112+messages. An attacker can claim any `fromServer` hostname. The server writes presence rows with
113+that hostname and forwards queued messages there. This is the root cause of the gossip-redirect
114+attack and is already documented, but the spec's §10 should state more prominently that **the
115+server has no mechanism to verify that the connecting peer is who it claims to be**.
116+
117+The `fromServer` field is a `ServerName` (format-validated), but format validation only checks
118+length, lowercase, and no trailing dot — it does not verify the peer controls that hostname.
119+
120+**Suggested fix:** Document more prominently. Real fix needs reverse-DNS verification or peer
121+authentication (signed gossip).
122+
123+### 🔵 N6. `genToken` generates one hex digit per `randomInt 16` call — entropy waste
124+
125+**Type:** Implementation concern (server.knot)
126+**Files:** `server.knot:532-537`
127+
128+AUDIT-NEW.md Q10 notes the CSPRNG dependency. Additionally, generating one hex digit at a time
129+is inefficient — each `randomInt 16` call should produce at least 4 bits, but if the runtime's
130+`randomInt` draws from a wider source (e.g., 32-bit `getrandom`), it wastes 28 bits per call.
131+48 calls × 28 wasted bits = 1344 bits wasted per token. Not a security issue (entropy is
132+abundant) but inefficient.
133+
134+**Verdict:** Info.
135+
136+---
137+
138+## 3. Client Core Logic
139+
140+### 🟡 N7. `seen_ids` dedup set is unbounded — grows without limit per peer
141+
142+**Type:** Implementation concern (app.rs)
143+**Files:** `core/src/app.rs:325` (`seen_ids: BTreeMap<String, HashSet<String>>`)
144+
145+The `seen_ids` map (Q18 fix) is persisted to kv and loaded at startup. It's a `BTreeMap<String,
146+HashSet<String>>` — one entry per peer, each a set of message IDs. The set is never trimmed.
147+Over time, a peer who sends thousands of messages accumulates thousands of IDs. The kv blob
148+(`seen_ids`) grows without bound.
149+
150+With `MAX_ID_LEN` = 128 chars and UUIDs = 36 chars, 1000 messages per peer (the conversation cap)
151+= 36 KB per peer. With 500 contacts (`MAX_CONTACTS`), that's 18 MB. The `seen_ids` kv blob is
152+rewritten in full on every conversation touch (line 2316), so a large set makes every message
153+receipt slower. On iOS, a 18 MB JSON blob written to Application Support on every incoming
154+message is a real performance issue.
155+
156+**Suggested fix:** Cap the `seen_ids` set per peer (e.g., keep the last 2000 IDs) and trim
157+periodically. Or use a time-based expiry. Or a Bloom filter with a fixed size.
158+
159+### 🟡 N8. `ingest_poll` advances cursor before processing events — crash between advance and persist loses messages
160+
161+**Type:** Implementation concern (app.rs)
162+**Files:** `core/src/app.rs:2024-2089`
163+
164+The cursor is advanced to `page.cursor` on line 2038 (before the event loop), and restored to
165+`prev_cursor` on budget exhaustion (line 2086) or event count overflow (line 2069). The cursor
166+is persisted to kv AFTER `ingest_poll` returns (line 1369), via a `KeyValue::set` effect. This
167+means the cursor write only happens after the event loop completes successfully.
168+
169+However, there's a crash window: if the app crashes between `model.cursor = clamped_cursor`
170+(line 2038) and the cursor persist effect (line 1369), the in-memory cursor is advanced but
171+the persisted cursor is still `prev_cursor`. On relaunch, the client sends `prev_cursor` to
172+the relay, which re-serves the events. No message loss — the persisted cursor is the safe one.
173+
174+**Verdict:** Safe. The cursor is only authoritative when persisted, and the persist happens
175+after `ingest_poll` completes. A crash during `ingest_poll` loses no data because the persisted
176+cursor hasn't changed. Info.
177+
178+### 🟡 N9. `ingest_poll` budget exhaustion drops remaining events — client is stuck if the relay always serves an oversized page
179+
180+**Type:** Implementation concern (app.rs)
181+**Files:** `core/src/app.rs:2054-2089`
182+
183+When the `MAX_POLL_TOTAL_BYTES` budget is exhausted mid-page, the loop breaks, the cursor is
184+restored to `prev_cursor`, and an error is set. The next poll sends `prev_cursor` again, and
185+the relay re-serves the same events. If the page always exceeds the budget (e.g., the relay
186+serves 50 large blobs and the budget only covers 10), the client is stuck: poll, process 10,
187+drop 40, re-poll, relay serves the same 50, process the same 10 (deduped, fast), drop 40, etc.
188+
189+The dedup makes re-processing fast (already-seen IDs are O(1) drops), but the client never
190+advances past the oversized page. The relay's events are served in `receivedAt` order, so the
191+same 50 events come back every time. The cursor never moves. The client is stuck in a loop.
192+
193+This is a **livelock**: the client polls, processes some events, but can never ack them. The
194+relay keeps serving the same page. The client's error message ("relay sent an oversized poll
195+page") is set but may not be visible in the UI.
196+
197+**Attack scenario:** A malicious relay serves a page of 50 blobs, each just under `MAX_BLOB_LEN`
198+(42 MiB). The total page is ~2 GiB, far exceeding the 64 MiB budget. The client processes a few,
199+drops the rest, and is stuck. Or more subtly: the relay serves 50 blobs that are each ~1.3 MiB
200+(total ~65 MiB, just over the 64 MiB budget). The client processes 49, drops 1, and is stuck
201+because the cursor never advances past the 50th event.
202+
203+**Suggested fix:** When the budget is exhausted, advance the cursor to the last *successfully
204+processed* event's `receivedAt` (not `page.cursor` and not `prev_cursor`). This acks the
205+processed events and lets the client move forward, dropping only the unprocessed tail. The
206+relay re-serves only the unprocessed events on the next poll. This changes the behavior from
207+"all-or-nothing cursor advance" to "partial ack."
208+
209+### 🟡 N10. Profile `ts` staleness check uses `>=` — equal-timestamp replay is a no-op, but the check is per-contact, not per-field
210+
211+**Type:** Implementation concern (app.rs)
212+**Files:** `core/src/app.rs:2174-2179` (approximate)
213+
214+AUDIT-NEW.md Q19 analyzes this and concludes it's safe because the attacker can't modify the
215+content without breaking the signature. I agree — the `>=` is correct and a same-`ts` replay
216+is a true no-op (same ciphertext, same signature, same content). Info.
217+
218+---
219+
220+## 4. Wire Format / Parsing
221+
222+### 🟡 N11. `protocol.rs` `parse_payload` does not validate `ts` is positive — negative `ts` clamped to 0 is harmless but surprising
223+
224+**Type:** Implementation concern (protocol.rs)
225+**Files:** `core/src/protocol.rs:100`
226+
227+AUDIT-NEW.md Q24 notes this. A `ts` of 0 or negative is clamped to `max(0, ...)` in `ingest_poll`.
228+A message with `ts = 0` is inserted at the earliest chronological position and trimmed first.
229+This is harmless. Info.
230+
231+### 🟡 N12. `delivery.ack` parsing accepts non-string entries in `ack_ids` — filtered silently, no length check on individual IDs after filtering
232+
233+**Type:** Implementation concern (protocol.rs)
234+**Files:** `core/src/protocol.rs:116-126`
235+
236+AUDIT-NEW.md Q26 notes this. The double-check (raw count + post-filter count) is correct.
237+Non-string entries are filtered. Individual ID length is checked (`ack_ids.iter().any(|id|
238+id.len() > MAX_ID_LEN)`). This is correct. Info.
239+
240+---
241+
242+## 5. Federation
243+
244+### 🟠 N13. Gossip amplification: 100 online events trigger 100 mailbox scans + forward staging — O(100 × |messages|) per request
245+
246+**Type:** Design gap (server.knot)
247+**Files:** `server.knot:1544-1578` (`handleRecvGossip`)
248+
249+AUDIT-NEW.md Q27 notes this. A single gossip request with 100 `online` events for 100 different
250+keys triggers `forwardQueuedToServer` for each key, which scans the entire mailbox. With
251+|messages| = 10,000, that's 100 × 10,000 = 1M comparisons per request. At 60/min per IP, that's
252+60M comparisons/min. An attacker with 10 IPs can do 600M comparisons/min.
253+
254+Additionally, each `forwardQueuedToServer` call stages forward entries, which consume
255+`maxForwardsPerServer` slots and trigger HTTP POSTs to the attacker's server. The attacker
256+receives copies of the victim's ciphertext.
257+
258+**Suggested fix:** Limit the number of newly-online keys per gossip batch that trigger forwards
259+(e.g., cap at 10). Or batch the mailbox scan across all newly-online keys in one pass.
260+
261+### 🟡 N14. `offline` gossip events don't cancel pending forwards — wasted retries for up to 80 minutes
262+
263+**Type:** Design gap (server.knot)
264+**Files:** `server.knot:1535-1541`
265+
266+AUDIT-NEW.md Q28 notes this. When an `offline` event is received, pending forwards to that server
267+are not cancelled. They retry for up to `retryBackoffMax × maxForwardRetries` = 8 min × 10 = 80
268+minutes, wasting `maxForwardsPerServer` slots and generating failed HTTP requests.
269+
270+If the offline event was spoofed, the forwards continue — and if the key is actually still
271+online at that server, the forward succeeds on retry. The impact is minimal.
272+
273+**Suggested fix:** When processing an `offline` event, optionally drop pending forwards to that
274+server for the offline key. But this could cause message loss if the offline event is spoofed.
275+Better to leave as-is and document.
276+
277+---
278+
279+## 6. Deployment / install.sh
280+
281+### 🟡 N15. `install.sh` downloads binary over HTTPS but checksum is best-effort — missing checksum silently proceeds
282+
283+**Type:** Deployment concern (install.sh)
284+**Files:** `install.sh:150-181`
285+
286+The checksum verification is best-effort: if the `.sha256` file doesn't exist (line 162 fails),
287+the script warns and proceeds. If `sha256sum`/`shasum` is not available, it warns and proceeds.
288+A checksum that exists but doesn't match is fatal (line 169-174) — correct.
289+
290+The concern: a compromised release asset without a published `.sha256` file (e.g., a pre-checksum
291+release, or a supply-chain attack that removes the checksum file) would be installed without
292+verification. The script trusts GitHub's HTTPS, but HTTPS authenticates the CDN, not the
293+artifact. A compromised GitHub release would sail through.
294+
295+**Suggested fix:** Make the checksum mandatory for all new installs. Keep the best-effort
296+behavior only for upgrades from pre-checksum releases, with a prominent warning.
297+
298+### ⚪ N16. `install.sh` DOMAIN validation rejects uppercase but the Caddyfile interpolation is safe
299+
300+**Type:** Deployment review (install.sh)
301+**Files:** `install.sh:34-45`
302+
303+DOMAIN is lowercased and stripped of scheme/path/port before interpolation into the Caddyfile
304+and systemd unit. The validation rejects anything outside `[a-z0-9.-]`. This prevents config
305+injection (a stray quote or semicolon in the Caddyfile). Good.
306+
307+**Verdict:** Safe. Info.
308+
309+---
310+
311+## 7. Apple Shell
312+
313+### ⚪ N17. URLSession redirect cancellation is correct — bearer token exfiltration via 302 is prevented
314+
315+**Type:** Implementation review (Effects.swift)
316+**Files:** `apple/SkrepkaApp/Effects.swift:8-30`
317+
318+The `NoRedirectDelegate` cancels all HTTP redirects. This prevents a hostile relay from
319+answering `/poll` or `/messages` with a 302 to an attacker-controlled host, which would replay
320+the `Authorization: Bearer` header to the redirect target. The delegate returns `nil`, which
321+hands the 3xx response to the caller as a non-2xx status. The core treats it as a failed
322+request. Correct.
323+
324+**Verdict:** Safe. Good defense.
325+
326+### ⚪ N18. Keychain uses `kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly` — key never rides backup
327+
328+**Type:** Implementation review (Effects.swift)
329+**Files:** `apple/SkrepkaApp/Effects.swift:352`
330+
331+The identity key is stored with `ThisDeviceOnly` accessibility, meaning it never enters iCloud
332+or iTunes backups. The key is accessible after first unlock (not only when unlocked), so
333+background polling works. This is the correct accessibility class for a messaging identity key.
334+
335+**Verdict:** Safe. Good practice.
336+
337+---
338+
339+## Summary of New Findings
340+
341+| # | Severity | Type | Title |
342+|---|----------|------|-------|
343+| N1 | 🔴 critical | Design gap | No AAD — `to` field unauthenticated, silent targeted censorship |
344+| N2 | ⚪ info | Code hygiene | HKDF info and wire version byte are independent constants |
345+| N3 | 🟡 medium | DoS | appendMessage dedup O(n) — identical-blob replay amplifies to O(n × batch) |
346+| N4 | 🟡 medium | Impl bug | capForwards non-deterministic eviction when retry counts equal |
347+| N5 | 🟡 medium | Design gap | fromServer not verified against connecting peer address |
348+| N6 | 🔵 low | Info | genToken entropy waste (one digit per call) |
349+| N7 | 🟡 medium | Impl concern | seen_ids dedup set unbounded — grows without limit |
350+| N8 | ⚪ info | Safe | Cursor crash window — persisted cursor is safe |
351+| N9 | 🟡 medium | Impl concern | Budget exhaustion livelock — client stuck if relay always serves oversized page |
352+| N10 | ⚪ info | Safe | Profile ts >= replay is a no-op |
353+| N11 | ⚪ info | Safe | Negative ts clamped to 0 is harmless |
354+| N12 | ⚪ info | Safe | ack_ids non-string filtering is correct |
355+| N13 | 🟠 high | Design gap | Gossip amplification 100x — O(100 × |messages|) per request |
356+| N14 | 🟡 medium | Design gap | Offline events don't cancel pending forwards |
357+| N15 | 🟡 medium | Deploy | install.sh checksum is best-effort — missing checksum silently proceeds |
358+| N16 | ⚪ info | Safe | install.sh DOMAIN validation is correct |
359+| N17 | ⚪ info | Safe | Redirect cancellation prevents token exfiltration |
360+| N18 | ⚪ info | Safe | Keychain ThisDeviceOnly is correct |
361+
362+**Critical (1):** N1
363+**High (1):** N13
364+**Medium (6):** N3, N4, N5, N7, N9, N14, N15
365+**Low (1):** N6
366+**Info (7):** N2, N8, N10, N11, N12, N16, N17, N18
367+
368+---
369+
370+## Priorities
371+
372+1. **N1 (critical):** Add AAD to AEAD. This is the most impactful finding — it enables
373+ undetectable censorship by any relay. Requires wire-format change (v0x02).
374+
375+2. **N13 (high):** Document or limit gossip amplification. A single request triggers 100
376+ mailbox scans. Cap newly-online keys that trigger forwards per batch.
377+
378+3. **N9 (medium):** Fix budget exhaustion livelock. Advance cursor to last processed event
379+ instead of restoring to pre-page cursor. This prevents a malicious relay from permanently
380+ wedging the client with oversized poll pages.
381+
382+4. **N7 (medium):** Bound `seen_ids` set. An unbounded dedup set that's rewritten on every
383+ message receipt will degrade performance over time.
384+
385+5. **N3 (medium):** Index `*messages` for dedup. The O(n) scan is a CPU amplification vector.