From 38ec36f8e939f1446bacca4d9918818d5bea34a4 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Thu, 23 Jan 2020 11:44:21 -0600 Subject: [PATCH] Consolidated Security Fixes for 2.0.1 - Reduce net plugin logging and handshake size limits. - Improved handling of deferred transactions during block production. - Earlier block validation for greater security. Co-Authored-By: Kevin Heifner Co-Authored-By: Kayan --- libraries/chain/controller.cpp | 16 ++++++-- .../include/eosio/net_plugin/protocol.hpp | 7 ++++ plugins/net_plugin/net_plugin.cpp | 40 +++++++++++++++---- unittests/block_tests.cpp | 7 ++++ unittests/forked_tests.cpp | 2 +- 5 files changed, 59 insertions(+), 13 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index 480ea01cbd1..6eac8f10361 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -123,6 +123,7 @@ struct building_block { vector _pending_trx_metas; vector _pending_trx_receipts; vector _actions; + optional _transaction_mroot; }; struct assembled_block { @@ -1308,7 +1309,7 @@ struct controller_impl { // Only subjective OR soft OR hard failure logic below: - if( gtrx.sender != account_name() && !failure_is_subjective(*trace->except)) { + if( gtrx.sender != account_name() && !(explicit_billed_cpu_time ? failure_is_subjective(*trace->except) : scheduled_failure_is_subjective(*trace->except))) { // Attempt error handling for the generated transaction. auto error_trace = apply_onerror( gtrx, deadline, trx_context.pseudo_start, @@ -1677,7 +1678,7 @@ struct controller_impl { // Create (unsigned) block: auto block_ptr = std::make_shared( pbhs.make_block_header( - calculate_trx_merkle(), + bb._transaction_mroot ? *bb._transaction_mroot : calculate_trx_merkle( bb._pending_trx_receipts ), calculate_action_merkle(), bb._new_pending_producer_schedule, std::move( bb._new_protocol_feature_activations ), @@ -1898,6 +1899,9 @@ struct controller_impl { ("producer_receipt", receipt)("validator_receipt", trx_receipts.back()) ); } + // validated in create_block_state_future() + pending->_block_stage.get()._transaction_mroot = b->transaction_mroot; + finalize_block(); auto& ab = pending->_block_stage.get(); @@ -1936,6 +1940,11 @@ struct controller_impl { return async_thread_pool( thread_pool.get_executor(), [b, prev, control=this]() { const bool skip_validate_signee = false; + + auto trx_mroot = calculate_trx_merkle( b->transactions ); + EOS_ASSERT( b->transaction_mroot == trx_mroot, block_validate_exception, + "invalid block transaction merkle root ${b} != ${c}", ("b", b->transaction_mroot)("c", trx_mroot) ); + return std::make_shared( *prev, move( b ), @@ -2126,9 +2135,8 @@ struct controller_impl { return merkle( move(action_digests) ); } - checksum256_type calculate_trx_merkle() { + static checksum256_type calculate_trx_merkle( const vector& trxs ) { vector trx_digests; - const auto& trxs = pending->_block_stage.get()._pending_trx_receipts; trx_digests.reserve( trxs.size() ); for( const auto& a : trxs ) trx_digests.emplace_back( a.digest() ); diff --git a/plugins/net_plugin/include/eosio/net_plugin/protocol.hpp b/plugins/net_plugin/include/eosio/net_plugin/protocol.hpp index a806486b50a..8ce781cefd5 100644 --- a/plugins/net_plugin/include/eosio/net_plugin/protocol.hpp +++ b/plugins/net_plugin/include/eosio/net_plugin/protocol.hpp @@ -17,6 +17,13 @@ namespace eosio { block_id_type head_id; }; + // Longest domain name is 253 characters according to wikipedia. + // Addresses include ":port" where max port is 65535, which adds 6 chars. + // We also add our own extentions of "[:trx|:blk] - xxxxxxx", which adds 14 chars, total= 273. + // Allow for future extentions as well, hence 384. + constexpr size_t max_p2p_address_length = 253 + 6; + constexpr size_t max_handshake_str_length = 384; + struct handshake_message { uint16_t network_version = 0; ///< incremental value above a computed base chain_id_type chain_id; ///< used to identify chain diff --git a/plugins/net_plugin/net_plugin.cpp b/plugins/net_plugin/net_plugin.cpp index d8c475dbc39..b50abce4f5c 100644 --- a/plugins/net_plugin/net_plugin.cpp +++ b/plugins/net_plugin/net_plugin.cpp @@ -828,7 +828,7 @@ namespace eosio { last_handshake_recv(), last_handshake_sent() { - fc_ilog( logger, "accepted network connection" ); + fc_dlog( logger, "new connection object created" ); } void connection::update_endpoints() { @@ -855,13 +855,13 @@ namespace eosio { peer_add.substr( colon2 + 1 ) : peer_add.substr( colon2 + 1, end - (colon2 + 1) ); if( type.empty() ) { - fc_ilog( logger, "Setting connection type for: ${peer} to both transactions and blocks", ("peer", peer_add) ); + fc_dlog( logger, "Setting connection type for: ${peer} to both transactions and blocks", ("peer", peer_add) ); connection_type = both; } else if( type == "trx" ) { - fc_ilog( logger, "Setting connection type for: ${peer} to transactions only", ("peer", peer_add) ); + fc_dlog( logger, "Setting connection type for: ${peer} to transactions only", ("peer", peer_add) ); connection_type = transactions_only; } else if( type == "blk" ) { - fc_ilog( logger, "Setting connection type for: ${peer} to blocks only", ("peer", peer_add) ); + fc_dlog( logger, "Setting connection type for: ${peer} to blocks only", ("peer", peer_add) ); connection_type = blocks_only; } else { fc_wlog( logger, "Unknown connection type: ${t}", ("t", type) ); @@ -2190,6 +2190,7 @@ namespace eosio { c->connect( resolver, endpoints ); } else { fc_elog( logger, "Unable to resolve ${add}: ${error}", ("add", c->peer_name())( "error", err.message() ) ); + c->connecting = false; ++c->consecutive_immediate_connection_close; } } ) ); @@ -2261,10 +2262,10 @@ namespace eosio { } else { if( from_addr >= max_nodes_per_host ) { - fc_elog( logger, "Number of connections (${n}) from ${ra} exceeds limit ${l}", + fc_dlog( logger, "Number of connections (${n}) from ${ra} exceeds limit ${l}", ("n", from_addr + 1)( "ra", paddr_str )( "l", max_nodes_per_host )); } else { - fc_elog( logger, "Error max_client_count ${m} exceeded", ("m", max_client_count)); + fc_dlog( logger, "max_client_count ${m} exceeded", ("m", max_client_count)); } // new_connection never added to connections and start_session not called, lifetime will end boost::system::error_code ec; @@ -2524,10 +2525,21 @@ namespace eosio { if (msg.p2p_address.empty()) { fc_wlog( logger, "Handshake message validation: p2p_address is null string" ); valid = false; + } else if( msg.p2p_address.length() > max_handshake_str_length ) { + // see max_handshake_str_length comment in protocol.hpp + fc_wlog( logger, "Handshake message validation: p2p_address to large: ${p}", ("p", msg.p2p_address.substr(0, max_handshake_str_length) + "...") ); + valid = false; } if (msg.os.empty()) { fc_wlog( logger, "Handshake message validation: os field is null string" ); valid = false; + } else if( msg.os.length() > max_handshake_str_length ) { + fc_wlog( logger, "Handshake message validation: os field to large: ${p}", ("p", msg.os.substr(0, max_handshake_str_length) + "...") ); + valid = false; + } + if( msg.agent.length() > max_handshake_str_length ) { + fc_wlog( logger, "Handshake message validation: agent field to large: ${p}", ("p", msg.agent.substr(0, max_handshake_str_length) + "...") ); + valid = false; } if ((msg.sig != chain::signature_type() || msg.token != sha256()) && (msg.token != fc::sha256::hash(msg.time))) { fc_wlog( logger, "Handshake message validation: token field invalid" ); @@ -3066,7 +3078,7 @@ namespace eosio { std::unique_lock g( connections_mtx ); auto it = (from ? connections.find(from) : connections.begin()); if (it == connections.end()) it = connections.begin(); - size_t num_rm = 0; + size_t num_rm = 0, num_clients = 0, num_peers = 0; while (it != connections.end()) { if (fc::time_point::now() >= max_time) { connection_wptr wit = *it; @@ -3077,13 +3089,16 @@ namespace eosio { } return; } + (*it)->peer_address().empty() ? ++num_clients : ++num_peers; if( !(*it)->socket_is_open() && !(*it)->connecting) { - if( (*it)->peer_address().length() > 0) { + if( !(*it)->peer_address().empty() ) { if( !(*it)->resolve_and_connect() ) { it = connections.erase(it); + --num_peers; ++num_rm; continue; } } else { + --num_clients; ++num_rm; it = connections.erase(it); continue; } @@ -3091,6 +3106,9 @@ namespace eosio { ++it; } g.unlock(); + if( num_clients > 0 || num_peers > 0 ) + fc_ilog( logger, "p2p client connections: ${num}/${max}, peer connections: ${pnum}/${pmax}", + ("num", num_clients)("max", max_client_count)("pnum", num_peers)("pmax", supplied_peers.size()) ); fc_dlog( logger, "connection monitor, removed ${n} connections", ("n", num_rm) ); if( reschedule ) { start_conn_timer( connector_period, std::weak_ptr()); @@ -3321,9 +3339,13 @@ namespace eosio { if( options.count( "p2p-listen-endpoint" ) && options.at("p2p-listen-endpoint").as().length()) { my->p2p_address = options.at( "p2p-listen-endpoint" ).as(); + EOS_ASSERT( my->p2p_address.length() <= max_p2p_address_length, chain::plugin_config_exception, + "p2p-listen-endpoint to long, must be less than ${m}", ("m", max_p2p_address_length) ); } if( options.count( "p2p-server-address" ) ) { my->p2p_server_address = options.at( "p2p-server-address" ).as(); + EOS_ASSERT( my->p2p_server_address.length() <= max_p2p_address_length, chain::plugin_config_exception, + "p2p_server_address to long, must be less than ${m}", ("m", max_p2p_address_length) ); } my->thread_pool_size = options.at( "net-threads" ).as(); @@ -3335,6 +3357,8 @@ namespace eosio { } if( options.count( "agent-name" )) { my->user_agent_name = options.at( "agent-name" ).as(); + EOS_ASSERT( my->user_agent_name.length() <= max_handshake_str_length, chain::plugin_config_exception, + "agent-name to long, must be less than ${m}", ("m", max_handshake_str_length) ); } if( options.count( "allowed-connection" )) { diff --git a/unittests/block_tests.cpp b/unittests/block_tests.cpp index 023907ce3e6..3b98e4082d3 100644 --- a/unittests/block_tests.cpp +++ b/unittests/block_tests.cpp @@ -30,6 +30,13 @@ BOOST_AUTO_TEST_CASE(block_with_invalid_tx_test) auto invalid_packed_tx = packed_transaction(signed_tx); copy_b->transactions.back().trx = invalid_packed_tx; + // Re-calculate the transaction merkle + vector trx_digests; + const auto& trxs = copy_b->transactions; + for( const auto& a : trxs ) + trx_digests.emplace_back( a.digest() ); + copy_b->transaction_mroot = merkle( move(trx_digests) ); + // Re-sign the block auto header_bmroot = digest_type::hash( std::make_pair( copy_b->digest(), main.control->head_block_state()->blockroot_merkle.get_root() ) ); auto sig_digest = digest_type::hash( std::make_pair(header_bmroot, main.control->head_block_state()->pending_schedule.schedule_hash) ); diff --git a/unittests/forked_tests.cpp b/unittests/forked_tests.cpp index 7268a6d78c6..4ff7b07d350 100644 --- a/unittests/forked_tests.cpp +++ b/unittests/forked_tests.cpp @@ -265,7 +265,7 @@ BOOST_AUTO_TEST_CASE( forking ) try { wlog( "end push c2 blocks to c1" ); wlog( "now push dan's block to c1 but first corrupt it so it is a bad block" ); signed_block bad_block = std::move(*b); - bad_block.transaction_mroot = bad_block.previous; + bad_block.action_mroot = bad_block.previous; auto bad_block_bs = c.control->create_block_state_future( std::make_shared(std::move(bad_block)) ); c.control->abort_block(); BOOST_REQUIRE_EXCEPTION(c.control->push_block( bad_block_bs, forked_branch_callback{}, trx_meta_cache_lookup{} ), fc::exception,