archive, image, debug, encoding, x/net/html: DO NOT PANIC

[FiloSottile]

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

[gopherbot]

Change https://golang.org/cl/353850 mentions this issue: archive/zip, archive/tar: DO NOT PANIC

[gopherbot]

Change https://golang.org/cl/353851 mentions this issue: image/gif, image/jpeg, image/png: DO NOT PANIC

[gopherbot]

Change https://golang.org/cl/353852 mentions this issue: debug/macho: DO NOT PANIC

[mengzhuo]

Related to: #48085

[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.

[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.)

[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?

[gopherbot]

Change https://golang.org/cl/371394 mentions this issue: _content/doc/fuzz: fix example

[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!

[ianlancetaylor]

@FiloSottile @golang/security This is in the 1.18 milestone; time to move to 1.19? Thanks.

[rsc]

Agree about moving this to Go 1.19.

[gopherbot]

Change https://go.dev/cl/393874 mentions this issue: debug/dwarf: better error detection in parseUnits

[gopherbot]

Change https://go.dev/cl/396880 mentions this issue: debug/elf: check for negative shoff and phoff fields

[gopherbot]

Change https://go.dev/cl/408679 mentions this issue: debug/elf: use saferio to read section data

[gopherbot]

Change https://go.dev/cl/408678 mentions this issue: internal/saferio: new package to avoid OOM

[gopherbot]

Change https://go.dev/cl/412014 mentions this issue: debug/pe, internal/saferio: use saferio to read PE section data

[gopherbot]

Change https://go.dev/cl/413874 mentions this issue: debug/macho, internal/saferio: limit slice allocation

[gopherbot]

Change https://go.dev/cl/413875 mentions this issue: internal/xcoff: use saferio to read string table

[gopherbot]

Change https://go.dev/cl/413995 mentions this issue: debug/pe: use saferio to set symbol slice capacity

[ianlancetaylor]

Moving to 1.20.

[gopherbot]

Change https://go.dev/cl/425114 mentions this issue: debug/macho: don't use narch for seenArches map size

[gopherbot]

Change https://go.dev/cl/425115 mentions this issue: debug/plan9obj: don't crash on EOF before symbol type

[gopherbot]

Change https://go.dev/cl/469895 mentions this issue: debug/macho: use saferio to read symbol table strings

[catenacyber]

For reference here, https://github.com/catenacyber/ngolo-fuzzing is my attempt to go at this ;-)

[gopherbot]

Change https://go.dev/cl/470397 mentions this issue: debug/macho: don't crash if dynamic symtab with no symtab

[gopherbot]

Change https://go.dev/cl/471678 mentions this issue: internal/xcoff: use saferio to allocate slices

[gopherbot]

Change https://go.dev/cl/471835 mentions this issue: debug/macho: use saferio to read dynamic indirect symbols

[gopherbot]

Change https://go.dev/cl/473657 mentions this issue: debug/buildinfo: use saferio in ReadData methods

[gopherbot]

Change https://go.dev/cl/488475 mentions this issue: debug/pe: return error on reading from section with uninitialized data

[gopherbot]

Change https://go.dev/cl/499419 mentions this issue: doc/go1.21: reading from debug/pe uninitialized section fails

[nightlyone]

What exactly is blocking this issue? @FiloSottile @dmitshur