Reject Action Data With Bytes Left After the Arguments

contracts/sysio.wrap/sysio.wrap.cpp

@@ -17,6 +17,10 @@ void wrap::exec( ignore<name>, ignore<transaction> ) {
    _ds >> context_free_actions;
    check( context_free_actions.empty(), "not allowed to `exec` a transaction with context-free actions" );
    _ds >> actions;
+   extensions_type transaction_extensions;
+   _ds >> transaction_extensions;
+   check( transaction_extensions.empty(), "not allowed to `exec` a transaction with transaction extensions" );
+   check_action_data_consumed( _ds );
 
    for (const auto& act : actions) {
       act.send();

contracts/tests/sysio.limitauth_tests.cpp

@@ -511,4 +511,39 @@ BOOST_FIXTURE_TEST_CASE(disallow_perms_tests, limitauth_tester) try {
 } // disallow_perms_tests
 FC_LOG_AND_RETHROW()
 
+// The permission actions accept authorized_by, and no byte after it
+BOOST_FIXTURE_TEST_CASE(bytes_after_authorized_by_rejected, limitauth_tester) try {
+   add_roa_policy(NODE_DADDY, alice, "32.0000 SYS", "32.0000 SYS", "32.0000 SYS", 0, 0);
+   const authority freebie_key{get_public_key(alice, "freebie")};
+   const uint8_t extra_byte = 0;
+   const auto left_over = [](std::string_view action_name) {
+      return "action data of sysio::" + std::string(action_name) + " has 1 bytes left after the arguments";
+   };
+
+   BOOST_REQUIRE_EQUAL(
+      left_over("updateauth"),
+      push_action_raw(config::system_account_name, "updateauth"_n, {alice, active}, alice, freebie, active, freebie_key,
+                      active, extra_byte));
+   BOOST_REQUIRE_EQUAL("", updateauth({alice, active}, alice, freebie, active, freebie_key, active));
+
+   BOOST_REQUIRE_EQUAL(
+      left_over("linkauth"),
+      push_action_raw(config::system_account_name, "linkauth"_n, {alice, active}, alice, "sysio.null"_n, "noop"_n,
+                      freebie, active, extra_byte));
+   BOOST_REQUIRE_EQUAL("", linkauth({alice, active}, alice, "sysio.null"_n, "noop"_n, freebie, active));
+
+   BOOST_REQUIRE_EQUAL(
+      left_over("unlinkauth"),
+      push_action_raw(config::system_account_name, "unlinkauth"_n, {alice, active}, alice, "sysio.null"_n, "noop"_n,
+                      active, extra_byte));
+   BOOST_REQUIRE_EQUAL("", unlinkauth({alice, active}, alice, "sysio.null"_n, "noop"_n, active));
+
+   BOOST_REQUIRE_EQUAL(
+      left_over("deleteauth"),
+      push_action_raw(config::system_account_name, "deleteauth"_n, {alice, active}, alice, freebie, active,
+                      extra_byte));
+   BOOST_REQUIRE_EQUAL("", deleteauth({alice, active}, alice, freebie, active));
+} // bytes_after_authorized_by_rejected
+FC_LOG_AND_RETHROW()
+
 BOOST_AUTO_TEST_SUITE_END()

contracts/tests/sysio.msig_tests.cpp

@@ -280,6 +280,36 @@ BOOST_FIXTURE_TEST_CASE( propose_billed_to_proposer, sysio_msig_tester ) try {
 } FC_LOG_AND_RETHROW()
 
 
+// propose's data must end with the proposed transaction
+BOOST_FIXTURE_TEST_CASE( propose_rejects_bytes_after_transaction, sysio_msig_tester ) try {
+   const auto trx = reqauth( "alice"_n, {permission_level{"alice"_n, config::active_name}} );
+   const auto data = abi_ser.variant_to_binary( "propose", mvo()
+      ("proposer",      "alice")
+      ("proposal_name", "first")
+      ("requested",     vector<permission_level>{{ "alice"_n, config::active_name }})
+      ("trx",           trx), fixture_abi_yield_function() );
+   const auto push_raw = [&]( bytes action_data ) {
+      signed_transaction propose_trx;
+      propose_trx.actions.emplace_back( payer_auths( "alice"_n ), "sysio.msig"_n, "propose"_n, std::move(action_data) );
+      set_transaction_headers( propose_trx );
+      propose_trx.sign( get_private_key( "alice"_n, "active" ), control->get_chain_id() );
+      return push_transaction( propose_trx );
+   };
+
+   auto with_tail = data;
+   with_tail.push_back( 0 );
+   BOOST_REQUIRE_EXCEPTION( push_raw( with_tail ), wasm_execution_error,
+                            fc_exception_message_contains( "check_transaction_authorization: 1 trailing bytes" ) );
+
+   push_raw( data );
+   produce_block();
+   push_action( "alice"_n, "approve"_n, mvo()
+                  ("proposer",      "alice")
+                  ("proposal_name", "first")
+                  ("level",         permission_level{ "alice"_n, config::active_name })
+   );
+} FC_LOG_AND_RETHROW()
+
 // A proposal carrying a non-zero `delay_sec` executes once its delay has elapsed, and not
 // before. Nothing else in this suite proposes with a delay, so without this case the whole
 // delayed-execution path is unexercised.

contracts/tests/sysio.system_tests.cpp

@@ -585,4 +585,69 @@ BOOST_FIXTURE_TEST_CASE( setabi, sysio_system_tester ) try {
 
 } FC_LOG_AND_RETHROW()
 
+// Action data with bytes after the arguments fails
+BOOST_FIXTURE_TEST_CASE( action_data_checked, sysio_system_tester ) try {
+   const auto push_raw = [&]( name action_name, bytes data ) {
+      action act;
+      act.account = config::system_account_name;
+      act.name    = action_name;
+      act.data    = std::move(data);
+      return base_tester::push_contract_paid_action( std::move(act), "alice1111111"_n.to_uint64_t() );
+   };
+   const auto with_tail = []( bytes data, size_t tail ) {
+      data.insert( data.end(), tail, 0 );
+      return data;
+   };
+   constexpr size_t large_tail_size = 100'000;
+
+   // limitauthchg reads exactly its arguments.
+   add_roa_policy( NODE_DADDY, "alice1111111"_n, "32.0000 SYS", "32.0000 SYS", "32.0000 SYS", 0, 0 );
+   const auto limitauthchg = abi_ser.variant_to_binary( "limitauthchg", mvo()
+      ("account", "alice1111111")
+      ("allow_perms", std::vector<name>{})
+      ("disallow_perms", std::vector<name>{}), fixture_abi_yield_function() );
+   const auto left_over = wasm_assert_msg( "action data has bytes left after the arguments" );
+   BOOST_REQUIRE_EQUAL( left_over, push_raw( "limitauthchg"_n, with_tail( limitauthchg, 1 ) ) );
+   BOOST_REQUIRE_EQUAL( left_over, push_raw( "limitauthchg"_n, with_tail( limitauthchg, large_tail_size ) ) );
+   BOOST_REQUIRE_EQUAL( success(), push_raw( "limitauthchg"_n, limitauthchg ) );
+} FC_LOG_AND_RETHROW()
+
+// setabi and setcode accept the memo the system contract declares, and no byte after it
+BOOST_FIXTURE_TEST_CASE( setabi_setcode_memo, sysio_system_tester ) try {
+   const name alice = "alice1111111"_n;
+   add_roa_policy( NODE_DADDY, alice, "32.0000 SYS", "32.0000 SYS", "320.0000 SYS", 0, 0 );
+   const auto memo = fc::raw::pack( std::string( "deploy" ) );
+   const auto push_native = [&]( auto native, const bytes& tail ) {
+      action act( vector<permission_level>{{alice, config::active_name}}, native );
+      act.data.insert( act.data.end(), tail.begin(), tail.end() );
+      signed_transaction trx;
+      trx.actions.push_back( std::move(act) );
+      set_transaction_headers( trx );
+      trx.sign( get_private_key( alice, "active" ), control->get_chain_id() );
+      push_transaction( trx );
+   };
+   const auto with_byte = []( bytes data ) {
+      data.push_back( 0 );
+      return data;
+   };
+
+   const auto abi =
+      fc::raw::pack( fc::json::from_string( (const char*)contracts::token_abi().data() ).template as<abi_def>() );
+   const sysio::chain::setabi set_abi_action{ .account = alice, .abi = abi };
+   BOOST_CHECK_EXCEPTION( push_native( set_abi_action, with_byte( memo ) ), action_data_and_struct_mismatch,
+                          fc_exception_message_contains( "bytes left after the arguments" ) );
+   push_native( set_abi_action, memo );
+   const auto* metadata = control->find_account_metadata( alice );
+   BOOST_REQUIRE( metadata != nullptr && metadata->abi.size() != 0 );
+
+   const auto wasm = contracts::token_wasm();
+   const sysio::chain::setcode set_code_action{ .account = alice, .vmtype = 0, .vmversion = 0,
+                                                .code = bytes( wasm.begin(), wasm.end() ) };
+   BOOST_CHECK_EXCEPTION( push_native( set_code_action, with_byte( memo ) ), action_data_and_struct_mismatch,
+                          fc_exception_message_contains( "bytes left after the arguments" ) );
+   push_native( set_code_action, memo );
+   metadata = control->find_account_metadata( alice );
+   BOOST_REQUIRE( metadata != nullptr && metadata->code_hash != digest_type() );
+} FC_LOG_AND_RETHROW()
+
 BOOST_AUTO_TEST_SUITE_END()

contracts/tests/sysio.token_tests.cpp

@@ -364,6 +364,41 @@ BOOST_FIXTURE_TEST_CASE( transfer_tests, sysio_token_tester ) try {
 
 } FC_LOG_AND_RETHROW()
 
+// transfer's data must hold exactly its arguments
+BOOST_FIXTURE_TEST_CASE( transfer_rejects_bytes_after_arguments, sysio_token_tester ) try {
+   create( "alice"_n, asset::from_string("1000 CERO") );
+   issue( "alice"_n, asset::from_string("1000 CERO"), "issue" );
+
+   const auto push_raw = [&]( bytes data ) {
+      action act;
+      act.account = "sysio.token"_n;
+      act.name    = "transfer"_n;
+      act.data    = std::move(data);
+      return base_tester::push_contract_paid_action( std::move(act), "alice"_n.to_uint64_t() );
+   };
+   const auto with_tail = []( bytes data, size_t tail ) {
+      data.insert( data.end(), tail, 0 );
+      return data;
+   };
+   constexpr size_t large_tail_size = 100'000;
+
+   const auto transfer_data = abi_ser.variant_to_binary( "transfer", mvo()
+      ("from", "alice")
+      ("to", "bob")
+      ("quantity", "300 CERO")
+      ("memo", "hola"), fixture_abi_yield_function() );
+
+   const auto left_over = wasm_assert_msg( "action data has bytes left after the arguments" );
+   BOOST_REQUIRE_EQUAL( left_over, push_raw( with_tail( transfer_data, 1 ) ) );
+   BOOST_REQUIRE_EQUAL( left_over, push_raw( with_tail( transfer_data, large_tail_size ) ) );
+   BOOST_REQUIRE_EQUAL( true, get_account( "bob"_n, "0,CERO" ).is_null() );
+
+   BOOST_REQUIRE_EQUAL( success(), push_raw( transfer_data ) );
+   REQUIRE_MATCHING_OBJECT( get_account( "bob"_n, "0,CERO" ), mvo()
+      ("balance", "300 CERO")
+   );
+} FC_LOG_AND_RETHROW()
+
 BOOST_FIXTURE_TEST_CASE( open_tests, sysio_token_tester ) try {
 
    auto token = create( "alice"_n, asset::from_string("1000 CERO"));

contracts/tests/sysio.wrap_tests.cpp

@@ -188,6 +188,38 @@ BOOST_FIXTURE_TEST_CASE( wrap_exec_direct, sysio_wrap_tester ) try {
                          } );
 } FC_LOG_AND_RETHROW()
 
+// exec reads the whole wrapped transaction: bytes after it, or transaction extensions, fail the action
+BOOST_FIXTURE_TEST_CASE( wrap_exec_reads_whole_transaction, sysio_wrap_tester ) try {
+   const auto sign = [&]( signed_transaction& trx ) {
+      trx.sign( get_private_key( "alice"_n, "active" ), control->get_chain_id() );
+      for( const auto& actor : {"prod1"_n, "prod2"_n, "prod3"_n, "prod4"_n} ) {
+         trx.sign( get_private_key( actor, "active" ), control->get_chain_id() );
+      }
+   };
+   auto trx = reqauth( "bob"_n, {permission_level{"bob"_n, config::active_name}} );
+
+   signed_transaction with_tail( wrap_exec( "alice"_n, trx ), {}, {} );
+   with_tail.actions[0].data.push_back( 0 );
+   sign( with_tail );
+   BOOST_REQUIRE_EXCEPTION( push_transaction( with_tail ), sysio_assert_message_exception,
+                            sysio_assert_message_is( "action data has bytes left after the arguments" ) );
+
+   auto extended = trx;
+   extended.transaction_extensions.emplace_back( uint16_t{0}, bytes{} );
+   signed_transaction with_extension( wrap_exec( "alice"_n, extended ), {}, {} );
+   sign( with_extension );
+   BOOST_REQUIRE_EXCEPTION(
+      push_transaction( with_extension ), sysio_assert_message_exception,
+      sysio_assert_message_is( "not allowed to `exec` a transaction with transaction extensions" ) );
+
+   signed_transaction exact( wrap_exec( "alice"_n, trx ), {}, {} );
+   sign( exact );
+   check_traces( push_transaction( exact ), {
+                           {{"receiver", "sysio.wrap"_n}, {"act_name", "exec"_n}},
+                           {{"receiver", config::system_account_name}, {"act_name", "reqauth"_n}}
+                         } );
+} FC_LOG_AND_RETHROW()
+
 BOOST_FIXTURE_TEST_CASE( wrap_with_msig, sysio_wrap_tester ) try {
    auto trx = reqauth( "bob"_n, {permission_level{"bob"_n, config::active_name}} );
    auto wrap_trx = wrap_exec( "alice"_n, trx );

libraries/chain/include/sysio/chain/action.hpp

@@ -91,11 +91,22 @@ namespace sysio { namespace chain {
             : action_base(account, name, std::move(auth)), data(data)  {
       }
 
+      /**
+       * Unpacks the action's data as `T`, the struct of a native action.
+       *
+       * The data must hold exactly a `T`. Bytes left after it fail with action_data_and_struct_mismatch, so a later
+       * version of `T` can give them a meaning.
+       */
       template<typename T>
       T data_as()const {
          SYS_ASSERT( account == T::get_account(), action_type_exception, "account is not consistent with action struct" );
          SYS_ASSERT( name == T::get_name(), action_type_exception, "action name is not consistent with action struct"  );
-         return fc::raw::unpack<T>(data);
+         fc::datastream<const char*> ds( data.data(), data.size() );
+         T value;
+         fc::raw::unpack( ds, value );
+         SYS_ASSERT( ds.remaining() == 0, action_data_and_struct_mismatch,
+                     "action data of {}::{} has {} bytes left after the arguments", account, name, ds.remaining() );
+         return value;
       }
 
       // sysio_payer_name actor or contract

libraries/chain/include/sysio/chain/binary_extension.hpp

@@ -0,0 +1,77 @@
+#pragma once
+
+#include <fc/io/datastream.hpp>
+#include <fc/io/raw.hpp>
+#include <fc/variant.hpp>
+
+#include <optional>
+#include <utility>
+
+namespace sysio::chain {
+
+/**
+ * An optional field at the end of an action's data, encoded as the CDT's `binary_extension<T>` is: the field is
+ * present when bytes remain where it starts, and it adds no bytes when absent.
+ *
+ * The host declares one wherever the system contract adds such a field to a native action, so that the host's
+ * struct describes all of the action's data and nothing past it.
+ *
+ * @tparam T - The field's type
+ */
+template <typename T>
+struct binary_extension {
+   std::optional<T> value;
+
+   /// True when the field is present.
+   bool has_value() const { return value.has_value(); }
+
+   friend bool operator==(const binary_extension&, const binary_extension&) = default;
+};
+
+} // namespace sysio::chain
+
+namespace fc {
+
+/// Packs the field when it is present, and nothing when it is absent.
+template <typename ST, typename T>
+datastream<ST>& operator<<(datastream<ST>& s, const sysio::chain::binary_extension<T>& v) {
+   if (v.value)
+      raw::pack(s, *v.value);
+   return s;
+}
+
+/// Reads the field when bytes remain, and leaves it absent otherwise.
+template <typename ST, typename T>
+datastream<ST>& operator>>(datastream<ST>& s, sysio::chain::binary_extension<T>& v) {
+   if (s.remaining()) {
+      T value;
+      raw::unpack(s, value);
+      v.value = std::move(value);
+   } else {
+      v.value.reset();
+   }
+   return s;
+}
+
+/// An absent field converts to null, a present one to its value.
+template <typename T>
+void to_variant(const sysio::chain::binary_extension<T>& e, fc::variant& v) {
+   if (e.value)
+      to_variant(*e.value, v);
+   else
+      v = fc::variant();
+}
+
+/// Null converts to an absent field, anything else to a present one.
+template <typename T>
+void from_variant(const fc::variant& v, sysio::chain::binary_extension<T>& e) {
+   if (v.is_null()) {
+      e.value.reset();
+   } else {
+      T value;
+      from_variant(v, value);
+      e.value = std::move(value);
+   }
+}
+
+} // namespace fc