[tarantool-patches] Re: [security 1/2] security: Refactor reads from systems spaces

[Konstantin Osipov]

* Ilya Markov <imarkov at tarantool.org> [18/05/16 15:39]:
> Replace reads from systems spaces with reads from corresponding
> system views.
> 
> After this patch some error messages are changed:
>    * Accessing to objects that are not accessible for current user
>    raises the error claiming these objects don't exists.
>    * Attempt to add in transaction such methods as object create, drop
>    raises an multi-engine transaction error instead of multi-statement
>    transaction error.
> 
> In scope of #3250
> diff --git a/src/box/sysview_index.c b/src/box/sysview_index.c
> index 8bfc39b..c8e9a1d 100644
> --- a/src/box/sysview_index.c
> +++ b/src/box/sysview_index.c
> @@ -197,23 +197,35 @@ static const struct index_vtab sysview_index_vtab = {
>  	/* .end_build = */ generic_index_end_build,
>  };
>  
> +const uint32_t PRIV_WRDA = PRIV_W | PRIV_D | PRIV_A | PRIV_R;

Please add a comment explaining why you added each of the 4 ACLs
to this bag of ACLs.

> +
> +
> +/*
> + * System view filters.
> + * Filter must give read access to object, if:
> + * 1. User has modification or read access to universe.
> + * 2. User has read access to according system space.
> + * 3. User has modification or read access to object.
> + * 4. User is an owner of the object.
> + * 5. Other specific conditions of the object of object type.

5. is misleading. Please either give an example or remove from the
   comment, and describe these specific conditions in place.

This comment is related to all filters, so it's better to place it
in module header or near PRIV_WRDA, in other words, not next to
vspace_filter.

> + */
>  static bool
>  vspace_filter(struct space *source, struct tuple *tuple)
>  {
>  	struct credentials *cr = effective_user();
> -	if (PRIV_R & cr->universal_access)
> -		return true; /* read access to universe */
> -	if (PRIV_R & source->access[cr->auth_token].effective)
> -		return true; /* read access to _space space */
> -
> +	/* If user has global alter, drop privilege she may access all spaces. */
> +	if (cr->universal_access & PRIV_WRDA)
> +		return true;
> +	if (source->access[cr->auth_token].effective & PRIV_R)
> +		return true;

Stray change. This patch is full of these order changes. If you
don't like the way the filter conditions were coded, feel free to
apply style formatting in a separate patch. I'm fine either way.

>  	uint32_t space_id;
>  	if (tuple_field_u32(tuple, BOX_SPACE_FIELD_ID, &space_id) != 0)
>  		return false;
>  	struct space *space = space_cache_find(space_id);
>  	if (space == NULL)
>  		return false;
> -	user_access_t effective = space->access[cr->auth_token].effective;
> -	return ((PRIV_R | PRIV_W) & (cr->universal_access | effective) ||
> +	uint32_t effective = space->access[cr->auth_token].effective;

This reverts my change for user_access_t data type.

> +	return (PRIV_WRDA & effective ||
>  		space->def->uid == cr->uid);
>  }
>  
> @@ -221,10 +233,11 @@ static bool
>  vuser_filter(struct space *source, struct tuple *tuple)
>  {
>  	struct credentials *cr = effective_user();
> -	if (PRIV_R & cr->universal_access)
> -		return true; /* read access to universe */
> -	if (PRIV_R & source->access[cr->auth_token].effective)
> -		return true; /* read access to _user space */
> +	/* If user has global alter, drop privilege she may access all users. */
> +	if (cr->universal_access & PRIV_WRDA)
> +		return true;
> +	if (source->access[cr->auth_token].effective & PRIV_R)
> +		return true; /* read access to _user space. */

Stray change.
>  
>  	uint32_t uid;
>  	if (tuple_field_u32(tuple, BOX_USER_FIELD_ID, &uid) != 0)
> @@ -232,16 +245,19 @@ vuser_filter(struct space *source, struct tuple *tuple)
>  	uint32_t owner_id;
>  	if (tuple_field_u32(tuple, BOX_USER_FIELD_UID, &owner_id) != 0)
>  		return false;
> -	return uid == cr->uid || owner_id == cr->uid;
> +	return uid == cr->uid || owner_id == cr->uid || uid == PUBLIC;

Requires a minimal comment: 'PUBLIC' is a system role which any
user can see.

>  }
>  
>  static bool
>  vpriv_filter(struct space *source, struct tuple *tuple)
>  {
>  	struct credentials *cr = effective_user();
> -	if (PRIV_R & cr->universal_access)
> -		return true; /* read access to universe */
> -	if (PRIV_R & source->access[cr->auth_token].effective)
> +	/* If user has global alter, drop privilege
> +	 * she may access all privileges

they is gender neutral. This comment is misleading, what about WR
in WRDA? Why do they provide access to vpriv? write privilege, in
particular? I would try to avoid giving owners of write privilege read
access to _v* spaces, unless it's really necessary, and each such
access requires a special explanation and a test case.

> +	 */
> +	if (cr->universal_access & PRIV_WRDA)
> +		return true;
> +	if (source->access[cr->auth_token].effective & PRIV_R)
>  		return true; /* read access to _priv space */
Stray change.

>  
>  	uint32_t grantor_id;
> @@ -250,37 +266,43 @@ vpriv_filter(struct space *source, struct tuple *tuple)
>  	uint32_t grantee_id;
>  	if (tuple_field_u32(tuple, BOX_PRIV_FIELD_UID, &grantee_id) != 0)
>  		return false;
> -	return grantor_id == cr->uid || grantee_id == cr->uid;
> +	const char *type;
> +	uint32_t obj_id;
> +	if ((type = tuple_field_cstr(tuple, BOX_PRIV_FIELD_OBJECT_TYPE)) == NULL ||
> +		tuple_field_u32(tuple, BOX_PRIV_FIELD_OBJECT_ID, &obj_id) != 0)
> +		return false;
> +	return grantor_id == cr->uid || grantee_id == cr->uid ||
> +		(strncmp(type, "role", 4) == 0 && obj_id == PUBLIC);

Ugh. What's going on here? Is there a comment? A test case?

> index b81279c..ce09299 100644
> --- a/test/box/access_bin.result
> +++ b/test/box/access_bin.result
> @@ -50,19 +50,22 @@ box.schema.func.create('setuid_func')
>  box.schema.user.grant('guest', 'execute', 'function', 'setuid_func')
>  ---
>  ...
> +box.schema.user.grant('guest', 'read', 'universe')

Why did you add this?

Please explain the changes you made with the tests in the changeset
comment.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov