[tarantool-patches] Re: [security 2/2] security: Refactor system space access checks

[Konstantin Osipov <kostja@tarantool.org>]

* Ilya Markov <imarkov@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