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
prash2611 opened this issue May 18, 2019 · 7 comments
Closed

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

prash2611 opened this issue May 18, 2019 · 7 comments
Milestone

Comments

@prash2611
Copy link
Contributor

@prash2611 prash2611 commented May 18, 2019

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 18, 2019

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
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented May 19, 2019

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

Alex

@prash2611

This comment has been minimized.

Copy link
Contributor Author

@prash2611 prash2611 commented May 19, 2019

Thanks @alexbrainman, looking forward to your review comments.

@prash2611 prash2611 closed this May 19, 2019
@prash2611 prash2611 reopened this May 19, 2019
@nokute78

This comment has been minimized.

Copy link

@nokute78 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}]
@prash2611

This comment has been minimized.

Copy link
Contributor Author

@prash2611 prash2611 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

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Aug 9, 2019

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

This comment has been minimized.

Copy link

@nokute78 nokute78 commented Aug 9, 2019

@prash2611 @alexbrainman
Thanks.

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

@gopherbot gopherbot closed this in 3b92f36 Aug 29, 2019
tomocy added 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 added 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.