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 ABF132A2DE for ; Fri, 24 Aug 2018 07:21:32 -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 WN-Pp4ygsTUu for ; Fri, 24 Aug 2018 07:21:32 -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 67EBA2A2D7 for ; Fri, 24 Aug 2018 07:21:32 -0400 (EDT) Date: Fri, 24 Aug 2018 14:21:29 +0300 From: Kirill Yukhin Subject: [tarantool-patches] Re: [PATCH v2] sql: check read access while executing SQL query Message-ID: <20180824112129.jjsfzm3puoimcgxg@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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: "n.pettik" Cc: tarantool-patches@freelists.org Hello Nikita, Thanks for review. My answers inlined. On 24 авг 12:50, n.pettik wrote: > > > On 22 Aug 2018, at 09:53, Kirill Yukhin wrote: > > > > 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. I'll update description. > > 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. There'll be no new opcode at all. See below. > > 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 @@ > > +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: > > ‘'' > 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. > ‘’’ > Name of your function doesn’t seem to be predicate… > > I don't insist on this rule, tho (but smb else might). You are right. > > +{ > > + bool is_code_emitted = false; > > + for(int i = 0; i < v->nOp; ++i) { > > + uint8_t op = v->aOp[i].opcode; > > + /* FIXME: when write iterators will be removed, > > Nitpicking: comment should be like: > > /* > * TEXT STARTS HERE > */ Ditto. > > + * 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. Ditto. > > + */ > > + if (op == OP_OpenRead || op == OP_OpenWrite) { > > + sqlite3VdbeAddOp4(v, OP_AccessCheck, > > + PRIV_R, > > + 0, > > + 0, > > Smth wrong with indentation. Ditto. > > + (void *)v->aOp[i].p4.space, > > + P4_SPACEPTR); > > + is_code_emitted = 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 > 101> 1 OpenWrite 0 0 0 space 00 index id = 0, space ptr = space; T1 > 101> 2 OpenWrite 1 0 0 space 00 index id = 0, space ptr = space; T1 > 101> 3 Explain 0 0 0 SCAN TABLE T1 00 > 101> 4 Rewind 0 12 0 00 > 101> 12 Halt > > One AccessCheck is definitely redundant. I'd call it a prologue, not begin or end. Initial (v1) version of the patch supported dedicated hash which was used to emit AccessCheck only once, but @kostya rejected it saying that this is over-engineering. After some re-thinking and taking that statement into account I've come up to following idea: we do not need new opcode at all, we'll use OpenRead/OpenWrite to do the check. When OpenWrite will be eliminated w/ #3182, access will be performed for OpenRead opcode, which is our ultimate goal. So, I'll resubmit v3 version of the patch. > > --- a/src/box/sql/vdbe.c > > +++ b/src/box/sql/vdbe.c > > @@ -5286,6 +5286,21 @@ case OP_IncMaxid: { > > break; > > } > > > > +/* Opcode: AccessCheck P1 * * P4 * > > + * Synopsis: Check P1 access rights for P4. > > + * > > + * Check access rights for space pointed by register P1. > > *Pointer by register P4* Not relevant anymore. > > 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 = require('test_run').new() > > +engine = test_run:get_cfg('engine') > > +nb = require('net.box') > > + > > +box.sql.execute("PRAGMA sql_default_engine='"..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’) > > Why do you give all privileges? To make this test be fair, you should > give only ‘read’ access. I would also add tests with join involving several > tables to make sure that read access is tested for all spaces. Fixed. Case added. > Next question is about SELECT from views: user gives view read privilege, > but will get error when try to select from that view. Case added. > 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 ‘access denied’ error if he tried to insert in child space. Case added. > I propose to investiagate how these things are handled in other DBs. I think there's nothing to investigate here: if view is referencing to a table access to which is denied - then error should be reported and no data retrieved from the table. -- Regards, Kirill Yukhin