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

[Dor1s]

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

[Dor1s]

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]

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)

[metzman]

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]

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]

split the implementation into platform specific files

[Dor1s]

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]

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

[Dor1s]

Ready for review, PTAL.

[Dor1s]

Ready for review, PTAL.

False start, looks like this needs more changes.

[Dor1s]

Should be better now, ready for review again :)

[morehouse]

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 the version of ExecuteCommand with piped output.

[Dor1s]

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]

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]

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

[Dor1s]

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]

In D82685#2133646, @Dor1s wrote:

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]

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]

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

compiler-rt/lib/fuzzer/FuzzerFork.cpp

if (!Seeds.empty()) {
       Cmd.addFlag("seed_inputs", "@" + Job->SeedListPath);
     }
     Job->LogPath = DirPlusFile(TempDir, std::to_string(JobId) + ".log");
     Job->CorpusDir = DirPlusFile(TempDir, "C" + std::to_string(JobId));
     Job->FeaturesDir = DirPlusFile(TempDir, "F" + std::to_string(JobId));
     Job->CFPath = DirPlusFile(TempDir, std::to_string(JobId) + ".merge");
     Job->JobId = JobId;
 
-
     Cmd.addArgument(Job->CorpusDir);
     Cmd.addFlag("features_dir", Job->FeaturesDir);
 
     for (auto &D : {Job->CorpusDir, Job->FeaturesDir}) {
       RmDirRecursive(D);
       MkDir(D);
     }
 

compiler-rt/lib/fuzzer/FuzzerUtilDarwin.cpp

if (ActiveThreadCount == 0) {
       }
       if (FailedRestore)
         ProcessStatus = -1;
     }
   }
   return ProcessStatus;
 }
 
+// Return true on success, false otherwise.
+bool ExecuteCommand(const Command &Cmd, std::string *CmdOutput) {
+  FILE *Pipe = popen(Cmd.toString().c_str(), "r");
+  if (!Pipe)
+    return false;
+
+  if (CmdOutput) {
+    char TmpBuffer[128];
+    while (fgets(TmpBuffer, sizeof(TmpBuffer), Pipe))
+      CmdOutput->append(TmpBuffer);
+  }
+  return pclose(Pipe) == 0;
+}
+
 void DiscardOutput(int Fd) {
   FILE* Temp = fopen("/dev/null", "w");
   if (!Temp)
     return;
   dup2(fileno(Temp), Fd);
   fclose(Temp);
 }
 
 } // namespace fuzzer
 
 #endif // LIBFUZZER_APPLE

compiler-rt/lib/fuzzer/FuzzerUtilLinux.cpp

 #include <sys/types.h>
 #include <sys/wait.h>
 #include <unistd.h>
 
 
 namespace fuzzer {
 
 int ExecuteCommand(const Command &Cmd) {
-  std::string CmdLine = Cmd.toString();
-  int exit_code = system(CmdLine.c_str());
-  if (WIFEXITED(exit_code))
-    return WEXITSTATUS(exit_code);
-  return exit_code;
+  int ExitCode, ProcessStatus = 0;
+  pid_t Pid = fork();
+
+  if (Pid == 0) { // Child process. Execute the given command here.
+    auto Args = Cmd.getArguments();
+    Vector<const char *> Argv(Args.size() + 1, nullptr);
+    std::transform(
+        Args.begin(), Args.end(), Argv.begin(),
+        [](const std::string &S) -> auto { return S.c_str(); });
+
+    if (Cmd.hasOutputFile()) {
+      FILE *OutputFile = fopen(Cmd.getOutputFile().c_str(), "w");
+      if (!OutputFile)
+        return -1;
+
+      dup2(fileno(OutputFile), 1);
+
+      if (Cmd.isOutAndErrCombined())
+        dup2(fileno(OutputFile), 2);
+
+      fclose(OutputFile);
+    } else if (Cmd.isOutAndErrCombined())
+      dup2(1, 2);
+
+    exit(execvp(Argv[0], const_cast<char *const *>(Argv.data())));
+  } else if (Pid > 0) { // Parent process. Wait for the child's exit code.
+    Pid = waitpid(Pid, &ProcessStatus, 0);
+    if (Pid == -1)
+      ExitCode = -1;
+    else
+      ExitCode = ProcessStatus;
+  } else {
+    ExitCode = -1;
+  }
+
+  if (WIFEXITED(ExitCode))
+    return WEXITSTATUS(ExitCode);
+  return ExitCode;
+}
+
+// Return true on success, false otherwise.
+bool ExecuteCommand(const Command &BaseCmd, std::string *CmdOutput) {
+  auto LogFilePath = TempPath("SimPopenOut", ".txt");
+  Command Cmd(BaseCmd);
+  Cmd.setOutputFile(LogFilePath);
+  int Ret = ExecuteCommand(Cmd);
+  *CmdOutput = FileToString(LogFilePath);
+  RemoveFile(LogFilePath);
+  return Ret == 0;
 }
 
 void DiscardOutput(int Fd) {
   FILE* Temp = fopen("/dev/null", "w");
   if (!Temp)
     return;
   dup2(fileno(Temp), Fd);
   fclose(Temp);
 }
 
 } // namespace fuzzer
 
 #endif

compiler-rt/lib/fuzzer/FuzzerUtilPosix.cpp

static void SetSigaction(int signum,
   sigact.sa_flags = SA_SIGINFO;
   sigact.sa_sigaction = callback;
   if (sigaction(signum, &sigact, 0)) {
     Printf("libFuzzer: sigaction failed with %d\n", errno);
     exit(1);
   }
 }
 
-// Return true on success, false otherwise.
-bool ExecuteCommand(const Command &Cmd, std::string *CmdOutput) {
-  FILE *Pipe = popen(Cmd.toString().c_str(), "r");
-  if (!Pipe)
-    return false;
-
-  if (CmdOutput) {
-    char TmpBuffer[128];
-    while (fgets(TmpBuffer, sizeof(TmpBuffer), Pipe))
-      CmdOutput->append(TmpBuffer);
-  }
-  return pclose(Pipe) == 0;
-}
-
 void SetTimer(int Seconds) {
   struct itimerval T {
     {Seconds, 0}, { Seconds, 0 }
   };
   if (setitimer(ITIMER_REAL, &T, nullptr)) {
     Printf("libFuzzer: setitimer failed with %d\n", errno);
     exit(1);
   }

compiler-rt/test/fuzzer/minimize_crash_with_bad_filename.test

+// Test verifies that libFuzzer handles filenames that may break shell commands.
+UNSUPPORTED: darwin, windows
+
+RUN: %cpp_compiler %S/NullDerefTest.cpp -o %t-NullDerefTest
+RUN: mkdir -p %t.dir
+
+RUN: echo -n 'Hi!AAA' > '%t.dir/bad;(%file)name'
+RUN: %run %t-NullDerefTest -minimize_crash=1 '%t.dir/bad;(%file)name' -max_total_time=2 2>&1 | FileCheck %s --check-prefix=BAD_FILENAME
+BAD_FILENAME: failed to minimize beyond