From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 E23554765E0 for ; Wed, 23 Dec 2020 12:44:28 +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: <20201222174449.GA199429@tarantool.org> Date: Wed, 23 Dec 2020 12:44:27 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <2cc8e87a9cf72e326e0eacec99eb127a90b3f5e1.1608547708.git.imeevma@gmail.com> <6F4504F6-F009-4BE5-9FF2-097AEEC0992A@corp.mail.ru> <20201222174449.GA199429@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: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org Hi! LGTM. Sergos > On 22 Dec 2020, at 20:44, Mergen Imeev via Tarantool-patches = wrote: >=20 > Hi! Thank you for the review! My answer and new patch below. >=20 > On Mon, Dec 21, 2020 at 10:23:01PM +0300, Sergey Ostanevich wrote: >> Hi! >>=20 >> Thanks for the patch! >>=20 >> 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_access=E2=80=99 >> is enough? > Fixed. >=20 >>=20 >> Otherwise LGTM. >>=20 >> Sergos >>=20 >>=20 >>> 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 >>=20 >=20 >=20 > =46rom 21efff563826e47ea049aff2c7215b5bac5883e6 Mon Sep 17 00:00:00 = 2001 > Message-Id: = <21efff563826e47ea049aff2c7215b5bac5883e6.1608658852.git.imeevma@gmail.com= > > From: Mergen Imeev > Date: Mon, 2 Nov 2020 13:21:58 +0300 > Subject: [PATCH v1 1/1] box: remove unnecessary rights from peristent > functions >=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..c4a70297aad138d426a24ee4447af485= e3597536 100644 >=20 > diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua > index a86a0d410..6fba260bd 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 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() > + 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