oxen-io / oxen-core

Oxen core repository, containing oxend and oxen cli wallets
https://oxen.io
Other
317 stars 120 forks source link

Reject malformed blocks in the parse and validate stage #1713

Closed Doy-lee closed 2 months ago

Doy-lee commented 2 months ago

It's possible to submit a block on the P2P layer that parses correctly but has an incorrect miner TX. This causes a crash because of a std::optional nullopt dereference when calculating the block hash that accesses the miner TX unsafely.

Additionally a default initialised block sets the miner transaction to nullopt but the major version to 7 which is where a miner TX is mandated. This block construction innocently passes a bunch of checks and then code that assumes that the block is trusted input data ends up leading to an exception being thrown when the block hash is calculated as it tries to read the miner TX.

This pulls the check much earlier so that the block is relatively "well-formed" when it is parsed and passes the validate stage (but the specific implementation details like TX content may still be invalid) that the block can be forwarded down to upper layers in the codebase

--

Default initialised blocks still pose a problem and is a footgun to be passing around default initialised blocks (because they generate a bad miner tx value). There's another bug somewhere hidden in the P2P layer where default initialised blocks are slipping through instead of the block parsed from the blob (and infact can be exploited by a malicious peer sending zero-init blocks when requested for a height to cause a crash which is mitigated in this PR).

jagerman commented 2 months ago

Aside from the return value, LGTM.