Fix 8 findings from independent code audit

docs/AUDIT_2026-07.md

@@ -0,0 +1,288 @@
+# Independent code audit — 2026-07-27
+
+Multi-agent review of the AmiAuth codebase at commit `e9e1e78` (main), covering
+core crypto/vault (`src/core/`), CLI (`src/cli/`), GUI (`src/gui/`), the Amiga
+platform/entropy layer (`src/amiga/`), and the QR decoder integration wrapper
+(`src/qr/qr.c`, `src/amiga/qrimage.c`). Vendored third-party quirc internals
+were excluded per [THIRDPARTY.md](../THIRDPARTY.md).
+
+Method: six independent reviewers each covered one dimension of the codebase;
+every finding was then checked by two independent adversarial verifiers before
+being included here. 17 candidate findings were raised, 9 were refuted as
+false positives on verification, 8 survived. See "Reviewed, no issues found"
+below for the areas that turned up nothing.
+
+Track fixes against this list; update or remove entries here as they're
+resolved rather than leaving them to go stale.
+
+**Status: all 8 findings fixed** (2026-07-27), verified against the host test
+suite (`make test`, 292/292 passing) and clean `make m68k-docker`/`make
+gui-docker` cross-builds. Each finding below records what was fixed and, where
+the audit's own suggested fix direction wasn't the right call for this
+codebase, why.
+
+## Executive summary
+
+Overall code health is good: no memory-corruption bugs, no buffer overflows,
+and no violations of the frozen vault format or the 68000/zero-dependency
+baseline were found. The codebase shows a consistent, deliberate security
+posture in its primary paths (vault key zeroing on lock/quit, format-version
+rejection, interactive-only passphrase entry). The findings that did surface
+cluster around two themes:
+
+1. **Inconsistent secret-hygiene discipline** — the project's own "zero key
+   material after use" practice, well-applied in `vault.c`, is not carried
+   through to the hot, high-frequency HMAC/PBKDF2 primitives or to one CLI
+   error path, leaving passphrase- and secret-derived bytes on the stack
+   longer than necessary on a platform with no memory protection.
+2. **Weak trust assumptions in network/UI-adjacent code** — the SNTP client
+   accepts a time reply from any host, and the GUI's passphrase entry
+   silently drops the keystroke-timing entropy source the design docs call
+   the primary defence on quiescent/emulated hardware.
+
+None of the findings are exploitable for remote compromise or vault
+corruption; the highest-severity items require either local code execution
+(already a stated "no memory protection" edge in the project's own threat
+model, see [SECURITY.md](SECURITY.md)) or LAN/on-path network position.
+
+## Findings
+
+### High
+
+#### 1. SNTP reply accepted from any source, not just the queried server
+
+- **File:** `src/amiga/sntp.c:71`
+- **Status:** fixed — the UDP socket now `connect()`s to the resolved server
+  address before exchanging packets (`send()`/`recv()` instead of
+  `sendto()`/`recvfrom()`), so the stack drops any reply not actually from
+  that peer. The "sanity bound on offset magnitude" part of the original
+  suggestion was deliberately **not** applied: `docs/CLOCK.md` documents that
+  RTC-less machines can legitimately need decades-large corrections, so
+  bounding the offset would reject valid syncs.
+
+`recvfrom()` is called with `NULL` for the source-address arguments, and the
+socket is never `connect()`-ed, so any UDP datagram delivered to the ephemeral
+port is treated as the server's reply.
+
+*Failure scenario:* An attacker on the same LAN/WiFi, a compromised router, or
+an off-path attacker racing the real server can inject a forged SNTP response
+before the genuine one (or before the 5s timeout) arrives. The only anti-spoof
+check (`orig != t1`, where `t1` is `time(NULL)` sent in cleartext) is trivially
+satisfied by anyone who can see the request or guess the current second. The
+forged offset is applied with no sanity bound and the clock is marked
+`CLOCK_SYNCED` (trusted/green in the UI), silently desynchronizing the
+corrected UTC used for TOTP generation and validation.
+
+*Fix direction:* Validate the reply's source address/port against the
+resolved server before accepting it, and add a sanity bound on the accepted
+offset magnitude; consider a less guessable originate-timestamp nonce.
+
+#### 2. GUI passphrase entry never feeds the entropy pool
+
+- **File:** `src/gui/main.c:729`
+- **Status:** fixed — added `amiga_stir_keystroke()` (exported from
+  `src/amiga/random.c`/`entropy.h`) and called it on every
+  `WMHI_VANILLAKEY` event in `passphrase_request()`, mirroring the CLI's
+  per-keystroke `stir_eclock()` call. `amiga_entropy_stir()` itself is left
+  in place — it's public API for folding externally-supplied samples, a
+  distinct role from the new keystroke-timing hook.
+
+`passphrase_request()` reads keystrokes via `IDCMP_VANILLAKEY` in a ReAction
+event loop and never calls `amiga_entropy_stir()` (confirmed dead code, never
+called anywhere) or otherwise stirs timing into the RNG state, unlike the
+CLI's `amiga_read_passphrase()`, which calls `stir_eclock()` per keystroke.
+
+*Failure scenario:* [SECURITY.md](SECURITY.md) states keystroke timing during
+passphrase entry is "the main real source at the moment it matters (vault
+creation)." GUI first-run vault creation calls `passphrase_request()` for the
+new passphrase, then generates the vault salt via `amiga_random()` with no
+timing contribution in between. Since the GUI is plausibly the primary
+first-run UX, GUI-created vaults fall back to `amiga_random()`'s passive
+entropy (EClock jitter, `DateStamp`, `AvailMem`, addresses) — which the same
+doc says yields "little entropy" on a quiescent 68000 and "almost none" under
+a deterministic emulator — silently, with no user-visible warning.
+
+*Fix direction:* Wire keystroke-timing stirring into the GUI's
+`IDCMP_VANILLAKEY` handler the same way the CLI's RAW-read loop does, and
+delete or use `amiga_entropy_stir()`.
+
+### Medium
+
+#### 3. `passphrase_request()` leaks its ReAction gadget tree if window creation fails
+
+- **File:** `src/gui/main.c:729`
+- **Status:** fixed — `passphrase_request()`, `uri_request()`, `edit_request()`,
+  and `secret_meta_request()` (the four `window.class` requesters sharing this
+  pattern) now `DisposeObject(layoutobj)` on the `!win` path. `qr_file_request()`
+  and `vault_location_request()` were checked too but use ASL requesters, not
+  `window.class`, so they don't share this leak.
+
+`layoutobj` (owning `labelobj`, `maskobj`, `okobj`, `cancelobj`, `buttons`) is
+built up front; if `NewObject(WINDOW_GetClass(), ...)` returns `NULL`, the
+function does `if (!win) return 0;` with no `DisposeObject(layoutobj)`.
+Ownership of the layout only transfers to the window object on success, so
+failure orphans the whole tree. The same pattern recurs at other requester
+call sites in the file (`uri_request` and others), so this is systemic rather
+than a one-off.
+
+*Failure scenario:* Under memory pressure, `NewObject(WINDOW_GetClass(), ...)`
+can fail while earlier, smaller allocations (labels, buttons, layout) already
+succeeded. Since this requester runs on every unlock attempt and every
+first-run passphrase entry, repeated failures under sustained memory pressure
+leak the gadget tree each time, worsening the very condition that caused the
+failure.
+
+*Fix direction:* On `!win`, explicitly `DisposeObject(layoutobj)` (and apply
+the same guard at the other requester call sites showing the identical
+pattern).
+
+#### 4. HMAC init leaves padded key and ipad unzeroed on the stack
+
+- **File:** `src/core/hmac.c:11` (also lines 67, 123)
+- **Status:** fixed — all three `hmac_*_init()` functions now `memset` their
+  `k[]`/`ipad[]` stack locals to zero before returning. `ctx->opad` is left
+  alone: it's still needed by the matching `_final()` call, and zeroing it as
+  part of a context-teardown step would be a broader API change (an explicit
+  teardown function doesn't currently exist) — out of scope for this fix.
+
+`hmac_sha1_init`/`hmac_sha256_init`/`hmac_sha512_init` build the block-padded
+key (`k[]`) and `ipad[]` (`k` XOR `0x36`) as stack locals and never `memset`
+them to zero before returning, unlike `vault.c`'s `derive_keys()`, which
+explicitly zeroes its own scratch buffer after use.
+
+*Failure scenario:* These functions run on every PBKDF2 iteration (with the
+raw passphrase as key), every vault MAC computation (with `mac_key`), every
+TOTP/HOTP code render (with the raw secret), and every DRBG step. Given
+AmigaOS's lack of memory protection (acknowledged in
+[SECURITY.md](SECURITY.md)), any other running process can read this
+process's stack and recover recently-used key/secret material long after the
+operation completed, undermining the documented "key material is zeroed on
+lock and quit" mitigation, which this code path runs independently of and far
+more frequently than.
+
+*Fix direction:* Zero `k[]` and `ipad[]` (and the context's `opad`/`inner`
+state) before returning/on context teardown, matching the zeroing discipline
+already used in `vault.c`.
+
+### Low
+
+#### 5. PBKDF2 per-iteration HMAC context and U/T buffers never zeroed
+
+- **File:** `src/core/pbkdf2.c:15`
+- **Status:** fixed — `pbkdf2_block()` now zeroes `idx`, `u`, `t`, and the
+  `hmac_sha1_ctx c` before returning; `pbkdf2_hmac_sha1()` also zeroes its
+  `block` scratch buffer after each iteration's `memcpy` into `out`.
+
+`pbkdf2_block()`'s stack locals `idx[4]`, `u[]`, `t[]`, and `hmac_sha1_ctx c`
+(whose `opad` is derived from the passphrase) persist unzeroed across up to
+the configured iteration count and after the function returns.
+
+*Failure scenario:* Same exposure class as finding 4 — a concurrently running
+process on the same non-protected AmigaOS instance can scan stack memory
+after a vault unlock/creation and recover passphrase-derived intermediate KDF
+state.
+
+*Fix direction:* Zero `idx`, `u`, `t`, and the HMAC context before
+`pbkdf2_block()`/`pbkdf2_hmac_sha1()` return.
+
+#### 6. `parse_payload()` does not validate the per-account `digits` byte
+
+- **File:** `src/core/vault.c:170`
+- **Status:** fixed — `parse_payload()` now rejects (`VAULT_ERR_FORMAT`) any
+  `digits` value AmiAuth itself never writes: exactly `STEAM_CODE_DIGITS` (5)
+  for Steam Guard accounts, 6-8 for everything else (matching what
+  `uri.c`'s otpauth:// parser and the GUI's `edit_request()` validation
+  actually allow to be written — the GUI permits 7 in addition to the
+  documented "6 or 8", not just those two values, so the check uses the
+  inclusive range rather than an exact `alg`-style enum match).
+
+`a.digits = in[p++]` is read with no range check, unlike the adjacent
+`alg`/`type` fields two lines later, which are explicitly rejected with
+`VAULT_ERR_FORMAT` if out of range. [VAULT_FORMAT.md](VAULT_FORMAT.md)
+documents `digits` as "6 or 8."
+
+*Failure scenario:* A corrupted or hand-edited vault with `digits=0` or
+`digits=200` loads successfully (`VAULT_OK`). No crash or overflow occurs —
+`otp_render()`/`hotp()` independently clamp out-of-range values to
+`OTP_DEFAULT_DIGITS` and the code buffer is sized safely — but the vault
+layer provides no defense-in-depth for this field the way it does for its
+siblings, and a corrupted `digits` byte silently produces a
+plausible-but-wrong code length instead of being reported as a format error.
+
+*Fix direction:* Validate `digits` (e.g., 6/7/8, or the accepted set
+including 5 for Steam Guard) in `parse_payload()` and return
+`VAULT_ERR_FORMAT` on violation, consistent with the `alg`/`type` checks.
+
+#### 7. Decoded secret not zeroed on `open_vault()` failure in `cmd_add()`
+
+- **File:** `src/cli/main.c` (`cmd_add()`)
+- **Status:** fixed — the `open_vault()` failure branch now
+  `memset(&acct, 0, sizeof(acct))` before returning, matching every other
+  exit path in the function and `cmd_add_secret()`'s equivalent branch.
+
+If `open_vault()` fails after `otpauth_parse()` has already decoded the
+Base32 secret into the stack-local `acct`, `cmd_add()` returns immediately
+without `memset(&acct, 0, sizeof(acct))`, unlike every other exit path in the
+same function and unlike the identical failure branch in the sibling
+`cmd_add_secret()`, which does zero `acct` in this case.
+
+*Failure scenario:* Running `AmiAuth ADD "otpauth://..."` against an
+encrypted vault with a mistyped passphrase (or any vault I/O error) leaves
+the freshly decoded TOTP/HOTP secret in stack memory, unscrubbed, contrary to
+the project's stated "key material is zeroed" practice.
+
+*Fix direction:* Add the same `memset(&acct, 0, sizeof(acct))` to this
+early-return branch that `cmd_add_secret()` already has.
+
+#### 8. `qr_decode_gray()` dereferences `uri` before validating it is non-NULL
+
+- **File:** `src/qr/qr.c:131`
+- **Status:** fixed — the argument-validation check now runs before the
+  `uri[0] = '\0'` write, so a `NULL uri` returns `QR_ERR_ARGS` as documented
+  instead of crashing.
+
+Line 131 writes `uri[0] = '\0'` when `cap > 0`, before the `!uri` check on
+line 133 (the same check that exists specifically to catch a NULL `uri`, per
+the documented `QR_ERR_ARGS` contract).
+
+*Failure scenario:* A caller invoking `qr_decode_gray(gray, w, h, NULL, cap)`
+with `cap > 0` crashes on the NULL dereference instead of receiving the
+documented `QR_ERR_ARGS`. Not reachable via the current single production
+caller (`src/gui/main.c`, which always passes a valid static buffer) and
+untested by the current unit tests, but it inverts the function's own
+argument-validation contract.
+
+*Fix direction:* Reorder the argument-validation check before the
+`uri[0] = '\0'` write.
+
+## Reviewed, no issues found
+
+The following areas were reviewed against the stated hard constraints (68000
+baseline, zero mandatory runtime dependencies, interactive-only passphrase,
+frozen vault format, ≤4 KB stack discipline, and standard memory-safety /
+timing-side-channel concerns) with no additional findings surviving review:
+
+- **Crypto primitives** (SHA-1/256/512, HMAC, PBKDF2, DRBG core logic,
+  TOTP/HOTP code generation) — beyond the stack-zeroing gaps above (findings
+  4–5), no buffer overflows, integer overflows, or weak/predictable
+  randomness in the algorithms themselves.
+- **Vault security** (format parsing, encryption/MAC, load/save, lock/quit
+  key zeroing) — beyond the `digits` validation gap above (finding 6), no
+  format-compatibility breaks, no stack-allocation-of-large-objects
+  regressions, no MAC/comparison timing issues found.
+- **CLI** (`src/cli/main.c` and friends) — passphrase handling confirmed
+  interactive-only with no CLI-arg/env/file/commodity-port hatch; beyond the
+  one unzeroed error path above (finding 7), no argument-parsing or
+  I/O-handling defects found.
+- **GUI** (`src/gui/main.c`, ReAction/BOOPSI usage) — beyond the entropy and
+  leak findings above (findings 2–3), no other memory-safety or logic
+  defects found in dialog/window handling.
+- **Amiga platform/entropy layer** (`src/amiga/random.c`, `entropy.c`,
+  clock/SNTP glue) — beyond the SNTP source-validation gap above (finding
+  1), entropy gathering and clock-offset application logic otherwise
+  consistent with documented design ([CLOCK.md](CLOCK.md),
+  [SECURITY.md](SECURITY.md)).
+- **QR wrapper integration** (`src/qr/qr.c`, `src/amiga/qrimage.c`) — beyond
+  the argument-check ordering above (finding 8), no issues found in
+  AmiAuth's own integration code calling into vendored quirc; quirc's
+  internals were out of scope per [THIRDPARTY.md](../THIRDPARTY.md).

