From 57f7c2e53fe4ba86fc5ac87ff0eb9c2bb389e936 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Thu, 31 Dec 2020 21:12:30 +0000 Subject: [PATCH] protocol: more sanity checks in new chain block hashes --- src/cryptonote_core/blockchain.cpp | 16 ++++- src/cryptonote_core/blockchain.h | 4 +- src/cryptonote_core/cryptonote_core.cpp | 9 ++- src/cryptonote_core/cryptonote_core.h | 5 +- .../cryptonote_protocol_handler.inl | 64 +++++++++++++++---- tests/core_proxy/core_proxy.cpp | 7 +- tests/core_proxy/core_proxy.h | 3 +- tests/unit_tests/node_server.cpp | 3 +- 8 files changed, 90 insertions(+), 21 deletions(-) diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index 594aa1e9c..cd5b4f455 100644 --- a/src/cryptonote_core/blockchain.cpp +++ b/src/cryptonote_core/blockchain.cpp @@ -2758,32 +2758,44 @@ void Blockchain::flush_invalid_blocks() m_invalid_blocks.clear(); } //------------------------------------------------------------------ -bool Blockchain::have_block(const crypto::hash& id) const +bool Blockchain::have_block_unlocked(const crypto::hash& id, int *where) const { + // WARNING: this function does not take m_blockchain_lock, and thus should only call read only + // m_db functions which do not depend on one another (ie, no getheight + gethash(height-1), as + // well as not accessing class members, even read only (ie, m_invalid_blocks). The caller must + // lock if it is otherwise needed. LOG_PRINT_L3("Blockchain::" << __func__); - CRITICAL_REGION_LOCAL(m_blockchain_lock); if(m_db->block_exists(id)) { LOG_PRINT_L2("block " << id << " found in main chain"); + if (where) *where = HAVE_BLOCK_MAIN_CHAIN; return true; } if(m_db->get_alt_block(id, NULL, NULL)) { LOG_PRINT_L2("block " << id << " found in alternative chains"); + if (where) *where = HAVE_BLOCK_ALT_CHAIN; return true; } if(m_invalid_blocks.count(id)) { LOG_PRINT_L2("block " << id << " found in m_invalid_blocks"); + if (where) *where = HAVE_BLOCK_INVALID; return true; } return false; } //------------------------------------------------------------------ +bool Blockchain::have_block(const crypto::hash& id, int *where) const +{ + CRITICAL_REGION_LOCAL(m_blockchain_lock); + return have_block_unlocked(id, where); +} +//------------------------------------------------------------------ bool Blockchain::handle_block_to_main_chain(const block& bl, block_verification_context& bvc, bool notify/* = true*/) { LOG_PRINT_L3("Blockchain::" << __func__); diff --git a/src/cryptonote_core/blockchain.h b/src/cryptonote_core/blockchain.h index 66c3bc395..3529255e1 100644 --- a/src/cryptonote_core/blockchain.h +++ b/src/cryptonote_core/blockchain.h @@ -376,10 +376,12 @@ namespace cryptonote * for a block with the given hash * * @param id the hash to search for + * @param where the type of block, if non NULL * * @return true if the block is known, else false */ - bool have_block(const crypto::hash& id) const; + bool have_block_unlocked(const crypto::hash& id, int *where = NULL) const; + bool have_block(const crypto::hash& id, int *where = NULL) const; /** * @brief gets the total number of transactions on the main chain diff --git a/src/cryptonote_core/cryptonote_core.cpp b/src/cryptonote_core/cryptonote_core.cpp index 507c47a51..12125fb9d 100644 --- a/src/cryptonote_core/cryptonote_core.cpp +++ b/src/cryptonote_core/cryptonote_core.cpp @@ -1625,9 +1625,14 @@ namespace cryptonote return m_mempool.get_transactions_count(include_sensitive_txes); } //----------------------------------------------------------------------------------------------- - bool core::have_block(const crypto::hash& id) const + bool core::have_block_unlocked(const crypto::hash& id, int *where) const { - return m_blockchain_storage.have_block(id); + return m_blockchain_storage.have_block_unlocked(id, where); + } + //----------------------------------------------------------------------------------------------- + bool core::have_block(const crypto::hash& id, int *where) const + { + return m_blockchain_storage.have_block(id, where); } //----------------------------------------------------------------------------------------------- bool core::parse_tx_from_blob(transaction& tx, crypto::hash& tx_hash, const blobdata& blob) const diff --git a/src/cryptonote_core/cryptonote_core.h b/src/cryptonote_core/cryptonote_core.h index fe55d76bf..a6cce8617 100644 --- a/src/cryptonote_core/cryptonote_core.h +++ b/src/cryptonote_core/cryptonote_core.h @@ -55,6 +55,8 @@ PUSH_WARNINGS DISABLE_VS_WARNINGS(4355) +enum { HAVE_BLOCK_MAIN_CHAIN, HAVE_BLOCK_ALT_CHAIN, HAVE_BLOCK_INVALID }; + namespace cryptonote { struct test_options { @@ -543,7 +545,8 @@ namespace cryptonote * * @note see Blockchain::have_block */ - bool have_block(const crypto::hash& id) const; + bool have_block_unlocked(const crypto::hash& id, int *where = NULL) const; + bool have_block(const crypto::hash& id, int *where = NULL) const; /** * @copydoc Blockchain::get_short_chain_history diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.inl b/src/cryptonote_protocol/cryptonote_protocol_handler.inl index c76e5628d..5b456cd39 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler.inl +++ b/src/cryptonote_protocol/cryptonote_protocol_handler.inl @@ -2570,17 +2570,6 @@ skip: return 1; } - std::unordered_set hashes; - for (const auto &h: arg.m_block_ids) - { - if (!hashes.insert(h).second) - { - LOG_ERROR_CCONTEXT("sent duplicate block, dropping connection"); - drop_connection(context, true, false); - return 1; - } - } - uint64_t n_use_blocks = m_core.prevalidate_block_hashes(arg.start_height, arg.m_block_ids, arg.m_block_weights); if (n_use_blocks == 0 || n_use_blocks + HASH_OF_HASHES_STEP <= arg.m_block_ids.size()) { @@ -2592,18 +2581,69 @@ skip: context.m_needed_objects.clear(); uint64_t added = 0; std::unordered_set blocks_found; + bool first = true; + bool expect_unknown = false; for (size_t i = 0; i < arg.m_block_ids.size(); ++i) { if (!blocks_found.insert(arg.m_block_ids[i]).second) { LOG_ERROR_CCONTEXT("Duplicate blocks in chain entry response, dropping connection"); - drop_connection(context, true, false); + drop_connection_with_score(context, 5, false); + return 1; + } + int where; + const bool have_block = m_core.have_block_unlocked(arg.m_block_ids[i], &where); + if (first && !have_block) + { + LOG_ERROR_CCONTEXT("First block hash is unknown, dropping connection"); + drop_connection_with_score(context, 5, false); return 1; } + if (!first) + { + // after the first, blocks may be known or unknown, but if they are known, + // they should be at the same height if on the main chain + if (have_block) + { + switch (where) + { + default: + case HAVE_BLOCK_INVALID: + LOG_ERROR_CCONTEXT("Block is invalid or known without known type, dropping connection"); + drop_connection(context, true, false); + return 1; + case HAVE_BLOCK_MAIN_CHAIN: + if (expect_unknown) + { + LOG_ERROR_CCONTEXT("Block is on the main chain, but we did not expect a known block, dropping connection"); + drop_connection_with_score(context, 5, false); + return 1; + } + if (m_core.get_block_id_by_height(arg.start_height + i) != arg.m_block_ids[i]) + { + LOG_ERROR_CCONTEXT("Block is on the main chain, but not at the expected height, dropping connection"); + drop_connection_with_score(context, 5, false); + return 1; + } + break; + case HAVE_BLOCK_ALT_CHAIN: + if (expect_unknown) + { + LOG_ERROR_CCONTEXT("Block is on the main chain, but we did not expect a known block, dropping connection"); + drop_connection_with_score(context, 5, false); + return 1; + } + break; + } + } + else + expect_unknown = true; + } const uint64_t block_weight = arg.m_block_weights.empty() ? 0 : arg.m_block_weights[i]; context.m_needed_objects.push_back(std::make_pair(arg.m_block_ids[i], block_weight)); if (++added == n_use_blocks) break; + first = false; } context.m_last_response_height -= arg.m_block_ids.size() - n_use_blocks; diff --git a/tests/core_proxy/core_proxy.cpp b/tests/core_proxy/core_proxy.cpp index 8095f03e3..09be8758b 100644 --- a/tests/core_proxy/core_proxy.cpp +++ b/tests/core_proxy/core_proxy.cpp @@ -245,12 +245,17 @@ bool tests::proxy_core::init(const boost::program_options::variables_map& /*vm*/ return true; } -bool tests::proxy_core::have_block(const crypto::hash& id) { +bool tests::proxy_core::have_block_unlocked(const crypto::hash& id, int *where) { if (m_hash2blkidx.end() == m_hash2blkidx.find(id)) return false; + if (where) *where = HAVE_BLOCK_MAIN_CHAIN; return true; } +bool tests::proxy_core::have_block(const crypto::hash& id, int *where) { + return have_block_unlocked(id, where); +} + void tests::proxy_core::build_short_history(std::list &m_history, const crypto::hash &m_start) { m_history.push_front(get_block_hash(m_genesis)); /*std::unordered_map::const_iterator cit = m_hash2blkidx.find(m_lastblk); diff --git a/tests/core_proxy/core_proxy.h b/tests/core_proxy/core_proxy.h index ebc3a89c2..94f148e8c 100644 --- a/tests/core_proxy/core_proxy.h +++ b/tests/core_proxy/core_proxy.h @@ -74,7 +74,8 @@ namespace tests bool init(const boost::program_options::variables_map& vm); bool deinit(){return true;} bool get_short_chain_history(std::list& ids); - bool have_block(const crypto::hash& id); + bool have_block(const crypto::hash& id, int *where = NULL); + bool have_block_unlocked(const crypto::hash& id, int *where = NULL); void get_blockchain_top(uint64_t& height, crypto::hash& top_id); bool handle_incoming_tx(const cryptonote::tx_blob_entry& tx_blob, cryptonote::tx_verification_context& tvc, cryptonote::relay_method tx_relay, bool relayed); bool handle_incoming_txs(const std::vector& tx_blobs, std::vector& tvc, cryptonote::relay_method tx_relay, bool relayed); diff --git a/tests/unit_tests/node_server.cpp b/tests/unit_tests/node_server.cpp index 0569d3748..4d6f09e69 100644 --- a/tests/unit_tests/node_server.cpp +++ b/tests/unit_tests/node_server.cpp @@ -55,7 +55,8 @@ public: bool init(const boost::program_options::variables_map& vm) {return true ;} bool deinit(){return true;} bool get_short_chain_history(std::list& ids) const { return true; } - bool have_block(const crypto::hash& id) const {return true;} + bool have_block(const crypto::hash& id, int *where = NULL) const {return false;} + bool have_block_unlocked(const crypto::hash& id, int *where = NULL) const {return false;} void get_blockchain_top(uint64_t& height, crypto::hash& top_id)const{height=0;top_id=crypto::null_hash;} bool handle_incoming_tx(const cryptonote::tx_blob_entry& tx_blob, cryptonote::tx_verification_context& tvc, cryptonote::relay_method tx_relay, bool relayed) { return true; } bool handle_incoming_txs(const std::vector& tx_blob, std::vector& tvc, cryptonote::relay_method tx_relay, bool relayed) { return true; }