Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
FiloSottile opened this issue Aug 11, 2021 · 31 comments
Open

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

FiloSottile opened this issue Aug 11, 2021 · 31 comments
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@FiloSottile
Copy link
Contributor

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

@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 11, 2021
@FiloSottile FiloSottile added this to the Go1.18 milestone Aug 11, 2021
@FiloSottile FiloSottile self-assigned this Aug 11, 2021
@FiloSottile FiloSottile changed the title archive, image, encoding, x/net/html: DO NOT PANIC archive, image, debug, encoding, x/net/html: DO NOT PANIC Aug 27, 2021
@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@mengzhuo
Copy link
Contributor

Related to: #48085

@bcmills
Copy link
Member

bcmills commented Oct 27, 2021

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
Copy link
Member

bcmills commented Oct 27, 2021

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
Copy link

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
Copy link

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

@odeke-em
Copy link
Member

@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
Copy link
Contributor

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

@rsc
Copy link
Contributor

rsc commented Feb 2, 2022

Agree about moving this to Go 1.19.

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Mar 21, 2022
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 #51758.
Updates #47653.

Change-Id: I6d67fcb53392acf651ceec636789ab9e49ad5a5c
Reviewed-on: https://go-review.googlesource.com/c/go/+/393874
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Than McIntosh <thanm@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Mar 31, 2022
No test because we could add an infinite number of tests of bogus data.

For #47653
Fixes #52035

Change-Id: Iec7e2fe23f2dd1cf14bad2475422f243f51028f5
Reviewed-on: https://go-review.googlesource.com/c/go/+/396880
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Than McIntosh <thanm@google.com>
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Aug 23, 2022
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 bother to specify a size for the seenArches map.

No debug/macho test case because the problem can only happen for
invalid data. Let the fuzzer find cases like this.

For #47653
For #52523

Change-Id: I5a3b0e3aa6172ddffd6f44d9ae513c39a00d8764
Reviewed-on: https://go-review.googlesource.com/c/go/+/425114
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Joseph Tsai <joetsai@digital-static.net>
gopherbot pushed a commit that referenced this issue Aug 23, 2022
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

Change-Id: I8d3e15725b9bc09dd0e6f2750769987021f5e982
Reviewed-on: https://go-review.googlesource.com/c/go/+/425115
Auto-Submit: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Joseph Tsai <joetsai@digital-static.net>
@gopherbot gopherbot modified the milestones: Go1.20, Go1.21 Feb 1, 2023
@gopherbot
Copy link

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

@catenacyber
Copy link
Contributor

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

gopherbot pushed a commit that referenced this issue Feb 21, 2023
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: I67fc45365c1a5b0b4b381f541bf2fee8ce8ddc3a
Reviewed-on: https://go-review.googlesource.com/c/go/+/469895
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 22, 2023
No test case because the problem can only happen for invalid data. Let
the fuzzer find cases like this.

For golang#47653
Fixes golang#58603

Change-Id: I67fc45365c1a5b0b4b381f541bf2fee8ce8ddc3a
Reviewed-on: https://go-review.googlesource.com/c/go/+/469895
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Feb 22, 2023
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: I19fee0dc9bd6239b520c15182b8f1e57bb0049bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/470397
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
@gopherbot
Copy link

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Feb 28, 2023
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: I5b95a21f47ec306ad90cd6221f0566c6f8b6c3ad
Reviewed-on: https://go-review.googlesource.com/c/go/+/471835
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
gopherbot pushed a commit that referenced this issue Feb 28, 2023
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: Ic3ef58b204b946f8bff80310d4c8dfcbb2939a1c
Reviewed-on: https://go-review.googlesource.com/c/go/+/471678
Auto-Submit: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Mar 6, 2023
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 happen for invalid data. Let
the fuzzer find cases like this.

For #47653
Fixes #58886

Change-Id: I4e80ba62a6416d010c8804e4f49ae81bdafaadb8
Reviewed-on: https://go-review.googlesource.com/c/go/+/473657
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue May 1, 2023
A section with uninitialized data contains no bytes and occupies
no space in the file. This change makes it return an error on reading
from this section so that it will force the caller to check for
a section with uninitialized data.

This is the debug/pe version of CL 429601.

This will break programs that expect a byte slice with the length
described by the SizeOfRawData field. There are two reasons to
introduce this breaking change: 1) uninitialized data is uninitialized
and there is no reason to allocate memory for it; 2) it could result
in an OOM if the file is corrupted and has a large invalid SizeOfRawData.

No test case because the problem can only happen for invalid data. Let
the fuzzer find cases like this.

For #47653
Fixes #59817

Change-Id: I1ae94e9508f803b37926275d9a571f724a09af9f
Reviewed-on: https://go-review.googlesource.com/c/go/+/488475
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: kortschak <dan@kortschak.io>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue May 31, 2023
For #47653

Change-Id: Id44c9dba58966f43f188030a53343d890a6ffde7
Reviewed-on: https://go-review.googlesource.com/c/go/+/499419
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Bypass: Ian Lance Taylor <iant@google.com>
Reviewed-by: Eli Bendersky <eliben@google.com>
Nasfame pushed a commit to golangFame/go that referenced this issue Jun 4, 2023
For golang#47653

Change-Id: Id44c9dba58966f43f188030a53343d890a6ffde7
Reviewed-on: https://go-review.googlesource.com/c/go/+/499419
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Bypass: Ian Lance Taylor <iant@google.com>
Reviewed-by: Eli Bendersky <eliben@google.com>
@gopherbot gopherbot modified the milestones: Go1.21, Go1.22 Aug 8, 2023
@nightlyone

This comment was marked as spam.

@odeke-em odeke-em modified the milestones: Go1.22, Go1.23 Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: No status
Development

No branches or pull requests

10 participants