bug fixes for permissions and reserved names

Permissions in updated authority should exist.

Non-privileged account creators should be restricted from using account
names that _start_ with "eosio.".

Reserve "eosio.any" as permission name.

Disallow empty parent for any permission other than owner.

Reserve id = 0 in permission_index so that a parent id of 0 always means
"no parent" rather than referring to a legitimate permission_object
(e.g. the owner permission of the system account).

libraries/chain/chain_controller.cpp

@@ -1066,7 +1066,7 @@ optional<permission_name> chain_controller::lookup_minimum_permission(account_na
       if (!linked_permission )
          return config::active_name;
 
-      if( *linked_permission == N(eosio.any) )
+      if( *linked_permission == config::eosio_any_name )
          return optional<permission_name>();
 
       return linked_permission;
@@ -1414,8 +1414,9 @@ const producer_object& chain_controller::get_producer(const account_name& owner_
 
 const permission_object&   chain_controller::get_permission( const permission_level& level )const
 { try {
+   FC_ASSERT( !level.actor.empty() && !level.permission.empty(), "Invalid permission" );
    return _db.get<permission_object, by_owner>( boost::make_tuple(level.actor,level.permission) );
-} EOS_RETHROW_EXCEPTIONS( chain::permission_query_exception, "Fail to retrieve permission: ${level}", ("level", level) ) }
+} EOS_RETHROW_EXCEPTIONS( chain::permission_query_exception, "Failed to retrieve permission: ${level}", ("level", level) ) }
 
 uint32_t chain_controller::last_irreversible_block_num() const {
    return get_dynamic_global_properties().last_irreversible_block_num;

libraries/chain/contracts/chain_initializer.cpp

@@ -316,7 +316,7 @@ abi_def chain_initializer::eos_contract_abi(const abi_def& eosio_system_abi)
          {"block_mroot", "checksum256"},
          {"producer", "account_name"},
          {"schedule_version", "uint32"},
-         {"new_producers", "producer_schedule?"}   
+         {"new_producers", "producer_schedule?"}
       }
    });
 
