debug/elf: don't check section size #33121

gregory-m · 2019-07-15T15:07:08Z

What version of Go are you using (`go version`)?

go1.12.7
also reproducible on tip

Does this issue reproduce with the latest release?

yes

What did you do?

https://play.golang.org/p/_5bUxWBh14g

package main

import (
	"debug/elf"
	"fmt"
	"strings"
)

func main() {
	data := "\u007fELF\x02\x01\x010000000000000" +
		"\x010000000000000000000" +
		"\x02\x00\x00\x00\x00\x00\x00\x0000000000\x00\x00\x00\x00" +
		"000\x0000"

	_, err := elf.NewFile(strings.NewReader(data))
	if err != nil {
		fmt.Println(err)
	}
}

What did you expect to see?

no panic

What did you see instead?

panic: runtime error: makeslice: len out of range

goroutine 1 [running]:
debug/elf.(*Section).Data(0x44b2c0, 0x43e300, 0x7f6900, 0x206590, 0x109440, 0x7fb880, 0x0, 0x0)
	/usr/local/go/src/debug/elf/file.go:105 +0x40
debug/elf.NewFile(0x15d640, 0x43e2e0, 0x43e2e0, 0x1293, 0x432070, 0x11)
	/usr/local/go/src/debug/elf/file.go:448 +0x1660
main.main()
	/tmp/sandbox898898450/prog.go:15 +0xa0

Fund with https://github.com/dvyukov/go-fuzz

The text was updated successfully, but these errors were encountered:

odeke-em · 2019-07-15T15:39:12Z

Thank you for this report @gregory-m!

Kindly paging @tklauser @heschik @aclements.

ianlancetaylor · 2019-07-15T21:28:16Z

I'm not really sure how hard debug/elf should work to handle invalid input files.

gregory-m · 2019-07-16T08:31:36Z

I have same question as @ianlancetaylor, because I have 2 more panics in different places.

Fuzzing the ELF loader isn't very succesful since it keeps crashing in debug/elf. Most of the time the culprit is a call to elf.Section.Data(). This method allocates a buffer with a size taken from the ELF, which can lead to outlandishly large allocations. It's not clear how to validate elf.Section.Size since the code that creates the section doesn't know the total length of the ELF. Instead, add a wrapper that catches panics due to ELF parsing, and turns them into errors. This isn't a fool proof solution since the runtime can still kill the process due to an OOM, but hopefully we will still crash less overall. See golang/go#33121

rockdaboot · 2022-10-24T12:32:03Z

I'm not really sure how hard debug/elf should work to handle invalid input files.

Here is at least one reason why NewFile() should not crash because of invalidated input.

We have a service that extracts BuildIDs from ELF files provided from different sources (distro packages or arbitrary upload from user).
For this we use NewFile() to load the ELF headers and then extract the BuildID from ".note.gnu.build-id" (or for kernel object we use ".notes").

We already saw some crashes of the service (luckily, it automatically recovers =)).

Now I ran into this again while experimenting with the Go fuzzer.
Here are some details, let me know if you prefer a separate issue.

Go built from commit 7e72d38.
"fatal error: runtime: out of memory" happens at

        /home/tim/src/go/src/debug/elf/file.go:470 +0x1351 fp=0xc0002b76f8 sp=0xc0002b7448 pc=0x5da291

The input file is
e088d7f758b82c73.gz

In line 470 (f.Sections = make([]*Section, shnum)), shnum is 4829504426752.

This crash prevents the fuzzer from going on and finding more/other issues.

ZekeLu · 2022-10-24T15:22:16Z

The issue reported on the first comment has been fixed on tip. Now it returns an error invalid ELF shentsize '0' in record at byte 0x0 instead of panics.

As for the issue reported by @rockdaboot, maybe we can fix it like this:

diff --git a/src/debug/elf/file.go b/src/debug/elf/file.go
index 708980bc1c..f4592f2cf2 100644
--- a/src/debug/elf/file.go
+++ b/src/debug/elf/file.go
@@ -467,8 +467,12 @@ func NewFile(r io.ReaderAt) (*File, error) {
    }
 
    // Read section headers
