syft/test/rules/rules.go
Alex Goodman 58a033f924
Prevent unnecessary allocations when parsing compressed ELF sections (#5187)
* fix(elf): bound compressed ELF section reads

`debug/elf` takes a section's decompressed size from that section's own
compression header, and a highly compressible stream really does deliver the
bytes that header promises, so `internal/saferio` does not help: it faithfully
allocates every one of them. A 2MB input file drives `elf.NewFile` to allocate
over 10GB and return no error, which is a fatal OOM rather than a recoverable
panic.

Which sections get read is not up to the caller. `elf.NewFile` always reads the
section-name string table, and `File.Symbols` reads `.symtab` plus whatever
section its `Link` field points at, so being selective about sections is not
enough to avoid it.

New `elfutil.NewFile` is a drop-in for `elf.NewFile` that rejects a declared
decompressed size over 128MB. Every production call site goes through it, and a
ruleguard rule keeps the next one from going direct.

The check runs in two parts, since `debug/elf` expands sections at two different
times. The section-name string table is the only one `elf.NewFile` expands
itself, so it is checked against the raw bytes before the call; everything else
is expanded lazily by `(*Section).Open` and is checked after the parse, where
names, types and decompressed sizes are already resolved.

Only the sections syft can actually reach are bounded, which keeps the guard
from costing real binaries. DWARF is excluded since nothing calls `File.DWARF`,
so a large compressed `.debug_info` no longer skips the whole file, and sections
`debug/elf` will not decompress anyway (`SHF_ALLOC`, `SHT_NOBITS`) are left
alone. The legacy `.zdebug` form is matched on the section name the way
`debug/elf` gates it rather than on the `ZLIB` magic, so an ordinary section
starting with those four bytes is not mistaken for a compressed one.

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* fix(elf): gate debug/buildinfo behind the compressed-section check

`debug/buildinfo.Read` opens ELF files with `debug/elf` itself, and `elf.NewFile`
expands the section-name string table as it parses, so the golang cataloger was
still reachable by the same bomb `elfutil` exists to stop. A 261KB fixture drove
1.4GB of allocation through `buildinfo.Read` and returned no error.

`elfutil.CheckSectionNameTable` is now exported for that case: callers that cannot
use `NewFile` because the `debug/elf` call is made for them inside another package.
Both `buildinfo.Read` call sites go through it, including the UPX-decompressed one.

Also corrects claims that did not hold up:

- the package doc's 2MB-to-10GB figure is not reachable with zlib (~1000:1), so it
  now carries the measured 510KB-to-2.6GB, and names zstd's 32767:1 since that is
  what makes the small inputs possible

- `.go.buildinfo` was listed as a hot-path section elfutil covers, but it is read
  through `debug/buildinfo` and never touches `Section.Data`

- the graalvm comment claimed routing size rejections away from `*elf.FormatError`
  improved reporting; both branches are skipped by the caller and only the
  FormatError branch logs, so it did the opposite

- `sharedLibraries` logged short and truncated files as real ELF failures, since
  `debug/elf` returns a bare `io.EOF` rather than an `*elf.FormatError` for those

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* added test comments around the negative cases

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* additional tests

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* better decomposition and comments

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* use a less brittle constant for error detection

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

---------

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
2026-08-14 20:08:58 +00:00

106 lines
3.9 KiB
Go

//go:build gorules
package rules
import (
"strings"
"github.com/quasilyte/go-ruleguard/dsl"
)
// nolint:unused
func resourceCleanup(m dsl.Matcher) {
// this rule defends against use of internal.CloseAndLogError() without a defer statement
m.Match(`$res, $err := $resolver.FileContentsByLocation($loc); if $*_ { $*_ }; $next`).
Where(m["res"].Type.Implements(`io.Closer`) &&
m["res"].Type.Implements(`io.Reader`) &&
m["err"].Type.Implements(`error`) &&
!m["next"].Text.Matches(`defer internal.CloseAndLogError`)).
Report(`please call "defer internal.CloseAndLogError($res, $loc.RealPath)" right after checking the error returned from $resolver.FileContentsByLocation.`)
}
// nolint:unused
func isPtr(ctx *dsl.VarFilterContext) bool {
return strings.HasPrefix(ctx.Type.String(), "*") || strings.HasPrefix(ctx.Type.Underlying().String(), "*")
}
// nolint:unused
func noUnboundedReads(m dsl.Matcher) {
// flag io.ReadAll where the argument is not already wrapped in io.LimitReader
m.Match(`io.ReadAll($reader)`).
Where(!m["reader"].Text.Matches(`(?i)LimitReader|LimitedReader`)).
Report("do not use unbounded io.ReadAll; wrap the reader with io.LimitReader or use a streaming parser")
// flag io.Copy only when the destination is an in-memory buffer
// io.Copy to files, hash writers, encoders, etc. is streaming and safe
m.Match(`io.Copy($dst, $src)`).
Where((m["dst"].Type.Is(`*bytes.Buffer`) || m["dst"].Type.Is(`*strings.Builder`)) && !m["src"].Text.Matches(`(?i)LimitReader|LimitedReader`)).
Report("do not use unbounded io.Copy to in-memory buffer; wrap the source reader with io.LimitReader")
}
// nolint:unused
func noDirectTempFiles(m dsl.Matcher) {
// catalogers must use tmpdir.FromContext(ctx) instead of creating temp files/dirs directly,
// so that all temp storage is centrally managed and cleaned up
m.Match(
`os.CreateTemp($*_)`,
`os.MkdirTemp($*_)`,
).
Where(m.File().PkgPath.Matches(`/cataloger/`)).
Report("do not use os.CreateTemp/os.MkdirTemp in catalogers; use tmpdir.FromContext(ctx) instead")
}
// nolint:unused
func tmpCleanupDeferred(m dsl.Matcher) {
// ensure the cleanup function returned by NewFile/NewChild is deferred, not discarded
m.Match(
`$_, $cleanup, $err := $x.NewFile($*_); if $*_ { $*_ }; $next`,
`$_, $cleanup, $err = $x.NewFile($*_); if $*_ { $*_ }; $next`,
).
Where(!m["next"].Text.Matches(`^defer `)).
Report("defer the cleanup function returned by NewFile immediately after the error check")
m.Match(
`$_, $cleanup, $err := $x.NewChild($*_); if $*_ { $*_ }; $next`,
`$_, $cleanup, $err = $x.NewChild($*_); if $*_ { $*_ }; $next`,
).
Where(!m["next"].Text.Matches(`^defer `)).
Report("defer the cleanup function returned by NewChild immediately after the error check")
}
// nolint:unused
func packagesInRelationshipsAsValues(m dsl.Matcher) {
m.Import("github.com/anchore/syft/syft/artifact")
isRelationship := func(m dsl.Matcher) bool {
return m["x"].Type.Is("artifact.Relationship")
}
hasPointerType := func(m dsl.Matcher) bool {
return m["y"].Filter(isPtr)
}
// this rule defends against using pointers as values in artifact.Relationship
m.Match(
`$x{$*_, From: $y, $*_}`,
`$x{$*_, To: $y, $*_}`,
`$x.From = $y`,
`$x.To = $y`,
).
Where(isRelationship(m) && hasPointerType(m)).
Report("pointer used as a value for From/To field in artifact.Relationship (use values instead)")
}
// nolint:unused
func noDirectELFOpen(m dsl.Matcher) {
// debug/elf sizes a compressed section's buffer from that section's own header, so opening an
// attacker-supplied binary with it directly is an unbounded allocation. elfutil.NewFile is a
// drop-in that bounds the sections syft can actually reach.
m.Match(
`elf.NewFile($_)`,
`elf.Open($_)`,
).
Where(!m.File().PkgPath.Matches(`/syft/internal/elfutil$`)).
Report("do not open ELF files with debug/elf directly; use elfutil.NewFile, which bounds declared decompressed section sizes")
}