Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 21, 2021
Merged

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

merged 1 commit into from
May 21, 2021

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented May 12, 2021 •
edited
Loading

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 practicalswift changed the title 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) May 12, 2021
@DrahtBot DrahtBot added the Tests label May 12, 2021
@DrahtBot
Copy link
Contributor

DrahtBot commented May 13, 2021 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

This was referenced May 13, 2021
Merged
Closed
Closed
Merged
@michaelfolkson
Copy link

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
Copy link
Contributor Author

practicalswift commented May 15, 2021 •
edited
Loading

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! :)

vasild
vasild approved these changes May 17, 2021
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 5454c9f

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:

bitcoin/src/test/fuzz/i2p.cpp

Line 26 in 7b87fca

CreateSock = [&fuzzed_data_provider](const CService&) {

@practicalswift practicalswift force-pushed the terminate-on-tcp-socket branch from 5454c9f to 39394d9 Compare May 19, 2021 20:17
vasild
vasild approved these changes May 20, 2021
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 39394d9

maflcko
maflcko reviewed May 20, 2021
View reviewed changes
@practicalswift practicalswift force-pushed the terminate-on-tcp-socket branch from 39394d9 to 393992b Compare May 20, 2021 19:14
@maflcko
Copy link
Member

maflcko commented May 21, 2021

ACK 393992b

@maflcko maflcko merged commit ac5f7f4 into bitcoin:master May 21, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 21, 2021
Merge bitcoin#21936: fuzz: Terminate immediately if a fuzzing harness…
51b3c56 
… tries to create a TCP socket (belt and suspenders)

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

Pull request description:

  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 :)

ACKs for top commit:
  MarcoFalke:
    ACK 393992b

Tree-SHA512: 5bbff1f7e9a58b3eae24f742b7daf3fc870424c985f29bed5931e47a708d9c0984bfd8762f43658cffa9c69d32f86d56deb48bc7e43821e3398052174b6a160e
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge ac5f7f4 into merged_master (Bitcoin PR bitcoin/bitcoin#21936)
d509636
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
2 more reviewers

@maflcko maflcko maflcko left review comments

@vasild vasild vasild approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@practicalswift @DrahtBot @michaelfolkson @maflcko @vasild