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 020D32779C for ; Tue, 17 Jul 2018 12:08:52 -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 rZd93mzahsVq for ; Tue, 17 Jul 2018 12:08:51 -0400 (EDT) Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97]) (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 89F4627478 for ; Tue, 17 Jul 2018 12:08:51 -0400 (EDT) From: Serge Petrenko Subject: [tarantool-patches] [PATCH v2 1/4] Make access_check_ddl check for entity privileges. Date: Tue, 17 Jul 2018 19:08:38 +0300 Message-Id: In-Reply-To: References: In-Reply-To: References: 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: tarantool-patches@freelists.org Cc: Serge Petrenko Function access_check_ddl checked only for universal access, thus granting entity or singe object access to a user would have no effect in scope of this function. Fix this by adding entity access checks. Also attaching an existing sequence to a space checked for create privilege on both space (instead of alter) and sequence (instead of read + write). Fixed both and changed the tests accordingly. Now creating an index demands alter on a space, because checking for create would lead to anyone with create access to entity space be able to create indices in other users spaces. Also checking for alter or drop on dropping an index, since any operations on space indices are altering a space, and dropping a space requires dropping all indices, so have to check for alter (in case of just dropping an index) or drop (in case of dropping the space with all its indices, to not require additional alter privilege). Closes #3516 --- src/box/alter.cc | 53 +++++++++++++++---- test/box/access.result | 29 ++++++++-- test/box/access.test.lua | 21 ++++++-- test/box/sequence.result | 128 ++++++++++++++++++++++++++++++++++++++++----- test/box/sequence.test.lua | 58 +++++++++++++++----- 5 files changed, 245 insertions(+), 44 deletions(-) diff --git a/src/box/alter.cc b/src/box/alter.cc index b74369321..bd50e457a 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -85,6 +85,13 @@ access_check_ddl(const char *name, uint32_t owner_uid, user_access_t access = ((PRIV_U | (user_access_t) priv_type) & ~has_access); bool is_owner = owner_uid == cr->uid || cr->uid == ADMIN; + if (access == 0) + return; /* Access granted. */ + /* Check for specific entity access. */ + struct access *object = entity_access_get(type); + if (object) { + access &= ~object[cr->auth_token].effective; + } /* * Only the owner of the object or someone who has * specific DDL privilege on the object can execute @@ -94,7 +101,7 @@ access_check_ddl(const char *name, uint32_t owner_uid, * the owner of the object, but this should be ignored -- * CREATE privilege is required. */ - if (access == 0 || (is_owner && !(access & (PRIV_U|PRIV_C)))) + if (access == 0 || (is_owner && !(access & (PRIV_U | PRIV_C)))) return; /* Access granted. */ /* Create a meaningful error message. */ @@ -1764,11 +1771,20 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) uint32_t iid = tuple_field_u32_xc(old_tuple ? old_tuple : new_tuple, BOX_INDEX_FIELD_ID); struct space *old_space = space_cache_find_xc(id); - enum priv_type priv_type = new_tuple ? PRIV_C : PRIV_D; - if (old_tuple && new_tuple) - priv_type = PRIV_A; - access_check_ddl(old_space->def->name, old_space->def->uid, SC_SPACE, - priv_type, true); + bool have_alter = true; + try { + access_check_ddl(old_space->def->name, old_space->def->uid, SC_SPACE, + PRIV_A, true); + } catch(AccessDeniedError *e) { + /* + * We need alter access on space in case of creating or + * altering an index. But we can have alter OR drop on + * space in case of dropping an index. + */ + have_alter = false; + if(new_tuple) + throw; + } struct index *old_index = space_index(old_space, iid); /* @@ -1826,6 +1842,13 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) */ /* Case 1: drop the index, if it is dropped. */ if (old_index != NULL && new_tuple == NULL) { + /* + * In case we don't have alter on space when + * dropping an index, check for drop on space. + */ + if (!have_alter) + access_check_ddl(old_space->def->name, old_space->def->uid, + SC_SPACE, PRIV_D, true); alter_space_move_indexes(alter, 0, iid); (void) new DropIndex(alter, old_index->def); } @@ -3032,7 +3055,7 @@ on_replace_dd_sequence(struct trigger * /* trigger */, void *event) ER_CREATE_SEQUENCE); assert(sequence_by_id(new_def->id) == NULL); access_check_ddl(new_def->name, new_def->uid, SC_SEQUENCE, - PRIV_C, false); + PRIV_C, false); sequence_cache_replace(new_def); alter->new_def = new_def; } else if (old_tuple != NULL && new_tuple == NULL) { /* DELETE */ @@ -3135,10 +3158,20 @@ on_replace_dd_space_sequence(struct trigger * /* trigger */, void *event) enum priv_type priv_type = stmt->new_tuple ? PRIV_C : PRIV_D; if (stmt->new_tuple && stmt->old_tuple) priv_type = PRIV_A; - /* Check we have the correct access type on the sequence. * */ - access_check_ddl(seq->def->name, seq->def->uid, SC_SEQUENCE, priv_type, - false); + if (is_generated || !stmt->new_tuple) { + access_check_ddl(seq->def->name, seq->def->uid, SC_SEQUENCE, + priv_type, false); + } else { + /* + * In case user wants to attach an existing sequence, + * check that it has read and write access. + */ + access_check_ddl(seq->def->name, seq->def->uid, SC_SEQUENCE, + PRIV_R, false); + access_check_ddl(seq->def->name, seq->def->uid, SC_SEQUENCE, + PRIV_W, false); + } /** Check we have alter access on space. */ access_check_ddl(space->def->name, space->def->uid, SC_SPACE, PRIV_A, false); diff --git a/test/box/access.result b/test/box/access.result index a1f3e996a..f4669a4a3 100644 --- a/test/box/access.result +++ b/test/box/access.result @@ -875,7 +875,12 @@ session = box.session box.schema.user.create('test') --- ... -box.schema.user.grant('test', 'read,write,create', 'universe') +box.schema.user.grant('test', 'read', 'space', '_collation') +--- +... +--box.schema.user.grant('test', 'write', 'space', '_collation') +-- FIXME: granting create on 'collation' only doesn't work +box.schema.user.grant('test', 'create', 'universe') --- ... session.su('test') @@ -1389,7 +1394,10 @@ box.schema.func.create('test_func') box.session.su("admin") --- ... -box.schema.user.grant("tester", "read", "universe") +box.schema.user.grant("tester", "read", "space", "_user") +--- +... +box.schema.user.grant("tester", "read", "space", "_func") --- ... -- failed create @@ -1416,7 +1424,20 @@ box.session.su("admin") -- explicitly since we still use process_rw to write to system -- tables from ddl -- -box.schema.user.grant("tester", "create,write", "universe") +box.schema.user.grant('tester', 'write', 'universe') +--- +... +-- no entity user currently, so have to grant create +-- on universe in order to create a user. +box.schema.user.grant('tester', 'create', 'universe') +--- +... +-- this should work instead: +--box.schema.user.grant('tester', 'create', 'user') +--box.schema.user.grant('tester', 'create', 'space') +--box.schema.user.grant('tester', 'create', 'function') +--box.schema.user.grant('tester', 'create' , 'sequence') +box.schema.user.grant('tester', 'read', 'space', '_sequence') --- ... box.session.su("tester") @@ -1824,7 +1845,7 @@ _ = box.schema.sequence.create('test_sequence') box.session.su('admin') --- ... -box.schema.user.grant('tester', 'create', 'universe') +box.schema.user.grant('tester', 'create', 'sequence') --- ... box.session.su('tester') diff --git a/test/box/access.test.lua b/test/box/access.test.lua index fb8f744e8..9ae0e1114 100644 --- a/test/box/access.test.lua +++ b/test/box/access.test.lua @@ -340,7 +340,10 @@ c:close() session = box.session box.schema.user.create('test') -box.schema.user.grant('test', 'read,write,create', 'universe') +box.schema.user.grant('test', 'read', 'space', '_collation') +--box.schema.user.grant('test', 'write', 'space', '_collation') +-- FIXME: granting create on 'collation' only doesn't work +box.schema.user.grant('test', 'create', 'universe') session.su('test') box.internal.collation.create('test', 'ICU', 'ru_RU') session.su('admin') @@ -520,7 +523,8 @@ box.schema.space.create("test_space") box.schema.user.create('test_user') box.schema.func.create('test_func') box.session.su("admin") -box.schema.user.grant("tester", "read", "universe") +box.schema.user.grant("tester", "read", "space", "_user") +box.schema.user.grant("tester", "read", "space", "_func") -- failed create box.session.su("tester") box.schema.space.create("test_space") @@ -533,7 +537,16 @@ box.session.su("admin") -- explicitly since we still use process_rw to write to system -- tables from ddl -- -box.schema.user.grant("tester", "create,write", "universe") +box.schema.user.grant('tester', 'write', 'universe') +-- no entity user currently, so have to grant create +-- on universe in order to create a user. +box.schema.user.grant('tester', 'create', 'universe') +-- this should work instead: +--box.schema.user.grant('tester', 'create', 'user') +--box.schema.user.grant('tester', 'create', 'space') +--box.schema.user.grant('tester', 'create', 'function') +--box.schema.user.grant('tester', 'create' , 'sequence') +box.schema.user.grant('tester', 'read', 'space', '_sequence') box.session.su("tester") -- successful create s1 = box.schema.space.create("test_space") @@ -712,7 +725,7 @@ box.schema.user.grant('tester', 'read,write', 'space', '_sequence') box.session.su('tester') _ = box.schema.sequence.create('test_sequence') box.session.su('admin') -box.schema.user.grant('tester', 'create', 'universe') +box.schema.user.grant('tester', 'create', 'sequence') box.session.su('tester') _ = box.schema.sequence.create('test_sequence') box.session.su('admin') diff --git a/test/box/sequence.result b/test/box/sequence.result index cbbd45080..75d5ea1e6 100644 --- a/test/box/sequence.result +++ b/test/box/sequence.result @@ -1471,8 +1471,17 @@ box.session.su('admin') sq:drop() --- ... +box.schema.user.revoke('user', 'read,write', 'universe') +--- +... -- A user can alter/use sequences that he owns. -box.schema.user.grant('user', 'create', 'universe') +box.schema.user.grant('user', 'create', 'sequence') +--- +... +box.schema.user.grant('user', 'write', 'space', '_sequence') +--- +... +box.schema.user.grant('user', 'write', 'space', '_sequence_data') --- ... box.session.su('user') @@ -1493,7 +1502,13 @@ sq = box.schema.sequence.create('seq') box.session.su('admin') --- ... -box.schema.user.revoke('user', 'read,write,create', 'universe') +box.schema.user.revoke('user', 'create', 'sequence') +--- +... +box.schema.user.revoke('user', 'write', 'space', '_sequence') +--- +... +box.schema.user.revoke('user', 'write', 'space', '_sequence_data') --- ... box.session.su('user') @@ -1515,7 +1530,8 @@ box.session.su('admin') sq:drop() --- ... --- A sequence can be attached to a space only if the user owns both. +-- A sequence can be attached to a space only if the user has +-- alter privilege on space and read/write on sequence. sq1 = box.schema.sequence.create('seq1') --- ... @@ -1525,10 +1541,22 @@ s1 = box.schema.space.create('space1') _ = s1:create_index('pk') --- ... -box.schema.user.grant('user', 'read,write', 'universe') +box.schema.user.grant('user', 'write', 'space', '_sequence') --- ... -box.schema.user.grant('user', 'create', 'universe') +box.schema.user.grant('user', 'write', 'space', '_sequence_data') +--- +... +box.schema.user.grant('user', 'write', 'space', '_schema') +--- +... +box.schema.user.grant('user', 'write', 'space', '_space') +--- +... +box.schema.user.grant('user', 'create', 'space') +--- +... +box.schema.user.grant('user', 'create', 'sequence') --- ... box.session.su('user') @@ -1540,17 +1568,53 @@ sq2 = box.schema.sequence.create('seq2') s2 = box.schema.space.create('space2') --- ... --- fixme: no error on using another user's sequence -_ = s2:create_index('pk', {sequence = 'seq1'}) +box.session.su('admin') +--- +... +box.schema.user.revoke('user', 'create', 'space') +--- +... +box.schema.user.revoke('user', 'create', 'sequence') +--- +... +box.schema.user.revoke('user', 'write', 'space', '_sequence') +--- +... +box.schema.user.revoke('user', 'write', 'space', '_sequence_data') +--- +... +box.schema.user.revoke('user', 'write', 'space', '_schema') +--- +... +box.schema.user.revoke('user', 'write', 'space', '_space') +--- +... +box.schema.user.grant('user', 'write', 'space', '_index') +--- +... +box.schema.user.grant('user', 'write', 'space', '_space_sequence') --- ... +box.schema.user.grant('user', 'read', 'space', '_index') +--- +... +box.schema.user.grant('user', 'read', 'space', '_space_sequence') +--- +... +box.session.su('user') +--- +... +_ = s2:create_index('pk', {sequence = 'seq1'}) -- error +--- +- error: Read access to sequence 'seq1' is denied for user 'user' +... s1.index.pk:alter({sequence = 'seq1'}) -- error --- - error: Alter access to space 'space1' is denied for user 'user' ... box.space._space_sequence:replace{s1.id, sq1.id, false} -- error --- -- error: Alter access to space 'space1' is denied for user 'user' +- error: Read access to sequence 'seq1' is denied for user 'user' ... box.space._space_sequence:replace{s1.id, sq2.id, false} -- error --- @@ -1558,7 +1622,7 @@ box.space._space_sequence:replace{s1.id, sq2.id, false} -- error ... box.space._space_sequence:replace{s2.id, sq1.id, false} -- error --- -- error: Alter access to sequence 'seq1' is denied for user 'user' +- error: Read access to sequence 'seq1' is denied for user 'user' ... s2.index.pk:alter({sequence = 'seq2'}) -- ok --- @@ -1569,10 +1633,22 @@ box.session.su('admin') -- If the user owns a sequence attached to a space, -- it can use it for auto increment, otherwise it -- needs privileges. -box.schema.user.revoke('user', 'read,write', 'universe') +box.schema.user.revoke('user', 'write', 'space', '_index') +--- +... +box.schema.user.revoke('user', 'write', 'space', '_space_sequence') +--- +... +box.schema.user.revoke('user', 'read', 'space', '_space') +--- +... +box.schema.user.revoke('user', 'read', 'space', '_sequence') +--- +... +box.schema.user.revoke('user', 'read', 'space', '_index') --- ... -box.schema.user.revoke('user', 'create', 'universe') +box.schema.user.revoke('user', 'read', 'space', '_space_sequence') --- ... box.session.su('user') @@ -1680,7 +1756,16 @@ s:drop() --- ... -- When a user is dropped, all his sequences are dropped as well. -box.schema.user.grant('user', 'read,write,create', 'universe') +box.schema.user.grant('user', 'write', 'space', '_sequence') +--- +... +box.schema.user.grant('user', 'read', 'space', '_sequence') +--- +... +box.schema.user.grant('user', 'write', 'space', '_space_sequence') +--- +... +box.schema.user.grant('user', 'create', 'sequence') --- ... box.session.su('user') @@ -1710,10 +1795,25 @@ box.schema.user.create('user1') box.schema.user.create('user2') --- ... -box.schema.user.grant('user1', 'read,write,create', 'universe') +box.schema.user.grant('user1', 'create', 'sequence') +--- +... +box.schema.user.grant('user1', 'write', 'space', '_sequence') +--- +... +box.schema.user.grant('user1', 'read', 'space', '_sequence') +--- +... +box.schema.user.grant('user1', 'read', 'space', '_user') +--- +... +box.schema.user.grant('user1', 'write', 'space', '_sequence_data') +--- +... +box.schema.user.grant('user1', 'write', 'space', '_priv') --- ... -box.schema.user.grant('user2', 'read,write,create', 'universe') +box.schema.user.grant('user2', 'read,write', 'universe') --- ... box.session.su('user1') diff --git a/test/box/sequence.test.lua b/test/box/sequence.test.lua index c119459b3..00261e397 100644 --- a/test/box/sequence.test.lua +++ b/test/box/sequence.test.lua @@ -488,16 +488,21 @@ sq:alter{step = 2} -- error sq:drop() -- error box.session.su('admin') sq:drop() +box.schema.user.revoke('user', 'read,write', 'universe') -- A user can alter/use sequences that he owns. -box.schema.user.grant('user', 'create', 'universe') +box.schema.user.grant('user', 'create', 'sequence') +box.schema.user.grant('user', 'write', 'space', '_sequence') +box.schema.user.grant('user', 'write', 'space', '_sequence_data') box.session.su('user') sq = box.schema.sequence.create('seq') sq:alter{step = 2} -- ok sq:drop() -- ok sq = box.schema.sequence.create('seq') box.session.su('admin') -box.schema.user.revoke('user', 'read,write,create', 'universe') +box.schema.user.revoke('user', 'create', 'sequence') +box.schema.user.revoke('user', 'write', 'space', '_sequence') +box.schema.user.revoke('user', 'write', 'space', '_sequence_data') box.session.su('user') sq:set(100) -- ok - user owns the sequence sq:next() -- ok @@ -505,17 +510,34 @@ sq:reset() -- ok box.session.su('admin') sq:drop() --- A sequence can be attached to a space only if the user owns both. +-- A sequence can be attached to a space only if the user has +-- alter privilege on space and read/write on sequence. sq1 = box.schema.sequence.create('seq1') s1 = box.schema.space.create('space1') _ = s1:create_index('pk') -box.schema.user.grant('user', 'read,write', 'universe') -box.schema.user.grant('user', 'create', 'universe') +box.schema.user.grant('user', 'write', 'space', '_sequence') +box.schema.user.grant('user', 'write', 'space', '_sequence_data') +box.schema.user.grant('user', 'write', 'space', '_schema') +box.schema.user.grant('user', 'write', 'space', '_space') +box.schema.user.grant('user', 'create', 'space') +box.schema.user.grant('user', 'create', 'sequence') box.session.su('user') sq2 = box.schema.sequence.create('seq2') s2 = box.schema.space.create('space2') --- fixme: no error on using another user's sequence -_ = s2:create_index('pk', {sequence = 'seq1'}) + +box.session.su('admin') +box.schema.user.revoke('user', 'create', 'space') +box.schema.user.revoke('user', 'create', 'sequence') +box.schema.user.revoke('user', 'write', 'space', '_sequence') +box.schema.user.revoke('user', 'write', 'space', '_sequence_data') +box.schema.user.revoke('user', 'write', 'space', '_schema') +box.schema.user.revoke('user', 'write', 'space', '_space') +box.schema.user.grant('user', 'write', 'space', '_index') +box.schema.user.grant('user', 'write', 'space', '_space_sequence') +box.schema.user.grant('user', 'read', 'space', '_index') +box.schema.user.grant('user', 'read', 'space', '_space_sequence') +box.session.su('user') +_ = s2:create_index('pk', {sequence = 'seq1'}) -- error s1.index.pk:alter({sequence = 'seq1'}) -- error box.space._space_sequence:replace{s1.id, sq1.id, false} -- error box.space._space_sequence:replace{s1.id, sq2.id, false} -- error @@ -526,8 +548,12 @@ box.session.su('admin') -- If the user owns a sequence attached to a space, -- it can use it for auto increment, otherwise it -- needs privileges. -box.schema.user.revoke('user', 'read,write', 'universe') -box.schema.user.revoke('user', 'create', 'universe') +box.schema.user.revoke('user', 'write', 'space', '_index') +box.schema.user.revoke('user', 'write', 'space', '_space_sequence') +box.schema.user.revoke('user', 'read', 'space', '_space') +box.schema.user.revoke('user', 'read', 'space', '_sequence') +box.schema.user.revoke('user', 'read', 'space', '_index') +box.schema.user.revoke('user', 'read', 'space', '_space_sequence') box.session.su('user') s2:insert{nil, 1} -- ok: {1, 1} box.session.su('admin') @@ -563,7 +589,10 @@ box.session.su('admin') s:drop() -- When a user is dropped, all his sequences are dropped as well. -box.schema.user.grant('user', 'read,write,create', 'universe') +box.schema.user.grant('user', 'write', 'space', '_sequence') +box.schema.user.grant('user', 'read', 'space', '_sequence') +box.schema.user.grant('user', 'write', 'space', '_space_sequence') +box.schema.user.grant('user', 'create', 'sequence') box.session.su('user') _ = box.schema.sequence.create('test1') _ = box.schema.sequence.create('test2') @@ -575,8 +604,13 @@ box.sequence -- to a sequence. box.schema.user.create('user1') box.schema.user.create('user2') -box.schema.user.grant('user1', 'read,write,create', 'universe') -box.schema.user.grant('user2', 'read,write,create', 'universe') +box.schema.user.grant('user1', 'create', 'sequence') +box.schema.user.grant('user1', 'write', 'space', '_sequence') +box.schema.user.grant('user1', 'read', 'space', '_sequence') +box.schema.user.grant('user1', 'read', 'space', '_user') +box.schema.user.grant('user1', 'write', 'space', '_sequence_data') +box.schema.user.grant('user1', 'write', 'space', '_priv') +box.schema.user.grant('user2', 'read,write', 'universe') box.session.su('user1') sq = box.schema.sequence.create('test') box.session.su('user2') -- 2.15.2 (Apple Git-101.1)