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.

Sign up for GitHub

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

Jump to bottom

debug/elf: index out of range triggered by DynamicSymbols() #56429

Closed
rockdaboot opened this issue Oct 26, 2022 · 4 comments
Closed

debug/elf: index out of range triggered by DynamicSymbols() #56429

rockdaboot opened this issue Oct 26, 2022 · 4 comments
Assignees
@thanm
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.20

Comments

@rockdaboot
Copy link

rockdaboot commented Oct 26, 2022 •
edited

A malformed ELF file (generated through fuzzing, attached) caused this issue when calling DynamicSymbols().

Stacktrace

/home/tim/src/go/bin/go test -run=FuzzGetBuildID/af27d09108d1449f--- FAIL: FuzzGetBuildID (0.08s)
    --- FAIL: FuzzGetBuildID/af27d09108d1449f (0.00s)
panic: runtime error: index out of range [1] with length 1 [recovered]
        panic: runtime error: index out of range [1] with length 1

goroutine 37 [running]:
testing.tRunner.func1.2({0xfe4dc0, 0xc000040e58})
        /home/tim/src/go/src/testing/testing.go:1451 +0x24e
testing.tRunner.func1()
        /home/tim/src/go/src/testing/testing.go:1454 +0x39f
panic({0xfe4dc0, 0xc000040e58})
        /home/tim/src/go/src/runtime/panic.go:884 +0x213
encoding/binary.littleEndian.Uint16(...)
        /home/tim/src/go/src/encoding/binary/binary.go:62
debug/elf.(*File).gnuVersion(0xc0005fed20, 0xc0001bc7e0?)
        /home/tim/src/go/src/debug/elf/file.go:1578 +0x77
debug/elf.(*File).DynamicSymbols(0xc0000cec80?)
        /home/tim/src/go/src/debug/elf/file.go:1457 +0xaa
github.com/optimyze/prodfiler/libpf/pfelf.GetDynamicSymbols(0xc0005fed20?)
        /home/tim/go/src/github.com/elastic/prodfiler/libpf/pfelf/pfelf.go:540 +0x19
github.com/optimyze/prodfiler/libpf/pfelf_test.FuzzGetBuildID.func1(0x0?, {0xc0003e7800?, 0x0?, 0x46c059?})
        /home/tim/go/src/github.com/elastic/prodfiler/libpf/pfelf/pfelf_test.go:80 +0xe9
reflect.Value.call({0xebb0a0?, 0x10e0920?, 0x13?}, {0x1057b29, 0x4}, {0xc000321e60, 0x2, 0x2?})
        /home/tim/src/go/src/reflect/value.go:586 +0xb09
reflect.Value.Call({0xebb0a0?, 0x10e0920?, 0x16fb7c0?}, {0xc000321e60?, 0x1054300?, 0xc0005be8b8?})
        /home/tim/src/go/src/reflect/value.go:370 +0xbc
testing.(*F).Fuzz.func1.1(0xc00043f208?)
        /home/tim/src/go/src/testing/fuzz.go:336 +0x3f3
testing.tRunner(0xc0004ca340, 0xc0004c4090)
        /home/tim/src/go/src/testing/testing.go:1501 +0x10b
created by testing.(*F).Fuzz.func1
        /home/tim/src/go/src/testing/fuzz.go:323 +0x5b9
exit status 2
FAIL    github.com/optimyze/prodfiler/libpf/pfelf       0.182s

The Go version is from latest master (commit 939f9fd).

$ go version
go version devel go1.20-939f9fd64a Wed Oct 26 05:51:33 2022 +0000 linux/amd64

Looking at the code

func (f *File) gnuVersion(i int) (library string, version string) { 
        // Each entry is two bytes.
        i = (i + 1) * 2
        if i >= len(f.gnuVersym) {
                return
        }
        j := int(f.ByteOrder.Uint16(f.gnuVersym[i:])) <--- this line triggers the panic (L1578)
        if j < 2 || j >= len(f.gnuNeed) {
                return
        }
        n := &f.gnuNeed[j]
        return n.File, n.Name
}

Here we can see that if len(f.gnuVersym) is 3 and the input value i is 0, a slice of len 1 is passed to Uint16(). But Uint16 requires the slice to have a len of at least two bytes.

I also wonder if it is intentional that a 0 input accesses the second array entry of f.gnuVersym instead of the first (is there a off-by-one issue hidden here ?).

Reproducer, ELF corpus

af27d09108d1449f.gz

Passing this file to elf.Open() and then calling DynamicSymbols() triggers the issue.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Oct 26, 2022
@rockdaboot
Copy link
Author

cc @ZekeLu (I consider you an expert here 😃)

@ZekeLu
Copy link
Contributor

ZekeLu commented Oct 26, 2022 •
edited

I'm not an expert 😂, but I will give it a try if no one beats me.

I also wonder if it is intentional that a 0 input accesses the second array entry of f.gnuVersym instead of the first (is there a off-by-one issue hidden here ?).

This one is intentional. See the doc: https://pkg.go.dev/debug/elf#File.DynamicSymbols

For compatibility with Symbols, DynamicSymbols omits the null symbol at index 0. After retrieving the symbols as symtab, an externally supplied index x corresponds to symtab[x-1], not symtab[x].

There was a comment when gnuVersion was first written:

go/src/pkg/debug/elf/file.go

Lines 651 to 653 in 09092a7

func (f *File) gnuVersion(i int, sym *ImportedSymbol) {
// Each entry is two bytes; skip undef entry at beginning.
i = (i + 1) * 2

The comment was changed in 8c96e6d. Later 5060dde reverted (?) the change, but the comment was not added back. I will bring back the comment while at here.

@ZekeLu ZekeLu mentioned this issue Oct 26, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/445555 mentions this issue: debug/elf: guard access to File.gnuVersym

@heschi
Copy link
Contributor

heschi commented Oct 26, 2022

cc @golang/compiler

@heschi heschi added this to the Go1.20 milestone Oct 26, 2022
@heschi heschi added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 26, 2022
romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Nov 3, 2022
@ZekeLu @romaindoumenc
debug/elf: guard access to File.gnuVersym
5178378 
The size of gnuVersym should be multiples of 2. If not, the input is
invalid. No Library and Version information is added to sym in this
case. The current implementation of gnuVersion does not report errors
for invalid input.

While at here, bring back the comment that states that the undef entry
at the beginning is skipped. This is not an off-by-one error.

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

Fixes golang#56429.

Change-Id: Ia39ad8bd509088a81cc77f7a76e23185d40a5765
GitHub-Last-Rev: 3be0cc1
GitHub-Pull-Request: golang#56431
Reviewed-on: https://go-review.googlesource.com/c/go/+/445555
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Run-TryBot: Meng Zhuo <mzh@golangcn.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@golang golang locked and limited conversation to collaborators Oct 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@thanm thanm

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.20
Development

Successfully merging a pull request may close this issue.

debug/elf: guard access to File.gnuVersym ZekeLu/go
5 participants
@ZekeLu @rockdaboot @gopherbot @thanm @heschi