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 6D4882A056 for ; Fri, 17 Aug 2018 13:59:09 -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 BIhldP1-9QYX for ; Fri, 17 Aug 2018 13:59:09 -0400 (EDT) Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (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 DD1C029D09 for ; Fri, 17 Aug 2018 13:59:08 -0400 (EDT) Date: Fri, 17 Aug 2018 20:59:06 +0300 From: Kirill Yukhin Subject: [tarantool-patches] Re: [PATCH] sql: check read access while executing SQL query Message-ID: <20180817175906.g47gusz3xomqecqu@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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 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