From cf02553ea725cba011b10131fc7d533d27198480 Mon Sep 17 00:00:00 2001 From: clesmian <18008727+clesmian@users.noreply.github.com> Date: Sun, 26 Feb 2023 02:00:28 +0100 Subject: [PATCH] Cleanup forkserver exec builder (#1094) * Don't use magic string but string constant * Don't allow to specify multiple input files with different names * Ensure that the file name for the current test case is unique for every fuzzer currently running * Add note advising users to choose distinct names for the input file * Move builder functions to more generic implementation to allow parse_afl_cmdline rewrite * Rewrite parse_afl_cmdline to reduce code duplication * Add remark to documentation regarding the program path * Change behavior to allow the usage of actual AFL command lines, hopefully without breaking existing code * Rustfmt * Move generation of unique filename to fs * Ensure default input filename for command executor is unique per fuzzing process * Pass the input to the target via stdin, when no input file is specified Previous solution of passing it via a standard file is useless, as the target does not know to read said file * Rustfmt --------- Co-authored-by: Dominik Maier Co-authored-by: Dongjia "toka" Zhang --- libafl/src/bolts/fs.rs | 8 ++ libafl/src/executors/command.rs | 4 +- libafl/src/executors/forkserver.rs | 124 ++++++++++++++++++----------- 3 files changed, 89 insertions(+), 47 deletions(-) diff --git a/libafl/src/bolts/fs.rs b/libafl/src/bolts/fs.rs index 7ea4ad59bc..181a2a02b9 100644 --- a/libafl/src/bolts/fs.rs +++ b/libafl/src/bolts/fs.rs @@ -10,6 +10,7 @@ use std::{ fs::{self, remove_file, File, OpenOptions}, io::{Seek, Write}, path::{Path, PathBuf}, + string::String, }; use crate::Error; @@ -17,6 +18,13 @@ use crate::Error; /// The default filename to use to deliver testcases to the target pub const INPUTFILE_STD: &str = ".cur_input"; +#[must_use] +/// Derives a filename from [`INPUTFILE_STD`] that may be used to deliver testcases to the target. +/// It ensures the filename is unique to the fuzzer process. +pub fn get_unique_std_input_file() -> String { + format!("{}_{}", INPUTFILE_STD, std::process::id()) +} + /// Creates a `.{file_name}.tmp` file, and writes all bytes to it. /// After all bytes have been written, the tmp-file is moved to it's original `path`. /// This way, on the majority of operating systems, the final file will never be incomplete or racey. diff --git a/libafl/src/executors/command.rs b/libafl/src/executors/command.rs index 19e63a6955..1b86f3708a 100644 --- a/libafl/src/executors/command.rs +++ b/libafl/src/executors/command.rs @@ -22,7 +22,7 @@ use super::HasObservers; use crate::executors::{Executor, ExitKind}; use crate::{ bolts::{ - fs::{InputFile, INPUTFILE_STD}, + fs::{get_unique_std_input_file, InputFile}, tuples::MatchName, AsSlice, }, @@ -456,7 +456,7 @@ impl CommandExecutorBuilder { /// Uses a default filename. /// Use [`Self::arg_input_file`] to specify a custom filename. pub fn arg_input_file_std(&mut self) -> &mut Self { - self.arg_input_file(INPUTFILE_STD); + self.arg_input_file(get_unique_std_input_file()); self } diff --git a/libafl/src/executors/forkserver.rs b/libafl/src/executors/forkserver.rs index 51e915e487..45353b1cb0 100644 --- a/libafl/src/executors/forkserver.rs +++ b/libafl/src/executors/forkserver.rs @@ -26,7 +26,7 @@ use nix::{ use crate::{ bolts::{ - fs::{InputFile, INPUTFILE_STD}, + fs::{get_unique_std_input_file, InputFile}, os::{dup2, pipes::Pipe}, shmem::{ShMem, ShMemProvider, UnixShMemProvider}, tuples::Prepend, @@ -607,6 +607,10 @@ pub struct ForkserverExecutorBuilder<'a, SP> { impl<'a, SP> ForkserverExecutorBuilder<'a, SP> { /// Builds `ForkserverExecutor`. + /// This Forkserver will attempt to provide inputs over shared mem when `shmem_provider` is given. + /// Else this forkserver will pass the input to the target via `stdin` + /// in case no input file is specified. + /// If `debug_child` is set, the child will print to `stdout`/`stderr`. #[allow(clippy::pedantic)] pub fn build(&mut self, observers: OT) -> Result, Error> where @@ -691,7 +695,10 @@ impl<'a, SP> ForkserverExecutorBuilder<'a, SP> { { let input_filename = match &self.input_filename { Some(name) => name.clone(), - None => OsString::from(".cur_input"), + None => { + self.use_stdin = true; + OsString::from(get_unique_std_input_file()) + } }; let input_file = InputFile::create(input_filename)?; @@ -821,59 +828,46 @@ impl<'a, SP> ForkserverExecutorBuilder<'a, SP> { #[must_use] /// Parse afl style command line - pub fn parse_afl_cmdline(mut self, args: IT) -> Self + /// + /// Replaces `@@` with the path to the input file generated by the fuzzer. If `@@` is omitted, + /// `stdin` is used to pass the test case instead. + /// + /// Interprets the first argument as the path to the program as long as it is not set yet. + /// You have to omit the program path in case you have set it already. Otherwise + /// it will be interpreted as a regular argument, leading to probably unintended results. + pub fn parse_afl_cmdline(self, args: IT) -> Self where IT: IntoIterator, O: AsRef, { - let mut res = vec![]; - let mut use_stdin = true; + let mut moved = self; + + let mut use_arg_0_as_program = false; + if moved.program.is_none() { + use_arg_0_as_program = true; + } for item in args { - if item.as_ref() == "@@" && use_stdin { - use_stdin = false; - res.push(OsString::from(".cur_input")); - } else if let Some(name) = &self.input_filename { - if name == item.as_ref() && use_stdin { - use_stdin = false; - res.push(name.clone()); + if use_arg_0_as_program { + moved = moved.program(item); + // After the program has been set, unset `use_arg_0_as_program` to treat all + // subsequent arguments as regular arguments + use_arg_0_as_program = false; + } else if item.as_ref() == "@@" { + if let Some(name) = &moved.input_filename.clone() { + // If the input file name has been modified, use this one + moved = moved.arg_input_file(name); } else { - res.push(item.as_ref().to_os_string()); + moved = moved.arg_input_file_std(); } } else { - res.push(item.as_ref().to_os_string()); + moved = moved.arg(item); } } - self.arguments = res; - self.use_stdin = use_stdin; - self - } -} - -impl<'a> ForkserverExecutorBuilder<'a, UnixShMemProvider> { - /// Creates a new `AFL`-style [`ForkserverExecutor`] with the given target, arguments and observers. - /// This is the builder for `ForkserverExecutor` - /// This Forkserver will attempt to provide inputs over shared mem when `shmem_provider` is given. - /// Else this forkserver will try to write the input to `.cur_input` file. - /// If `debug_child` is set, the child will print to `stdout`/`stderr`. - #[must_use] - pub fn new() -> ForkserverExecutorBuilder<'a, UnixShMemProvider> { - ForkserverExecutorBuilder { - program: None, - arguments: vec![], - envs: vec![], - debug_child: false, - use_stdin: false, - uses_shmem_testcase: false, - is_persistent: false, - is_deferred_frksrv: false, - autotokens: None, - input_filename: None, - shmem_provider: None, - map_size: None, - real_map_size: 0, - } + // If we have not set an input file, use stdin as it is AFLs default + moved.use_stdin = moved.input_filename.is_none(); + moved } /// The harness @@ -941,16 +935,29 @@ impl<'a> ForkserverExecutorBuilder<'a, UnixShMemProvider> { #[must_use] /// Place the input at this position and set the filename for the input. + /// + /// Note: If you use this, you should ensure that there is only one instance using this + /// file at any given time. pub fn arg_input_file>(self, path: P) -> Self { let mut moved = self.arg(path.as_ref()); - moved.input_filename = Some(path.as_ref().as_os_str().to_os_string()); + + let path_as_string = path.as_ref().as_os_str().to_os_string(); + + assert!( + // It's only save to set the input_filename, if it does not overwrite an existing one. + (moved.input_filename.is_none() || moved.input_filename.unwrap() == path_as_string), + "Already specified an input file under a different name. This is not supported" + ); + + moved.input_filename = Some(path_as_string); moved } #[must_use] /// Place the input at this position and set the default filename for the input. + /// The filename includes the PID of the fuzzer to ensure that no two fuzzers write to the same file pub fn arg_input_file_std(self) -> Self { - self.arg_input_file(INPUTFILE_STD) + self.arg_input_file(get_unique_std_input_file()) } #[must_use] @@ -980,6 +987,33 @@ impl<'a> ForkserverExecutorBuilder<'a, UnixShMemProvider> { self.map_size = Some(size); self } +} + +impl<'a> ForkserverExecutorBuilder<'a, UnixShMemProvider> { + /// Creates a new `AFL`-style [`ForkserverExecutor`] with the given target, arguments and observers. + /// This is the builder for `ForkserverExecutor` + /// This Forkserver will attempt to provide inputs over shared mem when `shmem_provider` is given. + /// Else this forkserver will pass the input to the target via `stdin` + /// in case no input file is specified. + /// If `debug_child` is set, the child will print to `stdout`/`stderr`. + #[must_use] + pub fn new() -> ForkserverExecutorBuilder<'a, UnixShMemProvider> { + ForkserverExecutorBuilder { + program: None, + arguments: vec![], + envs: vec![], + debug_child: false, + use_stdin: false, + uses_shmem_testcase: false, + is_persistent: false, + is_deferred_frksrv: false, + autotokens: None, + input_filename: None, + shmem_provider: None, + map_size: None, + real_map_size: 0, + } + } /// Shmem provider for forkserver's shared memory testcase feature. pub fn shmem_provider(