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 35DD72798E for ; Thu, 19 Jul 2018 15:35:50 -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 43ilMLxaIVGc for ; Thu, 19 Jul 2018 15:35:50 -0400 (EDT) Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 761932797F for ; Thu, 19 Jul 2018 15:35:49 -0400 (EDT) From: Serge Petrenko Subject: [tarantool-patches] [PATCH v3] Make access_check_ddl check for entity privileges. Date: Thu, 19 Jul 2018 22:35:33 +0300 Message-Id: <20180719193533.23249-1-sergepetrenko@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: tarantool-patches@freelists.org, kostja@tarantool.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 and sequence (instead of read + write on sequence). Fixed it and changed the tests accordingly. Closes #3516 --- https://github.com/tarantool/tarantool/issues/3516 https://github.com/tarantool/tarantool/tree/sergepetrenko/gh-3516-entity-access-checks Changes in v3 - creating / altering / dropping an index checks for create / alter / drop privilege on space respectively. Adding correct index privileges is moved to a separate patch. Changes in v2 - rebased to the latest 1.10 src/box/alter.cc | 26 ++++++++-- test/box/access.result | 29 +++++++++-- test/box/access.test.lua | 21 ++++++-- test/box/sequence.result | 125 ++++++++++++++++++++++++++++++++++++++++----- test/box/sequence.test.lua | 57 ++++++++++++++++----- 5 files changed, 220 insertions(+), 38 deletions(-) diff --git a/src/box/alter.cc b/src/box/alter.cc index b74369321..3007a131d 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. */ @@ -3032,7 +3039,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 */ @@ -3137,8 +3144,19 @@ on_replace_dd_space_sequence(struct trigger * /* trigger */, void *event) 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..a2a1a60ea 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 +-- create 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', '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', 'universe') +box.schema.user.grant('user', 'create', 'space') +--- +... +box.schema.user.grant('user', 'create', 'sequence') --- ... box.session.su('user') @@ -1540,17 +1568,50 @@ 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', '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 +1619,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 +1630,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 +1753,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 +1792,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..96297d6f2 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,33 @@ 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 +-- create 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', '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 +547,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 +588,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 +603,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)