Skip to content
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.

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

Limit the number of IPs addrman learns from each DNS seeder #12626

Merged
merged 1 commit into from Mar 7, 2018

Conversation

@EthanHeilman
Copy link
Contributor

commented Mar 6, 2018

A risk exists where a malicious DNS seeder eclipses a node by returning an enormous number of IP addresses. In this commit we mitigate this risk by limiting the number of IP addresses addrman learns to 256 per DNS seeder.

As discussed with @theuni

Limit the number of IPs we use from each DNS seeder
A risk exists where a malicious DNS seeder eclipses a node by returning an enormous number of IP addresses. In this commit we mitigate this risk by limiting the number of IP addresses addrman learns to 256 per DNS seeder.

@fanquake fanquake added the P2P label Mar 6, 2018

@randolf
randolf approved these changes Mar 7, 2018
Copy link
Contributor

left a comment

This is a very good first step in mitigating this type of DoS attack, and 256 seems me to be an extremely generous default.

@sipa

This comment has been minimized.

Copy link
Member

commented Mar 7, 2018

Since DNS responses generally are sent over UDP, all of them need to fit in a single IP packet (I believe), which puts a natural limit regardless. Having some explicit limit sounds good though.

@randolf

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2018

@sipa Packets larger than 512 bytes are supported with the introduction of EDNS (see RFC 6891 dated April 2013; earlier RFCs that reference EDNS0 that may also be of interest) that uses an unsigned 16-bit integer to specify RDLEN (Record Data Length). Also, while UDP is a MUST for DNS services, TCP is a SHOULD, and both of these transport layer protocols can, for the most part, support EDNS's larger packet size options.

In summary, the natural limit that is more well-known has effectively been extended (IP packet fragmentation and reassembly make it possible to venture beyond the MSU, which is commonly set to 1,500 bytes).

@practicalswift

This comment has been minimized.

Copy link
Member

commented Mar 7, 2018

utACK 46e7f80

@sipa

This comment has been minimized.

Copy link
Member

commented Mar 7, 2018

@randolf Thanks, TIL.

@sdaftuar

This comment has been minimized.

Copy link
Member

commented Mar 7, 2018

utACK 46e7f80

@theuni
theuni approved these changes Mar 7, 2018
Copy link
Member

left a comment

utACK 46e7f80

@EthanHeilman

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2018

Three years ago I tested the number of DNS entries I could get into Bitcoin for the eclipse attack paper. My test setup was Ubuntu Linux running Bitcoind querying a custom DNS server on localhost. We didn't end up using this attack so I wrote up a blog entry about the general question without mentioning bitcoin: How many IP addresses can a DNS query return?

@fanquake

This comment has been minimized.

Copy link
Member

commented Mar 7, 2018

utACK 46e7f80

@laanwj laanwj added this to the 0.16.1 milestone Mar 7, 2018

@laanwj laanwj merged commit 46e7f80 into bitcoin:master Mar 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Mar 7, 2018
Merge #12626: Limit the number of IPs addrman learns from each DNS se…
…eder

46e7f80 Limit the number of IPs we use from each DNS seeder (e0)

Pull request description:

  A risk exists where a malicious DNS seeder eclipses a node by returning an enormous number of IP addresses. In this commit we mitigate this risk by limiting the number of IP addresses addrman learns to 256 per DNS seeder.

  As discussed with @theuni

Tree-SHA512: 949e870765b1470200f2c650341d9e3308a973a7d1a6e557b944b0a2b8ccda49226fc8c4ff7d2a05e5854c4014ec0b67e37a3f2287556fe7dfa2048ede1f2e6f
@fanquake fanquake referenced this pull request Apr 12, 2018
fanquake added a commit to fanquake/bitcoin that referenced this pull request Apr 26, 2018
Limit the number of IPs we use from each DNS seeder
A risk exists where a malicious DNS seeder eclipses a node by returning an enormous number of IP addresses. In this commit we mitigate this risk by limiting the number of IP addresses addrman learns to 256 per DNS seeder.

GitHub-Pull: bitcoin#12626
Rebased-From: 46e7f80
laanwj added a commit that referenced this pull request May 16, 2018
Merge #12967: [0.16] Backports
8fca086 List support for BIP173 in bips.md (Pieter Wuille)
9645aa6 Remove blockmaxsize option from init.cpp (fanquake)
7847b92 Default to defining endian-conversion DECLs in compat w/o config (Matt Corallo)
1720eb3 qt:Show the entire Window when double clicking on taskbar (Chun Kuan Lee)
e055bc0 depends: Fix Qt build with XCode 9.3 (fanquake)
0684cf9 Avoid launching as admin when NSIS installer ends. (JeremyRand)
e802c22 [config] Remove blockmaxsize option (John Newbery)
f118a7a Fix illegal default `addProxy` and `addrSeparateProxyTor` settings. (251)
f60e84d Limit the number of IPs we use from each DNS seeder (e0)

Pull request description:

  Backports:
  - #12626 Limit the number of IPs addrman learns from each DNS seeder
  - #12650 gui: Fix issue: "default port not shown correctly in settings dialog"
  - #12756 [config] Remove blockmaxsize option
  - #12985 Windows: Avoid launching as admin when NSIS installer ends.
  - #12946 depends: Fix Qt build with XCode 9.3
  - #12998 Default to defining endian-conversion DECLs in compat w/o config
  - #12999 qt: Show the Window when double clicking the taskbar icon
  - #13064 List support for BIP173 in bips.md

  to the 0.16 branch.

Tree-SHA512: 3e6b47c54b2cd2bdd81fbc6176cb31e46423f6e05988984d3a09b3535e3cee101ffb071cf753a4beff3c9f0521eb5de4b7c0424a3e97da801d56b4015847ac0f
@fanquake

This comment has been minimized.

Copy link
Member

commented May 16, 2018

Backported in #12967

HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Jun 29, 2018
Limit the number of IPs we use from each DNS seeder
A risk exists where a malicious DNS seeder eclipses a node by returning an enormous number of IP addresses. In this commit we mitigate this risk by limiting the number of IP addresses addrman learns to 256 per DNS seeder.

GitHub-Pull: bitcoin#12626
Rebased-From: 46e7f80
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.