diff --git a/test/rules/rules.go b/test/rules/rules.go index f6f1a0e15..2145b967a 100644 --- a/test/rules/rules.go +++ b/test/rules/rules.go @@ -38,6 +38,71 @@ func noUnboundedReads(m dsl.Matcher) { Report("do not use unbounded io.Copy to in-memory buffer; wrap the source reader with io.LimitReader") } +// nolint:unused +func noUnboundedAllocations(m dsl.Matcher) { + // a make() length that is not a plain int almost always came off the wire: a size or count field + // read out of a binary header, an archive record, or a length-prefixed frame. Those are + // attacker-controlled, so a small crafted file can reserve gigabytes before a single byte is read. + // Lengths from len() or from a constant are plain int / untyped const and do not match. + m.Match( + `make([]$_, $n)`, + `make([]$_, $_, $n)`, + `make(map[$_]$_, $n)`, + ). + Where(!m["n"].Const && m["n"].Type.OfKind("integer") && !m["n"].Type.Is("int") && m["n"].Type.Size >= 4). + Report("unbounded allocation: $n is not a plain int, so it likely comes from parsed input. read with io.ReadAll(io.LimitReader(r, n)) so the buffer grows to what the input actually holds, or clamp $n against the real file size before allocating") +} + +// nolint:unused +func noUnboundedDecompression(m dsl.Matcher) { + // compression ratios are unbounded (gzip does ~1032:1), so a decompressed stream must be capped + // independently of the compressed input, which is already bounded by the file it came from. + // The constructor is flagged rather than the consumer because the consumer is often a + // third-party decoder that buffers the whole stream before parsing any of it. + m.Match( + `gzip.NewReader($_)`, + `zlib.NewReader($_)`, + `flate.NewReader($_)`, + `bzip2.NewReader($_)`, + `lzma.NewReader($_)`, + `xz.NewReader($_)`, + ). + Where(m.File().PkgPath.Matches(`/cataloger/`)). + Report("unbounded decompression in a cataloger: wrap the decompressed stream in io.LimitReader before anything consumes it, or nolint with the bound that already applies") +} + +// nolint:unused +func noOverflowingBoundsCheck(m dsl.Matcher) { + // an offset+size bounds check on unsigned operands wraps rather than saturating, so a large + // offset out of a file header can produce a small sum and pass a check it should have failed. + // Written as a subtraction against the limit, there is nothing to wrap. + m.Match( + `$offset + $size > $limit`, + `$offset + $size >= $limit`, + `$offset + $size < $limit`, + `$offset + $size <= $limit`, + ). + Where((m["offset"].Type.OfKind("unsigned") || m["size"].Type.OfKind("unsigned")) && + // a constant addend cannot be attacker-controlled, which is the common safe shape of + // stepping a loop cursor (`pos+4 <= uint32(len(buf))`) + !m["offset"].Const && !m["size"].Const). + Report("bounds check can overflow: $offset + $size wraps on unsigned operands. write it as a subtraction against the limit instead, e.g. `$offset > $limit || $size > $limit-$offset`") +} + +// nolint:unused +func noSwallowedEOF(m dsl.Matcher) { + // treating EOF as success hands the caller a zero value it cannot distinguish from real data. In a + // loop that advances by however much was read, that is not just bad data but a cursor that never + // moves, so a truncated record spins forever. + m.Match( + `if errors.Is($err, io.EOF) { return nil }`, + `if errors.Is($err, io.EOF) { return nil, nil }`, + `if $err == io.EOF { return nil }`, + `if $err == io.EOF { return nil, nil }`, + ). + Report("EOF returned as success: the caller gets a zero value it cannot tell from real data. return the error, or return an explicit sentinel the caller checks") +} + // nolint:unused func noDirectTempFiles(m dsl.Matcher) { // catalogers must use tmpdir.FromContext(ctx) instead of creating temp files/dirs directly,