On Tue, Jul 13, 2021 at 2:05 AM Evgeny Vereshchagin <evvers@xxxxx> wrote: > > It should make it easier to reproduce bugs found by OSS-Fuzz locally > without docker. The fuzz target can be built and run with the corpus > OSS-Fuzz has accumulated so far by running the following commands: > ``` > ./scripts/oss-fuzz.sh > wget https://storage.googleapis.com/selinux-backup.clusterfuzz-external.appspot.com/corpus/libFuzzer/selinux_secilc-fuzzer/public.zip > unzip -d CORPUS public.zip > ./out/secilc-fuzzer CORPUS/ > ``` Hello, Thanks for this patch! I have a couple of comments to improve it. First, the instructions you gave (with wget + unzip) are useful and in my humble opinion, they could be documented for example in a comment at the beginning of scripts/oss-fuzz.sh ("# Usage: ..."). Second, shellcheck (https://www.shellcheck.net/) reports many warnings, such as missing quotes: In scripts/oss-fuzz.sh line 19: mkdir -p $OUT ^--^ SC2086: Double quote to prevent globbing and word splitting. Even though naming directories with spaces breaks many tools, it is good practice to avoid introducing more breakage, which is why quoting paths is required. Could you please take a look at shellcheck output and fix the issues it identifies? More comments below... > It was tested in https://github.com/google/oss-fuzz/pull/6026 > by pointing OSS-Fuzz to the branch containing the patch and > running all the tests with all the sanitizers and fuzzing engines > there: https://github.com/google/oss-fuzz/actions/runs/1024673143 > > Signed-off-by: Evgeny Vereshchagin <evvers@xxxxx> > --- > libsepol/fuzz/secilc-fuzzer.c | 69 +++++++++++++++++++++++++++++++++++ > scripts/oss-fuzz.sh | 28 ++++++++++++++ > 2 files changed, 97 insertions(+) > create mode 100644 libsepol/fuzz/secilc-fuzzer.c > create mode 100755 scripts/oss-fuzz.sh > > diff --git a/libsepol/fuzz/secilc-fuzzer.c b/libsepol/fuzz/secilc-fuzzer.c > new file mode 100644 > index 00000000..255b3241 > --- /dev/null > +++ b/libsepol/fuzz/secilc-fuzzer.c > @@ -0,0 +1,69 @@ > +#include <stdlib.h> > +#include <stdio.h> > +#include <stdint.h> > +#include <string.h> > +#include <getopt.h> > +#include <sys/stat.h> > + > +#include <sepol/cil/cil.h> > +#include <sepol/policydb.h> > + > +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { > + enum cil_log_level log_level = CIL_ERR; > + struct sepol_policy_file *pf = NULL; > + FILE *dev_null = NULL; > + int target = SEPOL_TARGET_SELINUX; > + int disable_dontaudit = 0; > + int multiple_decls = 0; > + int disable_neverallow = 0; > + int preserve_tunables = 0; > + int policyvers = POLICYDB_VERSION_MAX; > + int mls = -1; > + int attrs_expand_generated = 0; > + struct cil_db *db = NULL; > + sepol_policydb_t *pdb = NULL; > + > + cil_set_log_level(log_level); > + > + cil_db_init(&db); > + cil_set_disable_dontaudit(db, disable_dontaudit); > + cil_set_multiple_decls(db, multiple_decls); > + cil_set_disable_neverallow(db, disable_neverallow); > + cil_set_preserve_tunables(db, preserve_tunables); > + cil_set_mls(db, mls); > + cil_set_target_platform(db, target); > + cil_set_policy_version(db, policyvers); > + cil_set_attrs_expand_generated(db, attrs_expand_generated); > + > + if (cil_add_file(db, "fuzz", (const char *)data, size) != SEPOL_OK) > + goto exit; > + > + if (cil_compile(db) != SEPOL_OK) > + goto exit; > + > + if (cil_build_policydb(db, &pdb) != SEPOL_OK) > + goto exit; > + > + if (sepol_policydb_optimize(pdb) != SEPOL_OK) > + goto exit; > + > + dev_null = fopen("/dev/null", "w"); > + if (dev_null == NULL) > + goto exit; > + > + if (sepol_policy_file_create(&pf) != 0) > + goto exit; > + > + sepol_policy_file_set_fp(pf, dev_null); > + > + if (sepol_policydb_write(pdb, pf) != 0) > + goto exit; > +exit: > + if (dev_null != NULL) > + fclose(dev_null); > + > + cil_db_destroy(&db); > + sepol_policydb_free(pdb); > + sepol_policy_file_free(pf); > + return 0; > +} > diff --git a/scripts/oss-fuzz.sh b/scripts/oss-fuzz.sh > new file mode 100755 > index 00000000..9e720a5c > --- /dev/null > +++ b/scripts/oss-fuzz.sh > @@ -0,0 +1,28 @@ > +#!/bin/bash > + > +set -eux > + > +export DESTDIR=$(pwd)/DESTDIR It is strange that $OUT is configurable but not $DESTDIR. Please add a way to specify DESTDIR too. > + > +SANITIZER=${SANITIZER:-address} > +flags="-O1 -fno-omit-frame-pointer -gline-tables-only -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fsanitize=$SANITIZER -fsanitize=fuzzer-no-link" > + > +export CC=${CC:-clang} > +export CFLAGS=${CFLAGS:-$flags} > + > +export CXX=${CXX:-clang++} > +export CXXFLAGS=${CXXFLAGS:-$flags} > + > +export LDFLAGS="${LDFLAGS:-} $CFLAGS" Why do you need to include CFLAGS in LDFLAGS? > + > +export OUT=${OUT:-$(pwd)/out} > +mkdir -p $OUT > + > +export LIB_FUZZING_ENGINE=${LIB_FUZZING_ENGINE:--fsanitize=fuzzer} > + > +find -name Makefile | xargs sed -i 's/,-z,defs//' This is horrible for two reasons: it touches libsemanage/src/Makefile even though the fuzzer only needs libsepol, and it modifies in-place files which are versionned (with git). I understand the issue you are trying to fix here is that building with sanitizers leads to undefined symbols, but there are other options: * The most straightforward one is to prevent make from setting the variable which adds "-z,defs", by building with "make ... LD_SONAME_FLAGS=" * Another one consists in introducing a new Makefile variable, for example named "ALLOW_UNDEFINED_SYMBOLS" and to modify the Makefiles to only add -z,defs when this option is not defined. > +make V=1 -j$(nproc) install You do not need to build every sub-project. You can restrict the build to libsepol with "make -C libsepol ...". Moreover if the script is being launched from scripts/ ("cd scripts && ./oss-fuzz.sh") this does not work. If you intend to be run from the root directory, please insert a command which changes to the root directory somewhere, like: cd "$(dirname -- "$0")/.." Or (if you do not want to chdir) please use a reference to this base directory when you are using make and when you are compiling files. Moreover if libsepol was already built, "make" will not rebuild it even if the compiling option changed. Adding "make -C libsepol clean" before invoking make could be useful. > + > +$CC $CFLAGS -I$DESTDIR/usr/include -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -c -o secilc-fuzzer.o libsepol/fuzz/secilc-fuzzer.c > +$CXX $CXXFLAGS $LIB_FUZZING_ENGINE secilc-fuzzer.o $DESTDIR/usr/lib/libsepol.a -o $OUT/secilc-fuzzer Why are you using a C++ compiler to link the files? To my knowledge, the fuzzing/sanitizing options available to clang++ linker are the same as with clang. It would make the script easier to use if only one compiler was used, but you have a reason to prefer using clang++, please add a comment (in the script) about this. > +zip -r $OUT/secilc-fuzzer_seed_corpus.zip secilc/test > -- > 2.31.1 > So I believe there are few things to improve in your patch. If you have questions or if you disagree with some of my comments, feel free to discuss. (And if other SELinux maintainers/members want to comment, feel free to do so too ;) ). Thanks, Nicolas
