[Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions
Mergen Imeev
imeevma at tarantool.org
Thu Dec 3 12:54:23 MSK 2020
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 at 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 at gmail.com>
From: Mergen Imeev <imeevma at gmail.com>
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
More information about the Tarantool-patches
mailing list