@@ -331,6 +331,10 @@ abi_def chain_initializer::eos_contract_abi(const abi_def& eosio_system_abi)
 
 void chain_initializer::prepare_database( chain_controller& chain,
                                                          chainbase::database& db) {
+   /// Reserve id of 0 in permission_index by creating dummy permission_object as the very first object in the index:
+   db.create<permission_object>([&](permission_object& p) {
+   });
+
    /// Create the native contract accounts manually; sadly, we can't run their contracts to make them create themselves
    auto create_native_account = [this, &chain, &db](account_name name) {
       db.create<account_object>([this, &name](account_object& a) {

libraries/chain/contracts/eosio_contract.cpp

@@ -29,14 +29,14 @@ namespace eosio { namespace chain { namespace contracts {
 void validate_authority_precondition( const apply_context& context, const authority& auth ) {
    for(const auto& a : auth.accounts) {
       context.db.get<account_object, by_name>(a.permission.actor);
-      context.db.find<permission_object, by_owner>(boost::make_tuple(a.permission.actor, a.permission.permission));
+      context.db.get<permission_object, by_owner>(boost::make_tuple(a.permission.actor, a.permission.permission));
    }
 }
 
 /**
  *  This method is called assuming precondition_system_newaccount succeeds a
  */
-void apply_eosio_newaccount(apply_context& context) { 
+void apply_eosio_newaccount(apply_context& context) {
    auto create = context.act.data_as<newaccount>();
    try {
    context.require_authorization(create.creator);
@@ -49,12 +49,16 @@ void apply_eosio_newaccount(apply_context& context) {
 
    auto& db = context.mutable_db;
 
-   EOS_ASSERT( create.name.to_string().size() <= 12, action_validate_exception, "account names can only be 12 chars long" );
+   auto name_str = name(create.name).to_string();
+
+   EOS_ASSERT( !create.name.empty(), action_validate_exception, "account name cannot be empty" );
+   EOS_ASSERT( name_str.size() <= 12, action_validate_exception, "account names can only be 12 chars long" );
 
    // Check if the creator is privileged
    const auto &creator = db.get<account_object, by_name>(create.creator);
    if( !creator.privileged ) {
-      EOS_ASSERT( name(create.name).to_string().find( "eosio." ) == std::string::npos, action_validate_exception, "only privileged accounts can have names that contain 'eosio.'" );
+      EOS_ASSERT( name_str.find( "eosio." ) != 0, action_validate_exception,
+                  "only privileged accounts can have names that start with 'eosio.'" );
    }
 
    auto existing_account = db.find<account_object, by_name>(create.name);
@@ -94,8 +98,10 @@ void apply_eosio_newaccount(apply_context& context) {
       return result;
    };
 
-   const auto& owner_permission = create_permission("owner", 0, std::move(create.owner));
-   create_permission("active", owner_permission.id, std::move(create.active));
+   // If a parent_id of 0 is going to be used to indicate the absence of a parent, then we need to make sure that the chain
+   // initializes permission_index with a dummy object that reserves the id of 0.
+   const auto& owner_permission = create_permission(config::owner_name, 0, std::move(create.owner));
+   create_permission(config::active_name, owner_permission.id, std::move(create.active));
 
 } FC_CAPTURE_AND_RETHROW( (create) ) }
 
@@ -126,7 +132,7 @@ void apply_eosio_setcode(apply_context& context) {
 //   wlog( "set code: ${size}", ("size",act.code.size()));
    db.modify( account, [&]( auto& a ) {
       /** TODO: consider whether a microsecond level local timestamp is sufficient to detect code version changes*/
-      #warning TODO: update setcode message to include the hash, then validate it in validate 
+      #warning TODO: update setcode message to include the hash, then validate it in validate
       a.code_version = code_id;
       // Added resize(0) here to avoid bug in boost vector container
       a.code.resize( 0 );
@@ -184,18 +190,22 @@ void apply_eosio_updateauth(apply_context& context) {
    auto& resources = context.mutable_controller.get_mutable_resource_limits_manager();
    context.require_write_lock( config::eosio_auth_scope );
 
+   auto& db = context.mutable_db;
+
    auto update = context.act.data_as<updateauth>();
    EOS_ASSERT(!update.permission.empty(), action_validate_exception, "Cannot create authority with empty name");
-   EOS_ASSERT(update.permission != update.parent, action_validate_exception,
-              "Cannot set an authority as its own parent");
+   EOS_ASSERT(update.permission != config::eosio_any_name, action_validate_exception,
+              "Cannot create authority for reserved 'eosio.any' permission name" );
+   EOS_ASSERT(update.permission != update.parent, action_validate_exception, "Cannot set an authority as its own parent");
+   db.get<account_object, by_name>(update.account);
    EOS_ASSERT(validate(update.data), action_validate_exception,
               "Invalid authority: ${auth}", ("auth", update.data));
-   if (update.permission == "active")
-      EOS_ASSERT(update.parent == "owner", action_validate_exception, "Cannot change active authority's parent from owner", ("update.parent", update.parent) );
-   if (update.permission == "owner")
+   if( update.permission == config::active_name )
+      EOS_ASSERT(update.parent == config::owner_name, action_validate_exception, "Cannot change active authority's parent from owner", ("update.parent", update.parent) );
+   if (update.permission == config::owner_name)
       EOS_ASSERT(update.parent.empty(), action_validate_exception, "Cannot change owner authority's parent");
-
-   auto& db = context.mutable_db;
+   else
+      EOS_ASSERT(!update.parent.empty(), action_validate_exception, "Only owner permission can have empty parent" );
 
    FC_ASSERT(context.act.authorization.size(), "updateauth can only have one action authorization");
    const auto& act_auth = context.act.authorization.front();
@@ -209,7 +219,7 @@ void apply_eosio_updateauth(apply_context& context) {
       if (current == nullptr) current = db.find<permission_object, by_owner>(boost::make_tuple(update.account, update.parent));
       // Ensure either the permission or parent's permission exists
       EOS_ASSERT(current != nullptr, permission_query_exception,
-                 "Fail to retrieve permission for: {\"actor\": \"${actor}\", \"permission\": \"${permission}\" }",
+                 "Failed to retrieve permission for: {\"actor\": \"${actor}\", \"permission\": \"${permission}\" }",
                  ("actor", update.account)("permission", update.parent));
 
       while(current->name != config::owner_name) {
@@ -224,13 +234,14 @@ void apply_eosio_updateauth(apply_context& context) {
 
    FC_ASSERT(act_auth.actor == update.account && permission_is_valid_for_update(), "updateauth must carry a permission equal to or in the ancestery of permission it updates");
 
-   db.get<account_object, by_name>(update.account);
    validate_authority_precondition(context, update.data);
 
    auto permission = db.find<permission_object, by_owner>(boost::make_tuple(update.account, update.permission));
-   
+
+   // If a parent_id of 0 is going to be used to indicate the absence of a parent, then we need to make sure that the chain
+   // initializes permission_index with a dummy object that reserves the id of 0.
    permission_object::id_type parent_id = 0;
-   if(update.permission != "owner") {
+   if(update.permission != config::owner_name) {
       auto& parent = db.get<permission_object, by_owner>(boost::make_tuple(update.account, update.parent));
       parent_id = parent.id;
    }
@@ -240,6 +251,9 @@ void apply_eosio_updateauth(apply_context& context) {
                  "Changing parent authority is not currently supported");
 
 
+      // TODO: Depending on an implementation detail like sizeof(permission_object) for consensus-affecting side effects like
+      //       RAM usage seems like a bad idea. For example, an upgrade of the implementation of boost::interprocess::vector
+      //       could cause a hardfork unless the old size calculation behavior was also carefully replicated in the same upgrade.
       int64_t old_size = (int64_t)(sizeof(permission_object) + permission->auth.get_billable_size());
 
       // TODO/QUESTION: If we are updating an existing permission, should we check if the message declared
@@ -282,11 +296,15 @@ void apply_eosio_updateauth(apply_context& context) {
 void apply_eosio_deleteauth(apply_context& context) {
    auto& resources = context.mutable_controller.get_mutable_resource_limits_manager();
    auto remove = context.act.data_as<deleteauth>();
-   EOS_ASSERT(remove.permission != "active", action_validate_exception, "Cannot delete active authority");
-   EOS_ASSERT(remove.permission != "owner", action_validate_exception, "Cannot delete owner authority");
+   EOS_ASSERT(remove.permission != config::active_name, action_validate_exception, "Cannot delete active authority");
+   EOS_ASSERT(remove.permission != config::owner_name, action_validate_exception, "Cannot delete owner authority");
 
    auto& db = context.mutable_db;
    context.require_authorization(remove.account);
+   // TODO/QUESTION:
+   //   Inconsistency between permissions that can be satisfied to create/modify (via updateauth) a permission and the
+   //   stricter requirements for deleting the permission using deleteauth.
+   //   If a permission can be updated, shouldn't it also be allowed to delete it without higher permissions required?
    const auto& permission = db.get<permission_object, by_owner>(boost::make_tuple(remove.account, remove.permission));
 
    { // Check for children
@@ -317,24 +335,24 @@ void apply_eosio_linkauth(apply_context& context) {
       EOS_ASSERT(!requirement.requirement.empty(), action_validate_exception, "Required permission cannot be empty");
 
       context.require_authorization(requirement.account);
-      
+
       auto& db = context.mutable_db;
       const auto *account = db.find<account_object, by_name>(requirement.account);
       EOS_ASSERT(account != nullptr, account_query_exception,
-                 "Fail to retrieve account: ${account}", ("account", requirement.account));
+                 "Failed to retrieve account: ${account}", ("account", requirement.account)); // Redundant?
       const auto *code = db.find<account_object, by_name>(requirement.code);
       EOS_ASSERT(code != nullptr, account_query_exception,
-                 "Fail to retrieve code for account: ${account}", ("account", requirement.code));
-      if( requirement.requirement != N(eosio.any) ) {
+                 "Failed to retrieve code for account: ${account}", ("account", requirement.code));
+      if( requirement.requirement != config::eosio_any_name ) {
          const auto *permission = db.find<permission_object, by_name>(requirement.requirement);
          EOS_ASSERT(permission != nullptr, permission_query_exception,
-                    "Fail to retrieve permission: ${permission}", ("permission", requirement.requirement));
+                    "Failed to retrieve permission: ${permission}", ("permission", requirement.requirement));
       }
-      
+
       auto link_key = boost::make_tuple(requirement.account, requirement.code, requirement.type);
       auto link = db.find<permission_link_object, by_action_name>(link_key);
-      
-      if (link) {
+
+      if( link ) {
          EOS_ASSERT(link->required_permission != requirement.requirement, action_validate_exception,
                     "Attempting to update required authority, but new requirement is same as old");
          db.modify(*link, [requirement = requirement.requirement](permission_link_object& link) {
@@ -354,7 +372,7 @@ void apply_eosio_linkauth(apply_context& context) {
             "New Permission Link ${code}::${act} -> ${a}@${p}", _V("code", l.code)("act",l.message_type)("a", l.account)("p",l.required_permission)
          );
       }
-  } FC_CAPTURE_AND_RETHROW((requirement)) 
+  } FC_CAPTURE_AND_RETHROW((requirement))
 }
 
 void apply_eosio_unlinkauth(apply_context& context) {

libraries/chain/include/eosio/chain/config.hpp

@@ -26,6 +26,7 @@ const static uint64_t eosio_all_scope        = N(eosio.all);
 
 const static uint64_t active_name = N(active);
 const static uint64_t owner_name  = N(owner);
+const static uint64_t eosio_any_name = N(eosio.any);
 
 const static int      block_interval_ms = 500;
 const static int      block_interval_us = block_interval_ms*1000;

tests/chain_tests/auth_tests.cpp

@@ -266,12 +266,12 @@ try {
    BOOST_CHECK_EXCEPTION(chain.create_account("aaaaaaaaaaaaa"), action_validate_exception,
                          assert_message_is("account names can only be 12 chars long"));
 
-   // Creating account with eosio. prefix with priveleged account
+   // Creating account with eosio. prefix with privileged account
    chain.create_account("eosio.test1");
 
    // Creating account with eosio. prefix with non-privileged account, should fail
    BOOST_CHECK_EXCEPTION(chain.create_account("eosio.test2", "joe"), action_validate_exception,
-                         assert_message_is("only privileged accounts can have names that contain 'eosio.'"));
+                         assert_message_is("only privileged accounts can have names that start with 'eosio.'"));
 
 
 } FC_LOG_AND_RETHROW() }
@@ -290,7 +290,7 @@ BOOST_AUTO_TEST_CASE( any_auth ) { try {
    chain.set_authority("bob", "spending", bob_spending_pub_key, "active");
 
    /// this should fail because spending is not active which is default for reqauth
-   BOOST_REQUIRE_THROW( chain.push_reqauth("alice", { permission_level{N(alice), "spending"} }, { spending_priv_key }), 
+   BOOST_REQUIRE_THROW( chain.push_reqauth("alice", { permission_level{N(alice), "spending"} }, { spending_priv_key }),
                         tx_irrelevant_auth );
 
    chain.produce_block();
@@ -302,7 +302,7 @@ BOOST_AUTO_TEST_CASE( any_auth ) { try {
 
    /// this should succeed because eosio::reqauth is linked to any permission
    chain.push_reqauth("alice", { permission_level{N(alice), "spending"} }, { spending_priv_key });
-   
+
    /// this should fail because bob cannot authorize for alice, the permission given must be one-of alices
    BOOST_REQUIRE_THROW( chain.push_reqauth("alice", { permission_level{N(bob), "spending"} }, { spending_priv_key }),
                         tx_missing_auth );