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 A6E192A611 for ; Fri, 24 Aug 2018 05:50:28 -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 AnrhP0vPK6pF for ; Fri, 24 Aug 2018 05:50:28 -0400 (EDT) Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (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 EC0F528873 for ; Fri, 24 Aug 2018 05:50:27 -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 v2] sql: check read access while executing SQL query From: "n.pettik" In-Reply-To: Date: Fri, 24 Aug 2018 12:50:18 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: 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: Kirill Yukhin > On 22 Aug 2018, at 09:53, Kirill Yukhin wrote: >=20 > Since SQL front-end is not using box API anymore, Basically, AFAIK selects have never checked access privileges. Box API was used only to handle DML routine. > no checkes for read access are performed by VDBE engine. > Fix this by introducing dedicated opcode which is executed > to perform read access check. Please, mention that new opcode is generated for EACH OP_OpenWrite/Read appearance in VDBE program. > Note, that there's is no need to perform DML/DDL checkes as > they're performed by Tarantool's core. >=20 > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index da7668b..c25715c 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -55,6 +55,38 @@ > #include "box/tuple_format.h" > #include "box/coll_id_cache.h" >=20 > +/** > + * Emit access control checks. It should be noted that only > + * checks for read need to be performed. DML/DDL requestes are > + * checked for access by Tarantool's core. > + * > + * @param v Byte-code structure being translated. > + * @retval true If a new opcode was emitted. > + */ > +static bool > +sql_emit_access_checks(struct Vdbe *v) AFAIR we decided to use vdbe_emit_ prefix for functions generating VDBE code. Moreover, according to codestyle: =E2=80=98'' If the name of a function is an action or an imperative command, the function should return an error-code integer. If the name is a predicate, the function should return a "succeeded" boolean. =E2=80=98=E2=80=99=E2=80=99 Name of your function doesn=E2=80=99t seem to be predicate=E2=80=A6 I don't insist on this rule, tho (but smb else might). > +{ > + bool is_code_emitted =3D false; > + for(int i =3D 0; i < v->nOp; ++i) { > + uint8_t op =3D v->aOp[i].opcode; > + /* FIXME: when write iterators will be removed, Nitpicking: comment should be like: /* * TEXT STARTS HERE */ > + * remove checks for OP_OpenWrite. Note, that > + * DML routines don't need access checks. > + * See gh-3182. Nitpicking: AFAIR references to issues are not desirable in source code. > + */ > + if (op =3D=3D OP_OpenRead || op =3D=3D OP_OpenWrite) { > + sqlite3VdbeAddOp4(v, OP_AccessCheck, > + PRIV_R, > + 0, > + 0, Smth wrong with indentation. > + (void *)v->aOp[i].p4.space, > + P4_SPACEPTR); > + is_code_emitted =3D true; Why do we need to generate these opcodes at the end of program? Please, leave explanation comment. Moreover, you emit OP_AccessCheck on each opcode, even if we have already checked access on this space. It may make sense in case you emit OP_AccessCheck opcode right before cursor opening, but in the beginning of VDBE execution it is wasting execution time. E.g.: SQL-trace: select * from t1, t1 as t2 101> 13 AccessCheck 1 0 0 space 00 Check 1 = access rights for space. 101> 14 AccessCheck 1 0 0 space 00 Check 1 = access rights for space. 101> 15 Goto 0 1 0 00=20 101> 1 OpenWrite 0 0 0 space 00 index id =3D = 0, space ptr =3D space; T1 101> 2 OpenWrite 1 0 0 space 00 index id =3D = 0, space ptr =3D space; T1 101> 3 Explain 0 0 0 SCAN TABLE T1 00=20 101> 4 Rewind 0 12 0 00=20 101> 12 Halt=20 One AccessCheck is definitely redundant. > + } > + } > + return is_code_emitted; > +} > + > void > sql_finish_coding(struct Parse *parse_context) > { > @@ -76,6 +108,7 @@ sql_finish_coding(struct Parse *parse_context) > int last_instruction =3D v->nOp; > if (parse_context->initiateTTrans) > sqlite3VdbeAddOp0(v, OP_TTransaction); > + bool is_acc_emitted =3D sql_emit_access_checks(v); > if (parse_context->pConstExpr !=3D NULL) { > assert(sqlite3VdbeGetOp(v, 0)->opcode =3D=3D OP_Init); > /* > @@ -101,7 +134,8 @@ sql_finish_coding(struct Parse *parse_context) > * vdbe_end: OP_Goto 0 1 ... > */ > if (parse_context->initiateTTrans || > - parse_context->pConstExpr !=3D NULL) { > + parse_context->pConstExpr !=3D NULL || > + is_acc_emitted) { > sqlite3VdbeChangeP2(v, 0, last_instruction); > sqlite3VdbeGoto(v, 1); > } > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index dc3f5c0..ec780a8 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -5286,6 +5286,21 @@ case OP_IncMaxid: { > break; > } >=20 > +/* Opcode: AccessCheck P1 * * P4 * > + * Synopsis: Check P1 access rights for P4. > + * > + * Check access rights for space pointed by register P1. *Pointer by register P4* > + * Access type is determined by P1. > + */ > +case OP_AccessCheck: { > + struct space *space =3D (struct space *)pOp->p4.space; > + if (access_check_space(space, pOp->p1) !=3D 0) { > + rc =3D SQL_TARANTOOL_ERROR; > + goto abort_due_to_error; > + } > + break; > +} > + >=20 > 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..d5d0a8e > --- /dev/null > +++ b/test/sql/gh-2362-select-access-rights.test.lua > @@ -0,0 +1,18 @@ > +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 te37 (s1 INT PRIMARY KEY);") > +box.sql.execute("INSERT INTO te37 VALUES (1);") > + > +box.schema.user.grant('guest','read,write,execute','universe=E2=80=99) Why do you give all privileges? To make this test be fair, you should give only =E2=80=98read=E2=80=99 access. I would also add tests with = join involving several tables to make sure that read access is tested for all spaces. Next question is about SELECT from views: user gives view read = privilege, but will get error when try to select from that view. The last one - what should we do with FK: to check constraint violations we must implicitly open read cursor on parent space. Hence, user again would get =E2=80=98access denied=E2=80=99 error if he tried to insert in = child space. I propose to investiagate how these things are handled in other DBs.