validation: don't try to invalidate genesis block in CChainState::InvalidateBlock #20921

pull theStack wants to merge 1 commits into bitcoin:master from theStack:2021-rpc-forbid_invalidateblock_on_genesisblock changing 1 files +4 −0

theStack commented at 12:25 AM on January 13, 2021: member

In the block invalidation method (CChainState::InvalidateBlock), the code for creating the candidate block map assumes that the passed block's previous block (pindex->pprev) is available and otherwise segfaults due to null-pointer deference in CBlockIndexWorkComparator() (see analysis by practicalswift in #20914), i.e. it doesn't work with the genesis block. Rather than analyzing all possible code paths and implications for this corner case, simply fail early if the genesis block is passed.

Fixes #20914.
fanquake added the label Validation on Jan 13, 2021
DrahtBot commented at 11:45 AM on January 13, 2021: member



🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.
luke-jr approved
luke-jr commented at 4:12 PM on January 13, 2021: member

utACK

in src/validation.cpp:2928 in 91ff544419 outdated

2922 | @@ -2923,6 +2923,9 @@ bool PreciousBlock(BlockValidationState& state, const CChainParams& params, CBlo
2923 |  
2924 |  bool CChainState::InvalidateBlock(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex *pindex)
2925 |  {
2926 | +    // Genesis block can't be invalidated
2927 | +    if (pindex->nHeight == 0) return false;

ViralTaco commented at 5:48 AM on February 3, 2021:

This is actually not preventing UB, it's adding more. Suggested fix here is adding a nullptrcheck.

if (pindex == nullptr) {
  // properly handle error
  throw std::runtime_error("nullptr dereference in \'src\/validation.cpp\' in \'bool CChainState::InvalidateBlock(BlockValidationState&, CChainParams const&, CBlockIndex* pindex)\' \npindex was nullptr");
} else if (pindex->nHeight == 0) {
  return false;
}

ViralTaco changes_requested
ViralTaco commented at 6:02 AM on February 3, 2021: none

Add nullptr check for pindex in BlockValidationState&, CChainParams const&, CBlockIndex*)

I'm sorry I can't commit I need to setup the environment
validation: don't try to invalidate genesis block 787df19b09
theStack force-pushed on Feb 9, 2021
theStack commented at 11:16 PM on February 9, 2021: member

@ViralTaco: Agree that there should also be a null pointer check on pindex before access. Solved this now by adding a simple assert (alternatively one could change the parameter to a reference).
sipa commented at 2:24 AM on March 19, 2021: member

ACK 787df19b09babf50dd8124b3ac990b29c33cfe93. Tested invalidation of generic on regtest.

Would be nice to add a functional test that attempts this, so that it gets detected if it were reintroduced.

practicalswift commented at 9:02 PM on March 19, 2021: contributor

Tested ACK 787df19b09babf50dd8124b3ac990b29c33cfe93

Before:

$ git checkout master
$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --with-sanitizers=address,undefined
$ make
$ src/bitcoind -regtest &
$ src/bitcoin-cli -regtest generatetodescriptor 1 'addr(bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3xueyj)#juyq9d97'
$ src/bitcoin-cli -regtest invalidateblock 0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206
$ src/bitcoin-cli -regtest invalidateblock 0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206
validation.cpp:89:30: runtime error: member access within null pointer of type 'const CBlockIndex'
==11604==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000044 (pc 0x56387ee23e87 bp 0x000000000008 sp 0x7f3a6e1b6570 T16)
==11604==The signal is caused by a READ memory access.
==11604==Hint: address points to the zero page.

After:

$ gh pr checkout 20921
$ make -C src/ bitcoind
$ src/bitcoind -regtest &
$ src/bitcoin-cli -regtest generatetodescriptor 1 'addr(bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3xueyj)#juyq9d97'
$ src/bitcoin-cli -regtest invalidateblock 0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206
$ src/bitcoin-cli -regtest invalidateblock 0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206
$ jobs
[1]+  Running                 src/bitcoind -regtest &

MarcoFalke merged this on Mar 20, 2021
MarcoFalke closed this on Mar 20, 2021
sidhujag referenced this in commit aca1a216dd on Mar 20, 2021
fanquake locked this on Jun 23, 2021

Contributors

Labels

Linked (view graph)

#20914 Null pointer derefence in CBlockIndexWorkComparator::operator() reachable via RPC call invalidateblock