fix a deserialization overflow edge case
ClosedPublic
Actions

Authored by Fabien on Jan 23 2020, 10:54.

Details

Reviewers

deadalnix
jasonbcox

Group Reviewers

Restricted Project

Commits

rSTAGINGec5df583ca34: fix a deserialization overflow edge case
rABCec5df583ca34: fix a deserialization overflow edge case

Summary

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.

Backport of core PR14685.

The bug this PR is fixing doesn't apply directly to our code, since we
are already using 32 bit indexes (renames indices) since D1146 and the
deserialization size is limited to 0x02000000 by the MAX_SIZE constant
(see https://bitcoin.org/en/developer-reference#message-headers), so
MAX_SIZE < max(uint32_t) is the limiting factor for us.
In the meantime the overflow issue will occur if the MAX_SIZE constant
is raised above max(uint32_t).

This diff is also adapted to test the BlockTransactionRequest
deserialization against MAX_SIZE and make sure the overflow edge-case
is un der control should the constant get raised. Static asserts will
cause the build to fail if the tests assumptions are no longer valid.

Test Plan

ninja all check
./test/functional/test_runner.py abc-p2p-compactblocks

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Fabien created this revision.Jan 23 2020, 10:54

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 23 2020, 10:54

Herald added a reviewer: deadalnix. · View Herald Transcript

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those Bitcoin Core PRs have been inserted into the summary for reference.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9115: Diff 15755.Jan 23 2020, 11:00

Fabien mentioned this in D5050: tests: Reduce noise level in test_bitcoin output.Jan 23 2020, 11:43

jasonbcox accepted this revision.Jan 24 2020, 18:27

jasonbcox added a subscriber: jasonbcox.

jasonbcox added inline comments.

src/test/blockencodings_tests.cpp
434 ↗	(On Diff #15755)	Nit: the plurality here is weird. should be `indexType` or `indicesType`, of which the latter is probably more correct.
436 ↗	(On Diff #15755)	This isn't the first place I would put this check (somewhere near the definition seems more natural to me), but I'm not able to put my finger on a strong objective reasoning for this.
466 ↗	(On Diff #15755)	ditto
486 ↗	(On Diff #15755)

This revision is now accepted and ready to land.Jan 24 2020, 18:27

Fabien added inline comments.Jan 24 2020, 20:49

src/test/blockencodings_tests.cpp
434 ↗	(On Diff #15755)	This is the type of an element of the vector of indices, so I think `indiceType` is correct.
436 ↗	(On Diff #15755)	I'm not sure this assertion should hold as a general case. If one day we raise MAX_SIZE to something larger, I'm make sure that this test will fail and will need to be updated. I'm not sure that the code will fail, only this test (and eventually the next one).
466 ↗	(On Diff #15755)	Ditto :)