From 4cc82ed12316ed685e86c73e0538e2242cb753c9 Mon Sep 17 00:00:00 2001 From: Keke Li Date: Tue, 23 Mar 2021 18:46:08 -0700 Subject: [PATCH 1/3] Mergeback Consolidated Security Fixes for 2.0.11 PR363 --- plugins/chain_plugin/account_query_db.cpp | 10 +- .../test/test_account_query_db.cpp | 94 +++++++++++++++++++ 2 files changed, 102 insertions(+), 2 deletions(-) diff --git a/plugins/chain_plugin/account_query_db.cpp b/plugins/chain_plugin/account_query_db.cpp index f3d5d8fffc2..a93d328a7f2 100644 --- a/plugins/chain_plugin/account_query_db.cpp +++ b/plugins/chain_plugin/account_query_db.cpp @@ -214,8 +214,13 @@ namespace eosio::chain_apis { auto& index = permission_info_index.get(); const auto& permission_by_owner = controller.db().get_index().indices().get(); + auto curr_iter = index.rbegin(); while (!index.empty()) { - const auto& pi = (*index.rbegin()); + if (curr_iter == index.rend()) { + break; + } + + const auto& pi = (*curr_iter); if (pi.last_updated < t) { break; } @@ -226,7 +231,7 @@ namespace eosio::chain_apis { auto itr = permission_by_owner.find(std::make_tuple(pi.owner, pi.name)); if (itr == permission_by_owner.end()) { // this permission does not exist at this point in the chains history - index.erase(index.iterator_to(pi)); + curr_iter = decltype(curr_iter)( index.erase(index.iterator_to(pi)) ); } else { const auto& po = *itr; index.modify(index.iterator_to(pi), [&po](auto& mutable_pi) { @@ -234,6 +239,7 @@ namespace eosio::chain_apis { mutable_pi.threshold = po.auth.threshold; }); add_to_bimaps(pi, po); + ++curr_iter; } } } diff --git a/plugins/chain_plugin/test/test_account_query_db.cpp b/plugins/chain_plugin/test/test_account_query_db.cpp index dee570a0911..e5024141153 100644 --- a/plugins/chain_plugin/test/test_account_query_db.cpp +++ b/plugins/chain_plugin/test/test_account_query_db.cpp @@ -97,5 +97,99 @@ BOOST_FIXTURE_TEST_CASE(updateauth_test, TESTER) { try { } FC_LOG_AND_RETHROW() } +BOOST_AUTO_TEST_CASE(fork_test) { try { + tester node_a(setup_policy::none); + tester node_b(setup_policy::none); + + // instantiate an account_query_db + auto aq_db = account_query_db(*node_a.control); + + //link aq_db to the `accepted_block` signal on the controller + auto c = node_a.control->accepted_block.connect([&](const block_state_ptr& blk) { + aq_db.commit_block( blk); + }); + + // create 10 blocks synced + for (int i = 0; i < 10; i++) { + node_b.push_block(node_a.produce_block()); + } + + // produce a block on node A with a new account and permission + const auto& tester_account = N(tester); + const auto& tester_account2 = N(tester2); + const string role = "first"; + node_a.create_account(tester_account); + node_a.create_account(tester_account2); + + const auto trace_ptr = node_a.push_action(config::system_account_name, updateauth::get_name(), tester_account, fc::mutable_variant_object() + ("account", tester_account) + ("permission", N(role)) + ("parent", "active") + ("auth", authority(node_a.get_public_key(tester_account, role), 5)), 1 + ); + aq_db.cache_transaction_trace(trace_ptr); + const auto trace_ptr2 = node_a.push_action(config::system_account_name, updateauth::get_name(), tester_account2, fc::mutable_variant_object() + ("account", tester_account2) + ("permission", N(role)) + ("parent", "active") + ("auth", authority(node_a.get_public_key(tester_account2, role), 5)), 2 + ); + aq_db.cache_transaction_trace(trace_ptr2); + node_a.produce_block(); + + params pars; + pars.keys.emplace_back(node_a.get_public_key(tester_account, role)); + + const auto pre_results = aq_db.get_accounts_by_authorizers(pars); + BOOST_TEST_REQUIRE(find_account_auth(pre_results, tester_account, N(role)) == true); + + // have node B take over from head-1 and also update permissions + node_b.create_account(tester_account); + node_b.create_account(tester_account2); + + const auto trace_ptr3 = node_b.push_action(config::system_account_name, updateauth::get_name(), tester_account, fc::mutable_variant_object() + ("account", tester_account) + ("permission", N(role)) + ("parent", "active") + ("auth", authority(node_b.get_public_key(tester_account, role), 6)), 1 + ); + aq_db.cache_transaction_trace(trace_ptr3); + const auto trace_ptr4 = node_b.push_action(config::system_account_name, updateauth::get_name(), tester_account2, fc::mutable_variant_object() + ("account", tester_account2) + ("permission", N(role)) + ("parent", "active") + ("auth", authority(node_b.get_public_key(tester_account2, role), 6)), 2 + ); + aq_db.cache_transaction_trace(trace_ptr4); + + // push b's onto a + node_a.push_block(node_b.produce_block()); + + const auto trace_ptr5 = node_b.push_action(config::system_account_name, updateauth::get_name(), tester_account, fc::mutable_variant_object() + ("account", tester_account) + ("permission", N(role)) + ("parent", "active") + ("auth", authority(node_b.get_public_key(tester_account, role), 5)), 3 + ); + aq_db.cache_transaction_trace(trace_ptr5); + const auto trace_ptr6 = node_b.push_action(config::system_account_name, updateauth::get_name(), tester_account2, fc::mutable_variant_object() + ("account", tester_account2) + ("permission", N(role)) + ("parent", "active") + ("auth", authority(node_b.get_public_key(tester_account2, role), 5)), 4 + ); + aq_db.cache_transaction_trace(trace_ptr6); + + node_a.push_block(node_b.produce_block()); + + // ensure the account was forked away + const auto post_results = aq_db.get_accounts_by_authorizers(pars); + + // verify correct account is in results + BOOST_TEST_REQUIRE(post_results.accounts.size() == 1); + + } FC_LOG_AND_RETHROW() } + + BOOST_AUTO_TEST_SUITE_END() From 82b0283be468228cdccea0cee8d77688e8437a09 Mon Sep 17 00:00:00 2001 From: Keke Li Date: Tue, 23 Mar 2021 19:26:13 -0700 Subject: [PATCH 2/3] change N(tester) to "tester"_n --- plugins/chain_plugin/test/test_account_query_db.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/chain_plugin/test/test_account_query_db.cpp b/plugins/chain_plugin/test/test_account_query_db.cpp index e5024141153..f3d136c4537 100644 --- a/plugins/chain_plugin/test/test_account_query_db.cpp +++ b/plugins/chain_plugin/test/test_account_query_db.cpp @@ -115,8 +115,8 @@ BOOST_AUTO_TEST_CASE(fork_test) { try { } // produce a block on node A with a new account and permission - const auto& tester_account = N(tester); - const auto& tester_account2 = N(tester2); + const auto& tester_account = "tester"_n; + const auto& tester_account2 = "tester2"_n; const string role = "first"; node_a.create_account(tester_account); node_a.create_account(tester_account2); From a1764d491633964d51bc502525be932491932be5 Mon Sep 17 00:00:00 2001 From: Keke Li Date: Tue, 23 Mar 2021 20:46:43 -0700 Subject: [PATCH 3/3] change N(role) to "role"_n --- .../chain_plugin/test/test_account_query_db.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/plugins/chain_plugin/test/test_account_query_db.cpp b/plugins/chain_plugin/test/test_account_query_db.cpp index f3d136c4537..469f37461e3 100644 --- a/plugins/chain_plugin/test/test_account_query_db.cpp +++ b/plugins/chain_plugin/test/test_account_query_db.cpp @@ -123,14 +123,14 @@ BOOST_AUTO_TEST_CASE(fork_test) { try { const auto trace_ptr = node_a.push_action(config::system_account_name, updateauth::get_name(), tester_account, fc::mutable_variant_object() ("account", tester_account) - ("permission", N(role)) + ("permission", "role"_n) ("parent", "active") ("auth", authority(node_a.get_public_key(tester_account, role), 5)), 1 ); aq_db.cache_transaction_trace(trace_ptr); const auto trace_ptr2 = node_a.push_action(config::system_account_name, updateauth::get_name(), tester_account2, fc::mutable_variant_object() ("account", tester_account2) - ("permission", N(role)) + ("permission", "role"_n) ("parent", "active") ("auth", authority(node_a.get_public_key(tester_account2, role), 5)), 2 ); @@ -141,7 +141,7 @@ BOOST_AUTO_TEST_CASE(fork_test) { try { pars.keys.emplace_back(node_a.get_public_key(tester_account, role)); const auto pre_results = aq_db.get_accounts_by_authorizers(pars); - BOOST_TEST_REQUIRE(find_account_auth(pre_results, tester_account, N(role)) == true); + BOOST_TEST_REQUIRE(find_account_auth(pre_results, tester_account, "role"_n) == true); // have node B take over from head-1 and also update permissions node_b.create_account(tester_account); @@ -149,14 +149,14 @@ BOOST_AUTO_TEST_CASE(fork_test) { try { const auto trace_ptr3 = node_b.push_action(config::system_account_name, updateauth::get_name(), tester_account, fc::mutable_variant_object() ("account", tester_account) - ("permission", N(role)) + ("permission", "role"_n) ("parent", "active") ("auth", authority(node_b.get_public_key(tester_account, role), 6)), 1 ); aq_db.cache_transaction_trace(trace_ptr3); const auto trace_ptr4 = node_b.push_action(config::system_account_name, updateauth::get_name(), tester_account2, fc::mutable_variant_object() ("account", tester_account2) - ("permission", N(role)) + ("permission", "role"_n) ("parent", "active") ("auth", authority(node_b.get_public_key(tester_account2, role), 6)), 2 ); @@ -167,14 +167,14 @@ BOOST_AUTO_TEST_CASE(fork_test) { try { const auto trace_ptr5 = node_b.push_action(config::system_account_name, updateauth::get_name(), tester_account, fc::mutable_variant_object() ("account", tester_account) - ("permission", N(role)) + ("permission", "role"_n) ("parent", "active") ("auth", authority(node_b.get_public_key(tester_account, role), 5)), 3 ); aq_db.cache_transaction_trace(trace_ptr5); const auto trace_ptr6 = node_b.push_action(config::system_account_name, updateauth::get_name(), tester_account2, fc::mutable_variant_object() ("account", tester_account2) - ("permission", N(role)) + ("permission", "role"_n) ("parent", "active") ("auth", authority(node_b.get_public_key(tester_account2, role), 5)), 4 );