From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 2875145C304 for ; Thu, 3 Dec 2020 12:54:26 +0300 (MSK) Date: Thu, 3 Dec 2020 12:54:23 +0300 From: Mergen Imeev Message-ID: <20201203095423.GA204182@tarantool.org> References: <27d8ef885dcb4726f4c7423e72a72038608a8628.1604361700.git.imeevma@gmail.com> <919343f4-214c-aa10-aec4-773ee510888a@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <919343f4-214c-aa10-aec4-773ee510888a@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Hi! Thank you for the review. Sorry for such late reply. New patch and my answers below. On Wed, Nov 11, 2020 at 10:48:54PM +0100, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > See 7 comments below. > > On 03.11.2020 01:03, imeevma@tarantool.org wrote: > > After this patch, the persistent functions "box.schema.user.info" and > > "LUA" will have the same rights as the user who executed them. > > 1. It would be good to see more info what was wrong. Not only what > you changed, but also why. AFAIU, the issue was with the setuid bit > set without necessity. > Fixed. > > Fixes tarantool/security#1 > > --- > > https://github.com/tarantool/security/issues/1 > > https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-function-access > > 2. Should there be a changelog record? > Added. > 3. What about other versions? If this is a bug, it should be backported. > We plan to release 2.6.2, 2.5.3, 1.10.9. LUA() may be not actual for 1.10. > > But I don't have a ready recipe how to backport a schema change. Assume > we will add upgrade_to_2_6_2(), which will do the same as upgrade_to_2_7_1(). > Then if someone upgrades from 2.6.1, he will execute upgrade_to_2_6_2(), and > he does not need to call upgrade_to_2_7_1() (at least in its current form). > > This is how I would try. I would move your fix to a new function with a > name like backport_upgrade_2_7_1_function_access(), or > backport_upgrade_security_1() (by repo and issue ID) which would check if > the functions have setuid set. If they do - remove it. Otherwise do nothing. > Then we call it in upgrade_to_1_10_9(), upgrade_to_2_5_3(), upgrade_to_2_6_2(), > and upgrade_to_2_7_1(). > Thanks. I did as you suggested. Still, this patch only for 2.7. I will create similar patches for other versions when this patch will be approved. > > diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua > > index add791cd7..671e441ca 100644 > > --- a/src/box/lua/upgrade.lua > > +++ b/src/box/lua/upgrade.lua > > @@ -971,6 +971,40 @@ local function upgrade_to_2_3_1() > > create_session_settings_space() > > end > > > > +-------------------------------------------------------------------------------- > > +-- Tarantool 2.7.1 > > +-------------------------------------------------------------------------------- > > + > > +local function upgrade_to_2_7_1() > > + local _func = box.space[box.schema.FUNC_ID] > > + local _priv = box.space[box.schema.PRIV_ID] > > 4. Why not _func = box.space._func and the same for _priv? Do you really need > to use the ids? > Fixed. > > + > > + local datetime = os.date("%Y-%m-%d %H:%M:%S") > > + > > + -- Re-create "box.schema.user.info" function. > > + log.info('remove old function "box.schema.user.info"') > > + _priv:delete({2, 'function', 1}) > > + _func:delete({1}) > > + log.info('create function "box.schema.user.info" with setuid') > > + _func:replace({1, ADMIN, 'box.schema.user.info', 0, 'LUA', '', 'function', > > + {}, 'any', 'none', 'none', false, false, true, {'LUA'}, > > + setmap({}), '', datetime, datetime}) > > + log.info('grant execute on function "box.schema.user.info" to public') > > + _priv:replace{ADMIN, PUBLIC, 'function', 1, box.priv.X} > > + > > + -- Re-create "LUA" function. > > + log.info('remove old function "LUA"') > > + _priv:delete({2, 'function', 65}) > > + _func:delete({65}) > > 5. Can you not use the numeric constants? Can you search the needed > functions by name, and extract the ID from the result tuple? > Rewrote this part. > > + log.info('create function "LUA"') > > + _func:replace({65, ADMIN, 'LUA', 0, 'LUA', > > + 'function(code) return assert(loadstring(code))() end', > > + 'function', {'string'}, 'any', 'none', 'none', false, false, > > + true, {'LUA', 'SQL'}, setmap({}), '', datetime, datetime}) > > 6. Why did you in this replace align non-first lines by { + 1, and in the > previous replace exactly by {? > Removed in rewroted part. > > + log.info('grant execute on function "LUA" to public') > > + _priv:replace{ADMIN, PUBLIC, 'function', 65, box.priv.X} > > +end > > diff --git a/test/box/access.result b/test/box/access.result > > index 20b1b8b35..92d6453d7 100644 > > --- a/test/box/access.result > > +++ b/test/box/access.result > > @@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe') > > sp:drop() > > --- > > ... > > +-- > > +-- Make sure that the functions "LUA" and "box.schema.user.info" do not have > > +-- excess rights. > > +-- > > +_ = box.schema.func.call("LUA", "return 1") > > +--- > > +... > > +_ = box.schema.func.call("LUA", "return box.space._space:get(257)") > > 7. Why 257? > Changed get() to count(). > > +--- > > +... > > +_ = box.schema.func.call("box.schema.user.info", 0) > > +--- > > +... > > +_ = box.schema.func.call("box.schema.user.info", 1) New patch: >From 785013ce70c43f68678aa3a558248979b6b7c6e1 Mon Sep 17 00:00:00 2001 Message-Id: <785013ce70c43f68678aa3a558248979b6b7c6e1.1606988615.git.imeevma@gmail.com> From: Mergen Imeev Date: Mon, 2 Nov 2020 13:21:58 +0300 Subject: [PATCH v2 1/1] box: remove unnecessary rights from peristent functions After this patch, the persistent functions "box.schema.user.info" and "LUA" will have the same rights as the user who executed them. The problem was that setuid was unnecessarily set. Because of this, these functions had the same rights as the user who created them. However, they must have the same rights as the user who used them. Fixes tarantool/security#1 --- https://github.com/tarantool/security/issues/1 https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-function-access @ChangeLog - The functions "LUA" and "box.schema.user.info" now have the same rights as the user who executes them. (gh-security-1). src/box/bootstrap.snap | Bin 5976 -> 5990 bytes src/box/lua/upgrade.lua | 31 ++++++++++++++++++++++++++++++ test/box-py/bootstrap.result | 4 ++-- test/box/access.result | 36 +++++++++++++++++++++++++++++++++++ test/box/access.test.lua | 15 +++++++++++++++ 5 files changed, 84 insertions(+), 2 deletions(-) diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua index add791cd7..b2475b0f6 100644 --- a/src/box/lua/upgrade.lua +++ b/src/box/lua/upgrade.lua @@ -971,6 +971,36 @@ local function upgrade_to_2_3_1() create_session_settings_space() end +-------------------------------------------------------------------------------- +-- Tarantool 2.7.1 +-------------------------------------------------------------------------------- +local function backport_upgrade_2_7_1_function_access() + local _func = box.space._func + local _priv = box.space._priv + local datetime = os.date("%Y-%m-%d %H:%M:%S") + local funcs_to_change = {'LUA', 'box.schema.user.info'} + for _, name in pairs(funcs_to_change) do + local func = _func.index['name']:get(name) + -- Change setuid of function function if it is not 0. + if func ~= nil and func.setuid ~= 0 then + local id = func.id + log.info('remove old function "'..name..'"') + _priv:delete({2, 'function', id}) + _func:delete({id}) + log.info('create function "'..name..'" with unset setuid') + local new_func = func:update({{'=', 4, 0}, {'=', 18, datetime}, + {'=', 19, datetime}}) + _func:replace(new_func) + log.info('grant execute on function "'..name..'" to public') + _priv:replace{ADMIN, PUBLIC, 'function', id, box.priv.X} + end + end +end + +local function upgrade_to_2_7_1() + backport_upgrade_2_7_1_function_access() +end + -------------------------------------------------------------------------------- local function get_version() @@ -1007,6 +1037,7 @@ local function upgrade(options) {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true}, {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true}, {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true}, + {version = mkversion(2, 7, 1), func = upgrade_to_2_7_1, auto = true}, } for _, handler in ipairs(handlers) do diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result index 0876e77a6..ed7accea3 100644 --- a/test/box-py/bootstrap.result +++ b/test/box-py/bootstrap.result @@ -4,7 +4,7 @@ box.internal.bootstrap() box.space._schema:select{} --- - - ['max_id', 511] - - ['version', 2, 3, 1] + - ['version', 2, 7, 1] ... box.space._cluster:select{} --- @@ -167,7 +167,7 @@ box.space._user:select{} ... for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end --- -- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], 'any', 'none', 'none', +- - [1, 1, 'box.schema.user.info', 0, 'LUA', '', 'function', [], 'any', 'none', 'none', false, false, true, ['LUA'], {}, '', '', ''] ... box.space._priv:select{} diff --git a/test/box/access.result b/test/box/access.result index 20b1b8b35..27e636122 100644 --- a/test/box/access.result +++ b/test/box/access.result @@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe') sp:drop() --- ... +-- +-- Make sure that the functions "LUA" and "box.schema.user.info" do not have +-- excess rights. +-- +_ = box.schema.func.call("LUA", "return 1") +--- +... +_ = box.schema.func.call("LUA", "return box.space._space:count()") +--- +... +_ = box.schema.func.call("box.schema.user.info", 0) +--- +... +_ = box.schema.func.call("box.schema.user.info", 1) +--- +... +session.su('guest') +--- +... +_ = box.schema.func.call("LUA", "return 1") +--- +... +_ = box.schema.func.call("LUA", "return box.space._space:count()") +--- +- error: Read access to space '_space' is denied for user 'guest' +... +_ = box.schema.func.call("box.schema.user.info", 0) +--- +... +_ = box.schema.func.call("box.schema.user.info", 1) +--- +- error: User '1' is not found +... +session.su('admin') +--- +... diff --git a/test/box/access.test.lua b/test/box/access.test.lua index 3e083a383..a62f87ad8 100644 --- a/test/box/access.test.lua +++ b/test/box/access.test.lua @@ -824,3 +824,18 @@ box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe') box.schema.user.revoke('guest', 'read,write,execute', 'universe') box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe') sp:drop() + +-- +-- Make sure that the functions "LUA" and "box.schema.user.info" do not have +-- excess rights. +-- +_ = box.schema.func.call("LUA", "return 1") +_ = box.schema.func.call("LUA", "return box.space._space:count()") +_ = box.schema.func.call("box.schema.user.info", 0) +_ = box.schema.func.call("box.schema.user.info", 1) +session.su('guest') +_ = box.schema.func.call("LUA", "return 1") +_ = box.schema.func.call("LUA", "return box.space._space:count()") +_ = box.schema.func.call("box.schema.user.info", 0) +_ = box.schema.func.call("box.schema.user.info", 1) +session.su('admin') -- 2.25.1