Skip to content

fuzz: Terminate immediately if a fuzzing harness tries to perform a DNS lookup (belt and suspenders) #21795

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
Jun 7, 2021
Merged

fuzz: Terminate immediately if a fuzzing harness tries to perform a DNS lookup (belt and suspenders) #21795

merged 1 commit into from
Jun 7, 2021

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Apr 28, 2021 •
edited
Loading

Terminate immediately if a fuzzing harness tries to perform a DNS lookup (belt and suspenders).

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

@fanquake fanquake added the Tests label Apr 28, 2021
maflcko
maflcko reviewed Apr 28, 2021
View reviewed changes
@practicalswift practicalswift force-pushed the fuzz-terminate-on-dns-lookup branch 2 times, most recently from 5454c7a to 5454f1a Compare April 28, 2021 12:55
@practicalswift practicalswift changed the title fuzz: Terminate immediately if a fuzzing input ever causes a DNS lookup (belts and suspenders) fuzz: Terminate immediately if a fuzzing harness ever tries to perform a DNS lookup (belts and suspenders) Apr 28, 2021
@practicalswift practicalswift force-pushed the fuzz-terminate-on-dns-lookup branch from 5454f1a to 5555f5c Compare April 28, 2021 13:49
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 28, 2021 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@DrahtBot DrahtBot mentioned this pull request Apr 28, 2021
Closed
@practicalswift practicalswift force-pushed the fuzz-terminate-on-dns-lookup branch from 5555f5c to 545497e Compare May 12, 2021 21:32
@practicalswift practicalswift changed the title fuzz: Terminate immediately if a fuzzing harness ever tries to perform a DNS lookup (belts and suspenders) fuzz: Terminate immediately if a fuzzing harness ever tries to perform a DNS lookup (belt and suspenders) May 12, 2021
@practicalswift practicalswift changed the title fuzz: Terminate immediately if a fuzzing harness ever tries to perform a DNS lookup (belt and suspenders) fuzz: Terminate immediately if a fuzzing harness tries to perform a DNS lookup (belt and suspenders) May 12, 2021
@DrahtBot DrahtBot mentioned this pull request May 13, 2021
Merged
@practicalswift practicalswift force-pushed the fuzz-terminate-on-dns-lookup branch from 545497e to 3939e8c Compare May 19, 2021 20:19
@DrahtBot DrahtBot mentioned this pull request May 20, 2021
Closed
@practicalswift practicalswift force-pushed the fuzz-terminate-on-dns-lookup branch from 3939e8c to 3939465 Compare May 20, 2021 19:30
@practicalswift practicalswift force-pushed the fuzz-terminate-on-dns-lookup branch from 3939465 to 3737d35 Compare May 21, 2021 19:51
@practicalswift
Copy link
Contributor Author

practicalswift commented Jun 5, 2021 •
edited
Loading

@MarcoFalke Thanks for reviewing. All feedback has been addressed. Let me know if there is anything more I can do :)

@maflcko
Copy link
Member

maflcko commented Jun 6, 2021

review ACK 3737d35

@maflcko maflcko merged commit 912cb59 into bitcoin:master Jun 7, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 9, 2021
Merge bitcoin#21795: fuzz: Terminate immediately if a fuzzing harness…
f632b40 
… tries to perform a DNS lookup (belt and suspenders)

3737d35 fuzz: Terminate immediately if a fuzzing harness ever tries to perform a DNS lookup (belts and suspenders) (practicalswift)

Pull request description:

  Terminate immediately if a fuzzing harness tries to perform a DNS lookup (belt and suspenders).

  Obviously this _should_ never happen, but if it _does_ happen we want immediate termination instead of a DNS lookup :)

ACKs for top commit:
  MarcoFalke:
    review ACK 3737d35

Tree-SHA512: 51cd2d32def7f9f052e02f99c354656af1f807cc9fdf592ab765e620bfe660f1ed26e0484763f94aba650424b44959eafaf352bfd0f81aa273e350510e97356e
@vasild
Copy link
Contributor

vasild commented Jun 30, 2021

This bite me for good in #21878 (now fixed). It works!

@practicalswift
Copy link
Contributor Author

@vasild This safety measure helped avoid an unwanted external DNS lookup? If so that's great news: thanks for sharing! :)

@vasild
Copy link
Contributor

vasild commented Jun 30, 2021

@practicalswift, yes! Fuzzing CConnman::OpenNetworkConnection(), it called CConnman::ConnectNode() which called Lookup() with the default fNameLookup which is true.

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge 912cb59 into merged_master (Bitcoin PR bitcoin/bitcoin#21795)
7fe97f2
@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
1 more reviewer

@maflcko maflcko maflcko left review comments

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 @maflcko @vasild @fanquake