src/amiga/entropy.h

@@ -16,6 +16,12 @@ int amiga_random(uint8_t *buf, size_t n);
 /* Fold an application-supplied sample into the entropy pool. */
 void amiga_entropy_stir(const void *p, size_t n);
 
+/* Fold a fresh E-clock reading into the entropy pool — call this on every
+ * keystroke of any UI's own passphrase input loop (amiga_read_passphrase()
+ * already does this internally; front ends with their own event-driven
+ * keystroke handling, e.g. the GUI, must call it explicitly per keystroke). */
+void amiga_stir_keystroke(void);
+
 /* Prompt and read a passphrase with no echo, using RAW console mode. Each
  * keystroke's arrival time is stirred into the entropy pool. Returns 0 on
  * success, -1 if there is no interactive console or on error. */

src/amiga/random.c

@@ -107,6 +107,12 @@ static void stir_eclock(void)
     sha1_update(&g_pool, &ev, sizeof ev);
 }
 
+void amiga_stir_keystroke(void)
+{
+    pool_ensure();
+    if (timer_ready()) stir_eclock();
+}
+
 /* Fold volatile system state that varies run-to-run. */
 static void stir_system_state(void)
 {

src/amiga/sntp.c

@@ -62,14 +62,18 @@ int clock_sntp_sync(clock_ctx *c, const char *server)
         tv.tv_usec = 0;
         setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, (char *)&tv, sizeof(tv));
 
