Merge pull request #7088 from EOSIO/6705-restrict-action-to-self

Implement RESTRICT_ACTION_TO_SELF protocol feature

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<account_name> 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;

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 {

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<digest_type>(),
+            // 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.
 */
             {}
          } )

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<permission_level>{{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<permission_level>{{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)

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;

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)

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;

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()

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 )

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()

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": []
+}