net: Fix an uninitialized read in ProcessMessage(…, "tx", …) when receiving a transaction we already have

[practicalswift]

Fix an uninitialized read in ProcessMessage(…, "tx", …) when receiving a transaction we already have.

The uninitialized value is read and used on L2526 in the case of AlreadyHave(inv) == true.

Proof of concept being run against a bitcoind built with MemorySanitizer (-fsanitize=memory):

$ ./p2p-uninit-read-in-conditional-poc.py
Usage: ./p2p-uninit-read-in-conditional-poc.py <dstaddr> <dstport> <net>
$ bitcoind -regtest &
$ ./p2p-uninit-read-in-conditional-poc.py 127.0.0.1 18444 regtest
SUMMARY: MemorySanitizer: use-of-uninitialized-value
[1]+  Exit 77                 bitcoind -regtest
$

Proof of concept being run against a bitcoind running under Valgrind (valgrind --exit-on-first-error):

$ valgrind -q --exit-on-first-error=yes --error-exitcode=1 bitcoind -regtest &
$ ./p2p-uninit-read-in-conditional-poc.py 127.0.0.1 18444 regtest
==27351== Conditional jump or move depends on uninitialised value(s)
[1]+  Exit 1                  valgrind -q --exit-on-first-error=yes --error-exitcode=1 bitcoind -regtest
$ 

Proof of concept script:

#!/usr/bin/env python3

import sys

from test_framework.mininode import NetworkThread
from test_framework.mininode import P2PDataStore
from test_framework.messages import CTransaction, CTxIn, CTxOut, msg_tx


def send_duplicate_tx(dstaddr="127.0.0.1", dstport=18444, net="regtest"):
    network_thread = NetworkThread()
    network_thread.start()

    node = P2PDataStore()
    node.peer_connect(dstaddr=dstaddr, dstport=dstport, net=net)()
    node.wait_for_verack()

    tx = CTransaction()
    tx.vin.append(CTxIn())
    tx.vout.append(CTxOut())
    node.send_message(msg_tx(tx))
    node.send_message(msg_tx(tx))
    node.peer_disconnect()
    network_thread.close()


if __name__ == "__main__":
    if len(sys.argv) != 4:
        print("Usage: {} <dstaddr> <dstport> <net>".format(sys.argv[0]))
        sys.exit(0)
    send_duplicate_tx(sys.argv[1], int(sys.argv[2]), sys.argv[3])

Note that the transaction in the proof of concept is the simplest possible, but really any transaction can be used. It does not have to be a valid transaction.

This bug was introduced in #15921 ("validation: Tidy up ValidationState interface") which was merged in to master 28 days ago.

Luckily this bug was caught before being part of any Bitcoin Core release :)

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #17557 (util: Refactor message hashing into a utility function by jkczyz)
• #17399 (validation: Templatize ValidationState instead of subclassing by jkczyz)

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.

[jnewbery]

utACK 73b96c9

Great catch @practicalswift . Are the else if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) and else branches really not covered by any of our tests? If so, that seems like a big hole in our testing. We should add tests for duplicate TX messages received.

[laanwj]

ACK 73b96c9, thanks for discovering and reporting this before it ended up in a release.

(travis fail is unrelated, restarted …)

[practicalswift]

@jnewbery

Both branches are covered by our functional tests (specifically test/functional/p2p_segwit.py), but unfortunately we're not running the functional tests under MemorySanitizer or Valgrind :(

I found this issue after observing some weirdness on a mainnet when running a live node with MemorySanitizer instrumentation enabled. Receiving duplicate transactions on mainnet happens quite regularly. I then reproduced the issue on regtest.

Since discovering the issue on mainnet I've "rediscovered" it 1.) using static analysis tooling (Coverity is one example), 2.) by running the ordinary functional tests under MemorySanitizer, and 3.) by running the ordinary functional tests under Valgrind.

My personal view is that we really really underuse the excellent modern tooling that is typically used in security critical C++ projects to guard against introduction of bugs like this. I find that a bit surprising and I promise to do my best to help improve that situation going forward :)

---

Rediscovery 1. Finding the issue using static analysis (Coverity in this example)

________________________________________________________________________________________________________
*** CID 350378:  Uninitialized members  (UNINIT_CTOR)
/src/consensus/validation.h: 117 in TxValidationState::TxValidationState()()
111     };
112     
113     inline ValidationState::~ValidationState() {};
114     
115     class TxValidationState : public ValidationState {
116     private:
>>>     CID 350378:  Uninitialized members  (UNINIT_CTOR)
>>>     The compiler-generated constructor for this class does not initialize "m_result".
117         TxValidationResult m_result;
118     public:
119         bool Invalid(TxValidationResult result,
120                      const std::string &reject_reason="",
121                      const std::string &debug_message="")
122         {
** CID 350377:  Uninitialized variables  (UNINIT)
________________________________________________________________________________________________________
*** CID 350377:  Uninitialized variables  (UNINIT)
/src/net_processing.cpp: 2526 in ProcessMessage(CNode *, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, CDataStream &, long, const CChainParams &, CConnman *, BanMan *, const std::atomic<bool> &)()
2520                     tx.GetHash().ToString(),
2521                     mempool.size(), mempool.DynamicMemoryUsage() / 1000);
2522     
2523                 // Recursively process any orphan transactions that depended on this one
2524                 ProcessOrphanTx(connman, pfrom->orphan_work_set, lRemovedTxn);
2525             }
>>>     CID 350377:  Uninitialized variables  (UNINIT)
>>>     Using uninitialized value "state.m_result" when calling "GetResult".
2526             else if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS)
2527             {
2528                 bool fRejectedParents = false; // It may be the case that the orphans parents have all been rejected
2529                 for (const CTxIn& txin : tx.vin) {
2530                     if (recentRejects->contains(txin.prevout.hash)) {
2531                         fRejectedParents = true;

Rediscovery 2. Finding the issue using dynamic analysis (MemorySanitizer)

Running test/functional/test_runner.py with bitcoind compiled with MSAN instrumentation:

$ test/functional/test_runner.py
…
2020-01-02T04:05:06.123456Z TestFramework (INFO): Subtest: test_non_standard_witness_blinding (Segwit active = True)
2020-01-02T04:05:06.123456Z TestFramework (ERROR): Assertion failed
…
SUMMARY: MemorySanitizer: use-of-uninitialized-value

Rediscovery 3. Finding the issue using dynamic analysis (Valgrind)

Running BITCOIND=bitcoind_valgrind test/functional/test_runner.py where bitcoind_valgrind is a shell script wrapper doing valgrind -q --exit-on-first-error=yes --error-exitcode=1 --gen-suppressions=all --leak-check=full --show-leak-kinds=all --suppressions=contrib/valgrind.supp src/bitcoind "$@":

$ BITCOIND=bitcoind_valgrind test/functional/test_runner.py
…
2020-01-02T04:05:06.123456Z TestFramework (INFO): Subtest: test_non_standard_witness_blinding (Segwit active = True)
2020-01-02T04:05:06.123456Z TestFramework (ERROR): Assertion failed
…
==47281== Conditional jump or move depends on uninitialised value(s)