+        /* Bind the datagram socket to the resolved server address/port so the
+         * stack drops any reply not actually from the server we queried —
+         * without this, recvfrom() would accept a UDP packet from anyone. */
+        if (connect(sock, (struct sockaddr *)&addr, sizeof(addr)) < 0)
+            break;
+
         t1 = (uint64_t)time(NULL);           /* local send time */
         clock_ntp_build_request(req, t1);
-        if (sendto(sock, (char *)req, NTP_PACKET_SIZE, 0,
-                   (struct sockaddr *)&addr, sizeof(addr)) != NTP_PACKET_SIZE)
+        if (send(sock, (char *)req, NTP_PACKET_SIZE, 0) != NTP_PACKET_SIZE)
             break;
 
-        if (recvfrom(sock, (char *)resp, NTP_PACKET_SIZE, 0, NULL, NULL)
-                != NTP_PACKET_SIZE)
+        if (recv(sock, (char *)resp, NTP_PACKET_SIZE, 0) != NTP_PACKET_SIZE)
             break;
         t4 = (uint64_t)time(NULL);           /* local receive time */
 

src/cli/main.c

@@ -656,7 +656,11 @@ static int cmd_add(const char *path, const char *uri)
         return 2;
     }
     rc = open_vault(&v, path);
-    if (rc != VAULT_OK) { fprintf(stderr, "AmiAuth: %s\n", vault_err(rc)); return 2; }
+    if (rc != VAULT_OK) {
+        memset(&acct, 0, sizeof(acct));
+        fprintf(stderr, "AmiAuth: %s\n", vault_err(rc));
+        return 2;
+    }
 
     rc = vault_add(&v, &acct);
     if (rc == VAULT_OK) rc = save_vault(&v, path);

