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

Crash on malformed ELF file #120

Open
Shnatsel opened this issue Mar 5, 2019 · 9 comments

Comments

Projects
None yet
3 participants
@Shnatsel
Copy link

commented Mar 5, 2019

Attempting to decode any of the attached files with goblin::elf::Elf::parse crashes the process. Memory allocator runs out of virtual memory and the process is aborted.

goblin-elf-oom-crashes.zip

Found via AFL.rs. Fuzzing harness: https://github.com/Shnatsel/goblin/blob/master/fuzz-afl/src/main.rs

@m4b

This comment has been minimized.

Copy link
Owner

commented Mar 6, 2019

Many parts of the parser rely on the binary giving lengths to various variable length entities; I guess the best we can do here is to clamp the maximum size to the size of the binary?

@Shnatsel

This comment has been minimized.

Copy link
Author

commented Mar 6, 2019

Sounds like a great idea!

Alternatively you could let the API user set the limits, which is what the decompression libraries do. But if you know the size up front, it's probably best to just use that.

@m4b

This comment has been minimized.

Copy link
Owner

commented Mar 6, 2019

@Shnatsel do you want to make a PR :)

@Shnatsel

This comment has been minimized.

Copy link
Author

commented Mar 6, 2019

It would be interesting, but I'm afraid I'll have to decline. I already have two ongoing Rust projects that have higher priority, and one of them isn't even announced yet.

@philipc

This comment has been minimized.

Copy link
Collaborator

commented Mar 7, 2019

In general, how does goblin want to handle errors: parse as much as it can, or return an error immediately when something is obviously wrong? For example, if the size of a section is larger than the size of the file, should it still try to parse whatever it can in that section? Or should it skip that section but keep trying to parse other sections? Or should it immediately return an error?

I can have a look at some of these. However, not all of them are giving errors for me.
Running:

for i in all-oom-crashes/*; do target/debug/examples/rdr $i 2&>1 >/dev/null && echo $i; done

gives:

all-oom-crashes/id:000000,sig:06,src:000001,op:flip1,pos:135
all-oom-crashes/id:000000,sig:06,src:000001,op:havoc,rep:32
all-oom-crashes/id:000000,sig:06,src:000001,op:havoc,rep:4
all-oom-crashes/id:000000,sig:06,src:000001,op:havoc,rep:64
all-oom-crashes/id:000001,sig:06,src:000001,op:flip1,pos:135
all-oom-crashes/id:000001,sig:06,src:000001,op:havoc,rep:128
all-oom-crashes/id:000001,sig:06,src:000001,op:havoc,rep:32
all-oom-crashes/id:000001,sig:06,src:000001,op:havoc,rep:64
all-oom-crashes/id:000001,sig:06,src:000016+000133,op:splice,rep:128
all-oom-crashes/id:000001,sig:06,src:000027,op:havoc,rep:16
all-oom-crashes/id:000001,sig:06,src:000094+000140,op:splice,rep:64
all-oom-crashes/id:000002,sig:06,src:000001+000048,op:splice,rep:128
all-oom-crashes/id:000002,sig:06,src:000001,op:havoc,rep:32
all-oom-crashes/id:000002,sig:06,src:000029+000342,op:splice,rep:64
all-oom-crashes/id:000002,sig:06,src:000031,op:havoc,rep:16
all-oom-crashes/id:000002,sig:06,src:000107+000371,op:splice,rep:128
all-oom-crashes/id:000002,sig:06,src:000123+000378,op:splice,rep:64
all-oom-crashes/id:000003,sig:06,src:000001,op:havoc,rep:128
all-oom-crashes/id:000003,sig:06,src:000009+000190,op:splice,rep:128
all-oom-crashes/id:000003,sig:06,src:000088+000299,op:splice,rep:128
all-oom-crashes/id:000003,sig:06,src:000107+000371,op:splice,rep:2
all-oom-crashes/id:000004,sig:06,src:000001,op:havoc,rep:128
all-oom-crashes/id:000004,sig:06,src:000006,op:havoc,rep:16
all-oom-crashes/id:000004,sig:06,src:000106,op:havoc,rep:2
all-oom-crashes/id:000004,sig:06,src:000111+000267,op:splice,rep:32
all-oom-crashes/id:000004,sig:06,src:000316+000306,op:splice,rep:128
all-oom-crashes/id:000005,sig:06,src:000001,op:havoc,rep:128
all-oom-crashes/id:000005,sig:06,src:000130,op:havoc,rep:2
all-oom-crashes/id:000005,sig:06,src:000147+000331,op:splice,rep:128
all-oom-crashes/id:000005,sig:06,src:000173,op:havoc,rep:4
all-oom-crashes/id:000006,sig:06,src:000106+000413,op:splice,rep:2
all-oom-crashes/id:000006,sig:06,src:000164+000258,op:splice,rep:32
all-oom-crashes/id:000006,sig:06,src:000359+000301,op:splice,rep:64
all-oom-crashes/id:000006,sig:06,src:000370+000361,op:splice,rep:8
all-oom-crashes/id:000007,sig:06,src:000012+000307,op:splice,rep:16
all-oom-crashes/id:000007,sig:06,src:000039+000227,op:splice,rep:128
all-oom-crashes/id:000007,sig:06,src:000280,op:havoc,rep:64
all-oom-crashes/id:000007,sig:06,src:000373+000330,op:splice,rep:32
all-oom-crashes/id:000007,sig:06,src:000405,op:havoc,rep:128
all-oom-crashes/id:000008,sig:06,src:000019+000207,op:splice,rep:128
all-oom-crashes/id:000008,sig:06,src:000064+000316,op:splice,rep:32
all-oom-crashes/id:000008,sig:06,src:000132+000195,op:splice,rep:64
all-oom-crashes/id:000008,sig:06,src:000362,op:flip1,pos:199
all-oom-crashes/id:000008,sig:06,src:000439+000429,op:splice,rep:128
all-oom-crashes/id:000008,sig:06,src:000445,op:havoc,rep:128
all-oom-crashes/id:000008,sig:06,src:000468,op:havoc,rep:64
all-oom-crashes/id:000009,sig:06,src:000105+000410,op:splice,rep:32
all-oom-crashes/id:000009,sig:06,src:000152+000210,op:splice,rep:128
all-oom-crashes/id:000009,sig:06,src:000162+000255,op:splice,rep:64
all-oom-crashes/id:000009,sig:06,src:000449,op:havoc,rep:64
all-oom-crashes/id:000009,sig:06,src:000462,op:havoc,rep:32
all-oom-crashes/id:000010,sig:06,src:000008+000197,op:splice,rep:64
all-oom-crashes/id:000010,sig:06,src:000057+000278,op:splice,rep:128
all-oom-crashes/id:000010,sig:06,src:000121+000443,op:splice,rep:128
all-oom-crashes/id:000010,sig:06,src:000547,op:havoc,rep:64
all-oom-crashes/id:000011,sig:06,src:000115+000003,op:splice,rep:128
all-oom-crashes/id:000011,sig:06,src:000231,op:havoc,rep:128
all-oom-crashes/id:000011,sig:06,src:000448,op:havoc,rep:64
all-oom-crashes/id:000011,sig:06,src:000641+000403,op:splice,rep:64
all-oom-crashes/id:000012,sig:06,src:000128,op:havoc,rep:8
all-oom-crashes/id:000012,sig:06,src:000133,op:havoc,rep:128
all-oom-crashes/id:000012,sig:06,src:000644+000138,op:splice,rep:32
all-oom-crashes/id:000012,sig:06,src:000649+000129,op:splice,rep:128
all-oom-crashes/id:000013,sig:06,src:000052+000303,op:splice,rep:128
all-oom-crashes/id:000013,sig:06,src:000133,op:havoc,rep:128
all-oom-crashes/id:000013,sig:06,src:000410,op:havoc,rep:64
all-oom-crashes/id:000014,sig:06,src:000281+000336,op:splice,rep:64
all-oom-crashes/id:000014,sig:06,src:000690,op:havoc,rep:32
all-oom-crashes/id:000015,sig:06,src:000690,op:havoc,rep:64
all-oom-crashes/id:000016,sig:06,src:000214+000230,op:splice,rep:128
all-oom-crashes/id:000016,sig:06,src:000339,op:havoc,rep:128
all-oom-crashes/id:000016,sig:06,src:000349+000075,op:splice,rep:64
all-oom-crashes/id:000016,sig:06,src:000641+000349,op:splice,rep:128
all-oom-crashes/id:000019,sig:06,src:000328+000071,op:splice,rep:64
all-oom-crashes/id:000019,sig:06,src:000376+000232,op:splice,rep:8
all-oom-crashes/id:000021,sig:06,src:000366+000186,op:splice,rep:16
all-oom-crashes/id:000021,sig:06,src:000468+000227,op:splice,rep:128
@m4b

This comment has been minimized.

Copy link
Owner

commented Mar 7, 2019

These are all good questions, and I'm not sure what the best answer is; I think we've tried to be as robust as possible in the past, but it starts getting into a grey area; overall however, I've found it almost always better for the parser to just keep going even when stuff is kind of broken, as opposed to just returning immediately and stopping all parsing, since that forces you to dive into a hexdump or some other tool if you want more details and the error message isn't super useful.

@philipc

This comment has been minimized.

Copy link
Collaborator

commented Mar 7, 2019

For my purposes, I think what I want is for goblin to provide lower level APIs that parse one thing, and return an error if that fails. It's then up to me to decide whether I want to continue trying to parse other things. goblin doesn't currently provide that sort of lower level API though. It tries to parse everything into a struct Elf, and for that API I agree that's it's better to try to keep going, otherwise one little error prevents you from getting anything.

@philipc

This comment has been minimized.

Copy link
Collaborator

commented Mar 7, 2019

Fixed what I could in #121. There's probably more similar problems. Not something I'm going to work on right now though.

@philipc

This comment has been minimized.

Copy link
Collaborator

commented Mar 7, 2019

Representative test cases for the problems I fixed:

all-oom-crashes/id:000000,sig:06,src:000001,op:flip1,pos:135
all-oom-crashes/id:000005,sig:06,src:000363,op:havoc,rep:2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.