-   f.Sections = make([]*Section, shnum)
-   names := make([]uint32, shnum)
+   safeShnum := 0xfffff
+   if safeShnum > shnum {
+       safeShnum = shnum
+   }
+   f.Sections = make([]*Section, 0, safeShnum)
+   names := make([]uint32, 0, safeShnum)
    for i := 0; i < shnum; i++ {
        off := shoff + int64(i)*int64(shentsize)
        sr.Seek(off, seekStart)
@@ -479,7 +483,7 @@ func NewFile(r io.ReaderAt) (*File, error) {
            if err := binary.Read(sr, f.ByteOrder, sh); err != nil {
                return nil, err
            }
-           names[i] = sh.Name
+           names = append(names, sh.Name)
            s.SectionHeader = SectionHeader{
                Type:      SectionType(sh.Type),
                Flags:     SectionFlag(sh.Flags),
@@ -496,7 +500,7 @@ func NewFile(r io.ReaderAt) (*File, error) {
            if err := binary.Read(sr, f.ByteOrder, sh); err != nil {
                return nil, err
            }
-           names[i] = sh.Name
+           names = append(names, sh.Name)
            s.SectionHeader = SectionHeader{
                Type:      SectionType(sh.Type),
                Flags:     SectionFlag(sh.Flags),
@@ -544,7 +548,7 @@ func NewFile(r io.ReaderAt) (*File, error) {
            }
        }
 
-       f.Sections[i] = s
+       f.Sections = append(f.Sections, s)
    }
 
    if len(f.Sections) == 0 {

@ianlancetaylor I will send a CL if you're okay with this solution.

rockdaboot · 2022-10-24T17:06:17Z

@ZekeLu Thanks, your suggestion looks much better than the hack that I applied here to continue with fuzzing 😄 :

diff --git a/src/debug/elf/file.go b/src/debug/elf/file.go
index 708980bc1c..2b1aaa3938 100644
--- a/src/debug/elf/file.go
+++ b/src/debug/elf/file.go
@@ -466,6 +466,13 @@ func NewFile(r io.ReaderAt) (*File, error) {
                return nil, &FormatError{0, "invalid ELF shentsize", shentsize}
        }
 
+       // Plausibility check to avoid an unbounded memory allocation.
+       var test [1]byte
+       sr.Seek(int64(shnum) * 40, seekStart)
+       if err := binary.Read(sr, f.ByteOrder, test); err != nil {
+               return nil, err
+       }
+
        // Read section headers
        f.Sections = make([]*Section, shnum)
        names := make([]uint32, shnum)

ianlancetaylor · 2022-10-24T21:23:20Z

We've more or less decided that debug/elf should do at least minimal work to avoid panicking on invalid files. See #47653 and several recent commits to the debug/elf package.

@ZekeLu Since ELF files can have enormous numbers of sections, we should probably use internal/saferio.SliceCap. In any case, fixing that code is fine. Thanks.

ZekeLu · 2022-10-24T23:31:41Z

Ah, I totally forgot that we already have internal/saferio.SliceCap. I will send a CL utilized it soon. Thank you!

gopherbot · 2022-10-24T23:37:48Z

Change https://go.dev/cl/445076 mentions this issue: debug/elf: use saferio.SliceCap when decoding ELF sections

This avoids allocating an overly large slice for corrupt input. No test case because the problem can only happen for invalid data. Let the fuzzer find cases like this. Updates #33121. Change-Id: Ie2d947a3865d3499034286f2d08d3e3204015f3e GitHub-Last-Rev: 6c62fc3 GitHub-Pull-Request: #56405 Reviewed-on: https://go-review.googlesource.com/c/go/+/445076 Run-TryBot: Ian Lance Taylor <iant@google.com> Reviewed-by: 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: Heschi Kreinick <heschi@google.com>

ZekeLu · 2022-10-25T17:21:12Z

The listed issues have been fixed.

I have 2 more panics in different places.

@gregory-m Can you confirm whether the 2 more panics can still be reproduced on go tip?

@gopherbot Please add label WaitingForInfo.

gregory-m · 2022-10-26T13:53:51Z

With CL445076 I can't reproduce it.
Thanks for fixing it.

This avoids allocating an overly large slice for corrupt input. No test case because the problem can only happen for invalid data. Let the fuzzer find cases like this. Updates golang#33121. Change-Id: Ie2d947a3865d3499034286f2d08d3e3204015f3e GitHub-Last-Rev: 6c62fc3 GitHub-Pull-Request: golang#56405 Reviewed-on: https://go-review.googlesource.com/c/go/+/445076 Run-TryBot: Ian Lance Taylor <iant@google.com> Reviewed-by: 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: Heschi Kreinick <heschi@google.com>

We should be running the local dev setup on 1.20 due to golang/go#33121 Signed-off-by: crozzy <joseph.crosland@gmail.com>

Some of the go detection logic was prone to this bug: golang/go#33121 in the go debug library, as 1.20 includes the fix we deem it required to avoid panics originating in claircore. Signed-off-by: crozzy <joseph.crosland@gmail.com>

odeke-em changed the title ~~debug/elf don't check section size~~ debug/elf: don't check section size Jul 15, 2019

odeke-em added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 15, 2019

gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022

seankhliao added this to the Unplanned milestone Aug 27, 2022

ZekeLu mentioned this issue Oct 24, 2022

debug/elf: use saferio.SliceCap when decoding ELF sections #56405

Closed

gopherbot added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Oct 25, 2022

gregory-m closed this as completed Oct 26, 2022

crozzy added a commit to crozzy/clair that referenced this issue Feb 9, 2023

chore: Update go-image version in docker-compose manifest

2dcdc5c

We should be running the local dev setup on 1.20 due to golang/go#33121 Signed-off-by: crozzy <joseph.crosland@gmail.com>

crozzy mentioned this issue Feb 9, 2023

chore: Update go-image version in docker-compose manifest quay/clair#1685

Merged

crozzy added a commit to crozzy/clair that referenced this issue Feb 10, 2023

chore: Update go-image version in docker-compose manifest

c5a53e9

We should be running the local dev setup on 1.20 due to golang/go#33121 Signed-off-by: crozzy <joseph.crosland@gmail.com>

crozzy added a commit to quay/clair that referenced this issue Feb 11, 2023

chore: Update go-image version in docker-compose manifest

12f38e4

We should be running the local dev setup on 1.20 due to golang/go#33121 Signed-off-by: crozzy <joseph.crosland@gmail.com>

crozzy mentioned this issue Apr 12, 2023

chore: Update go version in go.mod quay/clair#1711

Merged

golang locked and limited conversation to collaborators Oct 26, 2023

gopherbot added the FrozenDueToAge label Oct 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

debug/elf: don't check section size #33121

debug/elf: don't check section size #33121

gregory-m commented Jul 15, 2019

odeke-em commented Jul 15, 2019

ianlancetaylor commented Jul 15, 2019

gregory-m commented Jul 16, 2019

rockdaboot commented Oct 24, 2022 •

edited

ZekeLu commented Oct 24, 2022

rockdaboot commented Oct 24, 2022

ianlancetaylor commented Oct 24, 2022

ZekeLu commented Oct 24, 2022

gopherbot commented Oct 24, 2022

ZekeLu commented Oct 25, 2022

gregory-m commented Oct 26, 2022

debug/elf: don't check section size #33121

debug/elf: don't check section size #33121

Comments

gregory-m commented Jul 15, 2019

What version of Go are you using (go version)?

Does this issue reproduce with the latest release?

What did you do?

What did you expect to see?

What did you see instead?

odeke-em commented Jul 15, 2019

ianlancetaylor commented Jul 15, 2019

gregory-m commented Jul 16, 2019

rockdaboot commented Oct 24, 2022 • edited

ZekeLu commented Oct 24, 2022

rockdaboot commented Oct 24, 2022

ianlancetaylor commented Oct 24, 2022

ZekeLu commented Oct 24, 2022

gopherbot commented Oct 24, 2022

ZekeLu commented Oct 25, 2022

gregory-m commented Oct 26, 2022

What version of Go are you using (`go version`)?

rockdaboot commented Oct 24, 2022 •

edited