* [tarantool-patches] [PATCH] sql: check read access while executing SQL query
@ 2018-08-17 17:05 Kirill Yukhin
2018-08-17 17:59 ` [tarantool-patches] " Kirill Yukhin
0 siblings, 1 reply; 3+ messages in thread
From: Kirill Yukhin @ 2018-08-17 17:05 UTC (permalink / raw)
To: v.shpilevoy; +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 | 67 ++++++++++++++++++++++++--
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, 141 insertions(+), 5 deletions(-)
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..cef4cac 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -55,6 +55,65 @@
#include "box/tuple_format.h"
#include "box/coll_id_cache.h"
+#define mh_name _i32i32
+#define mh_key_t uint32_t
+struct mh_i32i32_node_t {
+ mh_key_t key;
+ uint32_t val;
+};
+
+#define mh_node_t struct mh_i32i32_node_t
+#define mh_arg_t uint32_t
+#define mh_hash(a, arg) (a->key)
+#define mh_hash_key(a, arg) (a)
+#define mh_cmp(a, b, arg) ((a->key) != (b->key))
+#define mh_cmp_key(a, b, arg) ((a) != (b->key))
+
+#define MH_SOURCE
+#include "salad/mhash.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.
+ */
+static void
+sql_emit_access_checks(struct Vdbe *v)
+{
+ struct mh_i32i32_t *space_set = mh_i32i32_new();
+ 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) {
+ uint32_t space_id = v->aOp[i].p4.space->def->id;
+ mh_int_t mi = mh_i32i32_find(space_set, space_id, 0);
+ if (mi != mh_end(space_set))
+ continue;
+ struct mh_i32i32_node_t node = {space_id, PRIV_R};
+ mh_i32i32_put(space_set, &node, NULL, 0);
+ }
+ }
+
+ mh_int_t i = mh_first(space_set);
+ while (i != mh_end(space_set)) {
+ struct mh_i32i32_node_t *node = mh_i32i32_node(space_set, i);
+ sqlite3VdbeAddOp4(v, OP_AccessCheck,
+ node->val,
+ 0,
+ 0,
+ (void *)space_cache_find(node->key),
+ P4_SPACEPTR);
+ i = mh_next(space_set, i);
+ }
+ mh_i32i32_delete(space_set);
+}
+
void
sql_finish_coding(struct Parse *parse_context)
{
@@ -76,6 +135,7 @@ sql_finish_coding(struct Parse *parse_context)
int last_instruction = v->nOp;
if (parse_context->initiateTTrans)
sqlite3VdbeAddOp0(v, OP_TTransaction);
+ sql_emit_access_checks(v);
if (parse_context->pConstExpr != NULL) {
assert(sqlite3VdbeGetOp(v, 0)->opcode == OP_Init);
/*
@@ -100,11 +160,8 @@ sql_finish_coding(struct Parse *parse_context)
* ...
* vdbe_end: OP_Goto 0 1 ...
*/
- if (parse_context->initiateTTrans ||
- parse_context->pConstExpr != NULL) {
- sqlite3VdbeChangeP2(v, 0, last_instruction);
- sqlite3VdbeGoto(v, 1);
- }
+ sqlite3VdbeChangeP2(v, 0, last_instruction);
+ sqlite3VdbeGoto(v, 1);
/* Get the VDBE program ready for execution. */
if (parse_context->nErr == 0 && !db->mallocFailed) {
assert(parse_context->iCacheLevel == 0);
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: check read access while executing SQL query
2018-08-17 17:05 [tarantool-patches] [PATCH] sql: check read access while executing SQL query Kirill Yukhin
@ 2018-08-17 17:59 ` Kirill Yukhin
0 siblings, 0 replies; 3+ messages in thread
From: Kirill Yukhin @ 2018-08-17 17:59 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* [tarantool-patches] [PATCH] sql: check read access while executing SQL query
@ 2018-10-11 8:53 Kirill Yukhin
0 siblings, 0 replies; 3+ messages in thread
From: Kirill Yukhin @ 2018-10-11 8:53 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches, Kirill Yukhin
Since SQL front-end is not using box API,
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
---
https://github.com/tarantool/tarantool/commits/kyukhin/gh-2362-sql-read-access-check
https://github.com/tarantool/tarantool/issues/2362
src/box/sql/vdbe.c | 6 ++
test/sql/gh-2362-select-access-rights.result | 113 +++++++++++++++++++++++++
test/sql/gh-2362-select-access-rights.test.lua | 44 ++++++++++
3 files changed, 163 insertions(+)
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/vdbe.c b/src/box/sql/vdbe.c
index 7c1015c..b4e2269 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3099,8 +3099,14 @@ case OP_IteratorOpen:
else
space = aMem[pOp->p3].u.p;
assert(space != NULL);
+ if (access_check_space(space, PRIV_R) != 0) {
+ rc = SQL_TARANTOOL_ERROR;
+ goto abort_due_to_error;
+ }
+
struct index *index = space_index(space, pOp->p2);
assert(index != NULL);
+
assert(pOp->p1 >= 0);
cur = allocateCursor(p, pOp->p1,
space->def->exact_field_count == 0 ?
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..1f0ca94
--- /dev/null
+++ b/test/sql/gh-2362-select-access-rights.result
@@ -0,0 +1,113 @@
+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 t1 (s1 INT PRIMARY KEY, s2 INT UNIQUE);")
+---
+...
+box.sql.execute("CREATE TABLE t2 (s1 INT PRIMARY KEY);")
+---
+...
+box.sql.execute("INSERT INTO t1 VALUES (1, 1);")
+---
+...
+box.sql.execute("INSERT INTO t2 VALUES (1);")
+---
+...
+box.schema.user.grant('guest','read', 'space', '_space')
+---
+...
+box.schema.user.grant('guest','read', 'space', 'T1')
+---
+...
+c = nb.connect(box.cfg.listen)
+---
+...
+c:execute("SELECT * FROM t1;")
+---
+- metadata:
+ - name: S1
+ - name: S2
+ rows:
+ - [1, 1]
+...
+box.schema.user.revoke('guest','read', 'space', 'T1')
+---
+...
+c = nb.connect(box.cfg.listen)
+---
+...
+c:execute("SELECT * FROM t1;")
+---
+- error: 'Failed to execute SQL statement: Read access to space ''T1'' is denied for
+ user ''guest'''
+...
+box.schema.user.grant('guest','read', 'space', 'T2')
+---
+...
+c = nb.connect(box.cfg.listen)
+---
+...
+c:execute('SELECT * FROM t1, t2 WHERE t1.s1 = t2.s1')
+---
+- error: 'Failed to execute SQL statement: Read access to space ''T1'' is denied for
+ user ''guest'''
+...
+box.sql.execute("CREATE VIEW v AS SELECT * FROM t1")
+---
+...
+box.schema.user.grant('guest','read', 'space', 'V')
+---
+...
+v = nb.connect(box.cfg.listen)
+---
+...
+c:execute('SELECT * FROM v')
+---
+- error: 'Failed to execute SQL statement: Read access to space ''T1'' is denied for
+ user ''guest'''
+...
+box.sql.execute('CREATE TABLE t3 (s1 INT PRIMARY KEY, fk INT, FOREIGN KEY (fk) REFERENCES t1(s2))')
+---
+...
+box.schema.user.grant('guest','read','space', 'T3')
+---
+...
+v = nb.connect(box.cfg.listen)
+---
+...
+c:execute('INSERT INTO t3 VALUES (1, 1)')
+---
+- error: 'Failed to execute SQL statement: Read access to space ''T1'' is denied for
+ user ''guest'''
+...
+-- Cleanup
+box.schema.user.revoke('guest','read','space', 'V')
+---
+...
+box.schema.user.revoke('guest','read','space', 'T2')
+---
+...
+box.schema.user.revoke('guest','read','space', 'T3')
+---
+...
+box.sql.execute('DROP VIEW v')
+---
+...
+box.sql.execute('DROP TABLE t3')
+---
+...
+box.sql.execute('DROP TABLE t2')
+---
+...
+box.sql.execute("DROP TABLE t1")
+---
+...
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..159373c
--- /dev/null
+++ b/test/sql/gh-2362-select-access-rights.test.lua
@@ -0,0 +1,44 @@
+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 t1 (s1 INT PRIMARY KEY, s2 INT UNIQUE);")
+box.sql.execute("CREATE TABLE t2 (s1 INT PRIMARY KEY);")
+box.sql.execute("INSERT INTO t1 VALUES (1, 1);")
+box.sql.execute("INSERT INTO t2 VALUES (1);")
+
+box.schema.user.grant('guest','read', 'space', '_space')
+
+box.schema.user.grant('guest','read', 'space', 'T1')
+c = nb.connect(box.cfg.listen)
+c:execute("SELECT * FROM t1;")
+
+box.schema.user.revoke('guest','read', 'space', 'T1')
+c = nb.connect(box.cfg.listen)
+c:execute("SELECT * FROM t1;")
+
+box.schema.user.grant('guest','read', 'space', 'T2')
+c = nb.connect(box.cfg.listen)
+c:execute('SELECT * FROM t1, t2 WHERE t1.s1 = t2.s1')
+
+box.sql.execute("CREATE VIEW v AS SELECT * FROM t1")
+
+box.schema.user.grant('guest','read', 'space', 'V')
+v = nb.connect(box.cfg.listen)
+c:execute('SELECT * FROM v')
+
+box.sql.execute('CREATE TABLE t3 (s1 INT PRIMARY KEY, fk INT, FOREIGN KEY (fk) REFERENCES t1(s2))')
+box.schema.user.grant('guest','read','space', 'T3')
+v = nb.connect(box.cfg.listen)
+c:execute('INSERT INTO t3 VALUES (1, 1)')
+
+-- Cleanup
+box.schema.user.revoke('guest','read','space', 'V')
+box.schema.user.revoke('guest','read','space', 'T2')
+box.schema.user.revoke('guest','read','space', 'T3')
+
+box.sql.execute('DROP VIEW v')
+box.sql.execute('DROP TABLE t3')
+box.sql.execute('DROP TABLE t2')
+box.sql.execute("DROP TABLE t1")
--
2.11.0
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-10-11 8:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-17 17:05 [tarantool-patches] [PATCH] sql: check read access while executing SQL query Kirill Yukhin
2018-08-17 17:59 ` [tarantool-patches] " Kirill Yukhin
2018-10-11 8:53 [tarantool-patches] " Kirill Yukhin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox