goblin icon indicating copy to clipboard operation
goblin copied to clipboard

Crash on malformed ELF file

Open Shnatsel opened this issue 5 years ago • 11 comments

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

Shnatsel avatar Mar 05 '19 23:03 Shnatsel

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?

m4b avatar Mar 06 '19 02:03 m4b

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.

Shnatsel avatar Mar 06 '19 11:03 Shnatsel

@shnatsel do you want to make a PR :)

m4b avatar Mar 06 '19 16:03 m4b

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.

Shnatsel avatar Mar 06 '19 18:03 Shnatsel

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

philipc avatar Mar 07 '19 06:03 philipc

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.

m4b avatar Mar 07 '19 06:03 m4b

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 avatar Mar 07 '19 06:03 philipc

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

philipc avatar Mar 07 '19 08:03 philipc

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

philipc avatar Mar 07 '19 08:03 philipc

@philipc Ran into a similar issue with section headers and Vec::with_capacity while fuzzing it myself. Took a stab at fixing it here: https://github.com/m4b/goblin/pull/219

jackcmay avatar May 05 '20 22:05 jackcmay

Looks reasonable to me, but I'll let @m4b review. Really need to audit every Vec::with_capacity instead of relying on fuzzing though.

philipc avatar May 06 '20 00:05 philipc