Re: [PATCH] libsepol/cil: move the fuzz target and build script to the selinux repository

[Nicolas Iooss <nicolas.iooss@m4x.org>]

On Tue, Jul 13, 2021 at 2:05 AM Evgeny Vereshchagin <evvers@ya.ru> 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@ya.ru>
> ---
>  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