Tarantool development patches archive
 help / color / mirror / Atom feed
* [tarantool-patches] [PATCH v2] sql: check read access while executing SQL query
@ 2018-08-22  6:53 Kirill Yukhin
  2018-08-24  9:50 ` [tarantool-patches] " n.pettik
  0 siblings, 1 reply; 3+ messages in thread
From: Kirill Yukhin @ 2018-08-22  6:53 UTC (permalink / raw)
  To: korablev; +Cc: tarantool-patches, 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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-08-24 11:21 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-22  6:53 [tarantool-patches] [PATCH v2] sql: check read access while executing SQL query Kirill Yukhin
2018-08-24  9:50 ` [tarantool-patches] " n.pettik
2018-08-24 11:21   ` Kirill Yukhin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox