sicd: bound the v1 length token and frame size (reviewer 925 #1 HIGH, #2 MED)

#1 [HIGH, was a real DoS]: readNetstringStream read the length token via bufio.ReadString(':'),
which buffers the whole pre-':' run UNBOUNDED — the 1<<24 cap only bounded the body, applied
AFTER the token was parsed. A protocol-confined caller sending an endless digit run with no ':'
drove sicd to ~1 GB RSS (measured by @reviewer). Now the token is read byte-by-byte with an
8-digit hard cap (a valid <=1<<24 length is 8 digits), so it rejects after <=9 bytes regardless
of input size — memory is bounded by construction, before the value is trusted.

#2 [MED]: nothing bounded the NUMBER of netstrings per frame (2M tiny ones ~140 MB before exec).
runV1 now caps element count (4096) and total argv bytes (1<<20).

#3 [MED, documented]: an empty netstring 0:, is always the terminator, so a legit empty ""
argument collides with it (silent argv truncation + stdin bleed). Inherent to the v1 wire, not
introduced here — noted in a comment; a fix would need a wire change.

Regression tests: overlong length token (100k digits) -> exit 1; 9-digit length -> exit 1;
frame over the element cap -> exit 1. go test ./... ok, go vet clean. Found by @reviewer's
probe (reproduced 1 GB RSS), not a re-read — exactly why the outside look isn't skippable.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

cmd/sicd/dualread_test.go

@@ -178,3 +178,40 @@ func TestV1EmptyExecArgvRejected(t *testing.T) {
 		t.Fatal("empty v1 exec argv must produce a diagnostic on stderr")
 	}
 }
+
+// --- DoS regression (reviewer 925 findings #1/#2): the length token and the frame element
+// count must both be bounded BEFORE the value is trusted, or a protocol-confined caller OOMs
+// the host (measured ~1 GB RSS from an unbounded digit run; ~140 MB from 2M netstrings). --
+
+func TestV1OverlongLengthTokenRejected(t *testing.T) {
+	// A run of digits with no ':' must be rejected after a small cap, never buffered unbounded.
+	r := runSicd(t, bytes.Repeat([]byte("9"), 100_000))
+	if r.code != 1 {
+		t.Fatalf("overlong v1 length token must exit 1, got %d (killedBy=%v)", r.code, r.killedBy)
+	}
+	if len(r.stdout) != 0 {
+		t.Fatalf("overlong length token must exec nothing, stdout=%q", r.stdout)
+	}
+}
+
+func TestV1LengthTokenNineDigitsRejected(t *testing.T) {
+	// 9 digits (> the 8 a valid <=1<<24 length needs) is malformed, even with a ':'.
+	r := runSicd(t, []byte("100000000:x,0:,"))
+	if r.code != 1 {
+		t.Fatalf("9-digit v1 length must exit 1, got %d", r.code)
+	}
+}
+
+func TestV1FrameElementCapRejected(t *testing.T) {
+	// Too many netstrings in one frame must be rejected, not accumulated to OOM.
+	var b bytes.Buffer
+	b.Write(netstring([]byte("exec")))
+	for i := 0; i < 5000; i++ { // > maxArgs (4096)
+		b.Write(netstring([]byte("a")))
+	}
+	b.WriteString("0:,")
+	r := runSicd(t, b.Bytes())
+	if r.code != 1 {
+		t.Fatalf("v1 frame exceeding the element cap must exit 1, got %d", r.code)
+	}
+}

cmd/sicd/main.go

@@ -51,20 +51,32 @@ func readNetstring(buf []byte) ([]byte, bool) {
 // 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:,.
 func readNetstringStream(r *bufio.Reader) ([]byte, bool) {
-	lenStr, err := r.ReadString(':')
-	if err != nil {
-		return nil, false // EOF before ':' — truncated
-	}
-	lenStr = lenStr[:len(lenStr)-1] // drop the ':'
-	if len(lenStr) == 0 {
-		return nil, false
-	}
-	for i := 0; i < len(lenStr); i++ {
-		if lenStr[i] < '0' || lenStr[i] > '9' {
+	// 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
+	// endless stream of digits with no ':' drives allocation to OOM (a confined caller measured
+	// ~1 GB RSS). A valid length is <= 1<<24 = 16777216 (8 digits), so >8 digits is malformed.
+	const maxLenDigits = 8
+	var lenBuf []byte
+	for {
+		b, err := r.ReadByte()
+		if err != nil {
+			return nil, false // EOF before ':' — truncated
+		}
+		if b == ':' {
+			break
+		}
+		if b < '0' || b > '9' {
+			return nil, false
+		}
+		lenBuf = append(lenBuf, b)
+		if len(lenBuf) > maxLenDigits {
 			return nil, false
 		}
 	}
-	n, err := strconv.Atoi(lenStr)
+	if len(lenBuf) == 0 {
+		return nil, false
+	}
+	n, err := strconv.Atoi(string(lenBuf))
 	if err != nil || n > 1<<24 {
 		return nil, false
 	}
@@ -201,7 +213,16 @@ func runV1(r *bufio.Reader) {
 	}
 
 	// Read argv/command netstrings until the empty-netstring terminator. EOF first = truncated.
+	// Cap the frame (element count AND total bytes): readNetstringStream bounds each netstring at
+	// 1<<24, but nothing bounds their NUMBER — 2M tiny netstrings measured ~140 MB before exec.
+	// NOTE: an empty netstring 0:, is ALWAYS the terminator, so a legitimate empty "" argument
+	// collides with it — the arg list truncates there and the rest bleeds into the child's stdin.
+	// That is inherent to the v1 wire (terminator overload), not a bug introduced here; the
+	// deployed cmd/sic produces it only if a caller passes a literal "".
+	const maxArgs = 4096
+	const maxFrameBytes = 1 << 20
 	var argv []string
+	total := 0
 	for {
 		ns, ok := readNetstringStream(r)
 		if !ok {
@@ -211,6 +232,11 @@ func runV1(r *bufio.Reader) {
 		if len(ns) == 0 {
 			break // the empty netstring 0:, terminates the frame
 		}
+		total += len(ns)
+		if len(argv) >= maxArgs || total > maxFrameBytes {
+			fmt.Fprintf(os.Stderr, "sicd: v1 frame too large\n")
+			os.Exit(1)
+		}
 		argv = append(argv, string(ns))
 	}