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 0103622FA1 for ; Mon, 9 Jul 2018 11:04:39 -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 TrIMjNvXPV0U for ; Mon, 9 Jul 2018 11:04:38 -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 15C6722EEB for ; Mon, 9 Jul 2018 11:04:37 -0400 (EDT) From: Serge Petrenko Subject: [tarantool-patches] [PATCH v2] Fix sequence privilege checks Date: Mon, 9 Jul 2018 18:04:14 +0300 Message-Id: <20180709150414.68666-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 There was no check for create privilege when creating a sequence. Added one, and modified the tests accordingly. 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. Closes #3512 --- https://github.com/tarantool/tarantool/tree/sergepetrenko/access-checks https://github.com/tarantool/tarantool/issues/3512 Changes in v2: - fixed privilege checks on attaching a sequence to a space. src/box/alter.cc | 22 +++++++++++++++------ test/box/access.result | 18 +++++++++++++++-- test/box/access.test.lua | 9 +++++++-- test/box/sequence.result | 49 ++++++++++++++++++++++++++++++++++------------ test/box/sequence.test.lua | 29 +++++++++++++++++---------- 5 files changed, 95 insertions(+), 32 deletions(-) diff --git a/src/box/alter.cc b/src/box/alter.cc index 509e4b7e3..58eb3ab82 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -1716,9 +1716,7 @@ 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; + enum priv_type priv_type = new_tuple ? PRIV_A : PRIV_D; access_check_ddl(old_space->def->name, old_space->def->uid, SC_SPACE, priv_type, true); struct index *old_index = space_index(old_space, iid); @@ -2981,6 +2979,8 @@ on_replace_dd_sequence(struct trigger * /* trigger */, void *event) new_def = sequence_def_new_from_tuple(new_tuple, 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); sequence_cache_replace(new_def); alter->new_def = new_def; } else if (old_tuple != NULL && new_tuple == NULL) { /* DELETE */ @@ -3083,10 +3083,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 7e070e6d5..928c3fcd4 100644 --- a/test/box/access.result +++ b/test/box/access.result @@ -1740,8 +1740,9 @@ c:close() --- ... -- --- A user with read/write access to sequence was able --- to create a sequence +-- A user with read/write access to sequence shouldn't +-- be able to create a sequence. It also needs a create privilege +-- on universe. -- box.schema.user.create('tester') --- @@ -1754,6 +1755,19 @@ box.session.su('tester') ... _ = box.schema.sequence.create('test_sequence') --- +- error: Create access to sequence 'test_sequence' is denied for user 'tester' +... +box.session.su('admin') +--- +... +box.schema.user.grant('tester', 'create', 'universe') +--- +... +box.session.su('tester') +--- +... +_ = box.schema.sequence.create('test_sequence') +--- ... box.session.su('admin') --- diff --git a/test/box/access.test.lua b/test/box/access.test.lua index a2988c4c0..7dc92ba52 100644 --- a/test/box/access.test.lua +++ b/test/box/access.test.lua @@ -670,13 +670,18 @@ box.schema.func.drop("func") c:close() -- --- A user with read/write access to sequence was able --- to create a sequence +-- A user with read/write access to sequence shouldn't +-- be able to create a sequence. It also needs a create privilege +-- on universe. -- box.schema.user.create('tester') 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.session.su('tester') +_ = box.schema.sequence.create('test_sequence') +box.session.su('admin') box.schema.user.drop('tester') diff --git a/test/box/sequence.result b/test/box/sequence.result index 0c9951d8b..95c6d623d 100644 --- a/test/box/sequence.result +++ b/test/box/sequence.result @@ -1472,6 +1472,9 @@ sq:drop() --- ... -- A user can alter/use sequences that he owns. +box.schema.user.grant('user', 'create', 'universe') +--- +... box.session.su('user') --- ... @@ -1490,13 +1493,13 @@ sq = box.schema.sequence.create('seq') box.session.su('admin') --- ... -box.schema.user.revoke('user', 'read,write', 'universe') +box.schema.user.revoke('user', 'read,write,create', 'universe') --- ... box.session.su('user') --- ... -sq:set(100) -- ok +sq:set(100) -- ok - user owns the sequence --- ... sq:next() -- ok @@ -1512,7 +1515,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') --- ... @@ -1537,17 +1541,35 @@ 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', 'read,write,create', 'universe') +--- +... +box.schema.user.grant('user', 'write', 'space', '_index') +--- +... +box.schema.user.grant('user', 'write', 'space', '_space_sequence') +--- +... +box.schema.user.grant('user', 'read', 'universe') --- ... +box.session.su('user') +--- +... +_ = s2:create_index('pk', {sequence = 'seq1'}) -- error +--- +- error: Write 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: Write access to sequence 'seq1' is denied for user 'user' ... box.space._space_sequence:replace{s1.id, sq2.id, false} -- error --- @@ -1555,7 +1577,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: Write access to sequence 'seq1' is denied for user 'user' ... s2.index.pk:alter({sequence = 'seq2'}) -- ok --- @@ -1566,10 +1588,13 @@ 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', 'create', 'universe') +box.schema.user.revoke('user', 'write', 'space', '_space_sequence') +--- +... +box.schema.user.revoke('user', 'read', 'universe') --- ... box.session.su('user') @@ -1677,7 +1702,7 @@ s:drop() --- ... -- When a user is dropped, all his sequences are dropped as well. -box.schema.user.grant('user', 'read,write', 'universe') +box.schema.user.grant('user', 'read,write,create', 'universe') --- ... box.session.su('user') @@ -1707,10 +1732,10 @@ box.schema.user.create('user1') box.schema.user.create('user2') --- ... -box.schema.user.grant('user1', 'read,write', 'universe') +box.schema.user.grant('user1', 'read,write,create', 'universe') --- ... -box.schema.user.grant('user2', 'read,write', 'universe') +box.schema.user.grant('user2', 'read,write,create', 'universe') --- ... box.session.su('user1') diff --git a/test/box/sequence.test.lua b/test/box/sequence.test.lua index 1bcb91a9c..4d481c4f0 100644 --- a/test/box/sequence.test.lua +++ b/test/box/sequence.test.lua @@ -490,21 +490,23 @@ box.session.su('admin') sq:drop() -- A user can alter/use sequences that he owns. +box.schema.user.grant('user', 'create', 'universe') 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', 'universe') +box.schema.user.revoke('user', 'read,write,create', 'universe') box.session.su('user') -sq:set(100) -- ok +sq:set(100) -- ok - user owns the sequence sq:next() -- ok 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') @@ -513,8 +515,14 @@ box.schema.user.grant('user', 'create', 'universe') 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', 'read,write,create', 'universe') +box.schema.user.grant('user', 'write', 'space', '_index') +box.schema.user.grant('user', 'write', 'space', '_space_sequence') +box.schema.user.grant('user', 'read', 'universe') +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 @@ -525,8 +533,9 @@ 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', 'universe') box.session.su('user') s2:insert{nil, 1} -- ok: {1, 1} box.session.su('admin') @@ -562,7 +571,7 @@ 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', 'universe') +box.schema.user.grant('user', 'read,write,create', 'universe') box.session.su('user') _ = box.schema.sequence.create('test1') _ = box.schema.sequence.create('test2') @@ -574,8 +583,8 @@ box.sequence -- to a sequence. box.schema.user.create('user1') box.schema.user.create('user2') -box.schema.user.grant('user1', 'read,write', 'universe') -box.schema.user.grant('user2', 'read,write', 'universe') +box.schema.user.grant('user1', 'read,write,create', 'universe') +box.schema.user.grant('user2', 'read,write,create', 'universe') box.session.su('user1') sq = box.schema.sequence.create('test') box.session.su('user2') -- 2.15.2 (Apple Git-101.1)