src/core/hmac.c

@@ -30,6 +30,9 @@ void hmac_sha1_init(hmac_sha1_ctx *ctx, const uint8_t *key, size_t keylen)
 
     sha1_init(&ctx->inner);
     sha1_update(&ctx->inner, ipad, SHA1_BLOCK_SIZE);
+
+    memset(k, 0, sizeof(k));
+    memset(ipad, 0, sizeof(ipad));
 }
 
 void hmac_sha1_update(hmac_sha1_ctx *ctx, const void *data, size_t len)
@@ -86,6 +89,9 @@ void hmac_sha256_init(hmac_sha256_ctx *ctx, const uint8_t *key, size_t keylen)
 
     sha256_init(&ctx->inner);
     sha256_update(&ctx->inner, ipad, SHA256_BLOCK_SIZE);
+
+    memset(k, 0, sizeof(k));
+    memset(ipad, 0, sizeof(ipad));
 }
 
 void hmac_sha256_update(hmac_sha256_ctx *ctx, const void *data, size_t len)
@@ -142,6 +148,9 @@ void hmac_sha512_init(hmac_sha512_ctx *ctx, const uint8_t *key, size_t keylen)
 
     sha512_init(&ctx->inner);
     sha512_update(&ctx->inner, ipad, SHA512_BLOCK_SIZE);
+
+    memset(k, 0, sizeof(k));
+    memset(ipad, 0, sizeof(ipad));
 }
 
 void hmac_sha512_update(hmac_sha512_ctx *ctx, const void *data, size_t len)

src/core/pbkdf2.c

@@ -37,6 +37,11 @@ static void pbkdf2_block(const uint8_t *pass, size_t passlen,
     }
 
     memcpy(out, t, SHA1_DIGEST_SIZE);
+
+    memset(idx, 0, sizeof(idx));
+    memset(u, 0, sizeof(u));
+    memset(t, 0, sizeof(t));
+    memset(&c, 0, sizeof(c));
 }
 
 void pbkdf2_hmac_sha1(const uint8_t *pass, size_t passlen,
@@ -59,4 +64,6 @@ void pbkdf2_hmac_sha1(const uint8_t *pass, size_t passlen,
         got += n;
         blockidx++;
     }
+
+    memset(block, 0, sizeof(block));
 }