sicd: bound outer netstring length before alloc + argv element count (reviewer 964 #2/#3)
#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.
This commit is contained in:
@@ -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 {
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user