From 4fcd8512ae6cd3ebe6d041f0f862e87f6d5c5902 Mon Sep 17 00:00:00 2001 From: "Claude (noether)" Date: Thu, 23 Jul 2026 12:06:58 +0200 Subject: [PATCH] sicd: canonical argc + wire-spec doc rot (reviewer 964 re-probe #1/#2/#3) @reviewer re-probed the argc path (18 hand-built frames, all rejection paths clean) and cleared it as deployable. Three non-blocking follow-ups, all closed here: #1 doc rot -- the comments ARE the spec on a hand-rolled wire, and three still described removed formats: readNetstringStream (claimed v1-only; it now serves both paths), parseV2Content doc (empty-netstring terminator), runV2 doc (the two-generations-old command/NUL/payload split), and the test frame() helper. All rewritten to netstring(argc) + argc argv netstrings + payload. #2 canonical argc -- strconv.Atoi accepted "+1" and "001" (one value, two encodings). Reject leading +/-, leading zeros, and empty before Atoi, mirroring readNetstringStream length-token validation. New regression TestV2NonCanonicalArgcRejected (01/+1/-1 etc. exit 1, no exec). #3 argc-low reclassification -- documented where payload is defined: a low argc turns the sender's own trailing netstrings into child stdin; only ever the sender frame, no trust boundary. By design, now stated. Also fixed TestV2FrameElementCapRejected, which under the argc wire was tripping on a bad argc token rather than the count cap it claims -- now sends argc=5000 to hit maxArgs honestly. Build/vet clean, full daemon + client suites green. Daemon is deploy-ready at this commit. Co-Authored-By: Claude Opus 4.8 --- cmd/sicd/main.go | 31 ++++++++++++++++++++++++------- cmd/sicd/sicd_test.go | 25 +++++++++++++++++++------ 2 files changed, 43 insertions(+), 13 deletions(-) diff --git a/cmd/sicd/main.go b/cmd/sicd/main.go index 3d6fb20..7443077 100644 --- a/cmd/sicd/main.go +++ b/cmd/sicd/main.go @@ -48,8 +48,9 @@ func readNetstring(buf []byte) ([]byte, bool) { // readNetstringStream reads ONE djb netstring (:,) from a stream (unlike // readNetstring, which parses a single fully-buffered frame). ok=false on EOF or malformed -// framing (non-digit length, short body, missing comma, length > 1<<24). Used by the v1 path, -// which is a SEQUENCE of netstrings terminated by the empty netstring 0:,. +// framing (non-digit length, short body, missing comma, length > 1<<24). Used by BOTH wire +// paths: v1 (a sequence of netstrings terminated by the empty netstring 0:,) and v2 (the argc +// count plus each argv netstring, inside parseV2Content). func readNetstringStream(r *bufio.Reader) ([]byte, bool) { // Read the length token digit-by-digit with a HARD cap. This must be bounded BEFORE the // value is trusted: bufio.ReadString(':') buffers the whole pre-':' run unbounded, so an @@ -91,10 +92,14 @@ func readNetstringStream(r *bufio.Reader) ([]byte, bool) { return content, true } -// parseV2Content splits a v2 netstring's content into argv (a run of netstrings terminated by -// the empty netstring 0:,) and the trailing payload. Length-framed argv preserves argument -// boundaries and lets any byte (space, NUL, 0xFF) appear in an argument; readNetstringStream -// bounds each element and its length token, so a hostile content cannot OOM here. +// parseV2Content splits a v2 netstring's content into argv and the trailing payload. Content is +// netstring(argc) + exactly argc argv netstrings + payload; reading a FIXED count (not an empty- +// netstring terminator) makes an empty "" a representable argument (reviewer 964 #1). Length-framed +// argv preserves argument boundaries and lets any byte (space, NUL, 0xFF) appear in an argument; +// readNetstringStream bounds each element and its length token, so a hostile content cannot OOM. +// NOTE: payload is simply everything after the argc-th argv netstring, so an argc lying LOW +// reclassifies the sender's own trailing netstrings as child stdin — only ever the sender's own +// frame, no trust boundary crossed (reviewer 964 #3). func parseV2Content(content []byte) (argv [][]byte, payload []byte, ok bool) { const maxArgs = 4096 const maxFrameBytes = 1 << 20 @@ -107,6 +112,17 @@ func parseV2Content(content []byte) (argv [][]byte, payload []byte, ok bool) { if !good { return nil, nil, false } + // Canonical decimal only: reject leading '+'/'-', leading zeros ("001"), and empty. Two wire + // encodings of one argc value is a canonical-form hole in a hand-rolled protocol (reviewer 964 + // #2). readNetstringStream digit-checks each netstring's LENGTH token; this checks the argc VALUE. + if len(argcB) == 0 || (len(argcB) > 1 && argcB[0] == '0') { + return nil, nil, false + } + for _, c := range argcB { + if c < '0' || c > '9' { + return nil, nil, false + } + } argc, err := strconv.Atoi(string(argcB)) if err != nil || argc < 0 || argc > maxArgs { return nil, nil, false @@ -170,7 +186,8 @@ func main() { } // runV2 is the v2 wire path (magic already consumed by main): 4-byte big-endian length, one -// netstring whose content is 0x00 , then trailing stdin forwarded to the child. +// netstring whose content is netstring(argc) + argc argv netstrings + payload, then trailing +// stdin forwarded to the child. func runV2() { lenBytes := make([]byte, 4) if _, err := io.ReadFull(os.Stdin, lenBytes); err != nil { diff --git a/cmd/sicd/sicd_test.go b/cmd/sicd/sicd_test.go index 14c605f..bf95c2d 100644 --- a/cmd/sicd/sicd_test.go +++ b/cmd/sicd/sicd_test.go @@ -75,9 +75,9 @@ func be32(n uint32) []byte { // frame builds one wire frame: magic, 4-byte BE length, netstring(command NUL payload). func frame(command, payload []byte) []byte { - // v2 content = argv netstrings + the empty-netstring terminator 0:, + payload. The command is - // split on spaces HERE (test-side convenience); the wire itself no longer space-splits — argv - // arrives length-framed so `touch 'a b'` keeps its boundary. + // v2 content = netstring(argc) + argc argv netstrings + payload. The command is split on + // spaces HERE (test-side convenience) to count args; the wire itself no longer space-splits — + // argv arrives length-framed so `touch 'a b'` keeps its boundary. args := bytes.Split(command, []byte(" ")) content := netstring([]byte(strconv.Itoa(len(args)))) for _, a := range args { @@ -649,15 +649,28 @@ func TestV2OuterLengthCapRejected(t *testing.T) { } func TestV2FrameElementCapRejected(t *testing.T) { - // > maxArgs (4096) argv netstrings in one frame must be rejected, not accumulated. - var content []byte + // argc over maxArgs (4096) must be rejected at the count check, before any element is read. + content := netstring([]byte("5000")) for i := 0; i < 5000; i++ { content = concat(content, netstring([]byte("a"))) } - content = concat(content, []byte("0:,")) ns := netstring(content) r := runSicd(t, concat([]byte{0x00}, be32(uint32(len(ns))), ns)) if r.code != 1 { t.Fatalf("v2 frame over the element cap must exit 1, got %d", r.code) } } + +func TestV2NonCanonicalArgcRejected(t *testing.T) { + // reviewer 964 #2: argc must be canonical decimal. Leading zero, leading '+', and a signed + // value are all one-value-two-encodings and must exit 1 (never exec), even though a well-formed + // argv follows. strconv.Atoi alone would accept "+1" and "001". + for _, bad := range []string{"01", "+1", "-1", " 1", "1 "} { + content := concat(netstring([]byte(bad)), netstring([]byte("echo"))) + ns := netstring(content) + r := runSicd(t, concat([]byte{0x00}, be32(uint32(len(ns))), ns)) + if r.code != 1 { + t.Fatalf("non-canonical argc %q must exit 1, got %d (stdout=%q)", bad, r.code, r.stdout) + } + } +}