diff --git a/libraries/chain/apply_context.cpp b/libraries/chain/apply_context.cpp index 37c78ff7c7b..ad711c0be84 100644 --- a/libraries/chain/apply_context.cpp +++ b/libraries/chain/apply_context.cpp @@ -208,7 +208,7 @@ void apply_context::execute_inline( action&& a ) { bool enforce_actor_whitelist_blacklist = trx_context.enforce_whiteblacklist && control.is_producing_block(); flat_set actors; - bool disallow_send_to_self_bypass = false; // eventually set to whether the appropriate protocol feature has been activated + bool disallow_send_to_self_bypass = control.is_builtin_activated( builtin_protocol_feature_t::restrict_action_to_self ); bool send_to_self = (a.account == receiver); bool inherit_parent_authorizations = (!disallow_send_to_self_bypass && send_to_self && (receiver == act.account) && control.is_producing_block()); @@ -318,7 +318,7 @@ void apply_context::schedule_deferred_transaction( const uint128_t& sender_id, a // So, the deferred transaction must always go through the authorization checking if it is not sent by a privileged contract. // However, the old logic must still be considered because it cannot objectively change until a consensus protocol upgrade. - bool disallow_send_to_self_bypass = false; // eventually set to whether the appropriate protocol feature has been activated + bool disallow_send_to_self_bypass = control.is_builtin_activated( builtin_protocol_feature_t::restrict_action_to_self ); auto is_sending_only_to_self = [&trx]( const account_name& self ) { bool send_to_self = true; diff --git a/libraries/chain/include/eosio/chain/protocol_feature_manager.hpp b/libraries/chain/include/eosio/chain/protocol_feature_manager.hpp index f9f55dffb7a..773546eead0 100644 --- a/libraries/chain/include/eosio/chain/protocol_feature_manager.hpp +++ b/libraries/chain/include/eosio/chain/protocol_feature_manager.hpp @@ -18,7 +18,8 @@ enum class builtin_protocol_feature_t : uint32_t { only_link_to_existing_permission, replace_deferred, fix_linkauth_restriction, - disallow_empty_producer_schedule + disallow_empty_producer_schedule, + restrict_action_to_self }; struct protocol_feature_subjective_restrictions { diff --git a/libraries/chain/protocol_feature_manager.cpp b/libraries/chain/protocol_feature_manager.cpp index 78c84ff9410..67dc76e36ef 100644 --- a/libraries/chain/protocol_feature_manager.cpp +++ b/libraries/chain/protocol_feature_manager.cpp @@ -73,6 +73,19 @@ updateauth, deleteauth, linkauth, unlinkauth, or canceldelay. Builtin protocol feature: DISALLOW_EMPTY_PRODUCER_SCHEDULE Disallows proposing an empty producer schedule. +*/ + {} + } ) + ( builtin_protocol_feature_t::restrict_action_to_self, builtin_protocol_feature_spec{ + "RESTRICT_ACTION_TO_SELF", + fc::variant("e71b6712188391994c78d8c722c1d42c477cf091e5601b5cf1befd05721a57f3").as(), + // SHA256 hash of the raw message below within the comment delimiters (do not modify message below). +/* +Builtin protocol feature: RESTRICT_ACTION_TO_SELF + +Disallows bypass of authorization checks by unprivileged contracts when sending inline actions or deferred transactions. +The original protocol rules allow a bypass of authorization checks for actions sent by a contract to itself. +This protocol feature removes that bypass. */ {} } ) diff --git a/tests/get_table_tests.cpp b/tests/get_table_tests.cpp index 6abeb325913..91fca59ef3b 100644 --- a/tests/get_table_tests.cpp +++ b/tests/get_table_tests.cpp @@ -38,6 +38,34 @@ using namespace fc; BOOST_AUTO_TEST_SUITE(get_table_tests) +transaction_trace_ptr +issue_tokens( TESTER& t, account_name issuer, account_name to, const asset& amount, + std::string memo = "", account_name token_contract = N(eosio.token) ) +{ + signed_transaction trx; + + trx.actions.emplace_back( t.get_action( token_contract, N(issue), + vector{{issuer, config::active_name}}, + mutable_variant_object() + ("to", issuer.to_string()) + ("quantity", amount) + ("memo", memo) + ) ); + + trx.actions.emplace_back( t.get_action( token_contract, N(transfer), + vector{{issuer, config::active_name}}, + mutable_variant_object() + ("from", issuer.to_string()) + ("to", to.to_string()) + ("quantity", amount) + ("memo", memo) + ) ); + + t.set_transaction_headers(trx); + trx.sign( t.get_private_key( issuer, "active" ), t.control->get_chain_id() ); + return t.push_transaction( trx ); +} + BOOST_FIXTURE_TEST_CASE( get_scope_test, TESTER ) try { produce_blocks(2); @@ -60,11 +88,7 @@ BOOST_FIXTURE_TEST_CASE( get_scope_test, TESTER ) try { // issue for (account_name a: accs) { - push_action( N(eosio.token), N(issue), "eosio", mutable_variant_object() - ("to", name(a) ) - ("quantity", eosio::chain::asset::from_string("999.0000 SYS") ) - ("memo", "") - ); + issue_tokens( *this, config::system_account_name, a, eosio::chain::asset::from_string("999.0000 SYS") ); } produce_blocks(1); @@ -136,11 +160,7 @@ BOOST_FIXTURE_TEST_CASE( get_table_test, TESTER ) try { // issue for (account_name a: accs) { - push_action( N(eosio.token), N(issue), "eosio", mutable_variant_object() - ("to", name(a) ) - ("quantity", eosio::chain::asset::from_string("10000.0000 SYS") ) - ("memo", "") - ); + issue_tokens( *this, config::system_account_name, a, eosio::chain::asset::from_string("10000.0000 SYS") ); } produce_blocks(1); @@ -151,11 +171,7 @@ BOOST_FIXTURE_TEST_CASE( get_table_test, TESTER ) try { push_action(N(eosio.token), N(create), N(eosio.token), act ); // issue for (account_name a: accs) { - push_action( N(eosio.token), N(issue), "eosio", mutable_variant_object() - ("to", name(a) ) - ("quantity", eosio::chain::asset::from_string("9999.0000 AAA") ) - ("memo", "") - ); + issue_tokens( *this, config::system_account_name, a, eosio::chain::asset::from_string("9999.0000 AAA") ); } produce_blocks(1); @@ -166,11 +182,7 @@ BOOST_FIXTURE_TEST_CASE( get_table_test, TESTER ) try { push_action(N(eosio.token), N(create), N(eosio.token), act ); // issue for (account_name a: accs) { - push_action( N(eosio.token), N(issue), "eosio", mutable_variant_object() - ("to", name(a) ) - ("quantity", eosio::chain::asset::from_string("7777.0000 CCC") ) - ("memo", "") - ); + issue_tokens( *this, config::system_account_name, a, eosio::chain::asset::from_string("7777.0000 CCC") ); } produce_blocks(1); @@ -181,11 +193,7 @@ BOOST_FIXTURE_TEST_CASE( get_table_test, TESTER ) try { push_action(N(eosio.token), N(create), N(eosio.token), act ); // issue for (account_name a: accs) { - push_action( N(eosio.token), N(issue), "eosio", mutable_variant_object() - ("to", name(a) ) - ("quantity", eosio::chain::asset::from_string("8888.0000 BBB") ) - ("memo", "") - ); + issue_tokens( *this, config::system_account_name, a, eosio::chain::asset::from_string("8888.0000 BBB") ); } produce_blocks(1); @@ -331,17 +339,13 @@ BOOST_FIXTURE_TEST_CASE( get_table_by_seckey_test, TESTER ) try { // issue for (account_name a: accs) { - push_action( N(eosio.token), N(issue), "eosio", mutable_variant_object() - ("to", name(a) ) - ("quantity", eosio::chain::asset::from_string("10000.0000 SYS") ) - ("memo", "") - ); + issue_tokens( *this, config::system_account_name, a, eosio::chain::asset::from_string("10000.0000 SYS") ); } produce_blocks(1); - + set_code( config::system_account_name, contracts::eosio_system_wasm() ); set_abi( config::system_account_name, contracts::eosio_system_abi().data() ); - + base_tester::push_action(config::system_account_name, N(init), config::system_account_name, mutable_variant_object() ("version", 0) diff --git a/unittests/api_tests.cpp b/unittests/api_tests.cpp index 889effc9cb3..cc03ce721ea 100644 --- a/unittests/api_tests.cpp +++ b/unittests/api_tests.cpp @@ -1263,7 +1263,8 @@ BOOST_FIXTURE_TEST_CASE(deferred_transaction_tests, TESTER) { try { CALL_TEST_FUNCTION(*this, "test_transaction", "send_deferred_tx_with_dtt_action", fc::raw::pack(dtt_act2)); // If the deferred tx receiver == this tx receiver, the authorization checking would originally be bypassed. - // But not anymore. Now it should subjectively fail because testapi@additional permission is not unilaterally satisfied by testapi@eosio.code. + // But not anymore. With the RESTRICT_ACTION_TO_SELF protocol feature activated, it should now objectively + // fail because testapi@additional permission is not unilaterally satisfied by testapi@eosio.code. dtt_action dtt_act3; dtt_act3.deferred_account = N(testapi); dtt_act3.permission_name = N(additional); @@ -1272,7 +1273,7 @@ BOOST_FIXTURE_TEST_CASE(deferred_transaction_tests, TESTER) { try { ("code", name(dtt_act3.deferred_account)) ("type", name(dtt_act3.deferred_action)) ("requirement", name(dtt_act3.permission_name))); - BOOST_CHECK_THROW(CALL_TEST_FUNCTION(*this, "test_transaction", "send_deferred_tx_with_dtt_action", fc::raw::pack(dtt_act3)), subjective_block_production_exception); + BOOST_CHECK_THROW(CALL_TEST_FUNCTION(*this, "test_transaction", "send_deferred_tx_with_dtt_action", fc::raw::pack(dtt_act3)), unsatisfied_authorization); // But it should again work if the deferred transaction has a sufficient delay. dtt_act3.delay_sec = 10; diff --git a/unittests/contracts.hpp.in b/unittests/contracts.hpp.in index bc61854d403..35c01501436 100644 --- a/unittests/contracts.hpp.in +++ b/unittests/contracts.hpp.in @@ -44,6 +44,7 @@ namespace eosio { MAKE_READ_WASM_ABI(noop, noop, test-contracts) MAKE_READ_WASM_ABI(payloadless, payloadless, test-contracts) MAKE_READ_WASM_ABI(proxy, proxy, test-contracts) + MAKE_READ_WASM_ABI(restrict_action_test, restrict_action_test, test-contracts) MAKE_READ_WASM_ABI(snapshot_test, snapshot_test, test-contracts) MAKE_READ_WASM_ABI(test_api, test_api, test-contracts) MAKE_READ_WASM_ABI(test_api_db, test_api_db, test-contracts) diff --git a/unittests/forked_tests.cpp b/unittests/forked_tests.cpp index feeebf7664f..6f8f1f29c84 100644 --- a/unittests/forked_tests.cpp +++ b/unittests/forked_tests.cpp @@ -162,15 +162,18 @@ BOOST_AUTO_TEST_CASE( forking ) try { ("maximum_supply", core_from_string("10000000.0000")) ); - wdump((fc::json::to_pretty_string(cr))); - cr = c.push_action( N(eosio.token), N(issue), config::system_account_name, mutable_variant_object() - ("to", "dan" ) + ("to", "eosio" ) ("quantity", core_from_string("100.0000")) ("memo", "") ); - wdump((fc::json::to_pretty_string(cr))); + cr = c.push_action( N(eosio.token), N(transfer), config::system_account_name, mutable_variant_object() + ("from", "eosio") + ("to", "dan" ) + ("quantity", core_from_string("100.0000")) + ("memo", "") + ); tester c2; diff --git a/unittests/protocol_feature_tests.cpp b/unittests/protocol_feature_tests.cpp index c510904864a..121c6e17354 100644 --- a/unittests/protocol_feature_tests.cpp +++ b/unittests/protocol_feature_tests.cpp @@ -607,4 +607,64 @@ BOOST_AUTO_TEST_CASE( disallow_empty_producer_schedule_test ) { try { } FC_LOG_AND_RETHROW() } +BOOST_AUTO_TEST_CASE( restrict_action_to_self_test ) { try { + tester c( setup_policy::preactivate_feature_and_new_bios ); + + const auto& pfm = c.control->get_protocol_feature_manager(); + const auto& d = pfm.get_builtin_digest( builtin_protocol_feature_t::restrict_action_to_self ); + BOOST_REQUIRE( d ); + + c.create_accounts( {N(testacc), N(acctonotify), N(alice)} ); + c.set_code( N(testacc), contracts::restrict_action_test_wasm() ); + c.set_abi( N(testacc), contracts::restrict_action_test_abi().data() ); + + c.set_code( N(acctonotify), contracts::restrict_action_test_wasm() ); + c.set_abi( N(acctonotify), contracts::restrict_action_test_abi().data() ); + + // Before the protocol feature is preactivated + // - Sending inline action to self = no problem + // - Sending deferred trx to self = throw subjective exception + // - Sending inline action to self from notification = throw subjective exception + // - Sending deferred trx to self from notification = throw subjective exception + BOOST_CHECK_NO_THROW( c.push_action( N(testacc), N(sendinline), N(alice), mutable_variant_object()("authorizer", "alice")) ); + BOOST_REQUIRE_EXCEPTION( c.push_action( N(testacc), N(senddefer), N(alice), + mutable_variant_object()("authorizer", "alice")("senderid", 0)), + subjective_block_production_exception, + fc_exception_message_starts_with( "Authorization failure with sent deferred transaction" ) ); + + BOOST_REQUIRE_EXCEPTION( c.push_action( N(testacc), N(notifyinline), N(alice), + mutable_variant_object()("acctonotify", "acctonotify")("authorizer", "alice")), + subjective_block_production_exception, + fc_exception_message_starts_with( "Authorization failure with inline action sent to self" ) ); + + BOOST_REQUIRE_EXCEPTION( c.push_action( N(testacc), N(notifydefer), N(alice), + mutable_variant_object()("acctonotify", "acctonotify")("authorizer", "alice")("senderid", 1)), + subjective_block_production_exception, + fc_exception_message_starts_with( "Authorization failure with sent deferred transaction" ) ); + + c.preactivate_protocol_features( {*d} ); + c.produce_block(); + + // After the protocol feature is preactivated, all the 4 cases will throw an objective unsatisfied_authorization exception + BOOST_REQUIRE_EXCEPTION( c.push_action( N(testacc), N(sendinline), N(alice), mutable_variant_object()("authorizer", "alice") ), + unsatisfied_authorization, + fc_exception_message_starts_with( "transaction declares authority" ) ); + + BOOST_REQUIRE_EXCEPTION( c.push_action( N(testacc), N(senddefer), N(alice), + mutable_variant_object()("authorizer", "alice")("senderid", 3)), + unsatisfied_authorization, + fc_exception_message_starts_with( "transaction declares authority" ) ); + + BOOST_REQUIRE_EXCEPTION( c.push_action( N(testacc), N(notifyinline), N(alice), + mutable_variant_object()("acctonotify", "acctonotify")("authorizer", "alice") ), + unsatisfied_authorization, + fc_exception_message_starts_with( "transaction declares authority" ) ); + + BOOST_REQUIRE_EXCEPTION( c.push_action( N(testacc), N(notifydefer), N(alice), + mutable_variant_object()("acctonotify", "acctonotify")("authorizer", "alice")("senderid", 4)), + unsatisfied_authorization, + fc_exception_message_starts_with( "transaction declares authority" ) ); + +} FC_LOG_AND_RETHROW() } + BOOST_AUTO_TEST_SUITE_END() diff --git a/unittests/test-contracts/CMakeLists.txt b/unittests/test-contracts/CMakeLists.txt index 59f4ec0c28d..749b0471349 100644 --- a/unittests/test-contracts/CMakeLists.txt +++ b/unittests/test-contracts/CMakeLists.txt @@ -13,6 +13,7 @@ add_subdirectory( integration_test ) add_subdirectory( noop ) add_subdirectory( payloadless ) add_subdirectory( proxy ) +add_subdirectory( restrict_action_test ) add_subdirectory( snapshot_test ) add_subdirectory( test_api ) add_subdirectory( test_api_db ) diff --git a/unittests/test-contracts/restrict_action_test/CMakeLists.txt b/unittests/test-contracts/restrict_action_test/CMakeLists.txt new file mode 100644 index 00000000000..5ffe32ae7da --- /dev/null +++ b/unittests/test-contracts/restrict_action_test/CMakeLists.txt @@ -0,0 +1,6 @@ +if( EOSIO_COMPILE_TEST_CONTRACTS ) + add_contract( restrict_action_test restrict_action_test restrict_action_test.cpp ) +else() + configure_file( ${CMAKE_CURRENT_SOURCE_DIR}/restrict_action_test.wasm ${CMAKE_CURRENT_BINARY_DIR}/restrict_action_test.wasm COPYONLY ) + configure_file( ${CMAKE_CURRENT_SOURCE_DIR}/restrict_action_test.abi ${CMAKE_CURRENT_BINARY_DIR}/restrict_action_test.abi COPYONLY ) +endif() diff --git a/unittests/test-contracts/restrict_action_test/restrict_action_test.abi b/unittests/test-contracts/restrict_action_test/restrict_action_test.abi new file mode 100644 index 00000000000..37db4926071 --- /dev/null +++ b/unittests/test-contracts/restrict_action_test/restrict_action_test.abi @@ -0,0 +1,98 @@ +{ + "____comment": "This file was generated with eosio-abigen. DO NOT EDIT ", + "version": "eosio::abi/1.1", + "types": [], + "structs": [ + { + "name": "noop", + "base": "", + "fields": [] + }, + { + "name": "notifydefer", + "base": "", + "fields": [ + { + "name": "acctonotify", + "type": "name" + }, + { + "name": "authorizer", + "type": "name" + }, + { + "name": "senderid", + "type": "uint32" + } + ] + }, + { + "name": "notifyinline", + "base": "", + "fields": [ + { + "name": "acctonotify", + "type": "name" + }, + { + "name": "authorizer", + "type": "name" + } + ] + }, + { + "name": "senddefer", + "base": "", + "fields": [ + { + "name": "authorizer", + "type": "name" + }, + { + "name": "senderid", + "type": "uint32" + } + ] + }, + { + "name": "sendinline", + "base": "", + "fields": [ + { + "name": "authorizer", + "type": "name" + } + ] + } + ], + "actions": [ + { + "name": "noop", + "type": "noop", + "ricardian_contract": "" + }, + { + "name": "notifydefer", + "type": "notifydefer", + "ricardian_contract": "" + }, + { + "name": "notifyinline", + "type": "notifyinline", + "ricardian_contract": "" + }, + { + "name": "senddefer", + "type": "senddefer", + "ricardian_contract": "" + }, + { + "name": "sendinline", + "type": "sendinline", + "ricardian_contract": "" + } + ], + "tables": [], + "ricardian_clauses": [], + "variants": [] +} \ No newline at end of file diff --git a/unittests/test-contracts/restrict_action_test/restrict_action_test.cpp b/unittests/test-contracts/restrict_action_test/restrict_action_test.cpp new file mode 100644 index 00000000000..5c8f2b596b5 --- /dev/null +++ b/unittests/test-contracts/restrict_action_test/restrict_action_test.cpp @@ -0,0 +1,48 @@ +/** + * @file + * @copyright defined in eos/LICENSE + */ +#include "restrict_action_test.hpp" +#include + +using namespace eosio; + +void restrict_action_test::noop( ) { + +} + +void restrict_action_test::sendinline( name authorizer ) { + action( + permission_level{authorizer,"active"_n}, + get_self(), + "noop"_n, + std::make_tuple() + ).send(); +} + +void restrict_action_test::senddefer( name authorizer, uint32_t senderid ) { + transaction trx; + trx.actions.emplace_back( + permission_level{authorizer,"active"_n}, + get_self(), + "noop"_n, + std::make_tuple() + ); + trx.send(senderid, get_self()); +} + +void restrict_action_test::notifyinline( name acctonotify, name authorizer ) { + require_recipient(acctonotify); +} + +void restrict_action_test::notifydefer( name acctonotify, name authorizer, uint32_t senderid ) { + require_recipient(acctonotify); +} + +void restrict_action_test::on_notify_inline( name acctonotify, name authorizer ) { + sendinline(authorizer); +} + +void restrict_action_test::on_notify_defer( name acctonotify, name authorizer, uint32_t senderid ) { + senddefer(authorizer, senderid); +} diff --git a/unittests/test-contracts/restrict_action_test/restrict_action_test.hpp b/unittests/test-contracts/restrict_action_test/restrict_action_test.hpp new file mode 100644 index 00000000000..f5ab48e385b --- /dev/null +++ b/unittests/test-contracts/restrict_action_test/restrict_action_test.hpp @@ -0,0 +1,34 @@ +/** + * @file + * @copyright defined in eos/LICENSE + */ +#pragma once + +#include + +class [[eosio::contract]] restrict_action_test : public eosio::contract { +public: + using eosio::contract::contract; + + [[eosio::action]] + void noop( ); + + [[eosio::action]] + void sendinline( eosio::name authorizer ); + + [[eosio::action]] + void senddefer( eosio::name authorizer, uint32_t senderid ); + + + [[eosio::action]] + void notifyinline( eosio::name acctonotify, eosio::name authorizer ); + + [[eosio::action]] + void notifydefer( eosio::name acctonotify, eosio::name authorizer, uint32_t senderid ); + + [[eosio::on_notify("testacc::notifyinline")]] + void on_notify_inline( eosio::name acctonotify, eosio::name authorizer ); + + [[eosio::on_notify("testacc::notifydefer")]] + void on_notify_defer( eosio::name acctonotify, eosio::name authorizer, uint32_t senderid ); +}; diff --git a/unittests/test-contracts/restrict_action_test/restrict_action_test.wasm b/unittests/test-contracts/restrict_action_test/restrict_action_test.wasm new file mode 100755 index 00000000000..31acb952b19 Binary files /dev/null and b/unittests/test-contracts/restrict_action_test/restrict_action_test.wasm differ diff --git a/unittests/test-contracts/test_api/test_transaction.cpp b/unittests/test-contracts/test_api/test_transaction.cpp index 065828ea307..535f9896e9b 100644 --- a/unittests/test-contracts/test_api/test_transaction.cpp +++ b/unittests/test-contracts/test_api/test_transaction.cpp @@ -283,7 +283,6 @@ void test_transaction::send_deferred_tx_with_dtt_action() { auto trx = transaction(); trx.actions.emplace_back(deferred_act); trx.delay_sec = dtt_act.delay_sec; - cancel_deferred( 0xffffffffffffffff ); // TODO: Remove this line after fixing deferred trx replacement RAM bug trx.send( 0xffffffffffffffff, name{dtt_act.payer}, true ); }