From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 1F8F52A114 for ; Fri, 17 Aug 2018 13:05:51 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id lsbO87aiCLoZ for ; Fri, 17 Aug 2018 13:05:51 -0400 (EDT) Received: from smtp21.mail.ru (smtp21.mail.ru [94.100.179.250]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id D33D02A0F6 for ; Fri, 17 Aug 2018 13:05:50 -0400 (EDT) From: Kirill Yukhin Subject: [tarantool-patches] [PATCH] sql: check read access while executing SQL query Date: Fri, 17 Aug 2018 20:05:41 +0300 Message-Id: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: v.shpilevoy@tarantool.org Cc: tarantool-patches@freelists.org, 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