From 1c43b6d9c075cb8e869f0785d6532bbb696ba238 Mon Sep 17 00:00:00 2001
From: "Claude (noether)" <noether@fritz.box>
Date: Thu, 23 Jul 2026 11:45:10 +0200
Subject: [PATCH] sicd: bound outer netstring length before alloc + argv
 element count (reviewer 964 #2/#3)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

#3 [DoS]: runV2 allocated make([]byte, nsLen) from a raw uint32 (up to 4 GiB) BEFORE
readNetstring capped it — 5 bytes committed ~4 GB. Now reject nsLen>1<<24 immediately.
#2 [DoS]: parseV2Content had no element-count/byte cap (16 MiB content = 4.2M argv, ~388 MB).
Now caps maxArgs=4096 / maxFrameBytes=1<<20, mirroring runV1. Regression tests for both.
#1 (empty-arg terminator collision) is a SEPARATE ship-blocker, deferred (needs a wire-design
call: explicit argc vs client-side reject) — do NOT deploy the daemon until #1 lands.
---
 cmd/sicd/main.go      | 13 +++++++++++++
 cmd/sicd/sicd_test.go | 23 +++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/cmd/sicd/main.go b/cmd/sicd/main.go
index d1816fb..a532fc8 100644
--- a/cmd/sicd/main.go
+++ b/cmd/sicd/main.go
@@ -96,7 +96,10 @@ func readNetstringStream(r *bufio.Reader) ([]byte, bool) {
 // 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.
 func parseV2Content(content []byte) (argv [][]byte, payload []byte, ok bool) {
+	const maxArgs = 4096
+	const maxFrameBytes = 1 << 20
 	r := bufio.NewReader(bytes.NewReader(content))
+	total := 0
 	for {
 		ns, good := readNetstringStream(r)
 		if !good {
@@ -105,6 +108,10 @@ func parseV2Content(content []byte) (argv [][]byte, payload []byte, ok bool) {
 		if len(ns) == 0 {
 			break // the empty netstring 0:, terminates argv
 		}
+		total += len(ns)
+		if len(argv) >= maxArgs || total > maxFrameBytes {
+			return nil, nil, false // reviewer #2: bound element count + bytes, like runV1
+		}
 		argv = append(argv, ns) // readNetstringStream returns a fresh slice; no aliasing
 	}
 	rest, err := io.ReadAll(r)
@@ -161,6 +168,12 @@ func runV2() {
 		os.Exit(1)
 	}
 	nsLen := binary.BigEndian.Uint32(lenBytes)
+	if nsLen > 1<<24 {
+		// reviewer #3: cap BEFORE make — a raw uint32 (up to 4 GiB) was allocated before
+		// readNetstring's cap ever ran (5-byte input committed ~4 GB).
+		fmt.Fprintf(os.Stderr, "sicd: netstring length %d exceeds cap\n", nsLen)
+		os.Exit(1)
+	}
 
 	nsBuf := make([]byte, nsLen)
 	if _, err := io.ReadFull(os.Stdin, nsBuf); err != nil {
diff --git a/cmd/sicd/sicd_test.go b/cmd/sicd/sicd_test.go
index 5350ea7..bc7cb9f 100644
--- a/cmd/sicd/sicd_test.go
+++ b/cmd/sicd/sicd_test.go
@@ -624,3 +624,26 @@ func TestV2ArgvBoundaryPreserved(t *testing.T) {
 		t.Fatalf("v2 argv boundary LOST: script saw %q, want %q", got, want)
 	}
 }
+
+// reviewer 964 #2/#3 DoS regression.
+func TestV2OuterLengthCapRejected(t *testing.T) {
+	// 0x00 + be32(0xF0000000) + no body: must exit 1 at the length check, BEFORE make() allocs.
+	r := runSicd(t, concat([]byte{0x00}, be32(0xF0000000)))
+	if r.code != 1 {
+		t.Fatalf("oversize outer netstring length must exit 1, got %d (killedBy=%v)", r.code, r.killedBy)
+	}
+}
+
+func TestV2FrameElementCapRejected(t *testing.T) {
+	// > maxArgs (4096) argv netstrings in one frame must be rejected, not accumulated.
+	var content []byte
+	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)
+	}
+}