fix a deserialization overflow edge case #14685

kazcw commented at 8:54 PM on November 7, 2018: contributor

A specially-constructed BlockTransactionsRequest can cause offset to wrap in deserialization. In the current code, there is not any way this could be dangerous; but disallowing it reduces the potential for future surprises.

MarcoFalke added the label P2P on Nov 7, 2018

MarcoFalke added the label Tests on Nov 7, 2018

practicalswift commented at 10:22 PM on November 7, 2018: contributor

@kazcw Very nice find! How did you find this issue?

in src/test/blockencodings_tests.cpp:373 in 2c9e12e6f5 outdated

 368 | +        // before patch: deserialize above succeeds and this check fails, demonstrating the overflow
 369 | +        BOOST_CHECK(req1.indexes[1] < req1.indexes[2]);
 370 | +        // this shouldn't be reachable before or after patch
 371 | +        BOOST_CHECK(0);
 372 | +    } catch(std::ios_base::failure &)
 373 | +    {

practicalswift commented at 10:28 PM on November 7, 2018:

Nit: Move { one line up to make consistent with opening try {

kazcw commented at 12:12 AM on November 8, 2018: contributor

@practicalswift well, back in 2016 I used to skim some of the diffs I didn't review. I made a mental note that this looked off but not exploitable. I have a serious backlog of mental notes and finally got around to this :laughing:.

DrahtBot commented at 12:38 AM on November 8, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#10785 (Serialization improvements by sipa)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

gmaxwell commented at 6:02 PM on November 13, 2018: contributor

I don't believe this change is the most correct change. The existing code upcasts a and b and then checks if a+b is greater than std::numeric_limits<uint16_t>::max which is the maximum value that can be represented in a uint16_t, if it is it rejects the encoding.

The change switches to >= which will reject a perfectly valid index of 0xFFFF in the last position, thus reducing the maximum size of a block (though irrelevantly since any block with 2^16 transactions would be over the weight limit).

A more correct change would be increasing the size of offset. There is no reason for offset to be small, it should have been chosen to be whatever is fastest that can represent all the maximum index +1. (The Indexes[] table is small to reduce the memory/cache footprint for it). @TheBlueMatt

add a test demonstrating an overflow in a deserialization edge case

Also add a test that the highest legal index is accepted.

051faf7e9d

fix a deserialization overflow edge case

A specially-constructed BlockTransactionsRequest can overflow in
deserialization in a way that is currently harmless.

6bed4b374d

disallow oversized CBlockHeaderAndShortTxIDs

Otherwise we'd reply with a bogus BlockTransactionsRequest trying to
request indexes with overflowed deltas.

b08af10fb2

kazcw force-pushed on Nov 13, 2018

kazcw commented at 8:47 PM on November 13, 2018: contributor

That is a cleaner solution. I redid it that way and added a test for the 0xffff case.

Also, I noticed if a peer sends a bogus CBlockHeaderAndShortTxIDs that has a BlockTxCount() > 0xffff, it looks like that isn't rejected upfront and we may respond with a bogus BlockTransactionsRequest containing illegal index deltas--so I added a check for that condition at compactblock deserialization time. Garbage In, Reject Messages Out!

in src/test/blockencodings_tests.cpp:387 in 051faf7e9d outdated

 382 | +        stream >> req1;
 383 | +        // before patch: deserialize above succeeds and this check fails, demonstrating the overflow
 384 | +        BOOST_CHECK(req1.indexes[1] < req1.indexes[2]);
 385 | +        // this shouldn't be reachable before or after patch
 386 | +        BOOST_CHECK(0);
 387 | +    } catch(std::ios_base::failure &) {

promag commented at 1:00 PM on November 15, 2018:

nit, space after catch, remove space before &.

in src/test/blockencodings_tests.cpp:386 in 051faf7e9d outdated

 381 | +    try {
 382 | +        stream >> req1;
 383 | +        // before patch: deserialize above succeeds and this check fails, demonstrating the overflow
 384 | +        BOOST_CHECK(req1.indexes[1] < req1.indexes[2]);
 385 | +        // this shouldn't be reachable before or after patch
 386 | +        BOOST_CHECK(0);

promag commented at 1:00 PM on November 15, 2018:

In one commit you could add the test like:

        stream >> req1;
        // deserialize above succeeds and this check fails, demonstrating the overflow
        BOOST_CHECK(req1.indexes[1] < req1.indexes[2]);
        BOOST_CHECK(0);

And then in the commit that fixes deserialization it could change the test to:

        stream >> req1;
        BOOST_CHECK(0);

since the check is not reachable with the fix.

kazcw commented at 5:52 PM on November 15, 2018:

Is it not useful for the test to distinguish between a regression to the original failure mode and other possible failures?

promag commented at 6:06 PM on November 15, 2018:

IMO if that really matters you can distinguish by blaming. Furthermore, if the fix is wrongly reverted the test doesn't fail?

kazcw commented at 6:21 PM on November 15, 2018:

Ok. I did not know BOOST_CHECK minimization was a goal.

promag commented at 10:27 PM on November 15, 2018:

Note that it's my opinion, wait until others weight in.

sipa commented at 1:51 AM on November 16, 2018:

I think it's fine.

laanwj commented at 9:12 AM on November 18, 2018:

Ok. I did not know BOOST_CHECK minimization was a goal.

It's not unless it's used extremely frequenly in inner loops, in which case it is quite a slow operation — and for some reason, much slower on some platforms than others. I think it's fine here.

gmaxwell commented at 11:11 PM on November 15, 2018: contributor

ACK.

sipa commented at 1:51 AM on November 16, 2018: member

utACK b08af10fb299dc3fdcd1f022619fb112c72e5d8e

MarcoFalke removed the label Tests on Nov 16, 2018

MarcoFalke added the label Refactoring on Nov 16, 2018

laanwj commented at 9:15 AM on November 18, 2018: member

utACK b08af10fb299dc3fdcd1f022619fb112c72e5d8e

laanwj merged this on Nov 18, 2018

laanwj closed this on Nov 18, 2018

laanwj referenced this in commit 6d58a5c3b0 on Nov 18, 2018

MarcoFalke referenced this in commit 9502a5ad51 on Nov 19, 2018

MarcoFalke referenced this in commit edcb7cc3e5 on Nov 19, 2018

MarcoFalke referenced this in commit dd6a0afe96 on Nov 19, 2018

sipa added the label Needs backport on Nov 20, 2018

MarcoFalke referenced this in commit 94065024c7 on Nov 28, 2018

MarcoFalke referenced this in commit 5331ad0506 on Nov 28, 2018

MarcoFalke referenced this in commit 2f9fd29321 on Nov 28, 2018

MarcoFalke removed the label Needs backport on Nov 28, 2018

MarcoFalke added this to the milestone 0.17.1 on Nov 28, 2018

luke-jr referenced this in commit 21da2bc870 on Dec 21, 2018

luke-jr referenced this in commit 66f09883ef on Dec 21, 2018

luke-jr referenced this in commit a1cac5682e on Dec 21, 2018

fanquake deleted a comment on Feb 26, 2019

fanquake locked this on Feb 26, 2019