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 BF7BE2D02B for ; Thu, 11 Oct 2018 07:33:02 -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 HGqUgrpZvq_O for ; Thu, 11 Oct 2018 07:33:02 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 20D962D02A for ; Thu, 11 Oct 2018 07:33:02 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH] sql: check read access while executing SQL query From: "n.pettik" In-Reply-To: <6dc6ee8b0356676cf6176dd6310109a09ce7b23d.1539247804.git.kyukhin@tarantool.org> Date: Thu, 11 Oct 2018 14:32:55 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <4D536713-510B-4F25-9455-F4090A29AF67@tarantool.org> References: <6dc6ee8b0356676cf6176dd6310109a09ce7b23d.1539247804.git.kyukhin@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 Cc: Kirill Yukhin > Since SQL front-end is not using box API, > no checkes for read access are performed by VDBE engine. > Fix this by introducing dedicated opcode which is executed But in your patch I don=E2=80=99t see any new opcodes. > to perform read access check. > Note, that there's is no need to perform DML/DDL checkes as > they're performed by Tarantool's core. >=20 > Closes #2362 Please, provide doc-bot request (especially taking into consideration the fact that =E2=80=98read=E2=80=99 privilege turns out to be not real = =E2=80=99select=E2=80=99 privilege). > --- > = https://github.com/tarantool/tarantool/commits/kyukhin/gh-2362-sql-read-ac= cess-check > https://github.com/tarantool/tarantool/issues/2362 >=20 > src/box/sql/vdbe.c | 6 ++ > test/sql/gh-2362-select-access-rights.result | 113 = +++++++++++++++++++++++++ > test/sql/gh-2362-select-access-rights.test.lua | 44 ++++++++++ > 3 files changed, 163 insertions(+) > create mode 100644 test/sql/gh-2362-select-access-rights.result > create mode 100644 test/sql/gh-2362-select-access-rights.test.lua >=20 > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 7c1015c..b4e2269 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -3099,8 +3099,14 @@ case OP_IteratorOpen: > else > space =3D aMem[pOp->p3].u.p; > assert(space !=3D NULL); > + if (access_check_space(space, PRIV_R) !=3D 0) { > + rc =3D SQL_TARANTOOL_ERROR; > + goto abort_due_to_error; > + } > + > struct index *index =3D space_index(space, pOp->p2); > assert(index !=3D NULL); > + Noise empty line. > assert(pOp->p1 >=3D 0); > cur =3D allocateCursor(p, pOp->p1, > space->def->exact_field_count =3D=3D 0 ? > diff --git a/test/sql/gh-2362-select-access-rights.test.lua = b/test/sql/gh-2362-select-access-rights.test.lua > new file mode 100644 > index 0000000..159373c > --- /dev/null > +++ b/test/sql/gh-2362-select-access-rights.test.lua I vote for moving these test cases to sql/iproto.test.lua > @@ -0,0 +1,44 @@ > +test_run =3D require('test_run').new() > +engine =3D test_run:get_cfg('engine') > +nb =3D require('net.box') > + > +box.sql.execute("PRAGMA sql_default_engine=3D'"..engine.."'") > +box.sql.execute("CREATE TABLE t1 (s1 INT PRIMARY KEY, s2 INT = UNIQUE);") > +box.sql.execute("CREATE TABLE t2 (s1 INT PRIMARY KEY);") > +box.sql.execute("INSERT INTO t1 VALUES (1, 1);") > +box.sql.execute("INSERT INTO t2 VALUES (1);") > + > +box.schema.user.grant('guest','read', 'space', '_space=E2=80=99) Hmm, why do you need to grand read privilege to _space? Without it test fails: error: 'Failed to execute SQL statement: no such table: T1=E2=80=99 I guess somewhere in SQL internals invocation to _space appears to fetch = space, but , it is not obvious. Can you come up with any workaround to avoid = it? Another problem is: box.schema.user.revoke('guest=E2=80=99,'read', 'space', 'T1') box.schema.user.grant('guest=E2=80=99,=E2=80=99write', 'space', 'T1') c:execute("delete from t1 where s1 =3D 1;=E2=80=9D)=20 --- - error: 'Failed to execute SQL statement: Read access to space ''T1'' = is denied for user ''guest''' ... In terms of Tarantool, it is OK (since we need to iterate through the = space to find tuple to be deleted). But for SQL I guess it is not acceptable: this = query doesn=E2=80=99t give user any data, so read (i.e. SELECT) privilege is not needed. The same for example involving FK constraints which you provided below. > + > +box.schema.user.grant('guest','read', 'space', 'T1') > +c =3D nb.connect(box.cfg.listen) > +c:execute("SELECT * FROM t1;") > + > +box.schema.user.revoke('guest','read', 'space', 'T1') > +c =3D nb.connect(box.cfg.listen) > +c:execute("SELECT * FROM t1;") > + > +box.schema.user.grant('guest','read', 'space', 'T2') > +c =3D nb.connect(box.cfg.listen) > +c:execute('SELECT * FROM t1, t2 WHERE t1.s1 =3D t2.s1') > + > +box.sql.execute("CREATE VIEW v AS SELECT * FROM t1") > + > +box.schema.user.grant('guest','read', 'space', 'V') > +v =3D nb.connect(box.cfg.listen) > +c:execute('SELECT * FROM v') > + > +box.sql.execute('CREATE TABLE t3 (s1 INT PRIMARY KEY, fk INT, FOREIGN = KEY (fk) REFERENCES t1(s2))') > +box.schema.user.grant('guest','read','space', 'T3') > +v =3D nb.connect(box.cfg.listen) > +c:execute('INSERT INTO t3 VALUES (1, 1)') > + > +-- Cleanup > +box.schema.user.revoke('guest','read','space', 'V') > +box.schema.user.revoke('guest','read','space', 'T2') > +box.schema.user.revoke('guest','read','space', 'T3') > + > +box.sql.execute('DROP VIEW v') > +box.sql.execute('DROP TABLE t3') > +box.sql.execute('DROP TABLE t2') > +box.sql.execute("DROP TABLE t1") > --=20 > 2.11.0