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

[Kirill Yukhin]

Hello,
On 17 авг 20:05, Kirill Yukhin wrote:
> 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
Please, disregard the initial patch. Using hash seems too be
overkill, so remove it. Updated patch is in the bottom,
branch force-pushed.

--
Regards, Kirill Yukhin

 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 cdf2bfc..35cce7b 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 dc5146f..6f9cced 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -5240,6 +5240,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