fuzzing: Add uri_parser setup #19057

Teufelchen1 · 2022-12-15T14:34:03Z

Hello!

Contribution description

This PR is a replacement for PR #18802

In this contribution:

The variable AFL_FLAGS is renamed to FLAGS_FOR_AFL because AFL is always complaining that AFL_FLAGS is not a valid env var for it. While this is not a bug nor an issue, I found it to be annoying.
A generic input reader is added to simplify building a test harness
The usage of this reader is demonstrated by adding a harness for fuzzing the uri_parser

(needs squashing after review)

Testing procedure

Go to fuzzing/uri_parser and run make all-asan and make fuzz to get some action going.
Also mildly interesting: ./dist/tools/compile_test/compile_like_murdock.py -b native -a fuzzing/uri_parser

Issues/PRs references

The original PR #18802 is replaced because the generic input reader is present in both PRs but this PoC harness is much simpler.

Teufelchen1 · 2022-12-15T14:36:53Z

fuzzing/Makefile.fuzzing_common

@@ -14,6 +14,7 @@ CFLAGS += -ggdb                # Make ASAN output more useful error messages
 CFLAGS += -D_FORTIFY_SOURCE=2  # Compiler hardening

 # Various utilitiy modules
+USEMODULE += gnrc_ipv6


This change was done due to gnrc_ipv6 being mandatory (for all harness types) at the moment because sys/fuzzing/ needs refactoring. :)

nmeum

Apart from the minor inline comments I created above this looks good to me 👍

I also prefer this over #18802.

Great work! 🤗

nmeum · 2022-12-16T12:45:27Z

Makefile.include

@@ -904,7 +904,7 @@ include $(RIOTMAKE)/tests/tests.inc.mk
 .PHONY: fuzz
 fuzz:
 	env FLASHFILE="$(FLASHFILE)" PORT="$(PORT)" TERMFLAGS="$(TERMFLAGS)" \
-		"$(RIOTBASE)"/dist/tools/fuzzing/afl.sh $(AFL_FLAGS)
+		"$(RIOTBASE)"/dist/tools/fuzzing/afl.sh $(FLAGS_FOR_AFL)


Could you elaborate why renaming the environment variable is necessary?

Sure!

If you try to compile using AFL++ (instead of the old AFL) you will encounter this warning:

[!] WARNING: Mistyped AFL environment variable: AFL_FLAGS= "make" -C RIOT/core/lib Did you mean AFL_AS? Did you mean AFL_CC?

This was introduced in Version ++3.10c of AFL++:
printing suggestions for mistyped AFL_ env variables
Check the changelog of AFL++ here.

I am aware that RIOTs fuzzing documentation states to use the old AFL 2.52b - where this warning isn't present. However, AFL is no longer maintained. Tho, we should move on towards AFL++. So far all my fuzzing with AFL++ is without issues and the backwards compatibility is nice. This warning being the only issue.

Edit:
Just realised: This can be turned of by setting AFL_IGNORE_UNKNOWN_ENVS.
I believe changing our name is the better approach as this way we still get hints if we do have typo in some of the AFL envs.

Moving to AFL++ is definitely a good idea! Maybe it also makes sense to update the documentation in this regard. However, I also wouldn't mind doing that in a separate merge request.

nmeum · 2022-12-16T12:46:47Z

fuzzing/uri_parser/main.c

+
+    uri_parser_process(&uri_res, input_buf, input_len);
+
+    exit(EXIT_SUCCESS);


For the uri_parser you might also be able to just use NATIVE_AUTO_EXIT but explicitly calling exit is of cause also fine.

Why would one prefer one over the other?

nmeum · 2022-12-16T12:49:04Z

sys/fuzzing/fuzzing.c

+uint8_t *
+fuzzing_read_bytes(int fd, size_t *size)
+{
+    uint8_t *buffer = NULL;
+    ssize_t r;
+    size_t csiz, rsiz;
+
+    csiz = 0;
+    rsiz = FUZZING_BSIZE;
+    if ((buffer = realloc(buffer, rsiz)) == NULL) {
+        return NULL;
+    }
+
+    while ((r = read(fd, &(buffer[csiz]), rsiz)) > 0) {
+        assert((size_t)r <= rsiz);
+
+        csiz += r;
+        rsiz -= r;
+
+        if (rsiz == 0) {
+            if ((buffer = realloc(buffer, csiz + FUZZING_BSTEP)) == NULL) {
+                return NULL;
+            }
+            rsiz += FUZZING_BSTEP;
+        }
+    }
+    if (r == -1) {
+        return NULL;
+    }
+
+    /* shrink packet to actual size */
+    if ((buffer = realloc(buffer, csiz)) == NULL) {
+        return NULL;
+    }
+
+    *size = csiz;
+    return buffer;
+}


