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 6696029C78 for ; Mon, 1 Apr 2019 10:06:22 -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 TAB4EtuoN-cw for ; Mon, 1 Apr 2019 10:06:22 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 AD5C429C18 for ; Mon, 1 Apr 2019 10:06:21 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v2 2/9] box: fix _trigger and _ck_constraint access check From: "n.pettik" In-Reply-To: <21a029d4-90de-48a8-8999-8baed0a7a76d@tarantool.org> Date: Mon, 1 Apr 2019 17:06:19 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <3226bb82065bbaea578a909f5382571bbe01fa3b.1548838034.git.kshcherbatov@tarantool.org> <29CE984E-A6A6-4E1D-AFFD-721477BD73B1@tarantool.org> <21a029d4-90de-48a8-8999-8baed0a7a76d@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 Shcherbatov > On 26 Mar 2019, at 13:59, Kirill Shcherbatov = wrote: >=20 >> Why did you check only alter privilege? On drop we should check >> drop privilege, on alter - alter privilege and on creation - creation >> privilege. > Hi! Here I check that we have alter privilege on corresponding "space" = because replace > in _trigger, _fk_constraint (and in further patches _ck_constaint) = cause space * > object change.=20 > This is very similar with access checks for _index space. 1913 static void 1914 on_replace_dd_index(struct trigger * /* trigger */, void *event) 1915 { ... 1930 enum priv_type priv_type =3D new_tuple ? PRIV_C : = PRIV_D; 1931 if (old_tuple && new_tuple) 1932 priv_type =3D PRIV_A; 1933 access_check_ddl(old_space->def->name, = old_space->def->id, 1934 old_space->def->uid, SC_SPACE, = priv_type); For indexes we check different privileges for drop, create and alter = cases. > As for _trigger, _fk_constraint regular access check everything is ok = already. I look at our docs: = https://tarantool.io/en/doc/2.1/book/box/authentication/ It says: =E2=80=98=E2=80=99=E2=80=99 To create objects, users need the =E2=80=98create=E2=80=99 privilege and = at least =E2=80=98read=E2=80=99 and =E2=80=98write=E2=80=99 privileges on the system space with a similar = name =E2=80=99'' Moreover, I see that we already have PRIV_TRIGGER in the list of object = types. AFAIK, ANSI states that user must have that privilege to create or drop = triggers. Please, consult other members of core team what check is expected here. Personally I intuitively think that we need PRIV_C/PRIV_D/PRIV_A + = PRIV_TRIGGER. If PRIV_A is enough for all cases, add docbot request explaining that to = create FK/TRIGGER user should add alter privilege to space(s). > Due to the fact that the insert in _fk_constraint and _trigger > leads to a change in object space * need to check the right to > change corresponding space before creating triggers or foreign > key. >=20 > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 080a72b9f..fb668aa4c 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -3514,6 +3514,11 @@ on_replace_dd_trigger(struct trigger * /* = trigger */, void *event) > uint32_t space_id =3D > tuple_field_u32_xc(old_tuple, > BOX_TRIGGER_FIELD_SPACE_ID); > + struct space *space =3D space_by_id(space_id); > + assert(space !=3D NULL); > + access_check_ddl(space->def->name, space->def->id, > + space->def->uid, SC_SPACE, PRIV_A); > + > char *trigger_name =3D > (char *)region_alloc_xc(&fiber()->gc, > trigger_name_len + 1); > @@ -3567,6 +3572,10 @@ on_replace_dd_trigger(struct trigger * /* = trigger */, void *event) > "trigger space_id does not match the = value " > "resolved on AST building from SQL"); > } > + struct space *space =3D space_by_id(space_id); > + assert(space !=3D NULL); > + access_check_ddl(space->def->name, space->def->id, > + space->def->uid, SC_SPACE, PRIV_A); Could you move this check a bit higher, like in DROP trigger code? We don=E2=80=99t need to compile trigger body to check access privilege. >=20 > struct sql_trigger *old_trigger; > if (sql_trigger_replace(trigger_name, > @@ -3897,6 +3906,8 @@ on_replace_dd_fk_constraint(struct trigger * /* = trigger*/, void *event) > fk_def->name, > "referencing space can't be VIEW"); > } > + access_check_ddl(child_space->def->name, = child_space->def->id, > + child_space->def->uid, SC_SPACE, = PRIV_A); > struct space *parent_space =3D > space_cache_find_xc(fk_def->parent_id); > if (parent_space->def->opts.is_view) { > @@ -3904,6 +3915,8 @@ on_replace_dd_fk_constraint(struct trigger * /* = trigger*/, void *event) > fk_def->name, > "referenced space can't be VIEW"); > } > + access_check_ddl(parent_space->def->name, = parent_space->def->id, > + parent_space->def->uid, SC_SPACE, = PRIV_A); > /* > * FIXME: until SQL triggers are completely > * integrated into server (i.e. we are able to > @@ -4031,6 +4044,10 @@ on_replace_dd_fk_constraint(struct trigger * /* = trigger*/, void *event) > space_cache_find_xc(fk_def->child_id); > struct space *parent_space =3D > space_cache_find_xc(fk_def->parent_id); > + access_check_ddl(child_space->def->name, = child_space->def->id, > + child_space->def->uid, SC_SPACE, = PRIV_A); > + access_check_ddl(parent_space->def->name, = parent_space->def->id, > + parent_space->def->uid, SC_SPACE, = PRIV_A); > struct fk_constraint *old_fk=3D > = fk_constraint_remove(&child_space->child_fk_constraint, > fk_def->name); >=20 > diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua > index 04d6c1903..074c1a0fe 100644 > --- a/test/box/net.box.test.lua > +++ b/test/box/net.box.test.lua > @@ -1435,3 +1435,15 @@ test_run:wait_log('default', 'readahead limit = is reached', 1024, 0.1) >=20 > s:drop() > box.cfg{readahead =3D readahead} > + > +-- Test alter privilege for space that is modified by insertion in > +-- _trigger, _ck_constraint space. Nit: _fk_constraint > +box.sql.execute("CREATE TABLE t5(x INT primary key, y INT, CHECK( x = + y < 2 ));") > +box.sql.execute("CREATE TRIGGER tr1 AFTER DELETE ON t5 BEGIN DELETE = FROM t5; END;") > +box.schema.user.grant('guest','create,read,write','universe') > +c =3D net.connect(box.cfg.listen) > +c.space._trigger:replace({'TR1', box.space.T5.id, {sql=3D'CREATE = TRIGGER tr1 AFTER DELETE ON t5 BEGIN DELETE FROM t5; END;'}}) Please, add three separate cases: trigger creation, trigger drop and trigger replace. The same for FK constraints.