[tarantool-patches] Re: [PATCH v2] sql: check read access while executing SQL query
Kirill Yukhin
kyukhin at tarantool.org
Fri Aug 24 14:21:29 MSK 2018
Hello Nikita,
Thanks for review. My answers inlined.
On 24 авг 12:50, n.pettik wrote:
>
> > On 22 Aug 2018, at 09:53, Kirill Yukhin <kyukhin at tarantool.org> 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<name=T1> 00 Check 1 access rights for space<name=T1>.
> 101> 14 AccessCheck 1 0 0 space<name=T1> 00 Check 1 access rights for space<name=T1>.
> 101> 15 Goto 0 1 0 00
> 101> 1 OpenWrite 0 0 0 space<name=T1> 00 index id = 0, space ptr = space<name=T1>; T1
> 101> 2 OpenWrite 1 0 0 space<name=T1> 00 index id = 0, space ptr = space<name=T1>; 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
More information about the Tarantool-patches
mailing list