fuzz: Terminate immediately if a fuzzing harness tries to create a TCP socket (belt and suspenders)

practicalswift commented at 10:22 pm on May 12, 2021: contributor

Terminate immediately if a fuzzing harness ever to create a TCP socket (belt and suspenders).

Obviously this should never happen, but if it does happen we want immediate termination instead of a TCP socket :)

practicalswift renamed this:
~~fuzz: Terminate immediately if a fuzzing harness ever tries to create a TCP socket (belt and suspenders)~~
fuzz: Terminate immediately if a fuzzing harness tries to create a TCP socket (belt and suspenders)
on May 12, 2021

DrahtBot added the label Tests on May 12, 2021

DrahtBot commented at 2:53 am on May 13, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21795 (fuzz: Terminate immediately if a fuzzing harness tries to perform a DNS lookup (belt and suspenders) by practicalswift)
#21538 (fuzz: Add fuzzing syscall sandbox: detect use of unexpected syscalls when fuzzing (“syscall sanitizer”) by practicalswift)
#21496 (fuzz: execute each file in dir without fuzz engine by AnthonyRonning)
#20487 (Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) by practicalswift)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

michaelfolkson commented at 9:44 am on May 13, 2021: contributor

Concept ACK. Creating a TCP socket shouldn’t happen accidentally :)

Is this precautionary for future fuzzers added or something you stumbled on with current fuzzers? I’m interested in whether there needs to be many more of these kinds of footgun protections in case fuzzing goes down these uncontrolled paths. The alternative (I think) would be to say “If you run experimental fuzzers you are on your own”. Protections such as the one introduced in this PR would just be there for the really dangerous paths.

practicalswift commented at 8:38 am on May 15, 2021: contributor

Is this precautionary for future fuzzers added or something you stumbled on with current fuzzers?

This is added purely as a cheap precautionary measure.

The only “surprising” use of networking syscalls we’ve ever seen in the code base historically (AFAIK) is that the socket syscall is currently being invoked when formatting an IP address a string (CNetAddr::ToString). That is documented in issue #21466 and being resolved by PR #21756. Review welcome! :)

I’m interested in whether there needs to be many more of these kinds of footgun protections in case fuzzing goes down these uncontrolled paths.

The bullet proof approach to avoiding unexpected syscalls in Linux is to use seccomp-bpf.

PR #20487 adds syscall sandbox support to Bitcoin Core using seccomp-bpf (opt-in at compile-time via --with-syscall-sandbox), and PR #21538 extends that support to detect use of unexpected syscalls when fuzzing (“syscall sanitizer”). Review welcome for both! :)

in src/test/fuzz/fuzz.cpp:15 in 5454c9febd outdated

 9 #include <test/util/setup_common.h>
10 #include <util/check.h>
11+#include <util/sock.h>
12 
13 #include <cstdint>
14+#include <memory>

vasild commented at 11:29 am on May 17, 2021:

A call to std::terminate() was added, which is defined in <exception>.

0#include <exception>
1#include <memory>

practicalswift commented at 8:17 pm on May 19, 2021:

Oh, thanks! Now fixed.

vasild approved

vasild commented at 11:33 am on May 17, 2021: member

ACK 5454c9febd9bd17d26119ada6ac3cba9c46d7a81

There are a bunch of CreateSock() calls in the code. Indeed those should either not be executed during fuzzing, or the particular test that executes them should have overriden CreateSock(), like, for example:

https://github.com/bitcoin/bitcoin/blob/7b87fca930ff7129f267906e71be217851146ade/src/test/fuzz/i2p.cpp#L26

practicalswift force-pushed on May 19, 2021

vasild approved

vasild commented at 8:48 am on May 20, 2021: member

ACK 39394d94591515b82f9bf3b086aa9b905cb006c9

fuzz: Terminate immediately if a fuzzing harness ever tries to create a TCP socket (belt and suspenders) 393992b049

in src/test/fuzz/fuzz.cpp:39 in 39394d9459 outdated

31@@ -27,8 +32,16 @@ void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target,
32 
33 static TypeTestOneInput* g_test_one_input{nullptr};
34 
35+static std::unique_ptr<Sock> TerminateOnTCPSocketCreation(const CService&)
36+{
37+    std::terminate();
38+}
39+

MarcoFalke commented at 10:44 am on May 20, 2021:

Can be removed if done with a simple one-line lambda:

 0diff --git a/src/test/fuzz/fuzz.cpp b/src/test/fuzz/fuzz.cpp
 1index b6ba44fe21..631c861bb6 100644
 2--- a/src/test/fuzz/fuzz.cpp
 3+++ b/src/test/fuzz/fuzz.cpp
 4@@ -32,15 +32,10 @@ void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target,
 5 
 6 static TypeTestOneInput* g_test_one_input{nullptr};
 7 
 8-static std::unique_ptr<Sock> TerminateOnTCPSocketCreation(const CService&)
 9-{
10-    std::terminate();
11-}
12-
13 void initialize()
14 {
15     // Terminate immediately if a fuzzing harness ever tries to create a TCP socket.
16-    CreateSock = TerminateOnTCPSocketCreation;
17+    CreateSock = [](const CService&) -> std::unique_ptr<Sock> { std::terminate(); };
18 
19     bool should_abort{false};
20     if (std::getenv("PRINT_ALL_FUZZ_TARGETS_AND_ABORT")) {

practicalswift commented at 7:15 pm on May 20, 2021:

Sure, why not! Now addressed :)

practicalswift force-pushed on May 20, 2021

MarcoFalke commented at 7:00 am on May 21, 2021: member

ACK 393992b049d3bcf0b2a3439be128d13d6567f0b1

MarcoFalke merged this on May 21, 2021

MarcoFalke closed this on May 21, 2021

sidhujag referenced this in commit 51b3c565d4 on May 21, 2021

gwillen referenced this in commit d509636ace on Jun 1, 2022

DrahtBot locked this on Aug 18, 2022

fuzz: Terminate immediately if a fuzzing harness tries to create a TCP socket (belt and suspenders) #21936

Conflicts