From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 5A4BE244B7 for ; Wed, 16 May 2018 15:22:09 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 5Q15OoQHUcoq for ; Wed, 16 May 2018 15:22:09 -0400 (EDT) Received: from smtp1.mail.ru (smtp1.mail.ru [94.100.179.111]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id BC22D2443E for ; Wed, 16 May 2018 15:22:08 -0400 (EDT) Date: Wed, 16 May 2018 22:22:05 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [security 1/2] security: Refactor reads from systems spaces Message-ID: <20180516192205.GA9640@atlas> References: <391cb156f0f321ea7c2b987669798c3e6846c3f5.1526474053.git.imarkov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <391cb156f0f321ea7c2b987669798c3e6846c3f5.1526474053.git.imarkov@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Ilya Markov Cc: georgy@tarantool.org, tarantool-patches@freelists.org * Ilya Markov [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