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

debug/pe: fails to parse variable length optional header #32126

Closed
pra-msft opened this issue May 18, 2019 · 7 comments
Closed

debug/pe: fails to parse variable length optional header #32126

pra-msft opened this issue May 18, 2019 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@pra-msft
Copy link
Contributor

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

$ go version
go version go1.11.5 linux/amd64

Does this issue reproduce with the latest release?

Yes it does! The issue is present is current master.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/prashant/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/prashant/go2"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build980405246=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I was trying to parse a PE file (linux Kernel to be specific) using debug/pe package.
A toy program for the same would look like:

package main

import (
    "debug/pe"
    "fmt"
)

func main() {
    file, err := pe.Open("/boot/vmlinuz-4.15.0-48-generic")
    if err != nil {
        file.Close()
        return
    }
    defer file.Close()

    fmt.Println(file.OptionalHeader)
}

What did you expect to see?

I would like to see parsed optional header.

What did you see instead?

Optional header turned out to be nil.

What is actual bug?

The debug/pe package assumes there are always 16 entries in [16]DataDirectory in OptionalHeader32/64 (ref pe.go)
NumberOfRvaAndSizes uint32
DataDirectory [16]DataDirectory
}

But that is not always the case, there could be less no of entries (PE signed linux kernel for example):
prashant@Pra-Work:~$ sudo pev /boot/vmlinuz-4.15.0-47-generic
....
Data-dictionary entries: 6
....

In such case, the parsing gives incorrect results.

@gopherbot
Copy link

Change https://golang.org/cl/177959 mentions this issue: debug/pe: enables parsing of variable length optional header in PE file

@ALTree ALTree added this to the Go1.14 milestone May 18, 2019
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 18, 2019
@alexbrainman
Copy link
Member

@prash2611 thank you for reporting the problem. I will review your change when I have time.

Alex

@pra-msft
Copy link
Contributor Author

Thanks @alexbrainman, looking forward to your review comments.

@pra-msft pra-msft reopened this May 19, 2019
@nokute78
Copy link

nokute78 commented Aug 8, 2019

Hi.
I have an similar issue and this patch helps me.
https://go-review.googlesource.com/c/go/+/177959

I tested this code.
https://gist.github.com/nokute78/46c1eb6a2f6050db4c5a87845dbdf87c

I tested using Ubuntu 18.04 kernel (vmlinuz-4.18.0-25-generic)

taka@ubuntu:~/tmp$ sudo cp /boot/vmlinuz-4.18.0-25-generic .
taka@ubuntu:~/tmp$ sudo chmod +r vmlinuz-4.18.0-25-generic
taka@ubuntu:~/tmp$ go version
go version go1.12.7 linux/amd64
taka@ubuntu:~/tmp$ go run pe.go vmlinuz-4.18.0-25-generic 
invalid format
exit status 1

With the patch, my program worked correctly.

taka@ubuntu:~/tmp$ ~/git/go/bin/go version
go version devel +0cb90728c9 Mon Jun 17 11:31:57 2019 -0700 linux/amd64
taka@ubuntu:~/tmp$ ~/git/go/bin/go run pe.go vmlinuz-4.18.0-25-generic 
Header64
dataDir:[{0 0} {0 0} {0 0} {0 0} {8550272 1912} {0 0} {0 0} {0 0} {0 0} {0 0} {0 0} {0 0} {0 0} {0 0} {0 0} {0 0}]

@pra-msft
Copy link
Contributor Author

pra-msft commented Aug 8, 2019

Thanks @nokute78, glad to know that the patch solves your issue!

I will let @alexbrainman comment on when it will be merged in master and GA.

@alexbrainman
Copy link
Member

I have an similar issue and this patch helps me.
https://go-review.googlesource.com/c/go/+/177959

I am glad to know. Thank you for confirming.

We cannot merge this change onto master branch until after go1.13 is released. Everyone is waiting for that.

Alex

@nokute78
Copy link

nokute78 commented Aug 9, 2019

@prash2611 @alexbrainman
Thanks.

I’m also looking forward to Go 1.13.
So, I will wait the patch patiently.

tomocy pushed a commit to tomocy/go that referenced this issue Sep 1, 2019
The debug/pe package assumes there are always 16 entries in
DataDirectory in OptionalHeader32/64
ref pe.go:
...
NumberOfRvaAndSizes uint32
DataDirectory [16]DataDirectory
}
...

But that is not always the case, there could be less no of
entries (PE signed linux kernel for example):
$ sudo pev /boot/vmlinuz-4.15.0-47-generic
....
Data-dictionary entries:	6
....

In such case, the parsing gives incorrect results.
This changes aims to fix that by:
1. Determining type of optional header by looking at header
   magic instead of size
2. Parsing optional header in 2 steps:
   a. Fixed part
   b. Variable data directories part

Testing:
1. Fixed existing test cases to reflect the change
2. Added new file (modified linux kernel image)
   which has smaller number of data directories

Fixes golang#32126

Change-Id: Iee56ecc4369a0e75a4be805e7cb8555c7d81ae2f
Reviewed-on: https://go-review.googlesource.com/c/go/+/177959
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
t4n6a1ka pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
The debug/pe package assumes there are always 16 entries in
DataDirectory in OptionalHeader32/64
ref pe.go:
...
NumberOfRvaAndSizes uint32
DataDirectory [16]DataDirectory
}
...

But that is not always the case, there could be less no of
entries (PE signed linux kernel for example):
$ sudo pev /boot/vmlinuz-4.15.0-47-generic
....
Data-dictionary entries:	6
....

In such case, the parsing gives incorrect results.
This changes aims to fix that by:
1. Determining type of optional header by looking at header
   magic instead of size
2. Parsing optional header in 2 steps:
   a. Fixed part
   b. Variable data directories part

Testing:
1. Fixed existing test cases to reflect the change
2. Added new file (modified linux kernel image)
   which has smaller number of data directories

Fixes golang#32126

Change-Id: Iee56ecc4369a0e75a4be805e7cb8555c7d81ae2f
Reviewed-on: https://go-review.googlesource.com/c/go/+/177959
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
@golang golang locked and limited conversation to collaborators Aug 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants