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

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

[Kirill Yukhin]

Since SQL front-end is not using box API anymore,
no checkes for read access are performed by VDBE engine.
Fix this by introducing dedicated opcode which is executed
to perform read access check.
Note, that there's is no need to perform DML/DDL checkes as
they're performed by Tarantool's core.

Closes #2362
---
Issue: https://github.com/tarantool/tarantool/issues/2362
Branch: https://github.com/tarantool/tarantool/commits/kyukhin/gh-2362-sql-read-access-check

 src/box/sql/build.c                            | 36 +++++++++++++++++++-
 src/box/sql/vdbe.c                             | 15 +++++++++
 test/sql/gh-2362-select-access-rights.result   | 46 ++++++++++++++++++++++++++
 test/sql/gh-2362-select-access-rights.test.lua | 18 ++++++++++
 4 files changed, 114 insertions(+), 1 deletion(-)
 create mode 100644 test/sql/gh-2362-select-access-rights.result
 create mode 100644 test/sql/gh-2362-select-access-rights.test.lua

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)
+{
+	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,
+		 * remove checks for OP_OpenWrite. Note, that
+		 * DML routines don't need access checks.
+		 * See gh-3182.
+		 */
+		if (op == OP_OpenRead || op == OP_OpenWrite) {
+			sqlite3VdbeAddOp4(v, OP_AccessCheck,
+					  PRIV_R,
+					  0,
+					  0,
+					  (void *)v->aOp[i].p4.space,
+					  P4_SPACEPTR);
+			is_code_emitted = true;
+		}
+	}
+	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.
+ * 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;
+}
+
 /* Opcode: Noop * * * * *
  *
  * Do nothing.  This instruction is often useful as a jump
diff --git a/test/sql/gh-2362-select-access-rights.result b/test/sql/gh-2362-select-access-rights.result
new file mode 100644
index 0000000..e17079e
--- /dev/null
+++ b/test/sql/gh-2362-select-access-rights.result
@@ -0,0 +1,46 @@
+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')
+---
+...
+c = nb.connect(box.cfg.listen)
+---
+...
+c:execute("SELECT * FROM te37;")
+---
+- metadata:
+  - name: S1
+  rows:
+  - [1]
+...
+box.schema.user.revoke('guest','read','universe')
+---
+...
+c = nb.connect(box.cfg.listen)
+---
+...
+c:execute("SELECT * FROM te37;")
+---
+- error: 'Failed to execute SQL statement: Read access to space ''TE37'' is denied
+    for user ''guest'''
+...
+-- Cleanup
+box.sql.execute("DROP TABLE te37")
+---
+...
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')
+c = nb.connect(box.cfg.listen)
+c:execute("SELECT * FROM te37;")
+
+box.schema.user.revoke('guest','read','universe')
+c = nb.connect(box.cfg.listen)
+c:execute("SELECT * FROM te37;")
+
+-- Cleanup
+box.sql.execute("DROP TABLE te37")
-- 
2.16.2

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

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

[Kirill Yukhin]

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