go: archive, image, debug, encoding, x/net/html: DO NOT PANIC
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
About this issue
- Original URL
- State: open
- Created 3 years ago
- Reactions: 7
- Comments: 31 (9 by maintainers)
Commits related to this issue
- debug/dwarf: better error detection in parseUnits Tweak the (*Data).parseUnits method to check a bit more carefully for buffer read errors, so as to avoid infinite looping on malformed inputs. Fixes... — committed to golang/go by thanm 2 years ago
- debug/elf: check for negative shoff and phoff fields No test because we could add an infinite number of tests of bogus data. For #47653 Fixes #52035 Change-Id: Iec7e2fe23f2dd1cf14bad2475422f243f510... — committed to golang/go by ianlancetaylor 2 years ago
- internal/saferio: new package to avoid OOM Broken out of debug/pe. Update debug/pe to use it. For #47653 Change-Id: Ib3037ee04073e005c4b435d0128b8437a075b00a Reviewed-on: https://go-review.googleso... — committed to golang/go by ianlancetaylor 2 years ago
- debug/elf: use saferio to read section data For #47653 Fixes #45599 Fixes #52522 Change-Id: Id6a80186434080cb0a205978ad7f224252674604 Reviewed-on: https://go-review.googlesource.com/c/go/+/408679 Au... — committed to golang/go by ianlancetaylor 2 years ago
- debug/pe, internal/saferio: use saferio to read PE section data For #47653 Fixes #53189 Change-Id: If35b968fc53e4c96b18964cfb020cdc003b881bf Reviewed-on: https://go-review.googlesource.com/c/go/+/41... — committed to golang/go by ianlancetaylor 2 years ago
- debug/macho, internal/saferio: limit slice allocation Don't allocate slices that are too large; choose a smaller capacity and build the slice using append. Use this in debug/macho to avoid over-alloc... — committed to golang/go by ianlancetaylor 2 years ago
- internal/xcoff: use saferio to read string table No test case because the problem can only happen for invalid data. Let the fuzzer find cases like this. For #47653 Fixes #52526 Change-Id: Id90a5e39... — committed to golang/go by ianlancetaylor 2 years ago
- debug/pe: use saferio to set symbol slice capacity No test case because the problem can only happen for invalid data. Let the fuzzer find cases like this. For #47653 Fixes #53530 Change-Id: If1cebb... — committed to golang/go by ianlancetaylor 2 years ago
- debug/macho: don't use narch for seenArches map size If narch is very large we would allocate a lot of memory for seenArches. In practice we aren't going to see many different architectures so don't ... — committed to golang/go by ianlancetaylor 2 years ago
- debug/plan9obj: don't crash on EOF before symbol type No debug/plan9obj test case because the problem can only happen for invalid data. Let the fuzzer find cases like this. For #47653 Fixes #54585 ... — committed to golang/go by ianlancetaylor 2 years ago
- debug/macho: use saferio to read symbol table strings No test case because the problem can only happen for invalid data. Let the fuzzer find cases like this. For #47653 Fixes #58603 Change-Id: I67f... — committed to golang/go by ianlancetaylor a year ago
- debug/macho: use saferio to read symbol table strings No test case because the problem can only happen for invalid data. Let the fuzzer find cases like this. For #47653 Fixes #58603 Change-Id: I67f... — committed to Pryz/go by ianlancetaylor a year ago
- debug/macho: don't crash if dynamic symtab with no symtab No test case because the problem can only happen for invalid data. Let the fuzzer find cases like this. For #47653 Fixes #58642 Change-Id: ... — committed to golang/go by ianlancetaylor a year ago
- debug/macho: use saferio to read dynamic indirect symbols No test case because the problem can only happen for invalid data. Let the fuzzer find cases like this. For #47653 Fixes #58755 Change-Id: ... — committed to golang/go by ianlancetaylor a year ago
- internal/xcoff: use saferio to allocate slices No test case because the problem can only happen for invalid data. Let the fuzzer find cases like this. For #47653 Fixes #58754 Change-Id: Ic3ef58b204... — committed to golang/go by ianlancetaylor a year ago
- debug/buildinfo: use saferio in ReadData methods This avoids a very large memory allocation if corrupt data says that we need to read a very long string. No test case because the problem can only ha... — committed to golang/go by ianlancetaylor a year ago
- debug/pe: return error on reading from section with uninitialized data A section with uninitialized data contains no bytes and occupies no space in the file. This change makes it return an error on r... — committed to golang/go by ianlancetaylor a year ago
- doc/go1.21: reading from debug/pe uninitialized section fails For #47653 Change-Id: Id44c9dba58966f43f188030a53343d890a6ffde7 Reviewed-on: https://go-review.googlesource.com/c/go/+/499419 Auto-Submi... — committed to golang/go by ianlancetaylor a year ago
- doc/go1.21: reading from debug/pe uninitialized section fails For #47653 Change-Id: Id44c9dba58966f43f188030a53343d890a6ffde7 Reviewed-on: https://go-review.googlesource.com/c/go/+/499419 Auto-Submi... — committed to golangFame/go by ianlancetaylor a year ago
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 torecover.(That’s in contrast with, say, packages with a large amount of nondeterminism like
net/http.)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 therecovercan 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.Callersto snoop the package for the first non-runtimeframe. 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 recoveredpanicwithout destroying it, and since the language defines the behavior ofpanicandrecover, 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
recoveronly if the stack originates in the specific package (or perhaps one of a specific set of packages): https://play.golang.org/p/NAzk0ATvGx5That would allow these packages to recover from internal bugs, but without masking (or destroying information from) panics in user code.