Use bounded network response buffers. #4769

jrick commented at 3:07 PM on August 27, 2014: none

I think that the memory buffer used to serialize peer responses should limit the total length to the maximum the peer is willing to accept. This can help prevent easily DoS-able requests from being added in the future and bringing down every Bitcoin Core node on the network, rather the relying on a soft limit of how big a response can actually be.

Opening this PR so more eyes can look at this, but I'm not able to test whether it actually works as intended. Don't merge until the error case is actually hit and the behavior is as intended. In particular, I don't know if that exception is actually caught anywhere, but I have received confirmation that the patch at least compiles.

jgarzik commented at 3:24 PM on August 27, 2014: contributor

Good start. Presumably needs a test that triggers a failure.

jrick commented at 4:04 PM on August 27, 2014: none

Agree, and one that passes through the "normal" response code instead of just a buffer unit test.

sipa commented at 7:41 PM on August 27, 2014: member

I think we want to be much more conservative than 32 MiB for outgoing messages. I believe that an inv with 50000 entries is the maximum we should be constructing, so 1800000 bytes for the payload.

jrick commented at 8:20 PM on August 27, 2014: none

A more conservative maximum would be welcome. I also don't see any payload length checks being done in ProcessMessages, so adding a constant and using it from both places is probably best.

edit: oh, it should be being checked from CMessageHeader::IsValid. That compares it against MAX_SIZE, which is currently 0x02000000 (32 MB).

jgarzik commented at 8:27 PM on August 27, 2014: contributor

@jrick ProcessMessages() is never handed a message larger than MAX_SIZE (0x02000000). Payload length is checked much earlier and at a lower level of processing, in CNetMessage()

The possibilities are: short message, complete message, complete message with added garbage (never read, yet reflected in message header's length member). @sipa +1 more conservative. "be liberal in what you accept, be conservative in what you emit"

laanwj added the label P2P on Aug 28, 2014

Use bounded network response buffers.

As the recently merged getutxos has shown, there is no enforcement
over how much a node can send to a peer.  The peer may even reject the
response for being too large, but as the entire request must stay in
memory (to calculate a checksum) before being sent over the network,
this can still be a great way to OOM any Bitcoin Core node while it
creates huge memory buffers for just a couple unauthenticated
requests.

This change preemptively stops this attack by preventing the network
send buffer from expanding past what the peer must be willing to
accept.

Even if getutxos is removed or reimplemented in a way that does not
cause bloated responses, this can prevent the attack from ever
occurring in the future should more ill-thought-out protocol changes
be made.

657c6535b4

jrick commented at 1:28 AM on September 1, 2014: none

@sipa I believe the maximum payload for a full inv is actually 1800003. The extra 3 bytes come from the varint which must represent an integer no greater than 50000.

Use a more conservative maximum buffer size. bfdda60071

Clarify comment. 8dc902d65f

jrick force-pushed on Sep 1, 2014

Add missing semicolon. 6007fa2609

BitcoinPullTester commented at 2:04 AM on September 1, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4769_6007fa26099682517b092e047104c44d68ad090a/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

jrick commented at 2:43 AM on September 1, 2014: none

Another change I'd like to see is having different maximum send buffer sizes depending on the message being serialized. This can help catch and prevent bugs where a message is serialized to a size greater than its expected maximum, but less than the max inv.

This would also require clearly defined maximum payload sizes for each P2P message, and newly added messages would have to have a sane maximum size defined before they could be included in the protocol.

jgarzik commented at 2:14 AM on September 15, 2014: contributor

hum. Re-reviewing, this is a bit odd in that it

Presented logic in comments implicitly assumes a single message in send buffer
Picks an arbitrary inv-related constant as maximum, yet the data we send might be entirely unrelated to inv.
Is ignorant of SendBufferSize(), in case that is larger than SEND_BUFFER_MAX_SIZE.

Notably, the response to a 50000-sized inv might push (50000*response) into the send buffer, before the send buffer has a chance to drain.

laanwj added this to the milestone 0.11.0 on Nov 3, 2014

fanquake commented at 1:31 PM on January 23, 2015: member

@jrick Needs rebase.

sipa commented at 12:03 PM on March 1, 2015: member

See also #5843 to deal with the receiver side of this.

laanwj commented at 9:28 AM on March 18, 2015: member

Needs rebase.

laanwj removed this from the milestone 0.11.0 on May 1, 2015

ghost commented at 8:53 PM on May 1, 2015: none

Hello, I noticed this was scheduled for 0.11 but the original committer hasn't been too responsive. I rebased his code and added some simple tests here https://github.com/faizkhan00/bitcoin/commit/6049eee8c5a34caaa1cb597d74984f31b7cef317

Sipa also mentioned possible bad behavior by nodes via dropping if the sent data is exceeded on IRC, so I'm looking to write another test for the networking / p2p quite soon, I just thought the code already committed could use some eyes.

jrick commented at 8:59 PM on May 1, 2015: none

Oops, apologies for dropping the ball on this one. Thanks for stepping up and rebasing it. I don't have a system currently that can build bitcoin core so I never got around to updating this.

ghost commented at 9:45 PM on May 5, 2015: none

@jrick just wondering, do you know what it would take to address @jgarzik 's concerns up above? I noticed at least the second point while rebasing (fixed constant for all message types) and @sipa has mentioned wanting more coverage of node-behavior from the sender (throwing behavior) side of things.

If more testing needs to be done on this I can spend some time and try to address those concerns, but just curious on your thoughts

jrick commented at 10:32 PM on May 5, 2015: none

If the buffer can hold multiple messages at once then my change is wrong. I'm not really familiar enough with the testing infrastructure to give any comments there.

jgarzik commented at 6:08 PM on July 23, 2015: contributor

Closing based on above comments and failing to garner ACKs after a long time.

Can re-open if something changes.

jgarzik closed this on Jul 23, 2015

MarcoFalke locked this on Sep 8, 2021