From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 52A41469719 for ; Thu, 12 Nov 2020 00:48:56 +0300 (MSK) References: <27d8ef885dcb4726f4c7423e72a72038608a8628.1604361700.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: <919343f4-214c-aa10-aec4-773ee510888a@tarantool.org> Date: Wed, 11 Nov 2020 22:48:54 +0100 MIME-Version: 1.0 In-Reply-To: <27d8ef885dcb4726f4c7423e72a72038608a8628.1604361700.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: imeevma@tarantool.org Cc: tarantool-patches@dev.tarantool.org 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. > 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? 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(). > 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? > + > + 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? > + 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 {? > + 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? > +--- > +... > +_ = box.schema.func.call("box.schema.user.info", 0) > +--- > +... > +_ = box.schema.func.call("box.schema.user.info", 1)