Tighten up message size sanity checking #6261

gavinandresen commented at 3:35 PM on June 9, 2015: contributor

These commits tighten up message size sanity checking by:

Adding a new signal (SanityCheckMessages) to the networking code
Refactoring MAX_PROTOCOL_MESSAGE_SIZE from the networking code to main
Adding additional checks for several messages (e.g. ping messages are at most 8 bytes) to the main signal handler (next to the code that handles each message type)

Extending the SanityCheckMessages signal handler (or adding an additional handler) to do more sophisticated DoS detection/prevention is on my longer-term TODO list.

gavinandresen force-pushed on Jun 9, 2015

laanwj added the label P2P on Jun 9, 2015

in src/main.cpp:None in fc820b39c5 outdated

3779 | +
3780 | +bool static SanityCheckMessage(CNode* peer, const CNetMessage& msg)
3781 | +{
3782 | +    if (msg.hdr.nMessageSize > MAX_PROTOCOL_MESSAGE_LENGTH) {
3783 | +        LogPrint("net", "Oversized message from peer=%i, disconnecting", peer->GetId());
3784 | +        return false;

jgarzik commented at 4:55 AM on June 10, 2015:

Harmonize the two LogPrint() calls. No reason to omit strCommand diagnostic output on this first LogPrint() here.

in src/net.cpp:None in fc820b39c5 outdated

2025 | +    uint256 hash = Hash(s.begin() + CMessageHeader::HEADER_SIZE, s.end());
2026 | +    unsigned int nChecksum = 0;
2027 | +    memcpy(&nChecksum, &hash, sizeof(nChecksum));
2028 | +    assert(s.size () >= CMessageHeader::CHECKSUM_OFFSET + sizeof(nChecksum));
2029 | +    memcpy((char*)&s[CMessageHeader::CHECKSUM_OFFSET], &nChecksum, sizeof(nChecksum));
2030 | +

jgarzik commented at 4:58 AM on June 10, 2015:

Code is correct. However, it seems odd to put this in CNode. </nit>

jgarzik commented at 4:58 AM on June 10, 2015: contributor

ut ACK modulo comments

gavinandresen force-pushed on Jun 10, 2015

gavinandresen commented at 1:50 PM on June 10, 2015: contributor

Addressed @jgarzik comments -- FinalizeHeader moved to CNetMessage (I considered moving it to CNetHeader, but didn't want to make protocol.h dependent on CDataStream). And unified the log message (and remembered to SanitizeString(command) at the very last moment).

sipa commented at 1:27 PM on June 14, 2015: member

Concept ACK, though I would just pass a pointer to CNetMessage rather than a wrapper-and-then-auto-unwrapped boost reference object.

gavinandresen commented at 6:10 PM on June 26, 2015: contributor

@sipa: it'll be std::cref() when we move to C++11... and passing a reference is, in my opinion, cleaner than passing a pointer since the routine doesn't have to worry about checking for NULL.

in src/main.cpp:None in 31c808d601 outdated

3780 | +    ("ping",8)
3781 | +    ("pong",8)
3782 | +    ("verack", 0)
3783 | +    ;
3784 | +
3785 | +bool static SanityCheckMessage(CNode* peer, const CNetMessage& msg)

Diapolo commented at 11:15 PM on June 26, 2015:

Why did you choose to not directly pass the NodeId? It seems it's the only field used in here.

in src/net.cpp:None in 31c808d601 outdated

 561 | @@ -564,6 +562,22 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes)
 562 |      return true;
 563 |  }
 564 |  
 565 | +unsigned int CNetMessage::FinalizeHeader(CDataStream& s)
 566 | +{
 567 | +    // Set the size
 568 | +    unsigned int nSize = s.size() - CMessageHeader::HEADER_SIZE;

Diapolo commented at 11:19 PM on June 26, 2015:

s.size() returns size_t, so we have unsigned int = size_t - int... is this correct?

gavinandresen commented at 4:27 PM on July 13, 2015: contributor

@dgenr8 is seeing peer disconnections due to this code (it is part of the BIP101 pull request), especially with 'addr' messages. I need to investigate why.

And I like detecting misbehavior early, but this should work with the existing Misbehaving() logic. I think I see a path to get there:

Refactor mapNodeState so it is protected by it's own lock instead of cs_main.
Rework this pull request to call Misbehaving() for oversize messages; never immediately disconnect.

sipa commented at 4:35 PM on July 13, 2015: member

@gavinandresen 1000 addr messages is 3 + 1000*30 bytes, as the serialized vector length for sizes over 253 takes 3 bytes,

gavinandresen force-pushed on Jul 14, 2015

gavinandresen commented at 3:31 PM on July 14, 2015: contributor

Reworked the mapNodeState encapsulation into a RAII NodeStatePtr smart-pointer class.

That allowed me to change the SanityCheckMessage() callback match the old behavior: if a peer send a message larger than 2MB, immediately drop their connection. If they send a message bigger than expected but less than 2MB, call Misbehaving(20) (which, by default, gets them dropped/banned if they send more than 5 oversize messages).

And I removed the addr/inv/getdata checks-- those messages already check for message length and call Misbehaving() if passed vectors that are too big.

mruddy commented at 7:07 PM on August 2, 2015: contributor

I believe that the "version" message can be limited to 344 bytes. Right now I can send the full 2MB and it's accepted. The "filterclear" and "reject" messages need lower limits too as 2MB works for them with no penalty. Then that leaves the obvious final case in ProcessMessage that other fictitious/junk commands can be used with a 2MB payload to hog bandwidth.

I came looking for a ticket with a fix like this ticket implements because, I was playing around on my local node with a script that basically connected, sent a good "version" message (actually I used 2MB, but that is easy to fix with this ticket), then looped sending 2MB junk command messages. That might eventually timeout.If so, my script would have eventually had to maybe add in some pong and verack handling, but nothing difficult.

Maybe there's another ticket for doing something about the junk case that I didn't find yet too?

jgarzik commented at 3:06 PM on September 16, 2015: contributor

concept ACK

Refactor: protect mapNodeState with its own lock

Encapsulate mapNodeState in a smart-pointer class with its own lock.

Why? So Misbehaving() can be called from the sanity-check-a-message code without holding cs_main.

And to get better exception safety (the smart-pointer approach gives RAII semantics).

And because protecting fewer things with cs_main is a good idea.

Tested by compiling with -DDEBUG_LOCKORDER, running all of the qa/rpc-tests, and running a node on the main network overnight.

0b7b8669cd

Refactor, new CNode::FinalizeHeader method

I need this to write some unit tests for the
CNode::ReceiveMsgBytes() function.

6edddab5e5

Unit test for CNode::ReceiveMsgBytes 8dbdcd5996

Allow per-message sanity checking when reading from wire a4a18f0369

gavinandresen force-pushed on Sep 28, 2015

MarcoFalke commented at 10:42 PM on November 10, 2015: member

@gavinandresen Travis failed with test_bitcoin: key.cpp:198: void ECC_Start(): Assertionsecp256k1_context == __null' failed.` This seems a temporary outage? Mind to run travis again?

laanwj commented at 2:30 PM on January 29, 2016: member

Closing due to inactivity, feel free to reopen if you pick this up again.

laanwj closed this on Jan 29, 2016

MarcoFalke locked this on Sep 8, 2021