Tarantool development patches archive
 help / color / mirror / Atom feed
From: "n.pettik" <korablev@tarantool.org>
To: tarantool-patches@freelists.org
Cc: Kirill Yukhin <kyukhin@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH] sql: check read access while executing SQL query
Date: Thu, 11 Oct 2018 14:32:55 +0300	[thread overview]
Message-ID: <4D536713-510B-4F25-9455-F4090A29AF67@tarantool.org> (raw)
In-Reply-To: <6dc6ee8b0356676cf6176dd6310109a09ce7b23d.1539247804.git.kyukhin@tarantool.org>


> 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

But in your patch I don’t see any new opcodes.

> 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

Please, provide doc-bot request (especially taking into consideration
the fact that ‘read’ privilege turns out to be not real ’select’ privilege).

> ---
> 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);
> +

Noise empty line.

> 	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.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

I vote for moving these test cases to sql/iproto.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’)

Hmm, why do you need to grand read privilege to _space?
Without it test fails:

error: 'Failed to execute SQL statement: no such table: T1’

I guess somewhere in SQL internals invocation to _space appears to fetch space,
but , it is not obvious. Can you come up with any workaround to avoid it?

Another problem is:

box.schema.user.revoke('guest’,'read', 'space', 'T1')
box.schema.user.grant('guest’,’write', 'space', 'T1')
c:execute("delete from t1 where s1 = 1;”) 
---
- error: 'Failed to execute SQL statement: Read access to space ''T1'' is denied for
    user ''guest'''
...

In terms of Tarantool, it is OK (since we need to iterate through the space to find
tuple to be deleted). But for SQL I guess it is not acceptable: this query doesn’t
give user any data, so read (i.e. SELECT) privilege is not needed.
The same for example involving FK constraints which you provided below.

> +
> +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

  reply	other threads:[~2018-10-11 11:33 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-11  8:53 [tarantool-patches] " Kirill Yukhin
2018-10-11 11:32 ` n.pettik [this message]
2018-10-12 11:41   ` [tarantool-patches] " Kirill Yukhin
  -- strict thread matches above, loose matches on Subject: below --
2018-08-17 17:05 [tarantool-patches] " Kirill Yukhin
2018-08-17 17:59 ` [tarantool-patches] " Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4D536713-510B-4F25-9455-F4090A29AF67@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=kyukhin@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH] sql: check read access while executing SQL query' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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