It might be worthwhile to refactor fuzzing_read_packet using this new function so we don't need to maintain two functions which read all input from stdin in the fuzzing module.

I updated the PR accordingly. Please review the change :)

I think in an ideal word fuzzing_read_bytes would write directly to the pktbuf instead of requiring the memcpy but since this is execute on native only anyhow I believe this to be good enough 👍

nmeum

LGTM! 🎉

benpicco

@nmeum are you sure you don't want to have maintainer capabilities? 😉

proxy-ACK

riot-ci · 2023-01-15T23:50:02Z

Murdock results

✔️ PASSED

82f44c5 fuzzing: Add uri_parser fuzzer setup

Success	Failures	Total	Runtime
6782	0	6782	14m:03s

Artifacts

Documentation preview

benpicco · 2023-01-15T23:53:37Z

bors merge

bors · 2023-01-16T09:39:34Z

Build succeeded:

Teufelchen1 added 2 commits December 15, 2022 13:10

fuzzing: Rename AFL flag to prevent spelling warning from AFL

f7f9edd

fuzzing: Add generic input reader

5c51686

Teufelchen1 requested a review from jia200x as a code owner December 15, 2022 14:34

github-actions bot added Area: build system Area: Build system Area: doc Area: Documentation Area: sys Area: System Area: tests Area: tests and testing framework labels Dec 15, 2022

Teufelchen1 commented Dec 15, 2022

View reviewed changes

benpicco requested review from nmeum and miri64 December 15, 2022 16:41

Teufelchen1 force-pushed the fuzzing/AFLpp branch from f2335a4 to 61cf1cb Compare December 15, 2022 16:50

nmeum approved these changes Dec 16, 2022

View reviewed changes

Teufelchen1 force-pushed the fuzzing/AFLpp branch from 61cf1cb to 82f44c5 Compare December 19, 2022 12:00

fuzzing: Add uri_parser fuzzer setup

82f44c5

Teufelchen1 mentioned this pull request Dec 19, 2022

fuzzing: Add ut_process application setup #18802

Closed

nmeum approved these changes Jan 13, 2023

View reviewed changes

benpicco approved these changes Jan 15, 2023

View reviewed changes

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 15, 2023

bors bot merged commit 6fb340d into RIOT-OS:master Jan 16, 2023

jia200x added this to the Release 2023.04 milestone Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fuzzing: Add uri_parser setup #19057

fuzzing: Add uri_parser setup #19057

Teufelchen1 commented Dec 15, 2022

Teufelchen1 Dec 15, 2022

nmeum left a comment •

edited

Loading

nmeum Dec 16, 2022

Teufelchen1 Dec 19, 2022 •

edited

Loading

nmeum Jan 13, 2023

nmeum Dec 16, 2022

Teufelchen1 Dec 19, 2022

nmeum Dec 16, 2022

Teufelchen1 Dec 19, 2022

nmeum Jan 13, 2023

nmeum left a comment

benpicco left a comment

riot-ci commented Jan 15, 2023

benpicco commented Jan 15, 2023

bors bot commented Jan 16, 2023


		uri_parser_process(&uri_res, input_buf, input_len);

		exit(EXIT_SUCCESS);

fuzzing: Add uri_parser setup #19057

fuzzing: Add uri_parser setup #19057

Conversation

Teufelchen1 commented Dec 15, 2022

Contribution description

Testing procedure

Issues/PRs references

Choose a reason for hiding this comment

nmeum left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Teufelchen1 Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmeum left a comment

Choose a reason for hiding this comment

benpicco left a comment

Choose a reason for hiding this comment

riot-ci commented Jan 15, 2023

Murdock results

Artifacts

benpicco commented Jan 15, 2023

bors bot commented Jan 16, 2023

nmeum left a comment •

edited

Loading

Teufelchen1 Dec 19, 2022 •

edited

Loading