[tarantool-patches] Re: [PATCH v2] sql: check read access while executing SQL query

[n.pettik]

> 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.

> 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.
> 
> 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"
> 
> +/**
> + * 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:

‘''
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).

> +{
> +	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
 */

> +		 * 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 == OP_OpenRead || op == 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 = 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.

> +		}
> +	}
> +	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 = v->nOp;
> 	if (parse_context->initiateTTrans)
> 		sqlite3VdbeAddOp0(v, OP_TTransaction);
> +	bool is_acc_emitted = sql_emit_access_checks(v);
> 	if (parse_context->pConstExpr != NULL) {
> 		assert(sqlite3VdbeGetOp(v, 0)->opcode == 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 != NULL) {
> +	    parse_context->pConstExpr != 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;
> }
> 
> +/* 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 = (struct space *)pOp->p4.space;
> +	if (access_check_space(space, pOp->p1) != 0) {
> +		rc = SQL_TARANTOOL_ERROR;
> +		goto abort_due_to_error;
> +	}
> +	break;
> +}
> +
> 
> 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.

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 ‘access denied’ error if he tried to insert in child space.

I propose to investiagate how these things are handled in other DBs.