[Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions
Mergen Imeev
imeevma at tarantool.org
Wed Dec 16 23:23:06 MSK 2020
Hi! Thank you for the review. My answer and patches for 2.7, 2.6 and 2.5
below.
On Sat, Dec 05, 2020 at 12:10:38AM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
>
> It looks good except for one comment below. Please, proceed to
> making it work on all versions, like we discussed.
>
> > 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.
>
> "function function"? Tbh, the entire comment looks unneeded. It
> just literally narrates the condition check. If you want to have
> a comment here, better explain what is wrong with having setuid
> set. Or nothing.
>
Removed.
> > + 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
Patch for 2.7:
>From 8c15cec7f73f0ba733b22e9e03e090c9e6f672eb Mon Sep 17 00:00:00 2001
Message-Id: <8c15cec7f73f0ba733b22e9e03e090c9e6f672eb.1608147417.git.imeevma at gmail.com>
From: Mergen Imeev <imeevma at gmail.com>
Date: Mon, 2 Nov 2020 13:21:58 +0300
Subject: [PATCH v1 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 -> 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(-)
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
+--------------------------------------------------------------------------------
+-- 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)
+ 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 handlers = {
@@ -985,6 +1014,7 @@ local handlers = {
{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},
}
-- 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 = {} 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
Patch for 2.6:
>From db5054f8781e0f438e7a29beb8e680afaffbb106 Mon Sep 17 00:00:00 2001
Message-Id: <db5054f8781e0f438e7a29beb8e680afaffbb106.1608148610.git.imeevma at gmail.com>
From: Mergen Imeev <imeevma at gmail.com>
Date: Mon, 2 Nov 2020 13:21:58 +0300
Subject: [PATCH v1 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.
Part of tarantool/security#1
---
src/box/bootstrap.snap | Bin 5976 -> 5997 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(-)
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index add791cd7..be1a0d47d 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
+--------------------------------------------------------------------------------
+-- Tarantool 2.6.2
+--------------------------------------------------------------------------------
+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)
+ 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_6_2()
+ backport_upgrade_2_7_1_function_access()
+end
+
--------------------------------------------------------------------------------
local function get_version()
@@ -1007,6 +1036,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, 6, 2), func = upgrade_to_2_6_2, auto = true},
}
for _, handler in ipairs(handlers) do
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index 0876e77a6..a371bb369 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, 6, 2]
...
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
Patch for 2.5:
>From e3c825d301bc699e2020cac861962aeb5b0758f1 Mon Sep 17 00:00:00 2001
Message-Id: <e3c825d301bc699e2020cac861962aeb5b0758f1.1608149922.git.imeevma at gmail.com>
From: Mergen Imeev <imeevma at gmail.com>
Date: Mon, 2 Nov 2020 13:21:58 +0300
Subject: [PATCH v1 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.
Part of tarantool/security#1
---
src/box/bootstrap.snap | Bin 5976 -> 5975 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(-)
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index add791cd7..e5ddd513f 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
+--------------------------------------------------------------------------------
+-- Tarantool 2.5.3
+--------------------------------------------------------------------------------
+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)
+ 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_5_3()
+ backport_upgrade_2_7_1_function_access()
+end
+
--------------------------------------------------------------------------------
local function get_version()
@@ -1007,6 +1036,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, 5, 3), func = upgrade_to_2_5_3, auto = true},
}
for _, handler in ipairs(handlers) do
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index 0876e77a6..9ebccc56d 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, 5, 3]
...
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