Limit message sizes before receiving. #5843

pull sipa wants to merge 1 commits into bitcoin:master from sipa:limitbuf changing 2 files +7 −0

sipa commented at 11:59 AM on March 1, 2015: member

There is currently no message over 2 MiB that is acceptable anyway, no need to support them inside our network buffers.

This is done by a dispatch mechanism in net, rather than a direct check, to support future modularity (where message handling is split over different modules). It's also required to deal with DoS management correctly (which is done outside of net).

It also doesn't touch serialize.h's MAX_SIZE, as it's a global for all serialization usage, including non-protocol processing.
gmaxwell commented at 10:54 AM on March 2, 2015: contributor

Tested by IBDing a node running this against a node running this.
laanwj added the label P2P on Mar 2, 2015
laanwj commented at 4:21 PM on March 2, 2015: member

Code looks correct to me, going to test.

in src/main.cpp:None in 4398b3b0db outdated

4257 | @@ -4253,6 +4258,18 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
4258 |      return true;
4259 |  }
4260 |  
4261 | +static bool CheckMessageHeader(CNode* pnode, const CMessageHeader* msg)
4262 | +{
4263 | +    if (msg->nMessageSize > MAX_PROTOCOL_MESSAGE_LENGTH) {
4264 | +        LOCK(cs_main);
4265 | +        Misbehaving(pnode->GetId(), 100);
4266 | +        LogPrintf("net", "Oversized message from peer=%d (size=%d)\n", pnode->GetId(), msg->nMessageSize);

laanwj commented at 7:41 AM on March 3, 2015:

I lobbed a 4MB message at my node and it crashed here:

EXCEPTION: St13runtime_error       
tinyformat: Not enough conversion specifiers in format string       
bitcoin in net

As a category is passed, this should be LogPrint, not LogPrintf [yes, stupid naming.].

sipa commented at 12:06 PM on March 3, 2015:

Fixed.

sipa force-pushed on Mar 3, 2015
gmaxwell commented at 12:26 PM on March 3, 2015: contributor

Retesting.

laanwj commented at 7:20 PM on March 3, 2015: member

Tested ACK

2015-03-03 19:19:20 initial getheaders (324870) to peer=1 (startheight:0)
2015-03-03 19:19:20 sending: getheaders (997 bytes) peer=1
2015-03-03 19:19:20 Misbehaving: 127.0.0.1:57403 (0 -> 100) BAN THRESHOLD EXCEEDED
2015-03-03 19:19:20 Oversized message from peer=1 (size=4194304)
2015-03-03 19:19:20 socket closed
2015-03-03 19:19:20 disconnecting peer=1

sipa force-pushed on Mar 5, 2015
sipa commented at 12:04 PM on March 5, 2015: member

@gmaxwell Please test again?
gmaxwell commented at 4:13 PM on March 5, 2015: contributor

Hurray, It disconnects now, but as I pointed out-- while it grants misbehaving-- it does not attempt to ban because now it disconnects before it gets a chance to do so. If it's not going to ban, there is no point in setting the misbehaving, and a simpler patch which doesn't use the signals would work the same. (though I suppose we should have the combiner fix for the other signal that uses the return value!).
gavinandresen commented at 5:58 PM on March 5, 2015: contributor

ACK on the signal combiner.

RE: Misbehaving/banning: seems like it should ban, but instead of introducing Yet Another Signal, seems to me a check for state->fShouldBan in main.cpp FinalizeNode (called when net disconnects a peer) might be the right approach.
gavinandresen commented at 6:10 PM on March 5, 2015: contributor

@sipa : nit: I think the combiner code could be replaced with boost::bind(boost::algorithm::all_of_equal, _1, _2, true)
sipa force-pushed on Mar 6, 2015
sipa commented at 11:59 AM on March 6, 2015: member

Moved the combiner logic, and simplified the code here.

A more complex signals-based solution can be done later.

Limit message sizes before transfer

This introduces a fixed limit for the size of p2p messages, and enforces it
before download.

ba04c4a780

sipa force-pushed on Mar 6, 2015
gmaxwell commented at 12:34 PM on March 6, 2015: contributor

I was a little surprised that you went with logging at only debug=net level, but it works. ACK.
laanwj merged this on Mar 6, 2015
laanwj closed this on Mar 6, 2015
laanwj referenced this in commit 51377c2dbe on Mar 6, 2015
laanwj commented at 2:31 PM on March 6, 2015: member

I like the simpler solution. ACK.
jgarzik commented at 3:03 AM on March 8, 2015: contributor

posthumous ACK for simpler solution
sipa referenced this in commit d5d8998028 on Mar 9, 2015
reddink referenced this in commit d02aa114e8 on May 27, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#5859 Add correct bool combiner for net signals