We have a number of packages that implement parsers where a panic might lead to a Denial of Service, but returning an invalid input error instead would be perfectly harmless. We should wrap them all in a recover() and prevent the panic from propagating, as a robustness and defense in depth measure.
We need to be careful about preserving documented panic conditions, and about not leaving behind persistent state that might be corrupt following a panic.
Ideas for other packages that can benefit are welcome. Crypto packages were intentionally left out, as we should be confident in their operation. math/big has a lot of entry points and persistent state by definition (and we have a plan to drag it out of the security perimeter).
/cc @golang/security
Comment From: gopherbot
Change https://golang.org/cl/353850 mentions this issue: archive/zip, archive/tar: DO NOT PANIC
Comment From: gopherbot
Change https://golang.org/cl/353851 mentions this issue: image/gif, image/jpeg, image/png: DO NOT PANIC
Comment From: gopherbot
Change https://golang.org/cl/353852 mentions this issue: debug/macho: DO NOT PANIC
Comment From: mengzhuo
Related to: #48085
Comment From: bcmills
For libraries that wrap user-provided interfaces (such as reading from a user-provided io.Reader), I think it's important that we recover only panics that originate within the library, and not those that originate in the user code. Otherwise the recover can turn a diagnosable crash (or, worse, an intended panic/recover pair) into a hard-to-diagnose deadlock, which has its own problems.
One way to detect panics that originate within the library might be to use runtime.Callers to snoop the package for the first non-runtime frame. But then, the caller may be doing that already, and recovering the panic at all (even if it is re-panicked) will destroy the original stack trace. (The language provides no way to re-throw a recovered panic without destroying it, and since the language defines the behavior of panic and recover, any change to a package's recovery behavior is arguably a breaking change!)
Fortunately, we can use a variable to detect an abnormal exit, then snoop the caller stack, and finally recover only if the stack originates in the specific package (or perhaps one of a specific set of packages): https://play.golang.org/p/NAzk0ATvGx5
That would allow these packages to recover from internal bugs, but without masking (or destroying information from) panics in user code.
Comment From: bcmills
That said, now that we have fuzzing coming I wonder whether the recovers are even necessary long-term. These packages seem like they should be extremely deterministic, so they should be very effective targets for fuzzer-generated inputs — and if we can find (and fix) the panics through fuzzing, there should be essentially nothing left to recover.
(That's in contrast with, say, packages with a large amount of nondeterminism like net/http.)
Comment From: fweimer-rh
Memory allocation failures remain an issue, though. I'm not sure to what extent the Go runtime even bothers to handle them.
With compression and APIs that expose entire sections of the file as arrays, it's really not possible to avoid memory allocation failures merely by checking the input file sizes prior to decoding. In other cases, there is a section which supposedly-small section that gets exposed as an array, and the bulk of the data is represented separately and can be accessed through a streaming interface (so its size does not matter for memory consumption purposes). Archive formats typically have this property.
For example, archive/zip represents the ZIP central directory as a []File array in Reader. Applications may want to process ZIP archives containing very large files (multiple gigabytes), not ZIP files with a billion entries in the central directory.
Maybe you could add a Security Considerations section to the package documentation detailing such issues?
Comment From: gopherbot
Change https://golang.org/cl/371394 mentions this issue: _content/doc/fuzz: fix example
Comment From: odeke-em
@FiloSottile thank you for filing this issue!
So we have a bunch of CLs addressing parts of this issue starting from October 2021, but none of them have been merged. Shall we punt this issue instead to Go1.19 when we shall have more adequate time? What do y'all think @FiloSottile @bcmills @katiehockman @julieqiu? Thank you!
Comment From: ianlancetaylor
@FiloSottile @golang/security This is in the 1.18 milestone; time to move to 1.19? Thanks.
Comment From: rsc
Agree about moving this to Go 1.19.
Comment From: gopherbot
Change https://go.dev/cl/393874 mentions this issue: debug/dwarf: better error detection in parseUnits
Comment From: gopherbot
Change https://go.dev/cl/396880 mentions this issue: debug/elf: check for negative shoff and phoff fields
Comment From: gopherbot
Change https://go.dev/cl/408679 mentions this issue: debug/elf: use saferio to read section data
Comment From: gopherbot
Change https://go.dev/cl/408678 mentions this issue: internal/saferio: new package to avoid OOM
Comment From: gopherbot
Change https://go.dev/cl/412014 mentions this issue: debug/pe, internal/saferio: use saferio to read PE section data
Comment From: gopherbot
Change https://go.dev/cl/413874 mentions this issue: debug/macho, internal/saferio: limit slice allocation
Comment From: gopherbot
Change https://go.dev/cl/413875 mentions this issue: internal/xcoff: use saferio to read string table
Comment From: gopherbot
Change https://go.dev/cl/413995 mentions this issue: debug/pe: use saferio to set symbol slice capacity
Comment From: ianlancetaylor
Moving to 1.20.
Comment From: gopherbot
Change https://go.dev/cl/425114 mentions this issue: debug/macho: don't use narch for seenArches map size
Comment From: gopherbot
Change https://go.dev/cl/425115 mentions this issue: debug/plan9obj: don't crash on EOF before symbol type
Comment From: gopherbot
Change https://go.dev/cl/469895 mentions this issue: debug/macho: use saferio to read symbol table strings
Comment From: catenacyber
For reference here, https://github.com/catenacyber/ngolo-fuzzing is my attempt to go at this ;-)
Comment From: gopherbot
Change https://go.dev/cl/470397 mentions this issue: debug/macho: don't crash if dynamic symtab with no symtab
Comment From: gopherbot
Change https://go.dev/cl/471678 mentions this issue: internal/xcoff: use saferio to allocate slices
Comment From: gopherbot
Change https://go.dev/cl/471835 mentions this issue: debug/macho: use saferio to read dynamic indirect symbols
Comment From: gopherbot
Change https://go.dev/cl/473657 mentions this issue: debug/buildinfo: use saferio in ReadData methods
Comment From: gopherbot
Change https://go.dev/cl/488475 mentions this issue: debug/pe: return error on reading from section with uninitialized data
Comment From: gopherbot
Change https://go.dev/cl/499419 mentions this issue: doc/go1.21: reading from debug/pe uninitialized section fails
Comment From: nightlyone
What exactly is blocking this issue? @FiloSottile @dmitshur
Comment From: gopherbot
Change https://go.dev/cl/632035 mentions this issue: debug/elf: check for multiplication overflow for shnum * shentsize
Comment From: gopherbot
Change https://go.dev/cl/698855 mentions this issue: debug/elf: don't panic if symtab too small