Check for oversized getblocktxn message. #9300

rebroad commented at 5:39 AM on December 8, 2016: contributor

Currently there is a check that each individual index is not out of bounds but there was no check that the number of indexes is not greater than the number possible. Given that data-wise the response is greater than the request, this closes off a potential attack vector.

Check for oversized getblocktxn message.

Currently there is a check that each individual index is not out of
bounds but there was no check that the number of indexes was greater
than the number possible. Given that the response is greater than the
request in size, this closes a potential attack vector.

558e967216

gmaxwell commented at 6:27 AM on December 8, 2016: contributor

@rebroad the total can't be greater than the block without at least one individual index being out of bound.

paveljanik commented at 8:12 AM on December 8, 2016: contributor

@gmaxwell IIUIC, it is about the number of requested indexes, not about the value of each individual index.

in src/net_processing.cpp:None in 558e967216

1518 | @@ -1519,6 +1519,12 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
1519 |          CBlock block;
1520 |          assert(ReadBlockFromDisk(block, it->second, chainparams.GetConsensus()));
1521 |  
1522 | +        if (req.indexes.size() >= block.vtx.size()) {
1523 | +            LogPrintf("recv getblocktxn. too many requests (%d >= %d) peer=%d\n", req.indexes.size(), block.vtx.size(), pfrom->id);
1524 | +            Misbehaving(pfrom->id, 100);

paveljanik commented at 8:20 AM on December 8, 2016:

id -> GetID()

in src/net_processing.cpp:None in 558e967216

1518 | @@ -1519,6 +1519,12 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
1519 |          CBlock block;
1520 |          assert(ReadBlockFromDisk(block, it->second, chainparams.GetConsensus()));
1521 |  
1522 | +        if (req.indexes.size() >= block.vtx.size()) {
1523 | +            LogPrintf("recv getblocktxn. too many requests (%d >= %d) peer=%d\n", req.indexes.size(), block.vtx.size(), pfrom->id);

paveljanik commented at 8:21 AM on December 8, 2016:

Please try to keep the style of logging consistent, see LogPrintf a few lines below.

paveljanik commented at 8:25 AM on December 8, 2016: contributor

Travis fails in p2p-compactblocks.py (https://travis-ci.org/bitcoin/bitcoin/jobs/182188214#L3516).

sipa commented at 8:31 AM on December 8, 2016: member

The indexes in a getblocktxn request are differentially encoded. If there are N entries, the index of the highest one will be at least N-1. Because of that, this test is unnecessary. However, it may be worthwhile to point this out in the code with a comment.

paveljanik commented at 8:53 AM on December 8, 2016: contributor

Yup, there is +1 in the ForRead part of SerializationOp (https://github.com/bitcoin/bitcoin/blob/master/src/blockencodings.h#L60).

So NACK.

gmaxwell commented at 9:17 AM on December 8, 2016: contributor

@paveljanik lets imagine that the data sent was 0,0,0,0,0,0,0. The indexes this signals are 0,1,2,3,4,5,6. You can imagine the encoding as signaling how many transactions to skip. Skip 0, include one, Skip 0 more, include one, Skip 0 ... This is run-length encoding.

There is no sequence of N inputs whos highest decoded index is less than N-1. Because of this there is no way for excessive entries to fail to also end up with a higher index number than the number of txn in the block. I suppose someone could compare the size of the vector with the block before actually decoding the indexes, but the computation is trivial and the same cases will already get rejected.

I think rebroad was expecting a getblocktxn that requested the same transaction over and over again many more times than there were even transactions in the block or something like that. But this kind of pointless request is simply impossible to represent in a getblocktxn. The efficient encoding also means whole classes of 'attack request' just aren't possible... which is no accident. :)

The worst possible getblocktxn is one that is N_transactions worth of 0s, which is functionally equivalent to just getdata-ing the whole block. (one of my earlier design drafts had us communicate if skips or transactions were signaled, so that including everything would simply be 'skips signaled' then an empty list... and the largest valid getblocktxn message would have been fetching every other transaction. But we found that requests that didn't have many skips were only common for the first block after restart.. so it wasn't worth the complexity just to save a few bytes in cases which never happen. :)

gmaxwell commented at 7:19 PM on December 10, 2016: contributor

Please close this.

rebroad closed this on Dec 11, 2016

rebroad commented at 5:14 PM on December 11, 2016: contributor

@gmaxwell thanks for the explanation and sorry for the time taken to do so. seems like more thought was put into the design than I'd been able to imagine.

MarcoFalke locked this on Sep 8, 2021