diff --git a/cmd/sicd/dualread_test.go b/cmd/sicd/dualread_test.go index 2dffb95..43cc2cd 100644 --- a/cmd/sicd/dualread_test.go +++ b/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) + } +} diff --git a/cmd/sicd/main.go b/cmd/sicd/main.go index 1279c6c..89b852b 100644 --- a/cmd/sicd/main.go +++ b/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)) }