[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

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

Can we refactor this into the platform-specific files?

175

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

[metzman]

Thanks!

compiler-rt/lib/fuzzer/FuzzerCommand.h

72

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

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

72

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

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 ↗

(On Diff #274910)

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 ↗

(On Diff #274910)

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 ↗

(On Diff #274960)

I think we can remove this and just do:

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

68 ↗

(On Diff #274960)

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 ↗

(On Diff #274960)

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/FuzzerCommand.h

public:
   const Vector<std::string> &getArguments() const { return Args; }
 
   // Adds the given argument before "-ignore_remaining_args=1", or at the end
   // if that flag isn't present.
   void addArgument(const std::string &Arg) {
     Args.insert(endMutableArgs(), Arg);
   }
 
+  // Add the given path argument before "-ignore_remaining_args=1", or at the
+  // end if that flag isn't present.
+  void addPathArgument(const std::string &Path) {
+#ifdef LIBFUZZER_WINDOWS
+    addArgument(Path);
+#else
+    // Wrap the path with single quotes to avoid shell injection into system().
+    addArgument("'" + Path + "'");
+#endif // LIBFUZZER_WINDOWS
+  }
+
   // Adds all given arguments before "-ignore_remaining_args=1", or at the end
   // if that flag isn't present.
   void addArguments(const Vector<std::string> &ArgsToAdd) {
     Args.insert(endMutableArgs(), ArgsToAdd.begin(), ArgsToAdd.end());
   }
 
   // Removes the given argument from the command argument list.  Ignores any
   // occurrences after "-ignore_remaining_args=1", if present.
private:
   Vector<std::string> Args;
 
   // True indicates stderr is redirected to stdout.
   bool CombinedOutAndErr;
 
   // If not empty, stdout is redirected to the named file.
   std::string OutputFile;
 };
-
 } // namespace fuzzer
 
 #endif // LLVM_FUZZER_COMMAND_H

compiler-rt/lib/fuzzer/FuzzerDataFlowTrace.cpp

for (auto &F : CorporaFiles) {
     // Data flow collection may fail if we request too many DFSan tags at once.
     // So, we start from requesting all tags in range [0,Size) and if that fails
     // we then request tags in [0,Size/2) and [Size/2, Size), and so on.
     // Function number => DFT.
     auto OutPath = DirPlusFile(DirPath, Hash(FileToVector(F.File)));
     std::unordered_map<size_t, Vector<uint8_t>> DFTMap;
     std::unordered_set<std::string> Cov;
     Command Cmd;
-    Cmd.addArgument(DFTBinary);
-    Cmd.addArgument(F.File);
-    Cmd.addArgument(OutPath);
+    Cmd.addPathArgument(DFTBinary);
+    Cmd.addPathArgument(F.File);
+    Cmd.addPathArgument(OutPath);
     Printf("CMD: %s\n", Cmd.toString().c_str());
     ExecuteCommand(Cmd);
   }
   // Write functions.txt if it's currently empty or doesn't exist.
   auto FunctionsTxtPath = DirPlusFile(DirPath, kFunctionsTxt);
   if (FileToString(FunctionsTxtPath).empty()) {
     Command Cmd;
-    Cmd.addArgument(DFTBinary);
+    Cmd.addPathArgument(DFTBinary);
     Cmd.setOutputFile(FunctionsTxtPath);
     ExecuteCommand(Cmd);
   }
   return 0;
 }
 
 }  // namespace fuzzer

compiler-rt/lib/fuzzer/FuzzerDriver.cpp

int MinimizeCrashInput(const Vector<std::string> &Args,
 
   std::string CurrentFilePath = InputFilePath;
   while (true) {
     Unit U = FileToVector(CurrentFilePath);
     Printf("CRASH_MIN: minimizing crash input: '%s' (%zd bytes)\n",
            CurrentFilePath.c_str(), U.size());
 
     Command Cmd(BaseCmd);
-    Cmd.addArgument(CurrentFilePath);
+    Cmd.addPathArgument(CurrentFilePath);
 
     Printf("CRASH_MIN: executing: %s\n", Cmd.toString().c_str());
     std::string CmdOutput;
     bool Success = ExecuteCommand(Cmd, &CmdOutput);
     if (Success) {
       Printf("ERROR: the input %s did not crash\n", CurrentFilePath.c_str());
       exit(1);
     }

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.addPathArgument(Job->CorpusDir);
     Cmd.addFlag("features_dir", Job->FeaturesDir);
 
     for (auto &D : {Job->CorpusDir, Job->FeaturesDir}) {
       RmDirRecursive(D);
       MkDir(D);
     }
 
     Cmd.setOutputFile(Job->LogPath);
struct GlobalEnv {
 
   void CollectDFT(const std::string &InputPath) {
     if (DataFlowBinary.empty()) return;
     if (!FilesWithDFT.insert(InputPath).second) return;
     Command Cmd(Args);
     Cmd.removeFlag("fork");
     Cmd.removeFlag("runs");
     Cmd.addFlag("data_flow_trace", DFTDir);
-    Cmd.addArgument(InputPath);
+    Cmd.addPathArgument(InputPath);
     for (auto &C : CorpusDirs) // Remove all corpora from the args.
       Cmd.removeArgument(C);
     Cmd.setOutputFile(DirPlusFile(TempDir, "dft.log"));
     Cmd.combineOutAndErr();
     // Printf("CollectDFT: %s\n", Cmd.toString().c_str());
     ExecuteCommand(Cmd);
   }
 

compiler-rt/lib/fuzzer/FuzzerUtil.h

inline uint8_t *RoundUpByPage(uint8_t *P) {
   return reinterpret_cast<uint8_t *>(X);
 }
 inline uint8_t *RoundDownByPage(uint8_t *P) {
   uintptr_t X = reinterpret_cast<uintptr_t>(P);
   size_t Mask = PageSize() - 1;
   X = X & ~Mask;
   return reinterpret_cast<uint8_t *>(X);
 }
-
 }  // namespace fuzzer
 
 #endif  // LLVM_FUZZER_UTIL_H

compiler-rt/test/fuzzer/minimize_crash.test

 RUN: rm %t.dir/not_minimal_crash %t.exact_minimized_path
 
 RUN: echo -n 'abcd*xyz' > %t.dir/not_minimal_crash
 RUN: %run %t-SingleByteInputTest -minimize_crash=1 %t.dir/not_minimal_crash -exact_artifact_path=%t.exact_minimized_path 2>&1 | FileCheck %s --check-prefix=MIN1
 MIN1: Test unit written to {{.*}}exact_minimized_path
 MIN1: Test unit written to {{.*}}exact_minimized_path
 MIN1: INFO: The input is small enough, exiting
 MIN1: CRASH_MIN: failed to minimize beyond {{.*}}exact_minimized_path (1 bytes), exiting
+
+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