a73x

41c19dfb

sessions_reply gets one walk and one trust policy

a73x   2026-08-31 16:54

Commit message
sessions_reply gets one walk and one trust policy

Seven hand-rolled walks of a `sessions_reply` payload applied two different
trust policies. Four filtered through `validSessionName` (wall_host's birth
diff, wall_picker's row count, webhub's tile diff) and three did not
(mux_main's `countSessions`, client's `nextFreeName`, webhub's `listHas`).
`protocol.sessionsIter` is now the one reader-side half of a wire contract
that has no codec, and `sessionsHas` is the membership test built on it. The
two long trust-boundary comments — near-verbatim copies of each other in
webhub and wall_host — collapse to one-line pointers; the rationale lives
once, on the iterator.

What the verification found, since the reported hazard drove the change:

CONFIRMED, that the payload is untrusted plain text. `protocol.zig`'s
`sessions_reply` comment documents '\n'-separation and declines a codec, and
a frame is bounded only in TOTAL, so one "name" can be anything up to the
frame cap.

DOWNGRADED, the reported crash. The claim was that `nextFreeName` walks the
payload unfiltered and its result reaches `encodeAttachNamed`, which memcpys
a name into a 32-byte tail behind an assert. It does not: `nextFreeName`
only READS peer names to compare against a candidate it builds itself with
`bufPrint("{d}")` into a `session_name_max` buffer. No peer byte is ever
forwarded, and `wall_pump.postAnswer` re-validates through `client.validPick`
regardless. There was no reachable assert and no overflow.

DOWNGRADED further, even the miscount. Filtering cannot change what
`nextFreeName` returns for any input at all: a candidate is always a decimal
string, which `validSessionName` accepts, so no line the filter drops could
ever have matched one, and dropping lines only shrinks a search range whose
answer is already inside it. The conversion there buys agreement with the
other readers, not a fix.

Behavior notes. On any payload a real daemon sends, every site is byte-for-byte
unchanged — valid names in, identical results out. Two sites change on a
hostile or buggy payload, which is the point: `mux hosts` now prints the
number of sessions a wall could actually attach to rather than the number of
lines the daemon sent, and webhub's keep-check no longer lets an unspellable
line hold a tile alive. The existing `countSessions` test pins blank lines and
missing trailing newlines and passes unchanged.

Tests: +4 (1060/1060 over 74 steps, from 1056). `sessionsIter` and
`sessionsHas` pin the filter against a 1056-byte line, a line with a space, a
control byte and empty lines, and pin that a normal payload walks exactly as a
plain split would. The two new site tests pin the changed display and that a
hostile list cannot move the free name. Mutating the filter to accept
everything fails five tests by name, one for each converted policy site.

Gates: make check rc 0; zig build test rc 0; e2e 09_hosts (12 scenarios),
06_web (4) and 13_birth (5) each rc 0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
src/cli/mux_main.zig
Old New
@@ -526,11 +526,13 @@ fn refuseFile(arena: std.mem.Allocator, who: []const u8, path: []const u8, err:
526 return 2; 526 return 2;
527 } 527 }
528 528
529 /// Count newline-separated session names in a `sessions_reply`, with or without 529 /// Count the session names in a `sessions_reply`, with or without a trailing
530 /// a trailing newline. 530 /// newline. `proto.sessionsIter` skips blank lines and anything that is not a
531 /// spellable name, so the number printed beside a host is the number of
532 /// sessions a wall would actually put on screen for it.
531 fn countSessions(list: []const u8) usize { 533 fn countSessions(list: []const u8) usize {
532 var n: usize = 0; 534 var n: usize = 0;
533 var it = std.mem.tokenizeScalar(u8, list, '\n'); 535 var it = proto.sessionsIter(list);
534 while (it.next()) |_| n += 1; 536 while (it.next()) |_| n += 1;
535 return n; 537 return n;
536 } 538 }
@@ -900,6 +902,14 @@ test "hosts: a session count is the daemon's lines, not its bytes" {
900 try std.testing.expectEqual(@as(usize, 2), countSessions("0\n\nwork\n")); 902 try std.testing.expectEqual(@as(usize, 2), countSessions("0\n\nwork\n"));
901 } 903 }
902 904
905 test "hosts: a count shows sessions a wall could paint, not a daemon's lines" {
906 // `mux hosts` prints this number beside a host. A daemon that answered
907 // with an over-long line or a line holding a space named no session the
908 // wall can attach to, so the row must not advertise one.
909 try std.testing.expectEqual(@as(usize, 1), countSessions("0\n" ++ ("x" ** 1056)));
910 try std.testing.expectEqual(@as(usize, 2), countSessions("0\nhas space\nwork"));
911 }
912
903 test "parseArgs: unknown flags and valueless flags are usage errors" { 913 test "parseArgs: unknown flags and valueless flags are usage errors" {
904 try std.testing.expectError(error.Usage, parse(&.{ "mux", "--wat" })); 914 try std.testing.expectError(error.Usage, parse(&.{ "mux", "--wat" }));
905 try std.testing.expectError(error.Usage, parse(&.{ "mux", "-x" })); 915 try std.testing.expectError(error.Usage, parse(&.{ "mux", "-x" }));
src/client/client.zig
Old New
@@ -60,12 +60,19 @@ pub fn validPick(pick: ?[]const u8) ?SessionName {
60 /// The name `Ctrl-\ c` creates: the lowest non-negative integer not already a 60 /// The name `Ctrl-\ c` creates: the lowest non-negative integer not already a
61 /// session name, out of a `sessions_reply` payload. Numbering rather than 61 /// session name, out of a `sessions_reply` payload. Numbering rather than
62 /// `new-1` because the default session is already called "0". 62 /// `new-1` because the default session is already called "0".
63 ///
64 /// Both walks go through `proto.sessionsIter`, the one trust policy for that
65 /// payload. The answer it gives is unchanged by the filter — a candidate is
66 /// always a decimal string, which is a name `validSessionName` accepts, so no
67 /// line the filter drops could ever have matched one — but agreeing with the
68 /// wall's diff and the picker's count is what makes "the name the `c` chord
69 /// would have landed on" one sentence rather than three implementations.
63 pub fn nextFreeName(out: *[proto.session_name_max]u8, list: []const u8) []const u8 { 70 pub fn nextFreeName(out: *[proto.session_name_max]u8, list: []const u8) []const u8 {
64 // Each existing name can rule out at most one candidate, so the first 71 // Each existing name can rule out at most one candidate, so the first
65 // free number is somewhere in 0..count — no cap constant needed, and in 72 // free number is somewhere in 0..count — no cap constant needed, and in
66 // particular no duplicate of the daemon's max_sessions. 73 // particular no duplicate of the daemon's max_sessions.
67 var count: usize = 0; 74 var count: usize = 0;
68 var counter = std.mem.tokenizeScalar(u8, list, '\n'); 75 var counter = proto.sessionsIter(list);
69 while (counter.next()) |_| count += 1; 76 while (counter.next()) |_| count += 1;
70 77
71 var n: usize = 0; 78 var n: usize = 0;
@@ -73,7 +80,7 @@ pub fn nextFreeName(out: *[proto.session_name_max]u8, list: []const u8) []const
73 // A usize's widest decimal form is 20 bytes, session_name_max is 80 // A usize's widest decimal form is 20 bytes, session_name_max is
74 // 32: this print cannot overflow `out` for any n reachable here. 81 // 32: this print cannot overflow `out` for any n reachable here.
75 const cand = std.fmt.bufPrint(out, "{d}", .{n}) catch unreachable; 82 const cand = std.fmt.bufPrint(out, "{d}", .{n}) catch unreachable;
76 var names = std.mem.tokenizeScalar(u8, list, '\n'); 83 var names = proto.sessionsIter(list);
77 const taken = while (names.next()) |name| { 84 const taken = while (names.next()) |name| {
78 if (std.mem.eql(u8, name, cand)) break true; 85 if (std.mem.eql(u8, name, cand)) break true;
79 } else false; 86 } else false;
@@ -2655,6 +2662,22 @@ test "client: the new session's name is the lowest free integer" {
2655 try std.testing.expectEqualStrings("2", nextFreeName(&buf, "1\n0")); 2662 try std.testing.expectEqualStrings("2", nextFreeName(&buf, "1\n0"));
2656 } 2663 }
2657 2664
2665 test "client: a hostile session list cannot move the free name" {
2666 var buf: [proto.session_name_max]u8 = undefined;
2667 // A `sessions_reply` is bounded only in total, so a buggy or hostile
2668 // daemon can put an over-long line, a line with a space, or a blank line
2669 // in the list. `proto.sessionsIter` drops each; the free name is the same
2670 // one the honest prefix alone would have produced, because a candidate is
2671 // always a decimal string and no line the filter drops can equal one.
2672 const long = "x" ** 1056;
2673 try std.testing.expectEqualStrings("1", nextFreeName(&buf, "0\n" ++ long));
2674 try std.testing.expectEqualStrings("2", nextFreeName(&buf, "0\nhas space\n1"));
2675 try std.testing.expectEqualStrings("1", nextFreeName(&buf, "\n\n0\n\n"));
2676 // And the counting walk shrinking does not cut the search short: the
2677 // honest names still rule out their own numbers.
2678 try std.testing.expectEqualStrings("3", nextFreeName(&buf, "0\n" ++ long ++ "\n1\n2"));
2679 }
2680
2658 test "client: a picked name off the wire is validated before it is copied" { 2681 test "client: a picked name off the wire is validated before it is copied" {
2659 // The reason the guard exists: `SessionName.of` memcpys into a 2682 // The reason the guard exists: `SessionName.of` memcpys into a
2660 // `session_name_max` buffer, so this name is an out-of-bounds write 2683 // `session_name_max` buffer, so this name is an out-of-bounds write
src/client/webhub.zig
Old New
@@ -160,14 +160,10 @@ pub const Hub = struct {
160 // failed poll would tear a wall down over one dropped packet, and 160 // failed poll would tear a wall down over one dropped packet, and
161 // spending the grace on silence would do it one poll later. 161 // spending the grace on silence would do it one poll later.
162 if (!reachable) return; 162 if (!reachable) return;
163 var it = std.mem.splitScalar(u8, list, '\n'); 163 // `proto.sessionsIter` carries the trust policy: a peer's reply is
164 // bounded only in total, so it yields only lines that are names.
165 var it = proto.sessionsIter(list);
164 while (it.next()) |name| { 166 while (it.next()) |name| {
165 if (name.len == 0) continue;
166 // The trust boundary: a peer's reply is bounded only in TOTAL,
167 // so one "name" in it can be 1056 bytes of anything.
168 // `encodeAttachNamed` memcpys a name into a 32-byte tail behind
169 // an assert, which states a bug rather than filtering input.
170 if (!proto.validSessionName(name)) continue;
171 if (self.find(host_idx, name) != null) continue; 167 if (self.find(host_idx, name) != null) continue;
172 self.birth(host_idx, name) catch continue; 168 self.birth(host_idx, name) catch continue;
173 } 169 }
@@ -180,7 +176,7 @@ pub const Hub = struct {
180 i += 1; 176 i += 1;
181 continue; 177 continue;
182 } 178 }
183 if (listHas(list, t.session)) { 179 if (proto.sessionsHas(list, t.session)) {
184 t.missed_once = false; 180 t.missed_once = false;
185 i += 1; 181 i += 1;
186 continue; 182 continue;
@@ -325,14 +321,6 @@ pub const Hub = struct {
325 } 321 }
326 }; 322 };
327 323
328 /// A '\n'-separated `sessions_reply` payload, asked whether it holds one
329 /// name. Never `indexOf`: `w` is in `work` and neither is the other.
330 fn listHas(list: []const u8, name: []const u8) bool {
331 var it = std.mem.splitScalar(u8, list, '\n');
332 while (it.next()) |n| if (std.mem.eql(u8, n, name)) return true;
333 return false;
334 }
335
336 /// The embedded page: webhub_main @embedFiles them, tests inject fakes. 324 /// The embedded page: webhub_main @embedFiles them, tests inject fakes.
337 pub const Assets = struct { 325 pub const Assets = struct {
338 index_html: []const u8, 326 index_html: []const u8,
src/engine/protocol.zig
Old New
@@ -44,7 +44,7 @@ pub const MsgType = enum(u8) {
44 term_title = 0x8e, // payload: UTF-8 title bytes, never empty (see sampleTermTitle), never longer than term_title_max 44 term_title = 0x8e, // payload: UTF-8 title bytes, never empty (see sampleTermTitle), never longer than term_title_max
45 term_event = 0x8f, // payload: 1 byte kind ++ kind-specific bytes (see TermEvent) 45 term_event = 0x8f, // payload: 1 byte kind ++ kind-specific bytes (see TermEvent)
46 selection_reply = 0x90, // payload: SelectionReply (see encodeSelectionReply) 46 selection_reply = 0x90, // payload: SelectionReply (see encodeSelectionReply)
47 sessions_reply = 0x91, // payload: the live session names, '\n'-separated, in slot order; empty payload = no sessions. A name can hold no whitespace (validSessionName), so the separator needs no escaping and no codec — the same reason stats_reply is plain text. 47 sessions_reply = 0x91, // payload: the live session names, '\n'-separated, in slot order; empty payload = no sessions. A name can hold no whitespace (validSessionName), so the separator needs no escaping and no codec — the same reason stats_reply is plain text. Readers walk it with sessionsIter, which is where the trust policy lives.
48 agent_open = 0x92, // payload: u32 LE channel id; daemon allocates ids, only the daemon opens 48 agent_open = 0x92, // payload: u32 LE channel id; daemon allocates ids, only the daemon opens
49 upgrade_reply = 0x93, // payload: u8 status (0 accepted, 1 refused) ++ reason text 49 upgrade_reply = 0x93, // payload: u8 status (0 accepted, 1 refused) ++ reason text
50 end_reply = 0x94, // payload: u8 status (0 accepted, 1 refused) ++ u8 others ++ reason text 50 end_reply = 0x94, // payload: u8 status (0 accepted, 1 refused) ++ u8 others ++ reason text
@@ -836,6 +836,52 @@ pub fn validSessionName(name: []const u8) bool {
836 return true; 836 return true;
837 } 837 }
838 838
839 /// Walk a `sessions_reply` payload: yields each line that is a valid session
840 /// name, skipping empties and anything `validSessionName` refuses — the reply
841 /// crossed a trust boundary and one "name" in it can be anything the daemon
842 /// put there, up to the whole frame cap. The payload is plain '\n'-separated
843 /// text with no codec (see `MsgType.sessions_reply`), so this iterator IS the
844 /// reader's half of that contract, and every reader shares it: the wall's
845 /// diff, the picker's count, the hub's tile diff, `mux hosts`, and the free-name
846 /// search all agree on which lines are names.
847 ///
848 /// What the filter prevents, concretely: `encodeAttachNamed`, `encodeEndReq`
849 /// and `encodeDebugDumpNamed` each memcpy a name into a `session_name_max`
850 /// tail behind an assert — an assert states a bug in the code that built the
851 /// name, it does not filter a peer's input. Names taken from here are safe to
852 /// carry to those encoders; lines this iterator skipped never were.
853 pub fn sessionsIter(payload: []const u8) SessionsIter {
854 return .{ .rest = payload };
855 }
856
857 pub const SessionsIter = struct {
858 rest: []const u8,
859
860 pub fn next(self: *SessionsIter) ?[]const u8 {
861 while (self.rest.len != 0) {
862 const line = if (std.mem.indexOfScalar(u8, self.rest, '\n')) |nl| blk: {
863 const l = self.rest[0..nl];
864 self.rest = self.rest[nl + 1 ..];
865 break :blk l;
866 } else blk: {
867 const l = self.rest;
868 self.rest = self.rest[self.rest.len..];
869 break :blk l;
870 };
871 if (validSessionName(line)) return line;
872 }
873 return null;
874 }
875 };
876
877 /// Whether a `sessions_reply` payload names this session. Never `indexOf`:
878 /// `w` is in `work` and neither is the other.
879 pub fn sessionsHas(payload: []const u8, name: []const u8) bool {
880 var it = sessionsIter(payload);
881 while (it.next()) |n| if (std.mem.eql(u8, n, name)) return true;
882 return false;
883 }
884
839 /// A `--session` field of this type is refused at the parse, so no caller 885 /// A `--session` field of this type is refused at the parse, so no caller
840 /// carries an unspellable name as far as the wire. 886 /// carries an unspellable name as far as the wire.
841 pub const SessionName = struct { 887 pub const SessionName = struct {
@@ -1321,6 +1367,59 @@ test "session names: 1..32 printable bytes, no space, no '#', no '/'" {
1321 try std.testing.expect(!validSessionName("ctrl\x01")); 1367 try std.testing.expect(!validSessionName("ctrl\x01"));
1322 } 1368 }
1323 1369
1370 test "sessionsIter: one walk, one trust policy, over a hostile payload" {
1371 const collect = struct {
1372 fn f(payload: []const u8, out: *[8][]const u8) usize {
1373 var n: usize = 0;
1374 var it = sessionsIter(payload);
1375 while (it.next()) |name| : (n += 1) out[n] = name;
1376 return n;
1377 }
1378 }.f;
1379 var got: [8][]const u8 = undefined;
1380
1381 // A normal payload walks exactly as a plain '\n' split would: this is the
1382 // pin that converting the readers changed nothing for a real daemon.
1383 try std.testing.expectEqual(@as(usize, 3), collect("0\nwork\ndev", &got));
1384 try std.testing.expectEqualStrings("0", got[0]);
1385 try std.testing.expectEqualStrings("work", got[1]);
1386 try std.testing.expectEqualStrings("dev", got[2]);
1387 // A trailing newline is a separator, not a fourth empty session.
1388 try std.testing.expectEqual(@as(usize, 3), collect("0\nwork\ndev\n", &got));
1389 try std.testing.expectEqual(@as(usize, 0), collect("", &got));
1390 try std.testing.expectEqual(@as(usize, 0), collect("\n\n\n", &got));
1391
1392 // The trust boundary. A `sessions_reply` is bounded only in TOTAL, so a
1393 // hostile or buggy daemon can put a 1056-byte "name", a name with a
1394 // space, or a control byte on one line. Each is skipped and the names
1395 // around it still arrive: a bad line costs its own row, never the list.
1396 const long = "x" ** 1056;
1397 try std.testing.expectEqual(@as(usize, 2), collect("0\n" ++ long ++ "\nwork", &got));
1398 try std.testing.expectEqualStrings("0", got[0]);
1399 try std.testing.expectEqualStrings("work", got[1]);
1400 try std.testing.expectEqual(@as(usize, 2), collect("0\nhas space\nwork", &got));
1401 try std.testing.expectEqualStrings("work", got[1]);
1402 try std.testing.expectEqual(@as(usize, 1), collect("bad\x01name\n0", &got));
1403 try std.testing.expectEqualStrings("0", got[0]);
1404
1405 // Every name it yields fits the tail `encodeAttachNamed` asserts on, so a
1406 // caller may hand one straight to the encoder.
1407 var it = sessionsIter("0\n" ++ long ++ "\nwork\n");
1408 while (it.next()) |name| try std.testing.expect(name.len <= session_name_max);
1409 }
1410
1411 test "sessionsHas: a membership test that a hostile line cannot answer" {
1412 try std.testing.expect(sessionsHas("0\nwork", "work"));
1413 try std.testing.expect(sessionsHas("0\nwork\n", "0"));
1414 // Never `indexOf`: `w` is in `work` and neither is the other.
1415 try std.testing.expect(!sessionsHas("0\nwork", "w"));
1416 try std.testing.expect(!sessionsHas("0\nwork", "workshop"));
1417 // A line the iterator refuses is not a session, so nothing matches it —
1418 // a daemon cannot keep a tile alive by naming it with an unspellable line.
1419 try std.testing.expect(!sessionsHas("has space\n0", "has space"));
1420 try std.testing.expect(!sessionsHas("", ""));
1421 }
1422
1324 test "delta build/iterate round trip" { 1423 test "delta build/iterate round trip" {
1325 const alloc = std.testing.allocator; 1424 const alloc = std.testing.allocator;
1326 var payload: std.ArrayList(u8) = .empty; 1425 var payload: std.ArrayList(u8) = .empty;
src/tui/wall_host.zig
Old New
@@ -184,14 +184,11 @@ pub fn planHostDiff(
184 binds: *TileIdxs, 184 binds: *TileIdxs,
185 vanish: *TileIdxs, 185 vanish: *TileIdxs,
186 ) void { 186 ) void {
187 var it = std.mem.splitScalar(u8, list, '\n'); 187 // `proto.sessionsIter` carries the trust policy: a peer's reply is bounded
188 // only in total, so it yields only lines that are names — which is what
189 // makes a birth's name safe to hand to `encodeAttachNamed`.
190 var it = proto.sessionsIter(list);
188 while (it.next()) |name| { 191 while (it.next()) |name| {
189 if (name.len == 0) continue;
190 // The trust boundary: a peer's reply is bounded only in TOTAL, so
191 // one "name" in it can be 1056 bytes of anything. `encodeAttachNamed`
192 // and `encodeEndReq` memcpy a birth's name into a 32-byte tail
193 // behind an assert, which states a bug rather than filtering input.
194 if (!proto.validSessionName(name)) continue;
195 if (self_name) |self| if (std.mem.eql(u8, self, name)) continue; 192 if (self_name) |self| if (std.mem.eql(u8, self, name)) continue;
196 var found = false; 193 var found = false;
197 for (tiles[0..live], present[0..live], 0..) |*t, p, i| { 194 for (tiles[0..live], present[0..live], 0..) |*t, p, i| {
@@ -211,14 +208,9 @@ pub fn planHostDiff(
211 // daemon does not have yet, not one it dropped — and `pokeHost` 208 // daemon does not have yet, not one it dropped — and `pokeHost`
212 // asks for this list at exactly that moment. 209 // asks for this list at exactly that moment.
213 if (t.creates and !t.ever_up.load(.acquire)) continue; 210 if (t.creates and !t.ever_up.load(.acquire)) continue;
214 var keep = false; 211 // The same walk the births came out of: a tile's own name was a name
215 var it2 = std.mem.splitScalar(u8, list, '\n'); 212 // when it was born, so filtering the list cannot drop a real match.
216 while (it2.next()) |name| { 213 const keep = proto.sessionsHas(list, proto.resolveName(t.r.session));
217 if (std.mem.eql(u8, proto.resolveName(t.r.session), name)) {
218 keep = true;
219 break;
220 }
221 }
222 if (keep) { 214 if (keep) {
223 t.missed_once = false; 215 t.missed_once = false;
224 continue; 216 continue;
src/tui/wall_picker.zig
Old New
@@ -72,13 +72,10 @@ pub fn hostState(buf: []u8, h: *Host) []const u8 {
72 } 72 }
73 var n: usize = 0; 73 var n: usize = 0;
74 h.poll.list_mu.lock(); 74 h.poll.list_mu.lock();
75 var it = std.mem.splitScalar(u8, h.poll.list[0..h.poll.list_len], '\n'); 75 // The same walk `planHostDiff` births through, so the count a row
76 while (it.next()) |name| { 76 // advertises is the number of tiles that host can actually put on the wall.
77 // The same filter `planHostDiff` births through, so the count a row 77 var it = proto.sessionsIter(h.poll.list[0..h.poll.list_len]);
78 // advertises is the number of tiles that host can actually put on 78 while (it.next()) |_| n += 1;
79 // the wall.
80 if (proto.validSessionName(name)) n += 1;
81 }
82 h.poll.list_mu.unlock(); 79 h.poll.list_mu.unlock();
83 if (n == 0) return "no sessions"; 80 if (n == 0) return "no sessions";
84 if (n == 1) return "1 session"; 81 if (n == 1) return "1 session";