Re: [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions

[Vladislav Shpilevoy <v.shpilevoy@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)