Skip to content

Commit 5f2ae69

Browse files
zaxifiedclaude
andcommitted
example-apps: review of the audit fixes — one was a regression, one was half a fix
Re-checking the audit commits by ATTACK and MUTATION rather than by "the gate is green" found two defects in the fixes themselves. Both are corrected here. 1. The watchdog change was a REGRESSION. Replacing `kill -9 $$` with `kill -TERM $$` plus a TERM trap does nothing in the case a watchdog exists for: bash DEFERS a trapped signal until the running foreground command returns, and in a real hang that is never. Measured on a copy of a real smoke script — watchdog fired at t=2 s, the trap ran at t=30 s, only once the blocking command ended by itself. So the previous version at least killed the script; mine killed nothing. Cleanup is now the WATCHDOG's job, since the script provably cannot do it: it kills the script's other children (`pgrep -P $$`, skipping itself), runs `cleanup`, and only then SIGKILLs the script. Verified end to end against an injected foreground hang: timed out on schedule, no orphaned processes, temp dir removed. (`kill -9 -$$` remains out: a smoke shares check-apps.sh's process group and would take the gate with it.) 2. The timecapsule overflow fix only MOVED the panic. Saturating `publishTime` left `formatUtc` casting a saturated timestamp's year into a u32, so a capsule claiming round 2^63 still aborted `info` with SIGABRT. The lesson is the general one: a clamp is not a fix until every consumer of the clamped value is total too. `formatUtc` now refuses to render outside the printable calendar. Verified by attack (rounds 0, 2^63, 2^64-1 → exit 1/3, never 134) and pinned by a new smoke case that is mutation-checked: with the guard removed the smoke fails with exactly that panic. Also, smaller, from the same pass: - getTask wrote its 404 through `jsonError` while holding the lock — the very pattern the audit fixed elsewhere, and inconsistent with every sibling handler, all of which unlock first. Now it unlocks first too. - raft-kv's smoke cleanup named `n*/raft.kv` and `*.log`, so `raft.kv.lock` and `status.out` kept a temp directory alive after every run (two per gate pass). It now clears the directory it made. - ssh-demo's new exclusive create turned a stale temp file into a permanent failure once a pid was reused; it unlinks first, which does not weaken the symlink guard (unlink removes the link, and a re-planted one still fails `exclusive`). - timecapsule's README records that keygen refuses to overwrite a keypair. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 6821243 commit 5f2ae69

9 files changed

Lines changed: 129 additions & 39 deletions

File tree

example-apps/http-service/smoke.sh

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,20 @@ cleanup() {
3434
rmdir "$WORK" 2>/dev/null || true
3535
}
3636
trap cleanup EXIT
37-
# A timeout must run cleanup, not just die: SIGKILL on $$ cannot be trapped
38-
# and would orphan the child processes (still bound to the port) and $WORK.
39-
# The watchdog sends SIGTERM instead; this handler prints and exits, so the
40-
# EXIT trap above fires and kills the tracked PIDs.
41-
trap 'echo "smoke: TIMED OUT" >&2; exit 124' TERM
4237

43-
( sleep 60; kill -TERM $$ 2>/dev/null ) &
38+
( sleep 60
39+
echo "smoke: TIMED OUT" >&2
40+
# ⚠ CLEANUP IS THE WATCHDOG'S JOB ON THIS PATH, because the script provably
41+
# cannot do it. SIGKILL cannot be trapped, and a SIGTERM trap is DEFERRED
42+
# until the foreground command returns — which, in the hang a watchdog exists
43+
# for, is never. Measured: watchdog fired at t=2 s, the TERM trap ran at
44+
# t=30 s, only once the blocking command ended by itself. So kill the
45+
# script's other children here, run cleanup here, and SIGKILL the script
46+
# last. (`kill -9 -$$` is not an option: a smoke test shares check-apps.sh's
47+
# process group, so that would kill the gate too.)
48+
for p in $(pgrep -P $$ 2>/dev/null); do [ "$p" = "$BASHPID" ] || kill -9 "$p" 2>/dev/null; done
49+
cleanup
50+
kill -9 $$ 2>/dev/null ) &
4451
PIDS+=($!)
4552

4653
fail() {

example-apps/http-service/src/main.zig

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -315,15 +315,21 @@ fn getTask(ctx: *router.Ctx) anyerror!void {
315315
const id = parseId(ctx) orelse return jsonError(ctx, 400, "bad task id");
316316
var aw: std.Io.Writer.Allocating = .init(app.gpa);
317317
defer aw.deinit();
318-
{
319-
appLock(app);
320-
defer app.lock.unlock();
321-
const idx = findTaskIndex(app, id) orelse return jsonError(ctx, 404, "no such task");
322-
// Render under the lock (a concurrent DELETE could free the title);
323-
// write to the socket after unlocking, same reason as listTasks.
324-
var jw: std.json.Stringify = .{ .writer = &aw.writer, .options = .{} };
325-
try writeTaskJson(&jw, &app.tasks.items[idx]);
326-
}
318+
// Render under the lock (a concurrent DELETE could free the title); every
319+
// response byte — including the 404 — is written after unlocking, which is
320+
// what the other handlers do and what makes the rule easy to keep.
321+
appLock(app);
322+
const idx = findTaskIndex(app, id) orelse {
323+
app.lock.unlock();
324+
return jsonError(ctx, 404, "no such task");
325+
};
326+
var jw: std.json.Stringify = .{ .writer = &aw.writer, .options = .{} };
327+
writeTaskJson(&jw, &app.tasks.items[idx]) catch |err| {
328+
app.lock.unlock();
329+
return err;
330+
};
331+
app.lock.unlock();
332+
327333
ctx.res.setStatus(200);
328334
try ctx.res.setHeader("Content-Type", "application/json");
329335
try ctx.res.writer().writeAll(aw.written());

example-apps/mls-chat/smoke.sh

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,15 +22,22 @@ cleanup() {
2222
rmdir "$WORK" 2>/dev/null || true
2323
}
2424
trap cleanup EXIT
25-
# A timeout must run cleanup, not just die: SIGKILL on $$ cannot be trapped
26-
# and would orphan the child processes (still bound to the port) and $WORK.
27-
# The watchdog sends SIGTERM instead; this handler prints and exits, so the
28-
# EXIT trap above fires and kills the tracked PIDs.
29-
trap 'echo "smoke: TIMED OUT" >&2; exit 124' TERM
3025

3126
# A watchdog, because every failure mode of a multi-process script that is not
3227
# an assertion is a hang. 60s is ~20x the run.
33-
( sleep 60; kill -TERM $$ 2>/dev/null ) &
28+
( sleep 60
29+
echo "smoke: TIMED OUT" >&2
30+
# ⚠ CLEANUP IS THE WATCHDOG'S JOB ON THIS PATH, because the script provably
31+
# cannot do it. SIGKILL cannot be trapped, and a SIGTERM trap is DEFERRED
32+
# until the foreground command returns — which, in the hang a watchdog exists
33+
# for, is never. Measured: watchdog fired at t=2 s, the TERM trap ran at
34+
# t=30 s, only once the blocking command ended by itself. So kill the
35+
# script's other children here, run cleanup here, and SIGKILL the script
36+
# last. (`kill -9 -$$` is not an option: a smoke test shares check-apps.sh's
37+
# process group, so that would kill the gate too.)
38+
for p in $(pgrep -P $$ 2>/dev/null); do [ "$p" = "$BASHPID" ] || kill -9 "$p" 2>/dev/null; done
39+
cleanup
40+
kill -9 $$ 2>/dev/null ) &
3441
WATCHDOG=$!
3542
PIDS+=("$WATCHDOG")
3643

example-apps/raft-kv/smoke.sh

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,19 +19,30 @@ WORK="$(mktemp -d)"
1919
PIDS=()
2020
cleanup() {
2121
for pid in ${PIDS+"${PIDS[@]}"}; do kill -9 "$pid" 2>/dev/null || true; done
22-
rm -f "$WORK"/n*/raft.kv "$WORK"/*.log 2>/dev/null || true
22+
# Everything, not a named subset: the node dirs also hold `raft.kv.lock`,
23+
# and $WORK holds `status.out` — naming only `raft.kv` and `*.log` left a
24+
# temp directory behind on EVERY run (measured: one per smoke, so two per
25+
# gate pass). Per-file rm + rmdir, never `rm -rf` on a variable.
26+
rm -f "$WORK"/n*/* "$WORK"/* 2>/dev/null || true
2327
rmdir "$WORK"/n* "$WORK" 2>/dev/null || true
2428
}
2529
trap cleanup EXIT
26-
# A timeout must run cleanup, not just die: SIGKILL on $$ cannot be trapped
27-
# and would orphan the child processes (still bound to the port) and $WORK.
28-
# The watchdog sends SIGTERM instead; this handler prints and exits, so the
29-
# EXIT trap above fires and kills the tracked PIDs.
30-
trap 'echo "smoke: TIMED OUT" >&2; exit 124' TERM
3130

3231
# Everything here is bounded by client budgets, so anything long-running is a
3332
# hang. 90s is ~3x a slow full run.
34-
( sleep 90; kill -TERM $$ 2>/dev/null ) &
33+
( sleep 90
34+
echo "smoke: TIMED OUT" >&2
35+
# ⚠ CLEANUP IS THE WATCHDOG'S JOB ON THIS PATH, because the script provably
36+
# cannot do it. SIGKILL cannot be trapped, and a SIGTERM trap is DEFERRED
37+
# until the foreground command returns — which, in the hang a watchdog exists
38+
# for, is never. Measured: watchdog fired at t=2 s, the TERM trap ran at
39+
# t=30 s, only once the blocking command ended by itself. So kill the
40+
# script's other children here, run cleanup here, and SIGKILL the script
41+
# last. (`kill -9 -$$` is not an option: a smoke test shares check-apps.sh's
42+
# process group, so that would kill the gate too.)
43+
for p in $(pgrep -P $$ 2>/dev/null); do [ "$p" = "$BASHPID" ] || kill -9 "$p" 2>/dev/null; done
44+
cleanup
45+
kill -9 $$ 2>/dev/null ) &
3546
WATCHDOG=$!
3647
PIDS+=("$WATCHDOG")
3748

example-apps/ssh-demo/smoke.sh

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,20 @@ cleanup() {
2626
rmdir "$WORK" 2>/dev/null || true
2727
}
2828
trap cleanup EXIT
29-
# A timeout must run cleanup, not just die: SIGKILL on $$ cannot be trapped
30-
# and would orphan the child processes (still bound to the port) and $WORK.
31-
# The watchdog sends SIGTERM instead; this handler prints and exits, so the
32-
# EXIT trap above fires and kills the tracked PIDs.
33-
trap 'echo "smoke: TIMED OUT" >&2; exit 124' TERM
3429

35-
( sleep 60; kill -TERM $$ 2>/dev/null ) &
30+
( sleep 60
31+
echo "smoke: TIMED OUT" >&2
32+
# ⚠ CLEANUP IS THE WATCHDOG'S JOB ON THIS PATH, because the script provably
33+
# cannot do it. SIGKILL cannot be trapped, and a SIGTERM trap is DEFERRED
34+
# until the foreground command returns — which, in the hang a watchdog exists
35+
# for, is never. Measured: watchdog fired at t=2 s, the TERM trap ran at
36+
# t=30 s, only once the blocking command ended by itself. So kill the
37+
# script's other children here, run cleanup here, and SIGKILL the script
38+
# last. (`kill -9 -$$` is not an option: a smoke test shares check-apps.sh's
39+
# process group, so that would kill the gate too.)
40+
for p in $(pgrep -P $$ 2>/dev/null); do [ "$p" = "$BASHPID" ] || kill -9 "$p" 2>/dev/null; done
41+
cleanup
42+
kill -9 $$ 2>/dev/null ) &
3643
PIDS+=($!)
3744

3845
fail() {

example-apps/ssh-demo/src/main.zig

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1314,6 +1314,13 @@ const Commands = struct {
13141314
const p = std.fmt.bufPrint(&stdin_path_buf, "/tmp/ssh-demo-stdin-{d}-{d}", .{
13151315
std.os.linux.getpid(), self.runs,
13161316
}) catch unreachable;
1317+
// Clear a stale file first. A previous incarnation holding this pid
1318+
// may have been SIGKILLed before its deferred delete ran, and an
1319+
// exclusive create would then fail on that path forever. Unlinking
1320+
// does not weaken the guard above: it removes the LINK, never its
1321+
// target, and a symlink re-planted between the two calls still
1322+
// meets `exclusive` and is refused.
1323+
std.Io.Dir.cwd().deleteFile(self.io, p) catch {};
13171324
var f = std.Io.Dir.cwd().createFile(self.io, p, .{ .truncate = true, .exclusive = true }) catch |err| {
13181325
std.debug.print("ssh-demo: cannot create stdin temp {s}: {t}\n", .{ p, err });
13191326
return err;

example-apps/timecapsule/README.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@ Then:
4040

4141
```sh
4242
./zig-out/bin/timecapsule keygen --out alice # alice.pk (share) + alice.sk (keep)
43+
# keygen refuses if alice.pk/alice.sk already exist — overwriting a secret key
44+
# orphans every capsule sealed to it, so it is never done silently.
4345
echo "the eagle lands at midnight" > msg.txt
4446
./zig-out/bin/timecapsule seal --to alice.pk --at +2m --in msg.txt --out msg.tc
4547
./zig-out/bin/timecapsule open --key alice.sk --in msg.tc --out msg.out

example-apps/timecapsule/smoke.sh

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,22 @@ cleanup() {
2323
rmdir "$WORK" 2>/dev/null || true
2424
}
2525
trap cleanup EXIT
26-
# A timeout must run cleanup, not just die: SIGKILL on $$ cannot be trapped and
27-
# would orphan the `open --wait` child and $WORK. The watchdog sends SIGTERM;
28-
# this handler exits so the EXIT trap fires.
29-
trap 'echo "smoke: TIMED OUT" >&2; exit 124' TERM
3026

3127
# A watchdog: the app is offline here (every beacon document comes from a
3228
# file), so anything long-running is a hang, not a download. 60s is ~20x.
33-
( sleep 60; kill -TERM $$ 2>/dev/null ) &
29+
( sleep 60
30+
echo "smoke: TIMED OUT" >&2
31+
# ⚠ CLEANUP IS THE WATCHDOG'S JOB ON THIS PATH, because the script provably
32+
# cannot do it. SIGKILL cannot be trapped, and a SIGTERM trap is DEFERRED
33+
# until the foreground command returns — which, in the hang a watchdog exists
34+
# for, is never. Measured: watchdog fired at t=2 s, the TERM trap ran at
35+
# t=30 s, only once the blocking command ended by itself. So kill the
36+
# script's other children here, run cleanup here, and SIGKILL the script
37+
# last. (`kill -9 -$$` is not an option: a smoke test shares check-apps.sh's
38+
# process group, so that would kill the gate too.)
39+
for p in $(pgrep -P $$ 2>/dev/null); do [ "$p" = "$BASHPID" ] || kill -9 "$p" 2>/dev/null; done
40+
cleanup
41+
kill -9 $$ 2>/dev/null ) &
3442
WATCHDOG=$!
3543
disown "$WATCHDOG"
3644

@@ -128,4 +136,25 @@ wait "$WAITER"
128136
grep -q "waiting — round 1000" wait.log || fail "open --wait never announced it was waiting — did it wait at all?"
129137
cmp -s msg.txt waited.out || fail "--wait plaintext differs"
130138

139+
# ── 6. a HOSTILE capsule must not crash the process ────────────────────────
140+
# The envelope's round field is 8 attacker-written bytes that no signature
141+
# covers on this path (`info` never reaches the AEAD tag). Patch a real capsule
142+
# to claim round 2^64-1 and to claim round 0. Both must produce a typed refusal
143+
# or a "still locked" verdict — never a panic. This case exists because the
144+
# first fix for it (saturating arithmetic) only MOVED the abort into the date
145+
# formatter, and nothing here would have noticed.
146+
# Offset 44 = capsule header (37) + the envelope's round_off (7).
147+
for hostile in ffffffffffffffff 0000000000000000; do
148+
cp msg.tc hostile.tc
149+
printf "$(echo "$hostile" | sed 's/../\\x&/g')" | dd of=hostile.tc bs=1 seek=44 conv=notrunc 2>/dev/null
150+
set +e
151+
"$BIN" info --in hostile.tc "${OFFLINE[@]}" > hostile.out 2>&1
152+
rc=$?
153+
set -e
154+
case "$rc" in
155+
1|3) ;;
156+
*) fail "a capsule claiming round 0x$hostile made \`info\` exit $rc (134 = panic); output: $(cat hostile.out)" ;;
157+
esac
158+
done
159+
131160
echo "smoke: OK"

example-apps/timecapsule/src/beacon.zig

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,8 +108,22 @@ pub fn wallNow() i64 {
108108

109109
/// `1997-08-29 02:14:00 UTC` — `datefmt`'s civil-calendar core does the
110110
/// date math; rendering stays here so the buffer is the caller's.
111+
///
112+
/// ⚠ TOTAL BY CONSTRUCTION. `unix` reaches here from `publishTime`, whose input
113+
/// round comes straight off a capsule an attacker writes, so it can be the
114+
/// saturated `i64` maximum. The civil conversion then yields a year no `u32`
115+
/// cast survives, and this aborted the process. Making `publishTime` saturating
116+
/// was not enough on its own — it only MOVED the panic here, which is the
117+
/// lesson worth keeping: a clamp is not a fix until every consumer of the
118+
/// clamped value is total too. Outside the printable calendar, say so instead.
119+
const min_printable_unix: i64 = -62135596800; // 0001-01-01T00:00:00Z
120+
const max_printable_unix: i64 = 253402300799; // 9999-12-31T23:59:59Z
121+
111122
pub fn formatUtc(buf: []u8, unix: i64) []const u8 {
123+
if (unix < min_printable_unix) return "before year 1 (malformed capsule)";
124+
if (unix > max_printable_unix) return "after year 9999 (malformed capsule)";
112125
const p = datefmt.unixToParts(unix);
126+
// Now in 1..9999 by the guard above, so the cast cannot trap.
113127
return std.fmt.bufPrint(buf, "{d:0>4}-{d:0>2}-{d:0>2} {d:0>2}:{d:0>2}:{d:0>2} UTC", .{
114128
@as(u32, @intCast(p.year)), p.month, p.day, p.hour, p.minute, p.second,
115129
}) catch unreachable;

0 commit comments

Comments
 (0)