From 56438fa62681446d09ac265109e5a2c2ac6df0e5 Mon Sep 17 00:00:00 2001
From: Konstantin Osipov <kostja@tarantool.org>
Date: Thu, 28 Dec 2017 20:26:12 +0300
Subject: [PATCH] security: add a test case fog gh-3023
box.session.su() changes both user and effective user right now.
Changing only the session user seems to be rather difficult:
we need to keep the object allocated somewhere, and keeping
in mind request multiplexor in iproto, with which many requests
can share the same session, it can only be Lua stack.
While at it, change current_user() to effective_user() to
make it less ambiguous.
---
src/box/alter.cc | 2 +-
src/box/box.cc | 4 ++--
src/box/call.cc | 4 ++--
src/box/lua/session.c | 7 ++++---
src/box/sequence.c | 2 +-
src/box/session.h | 6 +++---
src/box/space.c | 2 +-
src/box/sysview_index.c | 8 ++++----
src/box/user.cc | 2 +-
test/box/access.result | 22 ++++++++++++++++++++++
test/box/access.test.lua | 9 +++++++++
11 files changed, 50 insertions(+), 18 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 7df8e0ba6e..061647c4fe 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -63,7 +63,7 @@
static void
access_check_ddl(uint32_t owner_uid, enum schema_object_type type)
{
- struct credentials *cr = current_user();
+ struct credentials *cr = effective_user();
/*
* Only the owner of the object can be the grantor
* of the privilege on the object. This means that
diff --git a/src/box/box.cc b/src/box/box.cc
index bf46bdc30b..eb443c7441 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1008,7 +1008,7 @@ sequence_data_update(uint32_t seq_id, int64_t value)
mp_encode_uint(tuple_buf_end, value));
assert(tuple_buf_end < tuple_buf + tuple_buf_size);
- struct credentials *orig_credentials = current_user();
+ struct credentials *orig_credentials = effective_user();
fiber_set_user(fiber(), &admin_credentials);
int rc = box_replace(BOX_SEQUENCE_DATA_ID,
@@ -1033,7 +1033,7 @@ sequence_data_delete(uint32_t seq_id)
key_buf_end = mp_encode_uint(key_buf_end, seq_id);
assert(key_buf_end < key_buf + key_buf_size);
- struct credentials *orig_credentials = current_user();
+ struct credentials *orig_credentials = effective_user();
fiber_set_user(fiber(), &admin_credentials);
int rc = box_delete(BOX_SEQUENCE_DATA_ID, 0,
diff --git a/src/box/call.cc b/src/box/call.cc
index b5f4c4fa8a..4d74160b0b 100644
--- a/src/box/call.cc
+++ b/src/box/call.cc
@@ -59,7 +59,7 @@ static inline int
access_check_func(const char *name, uint32_t name_len, struct func **funcp)
{
struct func *func = func_by_name(name, name_len);
- struct credentials *credentials = current_user();
+ struct credentials *credentials = effective_user();
/*
* If the user has universal access, don't bother with checks.
* No special check for ADMIN user is necessary
@@ -191,7 +191,7 @@ box_process_call(struct call_request *request, struct obuf *out)
*/
struct credentials *orig_credentials = NULL;
if (func && func->def->setuid) {
- orig_credentials = current_user();
+ orig_credentials = effective_user();
/* Remember and change the current user id. */
if (func->owner_credentials.auth_token >= BOX_USER_MAX) {
/*
diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index 2538c543d5..3238221ef4 100644
--- a/src/box/lua/session.c
+++ b/src/box/lua/session.c
@@ -101,7 +101,7 @@ lbox_session_sync(struct lua_State *L)
/**
* Session effective user id.
- * Note: user id (current_user()->uid)
+ * Note: user id (effective_user()->uid)
* may be different in a setuid function.
*/
static int
@@ -123,7 +123,7 @@ lbox_session_euid(struct lua_State *L)
static int
lbox_session_uid(struct lua_State *L)
{
- lua_pushnumber(L, current_user()->uid);
+ lua_pushnumber(L, effective_user()->uid);
return 1;
}
@@ -135,7 +135,7 @@ lbox_session_uid(struct lua_State *L)
static int
lbox_session_user(struct lua_State *L)
{
- struct user *user = user_by_id(current_user()->uid);
+ struct user *user = user_by_id(effective_user()->uid);
if (user)
lua_pushstring(L, user->def->name);
else
@@ -179,6 +179,7 @@ lbox_session_su(struct lua_State *L)
/* sudo */
luaL_checktype(L, 2, LUA_TFUNCTION);
int error = lua_pcall(L, top - 2, LUA_MULTRET, 0);
+ /* Restore the original credentials. */
fiber_set_user(fiber(), &session->credentials);
if (error)
diff --git a/src/box/sequence.c b/src/box/sequence.c
index cd3ade6596..c7d7056dbd 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -241,7 +241,7 @@ sequence_next(struct sequence *seq, int64_t *result)
int
access_check_sequence(struct sequence *seq)
{
- struct credentials *cr = current_user();
+ struct credentials *cr = effective_user();
/*
* If the user has universal access, don't bother with checks.
* No special check for ADMIN user is necessary since ADMIN has
diff --git a/src/box/session.h b/src/box/session.h
index 6864c6e045..29efe2b092 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -87,7 +87,7 @@ struct session {
enum session_type type;
/** Authentication salt. */
char salt[SESSION_SEED_SIZE];
- /** Cached user id and global grants */
+ /** Session user id and global grants */
struct credentials credentials;
/** Trigger for fiber on_stop to cleanup created on-demand session */
struct trigger fiber_on_stop;
@@ -184,7 +184,7 @@ current_session()
* user on demand as in current_session() applies.
*/
static inline struct credentials *
-current_user()
+effective_user()
{
struct credentials *u =
(struct credentials *) fiber_get_key(fiber(),
@@ -247,7 +247,7 @@ session_run_on_auth_triggers(const char *user_name);
static inline void
access_check_universe(uint8_t access)
{
- struct credentials *credentials = current_user();
+ struct credentials *credentials = effective_user();
if (!(credentials->universal_access & access)) {
/*
* Access violation, report error.
diff --git a/src/box/space.c b/src/box/space.c
index 703270361f..359728aaf0 100644
--- a/src/box/space.c
+++ b/src/box/space.c
@@ -43,7 +43,7 @@
int
access_check_space(struct space *space, uint8_t access)
{
- struct credentials *cr = current_user();
+ struct credentials *cr = effective_user();
/*
* If a user has a global permission, clear the respective
* privilege from the list of privileges required
diff --git a/src/box/sysview_index.c b/src/box/sysview_index.c
index 27d3d55ee3..a76a67e444 100644
--- a/src/box/sysview_index.c
+++ b/src/box/sysview_index.c
@@ -183,7 +183,7 @@ static const struct index_vtab sysview_index_vtab = {
static bool
vspace_filter(struct space *source, struct tuple *tuple)
{
- struct credentials *cr = current_user();
+ struct credentials *cr = effective_user();
if (PRIV_R & cr->universal_access)
return true; /* read access to unverse */
if (PRIV_R & source->access[cr->auth_token].effective)
@@ -203,7 +203,7 @@ vspace_filter(struct space *source, struct tuple *tuple)
static bool
vuser_filter(struct space *source, struct tuple *tuple)
{
- struct credentials *cr = current_user();
+ struct credentials *cr = effective_user();
if (PRIV_R & cr->universal_access)
return true; /* read access to unverse */
if (PRIV_R & source->access[cr->auth_token].effective)
@@ -221,7 +221,7 @@ vuser_filter(struct space *source, struct tuple *tuple)
static bool
vpriv_filter(struct space *source, struct tuple *tuple)
{
- struct credentials *cr = current_user();
+ struct credentials *cr = effective_user();
if (PRIV_R & cr->universal_access)
return true; /* read access to unverse */
if (PRIV_R & source->access[cr->auth_token].effective)
@@ -239,7 +239,7 @@ vpriv_filter(struct space *source, struct tuple *tuple)
static bool
vfunc_filter(struct space *source, struct tuple *tuple)
{
- struct credentials *cr = current_user();
+ struct credentials *cr = effective_user();
if ((PRIV_R | PRIV_X) & cr->universal_access)
return true; /* read or execute access to unverse */
if (PRIV_R & source->access[cr->auth_token].effective)
diff --git a/src/box/user.cc b/src/box/user.cc
index d16695fbee..a0cbf4e981 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -242,7 +242,7 @@ access_find(struct priv_def *priv)
static void
user_set_effective_access(struct user *user)
{
- struct credentials *cr = current_user();
+ struct credentials *cr = effective_user();
struct privset_iterator it;
privset_ifirst(&user->privs, &it);
struct priv_def *priv;
diff --git a/test/box/access.result b/test/box/access.result
index ed8bf43649..7072de75d3 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -908,3 +908,25 @@ box.schema.user.drop('test_user')
box.schema.role.drop('test_role')
---
...
+-- gh-3023: box.session.su() changes both authenticated and effective
+-- user, while should only change the effective user
+--
+function uids() return { uid = box.session.uid(), euid = box.session.euid() } end
+---
+...
+box.session.su('guest')
+---
+...
+uids()
+---
+- uid: 0
+ euid: 0
+...
+box.session.su('admin')
+---
+...
+box.session.su('guest', uids)
+---
+- uid: 0
+ euid: 1
+...
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index 69217f6ba4..c353aad00b 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -347,3 +347,12 @@ box.schema.role.info('test_role')
box.schema.user.drop('test_user')
box.schema.role.drop('test_role')
+
+-- gh-3023: box.session.su() changes both authenticated and effective
+-- user, while should only change the effective user
+--
+function uids() return { uid = box.session.uid(), euid = box.session.euid() } end
+box.session.su('guest')
+uids()
+box.session.su('admin')
+box.session.su('guest', uids)
--
GitLab