Page MenuHomePhabricator

[libFuzzer] Rewrite Linux's ExecuteCommand to use fork-exec instead of system().
AbandonedPublic

Authored by Dor1s on Jun 26 2020, 1:48 PM.

Details

Summary

User may pass testcases with weird filesnames (e.g. from other fuzzing
engines) which may result in a shell injection via system() calls when
ExecuteCommand() method is called with a bad file path.

https://github.com/google/oss-fuzz/issues/3886

Diff Detail

Event Timeline

Dor1s created this revision.Jun 26 2020, 1:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2020, 1:48 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
Dor1s marked an inline comment as done.Jun 26 2020, 1:50 PM
Dor1s added inline comments.
compiler-rt/lib/fuzzer/FuzzerCommand.h
72 ↗(On Diff #273830)

I certainly think it's not good to have platform-specific thing here, but the other solution I had in mind was to get rid of system(), which would be a significant refactoring.

Any other ideas are welcome!

morehouse added inline comments.Jun 29 2020, 10:27 AM
compiler-rt/lib/fuzzer/FuzzerCommand.h
72 ↗(On Diff #273830)

Can we refactor this into the platform-specific files?

175 ↗(On Diff #273830)

Nit: let's not change this whitespace (same below)

Thanks!

compiler-rt/lib/fuzzer/FuzzerCommand.h
72 ↗(On Diff #273830)

Is the branching even necessary what's wrong with doing quotes for both?

Also, are we sure this method will catch all cases? What if there are quotes in the name of the file?

I think ideally we would replace system with a call that runs a shell command that doesn't have a problem parsing args (e.g. if this were os.system in python I'd replace with subprocess.run), but that may be too much effort.

Dor1s marked 2 inline comments as done.Jun 29 2020, 4:06 PM
Dor1s added inline comments.
compiler-rt/lib/fuzzer/FuzzerCommand.h
72 ↗(On Diff #273830)

I've started it initially but then it seemed like too much code. Will give another try.

72 ↗(On Diff #273830)

It errors out on Windows with single quotes :/

Right, using e.g. execv would be much better, but it feels like too much unnecessary effort at this point. I don't think sane users supply files with weird symbols in names.

Dor1s updated this revision to Diff 274658.Jun 30 2020, 5:19 PM
Dor1s marked 4 inline comments as done.

split the implementation into platform specific files

Dor1s added a comment.Jun 30 2020, 5:21 PM

I've split the implementation into platform specific files, but I still don't like it. Plus, as Jonathan mentioned, it still has edge cases e.g. if a filename has a single quote. I'll give a shot at refactoring ExecuteCommand into fork-exec. It looks like I need to re-write only Posix implementation, as Fuchsia is already doing good and Windows is less prone to such issues due to its more restrictive file naming.

So, don't review this yet :)

compiler-rt/lib/fuzzer/FuzzerCommand.h
175 ↗(On Diff #273830)

oops

Dor1s updated this revision to Diff 274910.Jul 1 2020, 1:51 PM

Re-write to use fork-exec instead of system() on Linux.

Dor1s retitled this revision from [libFuzzer] Add Command::addPathArgument method to wrap bad paths with quotes. to [libFuzzer] Rewrite Linux's ExecuteCommand to use fork-exec instead of system()..Jul 1 2020, 1:52 PM
Dor1s added a reviewer: kcc.
Dor1s removed a subscriber: kcc.

Ready for review, PTAL.

Dor1s added a comment.Jul 1 2020, 3:23 PM

Ready for review, PTAL.

False start, looks like this needs more changes.

Dor1s added a comment.Jul 1 2020, 4:05 PM

Should be better now, ready for review again :)

morehouse added inline comments.Jul 1 2020, 4:55 PM
compiler-rt/lib/fuzzer/FuzzerUtilLinux.cpp
41

This isn't quite right. We still want the 2>&1 behavior when there isn't an output file.

Dor1s updated this revision to Diff 274960.Jul 1 2020, 5:01 PM

Updated the version of ExecuteCommand with piped output.

Dor1s marked 2 inline comments as done.Jul 1 2020, 5:02 PM
Dor1s added inline comments.
compiler-rt/lib/fuzzer/FuzzerUtilLinux.cpp
41

oops, just noticed that my last uploading got stuck on pre-submit linter -- please see lines 45-46, that case is addressed now too

morehouse added inline comments.Jul 6 2020, 9:40 AM
compiler-rt/lib/fuzzer/FuzzerUtilLinux.cpp
42

I think we can remove this and just do:

if (Cmd.hasOutputFile()) {
  ...
}
if (Cmd.isOutAndErrCombined())
  dup2(1, 2);
68

If Cmd already has an output file, we should probably just use that one.

kcc added a comment.Jul 6 2020, 10:54 AM

My preference would be to reject weird file names instead of adding this extra complexity.

Dor1s marked an inline comment as done.Jul 6 2020, 11:24 AM
In D82685#2133565, @kcc wrote:

My preference would be to reject weird file names instead of adding this extra complexity.

so we'll have a list of allowed (or disallowed) characters and error out if any of the arguments passed do not comply?

kcc added a comment.Jul 6 2020, 11:25 AM
In D82685#2133565, @kcc wrote:

My preference would be to reject weird file names instead of adding this extra complexity.

so we'll have a list of allowed (or disallowed) characters and error out if any of the arguments passed do not comply?

yep.

Dor1s marked an inline comment as done.Jul 6 2020, 11:26 AM
Dor1s added inline comments.
compiler-rt/lib/fuzzer/FuzzerUtilLinux.cpp
42

I thought the same but it didn't seem to work! Will double check again if we decide to proceed with this change.

Dor1s abandoned this revision.Jul 10 2020, 3:31 PM

Alright, we've ended up disabling that behavior in honggfuzz build, so I'll just close this CL for now.