[tarantool-patches] Re: [security 2/2] security: Refactor system space access checks
Konstantin Osipov
kostja at tarantool.org
Wed May 16 22:27:26 MSK 2018
* Ilya Markov <imarkov at tarantool.org> [18/05/16 15:39]:
> /*
> - * XXX: pre 1.7.7 there was no specific 'CREATE' or
> - * 'ALTER' ACL, instead, read and write access on universe
> - * was used to allow create/alter.
> - * For backward compatibility, if a user has read and write
> - * access on the universe, grant it CREATE access
> - * automatically.
> - * The legacy fix does not affect sequences since they
> - * were added in 1.7.7 only.
> + * If a user has write access on the universe,
> + * grant it CREATE, DROP, ALTER access automatically.
> */
> - if (is_17_compat_mode && has_access & PRIV_R && has_access & PRIV_W)
> - has_access |= PRIV_C | PRIV_A;
> + if (has_access & PRIV_W)
> + has_access |= PRIV_C | PRIV_A | PRIV_D;
>
I see no point in such automatic grant other than
1.7.7 compatibility.
> - user_access_t access = ((PRIV_U | (user_access_t) priv_type) &
> - ~has_access);
> + user_access_t access = ((user_access_t) priv_type & ~has_access);
> bool is_owner = owner_uid == cr->uid || cr->uid == ADMIN;
> /*
> * Only the owner of the object or someone who has
> - * specific DDL privilege on the object can execute
> - * DDL. If a user has no USAGE access and is owner,
> - * deny access as well.
> + * specific DDL privilege on the object can execute DDL.
> */
> - if (access == 0 || (is_owner && !(access & PRIV_U)))
> + if (access == 0 || is_owner)
I don't understand this change. Why did 'usage' disappear? Even
the owner of the object can do nothing if they have no 'usage'
access.
> access_check_ddl(old_space->def->name, old_space->def->uid, SC_SPACE,
> - priv_type, true);
> + priv_type, BOX_SPACE_ID);
Why did you need these changes? Could you describe in the
changeset comment the idea of this patch?
> /*
> - * Check if a write privilege was given, raise an error if not.
> + * Perform checks specific for space. Then perform usual check_ddl.
> */
> - access_check_space_xc(old_space, PRIV_W);
> + credentials *cr = effective_user();
> + uint32_t has_access = cr->universal_access |
> + old_space->access[cr->auth_token].effective;
> + if (!(has_access & (PRIV_W | PRIV_D))) {
> + access_check_ddl(old_space->def->name, old_space->def->uid,
> + SC_SPACE,
> + PRIV_D, BOX_SPACE_ID);
> + }
(stopped the review)
This is a huge patch. Any chance of splitting it?
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
More information about the Tarantool-patches
mailing list