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 39B7F4765E0 for ; Mon, 21 Dec 2020 22:23:04 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) From: Sergey Ostanevich In-Reply-To: <2cc8e87a9cf72e326e0eacec99eb127a90b3f5e1.1608547708.git.imeevma@gmail.com> Date: Mon, 21 Dec 2020 22:23:01 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <6F4504F6-F009-4BE5-9FF2-097AEEC0992A@corp.mail.ru> References: <2cc8e87a9cf72e326e0eacec99eb127a90b3f5e1.1608547708.git.imeevma@gmail.com> 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: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! I wonder why we should have this strange naming for the function of backport 2_7_1 from 2_7_1 itself. Perhaps just the =E2=80=98function_acces= s=E2=80=99 is enough? Otherwise LGTM. Sergos > On 21 Dec 2020, at 13:51, Mergen Imeev via Tarantool-patches = wrote: >=20 > After this patch, the persistent functions "box.schema.user.info" and > "LUA" will have the same rights as the user who executed them. >=20 > 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. >=20 > Fixes tarantool/security#1 > --- > https://github.com/tarantool/security/issues/1 > = https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-func= tion-access >=20 > @ChangeLog > - The functions "LUA" and "box.schema.user.info" now have the > same rights as the user who executes them. (gh-security-1). >=20 >=20 > src/box/bootstrap.snap | Bin 5976 -> 5991 bytes > src/box/lua/upgrade.lua | 30 +++++++++++++++++++++++++++++ > test/box-py/bootstrap.result | 4 ++-- > test/box/access.result | 36 +++++++++++++++++++++++++++++++++++ > test/box/access.test.lua | 15 +++++++++++++++ > 5 files changed, 83 insertions(+), 2 deletions(-) >=20 > diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap > index = 8bd4f7ce24216a8bcced6aa97c20c83eb7a02c77..9e7a4e3291accfc387b315f85be9b01f= 443048fd 100644 >=20 > diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua > index a86a0d410..09cd015f0 100644 > --- a/src/box/lua/upgrade.lua > +++ b/src/box/lua/upgrade.lua > @@ -971,6 +971,35 @@ local function upgrade_to_2_3_1() > create_session_settings_space() > end >=20 > = +-------------------------------------------------------------------------= ------- > +-- Tarantool 2.7.1 > = +-------------------------------------------------------------------------= ------- > +local function backport_upgrade_2_7_1_function_access() > + local _func =3D box.space._func > + local _priv =3D box.space._priv > + local datetime =3D os.date("%Y-%m-%d %H:%M:%S") > + local funcs_to_change =3D {'LUA', 'box.schema.user.info'} > + for _, name in pairs(funcs_to_change) do > + local func =3D _func.index['name']:get(name) > + if func ~=3D nil and func.setuid ~=3D 0 then > + local id =3D 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 =3D func:update({{'=3D', 4, 0}, {'=3D', = 18, datetime}, > + {'=3D', 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 > + > = --------------------------------------------------------------------------= ------ >=20 > local handlers =3D { > @@ -985,6 +1014,7 @@ local handlers =3D { > {version =3D mkversion(2, 2, 1), func =3D upgrade_to_2_2_1, auto =3D= true}, > {version =3D mkversion(2, 3, 0), func =3D upgrade_to_2_3_0, auto =3D= true}, > {version =3D mkversion(2, 3, 1), func =3D upgrade_to_2_3_1, auto =3D= true}, > + {version =3D mkversion(2, 7, 1), func =3D upgrade_to_2_7_1, auto = =3D true}, > } >=20 > -- Schema version of the snapshot. > 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 =3D {} table.insert(r, = v:update({{"=3D", 18, ""}, {"=3D", 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. > +-- > +_ =3D box.schema.func.call("LUA", "return 1") > +--- > +... > +_ =3D box.schema.func.call("LUA", "return box.space._space:count()") > +--- > +... > +_ =3D box.schema.func.call("box.schema.user.info", 0) > +--- > +... > +_ =3D box.schema.func.call("box.schema.user.info", 1) > +--- > +... > +session.su('guest') > +--- > +... > +_ =3D box.schema.func.call("LUA", "return 1") > +--- > +... > +_ =3D box.schema.func.call("LUA", "return box.space._space:count()") > +--- > +- error: Read access to space '_space' is denied for user 'guest' > +... > +_ =3D box.schema.func.call("box.schema.user.info", 0) > +--- > +... > +_ =3D 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. > +-- > +_ =3D box.schema.func.call("LUA", "return 1") > +_ =3D box.schema.func.call("LUA", "return box.space._space:count()") > +_ =3D box.schema.func.call("box.schema.user.info", 0) > +_ =3D box.schema.func.call("box.schema.user.info", 1) > +session.su('guest') > +_ =3D box.schema.func.call("LUA", "return 1") > +_ =3D box.schema.func.call("LUA", "return box.space._space:count()") > +_ =3D box.schema.func.call("box.schema.user.info", 0) > +_ =3D box.schema.func.call("box.schema.user.info", 1) > +session.su('admin') > --=20 > 2.25.1 >=20