Merge #14685: fix a deserialization overflow edge case
b08af10fb2
disallow oversized CBlockHeaderAndShortTxIDs (Kaz Wesley)6bed4b374d
fix a deserialization overflow edge case (Kaz Wesley)051faf7e9d
add a test demonstrating an overflow in a deserialization edge case (Kaz Wesley) Pull request description: 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. Tree-SHA512: 1aaf7636e0801a905ed8807d0d1762132ac8b4421a600c35fb6d5e5033c6bfb587d8668cd9f48c7a08a2ae793a677b7649661e3ae248ab4f8499ab7b6ede483c
This commit is contained in:
commit
6d58a5c3b0
2 changed files with 51 additions and 3 deletions
|
@ -52,12 +52,12 @@ public:
|
|||
}
|
||||
}
|
||||
|
||||
uint16_t offset = 0;
|
||||
int32_t offset = 0;
|
||||
for (size_t j = 0; j < indexes.size(); j++) {
|
||||
if (uint64_t(indexes[j]) + uint64_t(offset) > std::numeric_limits<uint16_t>::max())
|
||||
if (int32_t(indexes[j]) + offset > std::numeric_limits<uint16_t>::max())
|
||||
throw std::ios_base::failure("indexes overflowed 16 bits");
|
||||
indexes[j] = indexes[j] + offset;
|
||||
offset = indexes[j] + 1;
|
||||
offset = int32_t(indexes[j]) + 1;
|
||||
}
|
||||
} else {
|
||||
for (size_t i = 0; i < indexes.size(); i++) {
|
||||
|
@ -186,6 +186,9 @@ public:
|
|||
|
||||
READWRITE(prefilledtxn);
|
||||
|
||||
if (BlockTxCount() > std::numeric_limits<uint16_t>::max())
|
||||
throw std::ios_base::failure("indexes overflowed 16 bits");
|
||||
|
||||
if (ser_action.ForRead())
|
||||
FillShortTxIDSelector();
|
||||
}
|
||||
|
|
|
@ -344,4 +344,49 @@ BOOST_AUTO_TEST_CASE(TransactionsRequestSerializationTest) {
|
|||
BOOST_CHECK_EQUAL(req1.indexes[3], req2.indexes[3]);
|
||||
}
|
||||
|
||||
BOOST_AUTO_TEST_CASE(TransactionsRequestDeserializationMaxTest) {
|
||||
// Check that the highest legal index is decoded correctly
|
||||
BlockTransactionsRequest req0;
|
||||
req0.blockhash = InsecureRand256();
|
||||
req0.indexes.resize(1);
|
||||
req0.indexes[0] = 0xffff;
|
||||
CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
|
||||
stream << req0;
|
||||
|
||||
BlockTransactionsRequest req1;
|
||||
stream >> req1;
|
||||
BOOST_CHECK_EQUAL(req0.indexes.size(), req1.indexes.size());
|
||||
BOOST_CHECK_EQUAL(req0.indexes[0], req1.indexes[0]);
|
||||
}
|
||||
|
||||
BOOST_AUTO_TEST_CASE(TransactionsRequestDeserializationOverflowTest) {
|
||||
// Any set of index deltas that starts with N values that sum to (0x10000 - N)
|
||||
// causes the edge-case overflow that was originally not checked for. Such
|
||||
// a request cannot be created by serializing a real BlockTransactionsRequest
|
||||
// due to the overflow, so here we'll serialize from raw deltas.
|
||||
BlockTransactionsRequest req0;
|
||||
req0.blockhash = InsecureRand256();
|
||||
req0.indexes.resize(3);
|
||||
req0.indexes[0] = 0x7000;
|
||||
req0.indexes[1] = 0x10000 - 0x7000 - 2;
|
||||
req0.indexes[2] = 0;
|
||||
CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
|
||||
stream << req0.blockhash;
|
||||
WriteCompactSize(stream, req0.indexes.size());
|
||||
WriteCompactSize(stream, req0.indexes[0]);
|
||||
WriteCompactSize(stream, req0.indexes[1]);
|
||||
WriteCompactSize(stream, req0.indexes[2]);
|
||||
|
||||
BlockTransactionsRequest req1;
|
||||
try {
|
||||
stream >> req1;
|
||||
// before patch: deserialize above succeeds and this check fails, demonstrating the overflow
|
||||
BOOST_CHECK(req1.indexes[1] < req1.indexes[2]);
|
||||
// this shouldn't be reachable before or after patch
|
||||
BOOST_CHECK(0);
|
||||
} catch(std::ios_base::failure &) {
|
||||
// deserialize should fail
|
||||
}
|
||||
}
|
||||
|
||||
BOOST_AUTO_TEST_SUITE_END()
|
||||
|
|
Loading…
Reference in a new issue