fix corruption
ober
0d02bdf31655fd41265ce43f553cafedb2c1353b
--- a/AGENTS.md +++ b/AGENTS.md @@ -348,6 +348,23 @@ Common sibling repos that exist but must NOT be touched without explicit instruc If a user instruction mentions a file path, use EXACTLY that path. Do not substitute a similar-looking path from another repo. +### Never Reference Sibling Checkouts in Build Files + +Build files (Makefile, shell scripts, CI config) must **never** resolve a +dependency via a relative sibling path (`../jerboa-foo`) or an absolute +`~/mine/jerboa-foo` path. That layout is specific to this one machine — +other users and CI do not have it. Always vendor instead: fetch/clone the +dependency into `vendor/` (or this repo's equivalent) at build time, or use +a pinned-release fetch script, so the build is reproducible without +assuming any sibling checkout exists. + +A sibling-path fallback is not just a portability bug: it can silently +substitute a full alternate source tree (build config, embedded data, +secrets) for the vendored one, with no equivalent safety default, changing +what actually gets built without any indication. If you find one +(`grep -rn '\.\./jerboa\|~/mine/jerboa'` over Makefiles/scripts), remove it +and vendor properly instead. + --- ## Build & Verification new file mode 100644 --- /dev/null +++ b/kimi3.md @@ -0,0 +1,205 @@ +# Handoff: Findings from 2026-07-23 security review + +## Context + +Reviewed today's commits (`07e3948` fix password issue, `c4e6f92` TUI string-trim +build fix, `ffafa19` fix scroll) against the security hardening from the previous +session (`ef4e7c3` + `e8a8553`, logdb container crypto P1 #38; `5869f42` +incremental append-only sealing). + +**Verdict: the security fixes were NOT broken.** `make test` passes in full, +including all P1 #38 negative regression tests. The findings below are *new, +smaller* issues spotted during the review — none are regressions of yesterday's +work. + +## Verified invariants — DO NOT "fix" these + +The next model must not weaken or remove any of the following. They are +intentional and covered by `tests/test-logdb-crypto.ss` / +`tests/test-logdb-incremental.ss`: + +1. `decrypt-container` (signal/logdb.ss:369) tries the current + generation-counter format first, the pre-hardening legacy format second, and + raises `logdb-passphrase-mismatch` only when BOTH fail. A wrong passphrase + MUST keep failing closed. +2. Legacy-format containers self-migrate on first open via + `persist-jlog-legacy!` (signal/logdb.ss:709-715). The legacy path is + decrypt-only; sealing always uses the current counter-nonce format. +3. Incremental checkpoints that decrypt via the legacy path MUST keep raising + the "unexpected container format" error (signal/logdb.ss:742-745). +4. The `.gen` anti-rollback marker check for current-format containers + (signal/logdb.ss:690-693), truncation-to-zero refusal + (signal/logdb.ss:797-802), and key zeroing in `jlog-close!` + (signal/logdb.ss:1406) are untouched and must stay. +5. The lenient marker check for legacy containers (`generation = marker + 1`, + signal/logdb.ss:689) is deliberate — see Finding C before touching it. + +--- + +## A. Bugs (TUI rendering) + +### A1. Down scroll-arrow overwrites a conversation row +**File:** `signal/tui/main.ss:2645-2676` (`draw-conversations!`) + +The item loop draws rows while `(< row (+ y height))`, so the last drawable row +is `y + height - 1`. The down arrow (line 2673-2676) is drawn at `y + height - +1` when `scroll > 0`, and at `y + height - 2` when `scroll = 0`. Both positions +are already occupied by a conversation row, so one conversation is hidden +whenever `(< (+ scroll height) total)`. + +**Fix:** reserve indicator rows in the loop bound — e.g. compute +`last-item-row` as `(- (+ y height) (if more-below? 1 0))` and stop the item +loop there, mirroring how the up-arrow case already shifts the start row by 1. + +### A2. Down scroll-arrow overwrites the last message row when not typing +**File:** `signal/tui/main.ss:2850-2873` (`draw-thread!`) + +`rows = height - 3 - reserved` where `reserved` is 1 only when a typing +indicator is active. The message loop's last drawable row is +`y + 3 + rows - 1` = `y + height - 2` when typing, but `y + height - 1` when +not typing. The down arrow is always drawn at `y + height - 1` (line 2873), so +with no typing indicator it clobbers the last rendered message row. + +**Fix:** same as A1 — subtract one more row for the down indicator when +`(< scroll max-scroll)`, or clamp the arrow to a row the loop cannot reach. + +--- + +## B. Dead code / unused bindings + +- **`scroll-conversations!` is never called** (`signal/tui/main.ss:2931`). + Either wire it to a key binding (PgUp/PgDown on the conversation pane) or + delete it. It also has an unused `left-w` binding (line 2934). +- **Unused `left-w`** in the auto-scroll block (`signal/tui/main.ss:1040`). +- **Unused `args`** in the no-args branch (`signal/main.ss:146-147`): + `(let-values ([(account args) (parse-account-flag '())]) (cmd-tui account))` + can be just `(cmd-tui #f)`. + +--- + +## C. Security residual (intentional, documented — do not "fix" blindly) + +### Legacy-container rollback window +**File:** `signal/logdb.ss:685-693` + +When a container only opens via the pre-hardening fallback, `generation` is set +to `marker + 1`, so the `(< generation marker)` rollback check can never fire. +Consequence: an attacker with write access to the log file who captured the +container *before* migration can restore that old legacy container and it will +open (and re-migrate) with stale content, without triggering the anti-rollback +error added in `ef4e7c3`. + +**Why it is this way:** the marker is written at line 694 *before* the +migration re-seal at line 715. If the process crashes in between, the container +is still legacy with a marker > 0; a strict "marker > 0 rejects legacy" rule +would permanently lock the user out. The lenient behavior is the crash- +recovery path. + +**Scope:** only ciphertext captured before the one-time migration. Post- +migration containers keep full rollback protection. + +**Optional improvement (low priority):** write a separate "migration complete" +flag file only *after* the re-seal succeeds, and reject legacy containers when +that flag exists. Whichever behavior is chosen, pin it with a regression test +(see G1). + +--- + +## D. Key hygiene (minor) + +**File:** `signal/logdb.ss:349-361, 369-392` + +`ef4e7c3` added zeroing of the live AEAD key on `jlog-close!`, but transient +derived keys from *failed* open attempts are dropped to the GC unzeroed: + +- Every wrong-passphrase open of a current-format container now derives up to + two scrypt keys (one in `decrypt-container`, one in + `try-decrypt-legacy-nonce-container`); neither is zeroed. +- On the legacy-success path, the key derived in `decrypt-container` (same + bytes as the retained legacy key) is discarded unzeroed. + +**Fix:** wrap the open attempts so the derived bytevector is +`bytevector-fill!`-ed to 0 on every path that doesn't transfer ownership to the +jlog handle (e.g. `unwind-protect` in `try-decrypt-legacy-nonce-container`, and +zero the first key in the legacy-success branch of `decrypt-container`). +Threat model requires memory-read access, so this is hardening, not a live vuln. + +--- + +## E. Performance (minor) + +1. **2x scrypt per failed open** (`signal/logdb.ss:369-392`): any failed + current-format open (wrong passphrase, corrupted container) now also runs + the legacy attempt, doubling KDF cost per failure. Side effect is a marginal + slowdown of online passphrase guessing (a small benefit), but it also + doubles the latency of a legit typo'd passphrase. Worth knowing; not worth + optimizing since the formats can't be distinguished by inspection. +2. **`list-ref` per visible row** (`signal/tui/main.ss:2658`): + `draw-conversations!` is O(height x (scroll + height)) per frame. Use + `(list-tail conversations scroll)` once and walk with `cdr`. +3. **Mention candidates rescan full history per keystroke** + (`signal/tui/main.ss`, `conversation-participant-senders`, ~line 2953): + called from `mention-candidates` while mention completion is active; walks + every message with a linear `member` dedup, so O(history^2) worst case in + long group chats. Cache the sender list per conversation (invalidate on new + message) or cap the scan to the last N messages. + +--- + +## F. Design / UX notes + +1. **Bare `jerboa-signal` now launches the TUI** (`signal/main.ss:144-147`) + instead of printing usage and exiting 2. Scripts or cron jobs invoking it + with no arguments will now block on a passphrase prompt / TUI init. Intended + or not, it should be documented; consider falling back to usage when stdout + is not a tty. +2. **Visible-height formula duplicated 4 times** + (`signal/tui/main.ss:1039, 2576, 2921, 2933`): `(max 4 (- h 4))` then + `(- body-h 3)`. Extract a helper (e.g. `(conversation-list-height state)`); + a layout change that misses one site will desync auto-scroll from drawing. +3. **Generation skips a value on migration** (`signal/logdb.ss:689,694,987`): + marker written as `marker+1` at open, then the migration persist seals the + container at `marker+2`. Harmless (monotonicity and nonce uniqueness are + preserved; gaps are fine). Cosmetic only. +4. **Migration notice printed from the storage layer** + (`signal/logdb.ss:709-714`): `display` to `current-error-port` inside + logdb open mixes UI into the library. Fine for the CLI; noise for any + headless/embedder use. Consider a callback or leaving it to the caller. +5. **`parse-account-flag` comment vs implementation** (`signal/main.ss:134`): + comment says "-a/--account anywhere in the first two slots" but only the + first slot is inspected. Pre-existing doc mismatch; harmless for the new + no-args call which passes `'()`. + +--- + +## G. Test coverage gaps + +1. **Pin the Finding C semantics:** no test covers reopening a legacy container + when the `.gen` marker is already > 0 (the rollback-window behavior). Add + one asserting whichever behavior is chosen, so a future change can't flip + it silently. +2. **Incremental-checkpoint legacy rejection** (`signal/logdb.ss:742-745`): + the new fail-closed branch has no test. Craft a `.ckpt` file in the legacy + format and assert the "unexpected container format" error. +3. **TUI logic untested:** scroll (A1/A2), auto-scroll-on-select, and the new + per-conversation mention scoping have no harness. Even a pure-function test + of the row/scroll arithmetic (extracted from the draw code) would catch + A1/A2-class bugs. +4. **Fixed `/tmp` test paths:** `tests/test-logdb-crypto.ss` (like its + siblings) uses constant paths such as + `/tmp/jerboa-signal-logdb-legacy-nonce-test.db`. Parallel or multi-user runs + can collide; unique temp names (e.g. including `(getpid)`) would be safer. + Pre-existing style, low priority. + +--- + +## How to verify any change + +- `make test` — runs the binary WPO build plus all suites, including + `test-logdb-crypto.ss`, `test-logdb-incremental.ss`, `test-logdb-jsqlite.ss`, + and the attachment/native-loader/secret-input security tests. This is the + gate; it passed cleanly at commit `07e3948`. +- On this macOS host, `make binary` is the canonical pre-commit build check + (do NOT use `make docker-build` here; that is the Linux pipeline). +- If an edit to a `.ss` file seems to have no effect, delete stale artifacts: + `find lib -name "*.so" -delete && find lib -name "*.wpo" -delete && make build`. --- a/signal/tui/main.ss +++ b/signal/tui/main.ss @@ -393,19 +393,25 @@ (def *idle-poll-ms* 250) (def *max-actor-events-per-tick* 128) + (def *invalidate-interval-s* 10) (def (event-loop state actor) - (let loop ([dirty? #t]) + (let loop ([dirty? #t] [last-invalidate (real-time)]) (let* ([actor-dirty? (handle-actor-events! state actor)] [send-dirty? (handle-send-events! state)] - [timer-dirty? (expire-typing-indicators! state)]) - (when (or dirty? actor-dirty? send-dirty? timer-dirty?) + [timer-dirty? (expire-typing-indicators! state)] + [now (real-time)] + [invalidate? (>= (- now last-invalidate) *invalidate-interval-s*)] + [need-draw? (or dirty? actor-dirty? send-dirty? timer-dirty? invalidate?)]) + (when need-draw? + (when invalidate? (tb-invalidate!)) (draw! state) (tb-present!)) (let* ([ev (tb-peek-event *idle-poll-ms*)] - [event-dirty? (and ev (begin (handle-event! state actor ev) #t))]) + [event-dirty? (and ev (begin (handle-event! state actor ev) #t))] + [last-invalidate (if invalidate? now last-invalidate)]) (unless (tui-state-quit? state) - (loop event-dirty?)))))) + (loop event-dirty? last-invalidate)))))) (def (expire-typing-indicators! state) (let ([now (real-time)]) @@ -2647,14 +2653,13 @@ [total (length conversations)] [selected-index (clamp-index (tui-state-selected-index state) total)] [scroll (tui-state-conv-scroll state)] - [max-scroll (max 0 (- total height))]) - ;; Scroll indicator (up arrow if scroll > 0) + [more-below? (< (+ scroll height) total)] + [max-row (- (+ y height) (if more-below? 1 0))]) (when (> scroll 0) (draw-text! x y width (fg-dim) (panel-bg) "^")) - ;; Draw visible slice (let loop ([idx scroll] [row (+ y (if (> scroll 0) 1 0))]) - (when (and (< idx total) (< row (+ y height))) + (when (and (< idx total) (< row max-row)) (let* ([conv (list-ref conversations idx)] [selected? (= idx selected-index)] [marker (if selected? "> " " ")] @@ -2669,11 +2674,8 @@ (conversation-title conv) suffix)) (loop (+ idx 1) (+ row 1))))) - ;; Scroll indicator (down arrow if more items below) - (when (< (+ scroll height) total) - (let* ([ind-row (+ y height -1)] - [ind-row (if (> scroll 0) ind-row (- ind-row 1))]) - (draw-text! x ind-row width (fg-dim) (panel-bg) "v"))))) + (when more-below? + (draw-text! x (- (+ y height) 1) width (fg-dim) (panel-bg) "v")))) (def (draw-new-message-page! state x y width height) (draw-panel! x y width height "New Message") @@ -2854,23 +2856,23 @@ [rendered (thread-render-rows (conversation-messages conv) width rows - scroll)]) - ;; Scroll indicator (up arrow if scroll > 0) + scroll)] + [max-scroll (max-thread-scroll conv width rows)] + [more-below? (< scroll max-scroll)] + [msg-rows (- rows (if more-below? 1 0))]) (when (> scroll 0) (draw-text! x (+ y 3) width (fg-dim) (bg) "^")) (let loop ([rs rendered] [row (+ y 3 (if (> scroll 0) 1 0))]) - (if (and (pair? rs) (< row (+ y 3 rows))) + (if (and (pair? rs) (< row (+ y 3 msg-rows))) (begin (draw-text! x row width (chat-message-fg (caar rs)) (bg) (cdar rs)) (loop (cdr rs) (+ row 1))) - (when (and typing (< row (+ y height))) + (when (and typing (< row (+ y height -1))) (draw-text! x row width (fg-dim) (bg) (string-append typing " is typing..."))))) - ;; Scroll indicator (down arrow if more rows below) - (let* ([max-scroll (max-thread-scroll conv width rows)]) - (when (< scroll max-scroll) - (draw-text! x (+ y height -1) width (fg-dim) (bg) "v"))))) + (when more-below? + (draw-text! x (+ y height -1) width (fg-dim) (bg) "v")))) (begin (draw-text! x y width (fg-strong) (bg) "jerboa-signal") (draw-text! x (+ y 1) width (fg-dim) (bg) @@ -2918,11 +2920,15 @@ (let* ([w (max 10 (- (tui-state-width state) (if (< (tui-state-width state) 80) 20 26) 2))] - [height (let* ([body-h (max 4 (- (tui-state-height state) 4))] - [typing (active-typing-sender conv)] - [reserved (if typing 1 0)]) - (max 0 (- body-h 3 reserved)))] - [max-scroll (max-thread-scroll conv w height)]) + [height (let* ([body-h (max 4 (- (tui-state-height state) 4))] + [typing (active-typing-sender conv)] + [reserved (if typing 1 0)]) + (max 0 (- body-h 3 reserved)))] + [max-scroll0 (max-thread-scroll conv w height)] + [height (if (> max-scroll0 0) (max 0 (- height 1)) height)] + [max-scroll (if (> max-scroll0 0) + (max-thread-scroll conv w height) + max-scroll0)]) (tui-state-thread-scroll-set! state (clamp-index (+ (tui-state-thread-scroll state) delta) (+ max-scroll 1))))))) @@ -2933,7 +2939,11 @@ [body-h (max 4 (- (tui-state-height state) 4))] [left-w (if (< (tui-state-width state) 80) 18 24)] [height (- body-h 3)] - [max-scroll (max 0 (- total height))] + [max-scroll0 (max 0 (- total height))] + [height (if (> max-scroll0 0) (- height 1) height)] + [max-scroll (if (> max-scroll0 0) + (max 0 (- total height)) + max-scroll0)] [scroll (tui-state-conv-scroll state)] [new-scroll (clamp-index (+ scroll delta) (+ max-scroll 1))]) (tui-state-conv-scroll-set! state new-scroll)))