User-Id Tracking when Portal was started
The attached patch is delivered from the discussion around row-level
access control feature. A problem Florian pointed out is refcursor
declared in security definer function. Even though all the permission
checks are applied based on privilege of the owner of security-definer
function in case when it tries to define a cursor bound to a particular
query, it shall be executed under the credential of executor.
In the result, "current_user" or "has_table_privilege()" will return
unexpected result, even if it would be used in as a part of security
policy for each row.
This patch enables to switch user-id when user tried to execute
a portal being started under the different user's credential.
postgres=# CREATE FUNCTION f1(refcursor) RETURNS refcursor
postgres-# SECURITY DEFINER LANGUAGE plpgsql
postgres-# AS 'BEGIN OPEN $1 FOR SELECT current_user,* FROM t1;
postgres'# RETURN $1; END';
CREATE FUNCTION
postgres=# SET SESSION AUTHORIZATION alice;
SET
postgres=> BEGIN;
BEGIN
postgres=> SELECT f1('abc');
f1
-----
abc
(1 row)
postgres=> FETCH abc;
current_user | a | b
--------------+---+-----
kaigai | 1 | aaa <=== 'abc' was started under the
kaigai's privilege
(1 row)
postgres=> SELECT current_user;
current_user
--------------
alice
(1 row)
postgres=> DECLARE xyz CURSOR FOR SELECT current_user, * FROM t1;
DECLARE CURSOR
postgres=> FETCH xyz;
current_user | a | b
--------------+---+-----
alice | 1 | aaa
(1 row)
postgres=> RESET SESSION AUTHORIZATION;
RESET
postgres=# FETCH xyz; <=== 'xyz' was started under the
kaigai's privilege
current_user | a | b
--------------+---+-----
alice | 2 | bbb
(1 row)
BTW, same problem will be caused in case when security label of
the client would be switched. Probably, a hook should be injected
around the places where I patched with this patch.
Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
Attachments:
pgsql-v9.3-track-userid-of-portal.v1.patchapplication/octet-stream; name=pgsql-v9.3-track-userid-of-portal.v1.patchDownload
src/backend/tcop/pquery.c | 28 +++++++++-
src/include/utils/portal.h | 2 +
src/test/regress/expected/portals.out | 92 +++++++++++++++++++++++++++++++++
src/test/regress/sql/portals.sql | 72 ++++++++++++++++++++++++++
4 files changed, 193 insertions(+), 1 deletion(-)
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index d0db7ad..6251a38 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -488,6 +488,11 @@ PortalStart(Portal portal, ParamListInfo params,
portal->portalParams = params;
/*
+ * Must track user-id and security context when the portal is started.
+ */
+ GetUserIdAndSecContext(&portal->userId, &portal->secContext);
+
+ /*
* Determine the portal execution strategy
*/
portal->strategy = ChoosePortalStrategy(portal->stmts);
@@ -715,6 +720,8 @@ PortalRun(Portal portal, long count, bool isTopLevel,
ResourceOwner saveResourceOwner;
MemoryContext savePortalContext;
MemoryContext saveMemoryContext;
+ Oid saveUserId;
+ int saveSecContext;
AssertArg(PortalIsValid(portal));
@@ -760,10 +767,15 @@ PortalRun(Portal portal, long count, bool isTopLevel,
saveResourceOwner = CurrentResourceOwner;
savePortalContext = PortalContext;
saveMemoryContext = CurrentMemoryContext;
+ GetUserIdAndSecContext(&saveUserId, &saveSecContext);
PG_TRY();
{
ActivePortal = portal;
CurrentResourceOwner = portal->resowner;
+ if (portal->userId != GetUserId())
+ SetUserIdAndSecContext(portal->userId, portal->secContext);
+ else
+ saveUserId = InvalidOid;
PortalContext = PortalGetHeapMemory(portal);
MemoryContextSwitchTo(PortalContext);
@@ -844,6 +856,8 @@ PortalRun(Portal portal, long count, bool isTopLevel,
CurrentResourceOwner = TopTransactionResourceOwner;
else
CurrentResourceOwner = saveResourceOwner;
+ if (OidIsValid(saveUserId))
+ SetUserIdAndSecContext(saveUserId, saveSecContext);
PortalContext = savePortalContext;
PG_RE_THROW();
@@ -859,6 +873,8 @@ PortalRun(Portal portal, long count, bool isTopLevel,
CurrentResourceOwner = TopTransactionResourceOwner;
else
CurrentResourceOwner = saveResourceOwner;
+ if (OidIsValid(saveUserId))
+ SetUserIdAndSecContext(saveUserId, saveSecContext);
PortalContext = savePortalContext;
if (log_executor_stats && portal->strategy != PORTAL_MULTI_QUERY)
@@ -1391,6 +1407,8 @@ PortalRunFetch(Portal portal,
ResourceOwner saveResourceOwner;
MemoryContext savePortalContext;
MemoryContext oldContext;
+ Oid saveUserId;
+ int saveSecContext;
AssertArg(PortalIsValid(portal));
@@ -1409,12 +1427,16 @@ PortalRunFetch(Portal portal,
saveActivePortal = ActivePortal;
saveResourceOwner = CurrentResourceOwner;
savePortalContext = PortalContext;
+ GetUserIdAndSecContext(&saveUserId, &saveSecContext);
PG_TRY();
{
ActivePortal = portal;
CurrentResourceOwner = portal->resowner;
PortalContext = PortalGetHeapMemory(portal);
-
+ if (portal->userId != GetUserId())
+ SetUserIdAndSecContext(portal->userId, portal->secContext);
+ else
+ saveUserId = InvalidOid; /* skip restore */
oldContext = MemoryContextSwitchTo(PortalContext);
switch (portal->strategy)
@@ -1454,6 +1476,8 @@ PortalRunFetch(Portal portal,
/* Restore global vars and propagate error */
ActivePortal = saveActivePortal;
CurrentResourceOwner = saveResourceOwner;
+ if (OidIsValid(saveUserId))
+ SetUserIdAndSecContext(saveUserId, saveSecContext);
PortalContext = savePortalContext;
PG_RE_THROW();
@@ -1467,6 +1491,8 @@ PortalRunFetch(Portal portal,
ActivePortal = saveActivePortal;
CurrentResourceOwner = saveResourceOwner;
+ if (OidIsValid(saveUserId))
+ SetUserIdAndSecContext(saveUserId, saveSecContext);
PortalContext = savePortalContext;
return result;
diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h
index 4833942..7a4af0b 100644
--- a/src/include/utils/portal.h
+++ b/src/include/utils/portal.h
@@ -117,6 +117,8 @@ typedef struct PortalData
const char *prepStmtName; /* source prepared statement (NULL if none) */
MemoryContext heap; /* subsidiary memory for portal */
ResourceOwner resowner; /* resources owned by portal */
+ Oid userId; /* user-id to perform with this portal */
+ int secContext; /* security context with this portal */
void (*cleanup) (Portal portal); /* cleanup hook */
SubTransactionId createSubid; /* the ID of the creating subxact */
diff --git a/src/test/regress/expected/portals.out b/src/test/regress/expected/portals.out
index 01152a9..defa558 100644
--- a/src/test/regress/expected/portals.out
+++ b/src/test/regress/expected/portals.out
@@ -1,3 +1,14 @@
+-- Clean up in case a prior regression run failed
+-- Suppress NOTICE messages when users/groups don't exist
+SET client_min_messages TO 'warning';
+DROP ROLE IF EXISTS regtestuser_portal1;
+DROP ROLE IF EXISTS regtestuser_portal2;
+DROP ROLE IF EXISTS regtestuser_portal3;
+RESET client_min_messages;
+-- Declaration
+CREATE USER regtestuser_portal1;
+CREATE USER regtestuser_portal2;
+CREATE USER regtestuser_portal3;
--
-- Cursor regression tests
--
@@ -1257,3 +1268,84 @@ FETCH ALL FROM c1;
COMMIT;
DROP TABLE cursor;
+-- Make sure the cursor should perform with user's privilege when
+-- it was declared, even if it was switched later.
+CREATE TABLE cursor_t2 (a int, b text);
+GRANT ALL ON TABLE cursor_t2 TO regtestuser_portal2;
+INSERT INTO cursor_t2 VALUES (1,'aaa'), (2,'bbb'), (3,'ccc'), (4,'ddd');
+CREATE TABLE cursor_t3 (x int, y text);
+GRANT ALL ON TABLE cursor_t3 TO regtestuser_portal3;
+INSERT INTO cursor_t3 VALUES (10, 'xxx'), (11,'yyy'), (12,'zzz');
+SET SESSION AUTHORIZATION regtestuser_portal2;
+CREATE FUNCTION fcursor_scan2(refcursor) RETURNS refcursor
+ SECURITY DEFINER LANGUAGE plpgsql
+ AS 'BEGIN OPEN $1 FOR SELECT current_user,* FROM cursor_t2;
+ RETURN $1; END';
+SET SESSION AUTHORIZATION regtestuser_portal1;
+SELECT * FROM cursor_t2; -- failed, due to permission checks
+ERROR: permission denied for relation cursor_t2
+SELECT * FROM cursor_t3; -- failed, due to permission checks
+ERROR: permission denied for relation cursor_t3
+BEGIN;
+SELECT fcursor_scan2('abc');
+ fcursor_scan2
+---------------
+ abc
+(1 row)
+
+FETCH abc; -- should perform with privilege of regtestuser_portal2
+ current_user | a | b
+---------------------+---+-----
+ regtestuser_portal2 | 1 | aaa
+(1 row)
+
+SET SESSION AUTHORIZATION regtestuser_portal3;
+DECLARE xyz CURSOR FOR SELECT current_user,* FROM cursor_t3;
+FETCH abc; -- should perform with privilege of regtestuser_portal2
+ current_user | a | b
+---------------------+---+-----
+ regtestuser_portal2 | 2 | bbb
+(1 row)
+
+FETCH xyz; -- should perform with privilege of regtestuser_portal3
+ current_user | x | y
+---------------------+----+-----
+ regtestuser_portal3 | 10 | xxx
+(1 row)
+
+SET SESSION AUTHORIZATION regtestuser_portal1;
+FETCH abc; -- should perform with privilege of regtestuser_portal2
+ current_user | a | b
+---------------------+---+-----
+ regtestuser_portal2 | 3 | ccc
+(1 row)
+
+FETCH xyz; -- should perform with privilege of regtestuser_portal3
+ current_user | x | y
+---------------------+----+-----
+ regtestuser_portal3 | 11 | yyy
+(1 row)
+
+RESET SESSION AUTHORIZATION;
+FETCH abc; -- should perform with privilege of regtestuser_portal2
+ current_user | a | b
+---------------------+---+-----
+ regtestuser_portal2 | 4 | ddd
+(1 row)
+
+FETCH xyz; -- should perform with privilege of regtestuser_portal3
+ current_user | x | y
+---------------------+----+-----
+ regtestuser_portal3 | 12 | zzz
+(1 row)
+
+COMMIT;
+DROP TABLE cursor_t2;
+DROP TABLE cursor_t3;
+DROP FUNCTION fcursor_scan2(refcursor);
+--
+-- Clean-up
+--
+DROP USER regtestuser_portal1;
+DROP USER regtestuser_portal2;
+DROP USER regtestuser_portal3;
diff --git a/src/test/regress/sql/portals.sql b/src/test/regress/sql/portals.sql
index 02286c4..5a70a80 100644
--- a/src/test/regress/sql/portals.sql
+++ b/src/test/regress/sql/portals.sql
@@ -1,3 +1,19 @@
+-- Clean up in case a prior regression run failed
+
+-- Suppress NOTICE messages when users/groups don't exist
+SET client_min_messages TO 'warning';
+
+DROP ROLE IF EXISTS regtestuser_portal1;
+DROP ROLE IF EXISTS regtestuser_portal2;
+DROP ROLE IF EXISTS regtestuser_portal3;
+
+RESET client_min_messages;
+
+-- Declaration
+CREATE USER regtestuser_portal1;
+CREATE USER regtestuser_portal2;
+CREATE USER regtestuser_portal3;
+
--
-- Cursor regression tests
--
@@ -470,3 +486,59 @@ UPDATE cursor SET a = 2;
FETCH ALL FROM c1;
COMMIT;
DROP TABLE cursor;
+
+-- Make sure the cursor should perform with user's privilege when
+-- it was declared, even if it was switched later.
+
+CREATE TABLE cursor_t2 (a int, b text);
+GRANT ALL ON TABLE cursor_t2 TO regtestuser_portal2;
+INSERT INTO cursor_t2 VALUES (1,'aaa'), (2,'bbb'), (3,'ccc'), (4,'ddd');
+
+CREATE TABLE cursor_t3 (x int, y text);
+GRANT ALL ON TABLE cursor_t3 TO regtestuser_portal3;
+INSERT INTO cursor_t3 VALUES (10, 'xxx'), (11,'yyy'), (12,'zzz');
+
+SET SESSION AUTHORIZATION regtestuser_portal2;
+CREATE FUNCTION fcursor_scan2(refcursor) RETURNS refcursor
+ SECURITY DEFINER LANGUAGE plpgsql
+ AS 'BEGIN OPEN $1 FOR SELECT current_user,* FROM cursor_t2;
+ RETURN $1; END';
+
+SET SESSION AUTHORIZATION regtestuser_portal1;
+
+SELECT * FROM cursor_t2; -- failed, due to permission checks
+SELECT * FROM cursor_t3; -- failed, due to permission checks
+
+BEGIN;
+
+SELECT fcursor_scan2('abc');
+FETCH abc; -- should perform with privilege of regtestuser_portal2
+
+SET SESSION AUTHORIZATION regtestuser_portal3;
+
+DECLARE xyz CURSOR FOR SELECT current_user,* FROM cursor_t3;
+
+FETCH abc; -- should perform with privilege of regtestuser_portal2
+FETCH xyz; -- should perform with privilege of regtestuser_portal3
+
+SET SESSION AUTHORIZATION regtestuser_portal1;
+
+FETCH abc; -- should perform with privilege of regtestuser_portal2
+FETCH xyz; -- should perform with privilege of regtestuser_portal3
+
+RESET SESSION AUTHORIZATION;
+
+FETCH abc; -- should perform with privilege of regtestuser_portal2
+FETCH xyz; -- should perform with privilege of regtestuser_portal3
+
+COMMIT;
+DROP TABLE cursor_t2;
+DROP TABLE cursor_t3;
+DROP FUNCTION fcursor_scan2(refcursor);
+
+--
+-- Clean-up
+--
+DROP USER regtestuser_portal1;
+DROP USER regtestuser_portal2;
+DROP USER regtestuser_portal3;
On Mon, Jul 2, 2012 at 10:55 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
The attached patch is delivered from the discussion around row-level
access control feature. A problem Florian pointed out is refcursor
declared in security definer function. Even though all the permission
checks are applied based on privilege of the owner of security-definer
function in case when it tries to define a cursor bound to a particular
query, it shall be executed under the credential of executor.
In the result, "current_user" or "has_table_privilege()" will return
unexpected result, even if it would be used in as a part of security
policy for each row.
Why not just save and restore the user ID and security context
unconditionally, instead of doing this kind of dance?
+ if (portal->userId != GetUserId())
+ SetUserIdAndSecContext(portal->userId, portal->secCo
+ else
+ saveUserId = InvalidOid;
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
2012/7/3 Robert Haas <robertmhaas@gmail.com>:
On Mon, Jul 2, 2012 at 10:55 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
The attached patch is delivered from the discussion around row-level
access control feature. A problem Florian pointed out is refcursor
declared in security definer function. Even though all the permission
checks are applied based on privilege of the owner of security-definer
function in case when it tries to define a cursor bound to a particular
query, it shall be executed under the credential of executor.
In the result, "current_user" or "has_table_privilege()" will return
unexpected result, even if it would be used in as a part of security
policy for each row.Why not just save and restore the user ID and security context
unconditionally, instead of doing this kind of dance?+ if (portal->userId != GetUserId()) + SetUserIdAndSecContext(portal->userId, portal->secCo + else + saveUserId = InvalidOid;
In case when CurrentUserId was updated during the code block
(E.g, execution of SET SESSION AUTHORIZATION), it overwrites
this update on restoring user-id and security-context.
Comparison of user-id is a simple marker to check whether this
portal contain an operation to switch user-id, because these
operations are prohibited within security restricted context.
(Is there some other good criteria?)
Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
2012/7/3 Robert Haas <robertmhaas@gmail.com>:
Why not just save and restore the user ID and security context
unconditionally, instead of doing this kind of dance?+ if (portal->userId != GetUserId()) + SetUserIdAndSecContext(portal->userId, portal->secCo + else + saveUserId = InvalidOid;In case when CurrentUserId was updated during the code block
(E.g, execution of SET SESSION AUTHORIZATION), it overwrites
this update on restoring user-id and security-context.
Um... what should happen if there was a SET SESSION AUTHORIZATION
to the portal's userId? This test will think nothing happened.
regards, tom lane
2012/7/3 Tom Lane <tgl@sss.pgh.pa.us>:
Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
2012/7/3 Robert Haas <robertmhaas@gmail.com>:
Why not just save and restore the user ID and security context
unconditionally, instead of doing this kind of dance?+ if (portal->userId != GetUserId()) + SetUserIdAndSecContext(portal->userId, portal->secCo + else + saveUserId = InvalidOid;In case when CurrentUserId was updated during the code block
(E.g, execution of SET SESSION AUTHORIZATION), it overwrites
this update on restoring user-id and security-context.Um... what should happen if there was a SET SESSION AUTHORIZATION
to the portal's userId? This test will think nothing happened.
In my test, all the jobs by SET SESSION AUTHORIZATION was cleaned-up...
It makes nothing happen from viewpoint of users.
Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
2012/7/3 Tom Lane <tgl@sss.pgh.pa.us>:
Um... what should happen if there was a SET SESSION AUTHORIZATION
to the portal's userId? This test will think nothing happened.
In my test, all the jobs by SET SESSION AUTHORIZATION was cleaned-up...
It makes nothing happen from viewpoint of users.
My point is that it seems like a bug that the secContext gets restored
in one case and not the other, depending on which user ID was specified
in SET SESSION AUTHORIZATION.
regards, tom lane
2012/7/3 Tom Lane <tgl@sss.pgh.pa.us>:
Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
2012/7/3 Tom Lane <tgl@sss.pgh.pa.us>:
Um... what should happen if there was a SET SESSION AUTHORIZATION
to the portal's userId? This test will think nothing happened.In my test, all the jobs by SET SESSION AUTHORIZATION was cleaned-up...
It makes nothing happen from viewpoint of users.My point is that it seems like a bug that the secContext gets restored
in one case and not the other, depending on which user ID was specified
in SET SESSION AUTHORIZATION.
Sorry, the above description mention about a case when it does not use
the marker to distinguish a case to switch user-id from a case not to switch.
(I though I was asked the behavior if this logic always switches /
restores ids.)
The patch itself works correctly, no regression test failed even though
several tests switches user-id using SET SESSION AUTHORIZATION.
Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
On Tue, Jul 3, 2012 at 12:46 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
My point is that it seems like a bug that the secContext gets restored
in one case and not the other, depending on which user ID was specified
in SET SESSION AUTHORIZATION.Sorry, the above description mention about a case when it does not use
the marker to distinguish a case to switch user-id from a case not to switch.
(I though I was asked the behavior if this logic always switches /
restores ids.)The patch itself works correctly, no regression test failed even though
several tests switches user-id using SET SESSION AUTHORIZATION.
I don't believe that proves anything. There are lots of things that
aren't tested by the regression tests, and there's no guarantee that
any you've added cover all bases, either. We always treat user-ID and
security context as a unit; you haven't given any reason why this case
should be handled differently, and I bet it shouldn't.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
2012/7/4 Robert Haas <robertmhaas@gmail.com>:
On Tue, Jul 3, 2012 at 12:46 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
My point is that it seems like a bug that the secContext gets restored
in one case and not the other, depending on which user ID was specified
in SET SESSION AUTHORIZATION.Sorry, the above description mention about a case when it does not use
the marker to distinguish a case to switch user-id from a case not to switch.
(I though I was asked the behavior if this logic always switches /
restores ids.)The patch itself works correctly, no regression test failed even though
several tests switches user-id using SET SESSION AUTHORIZATION.I don't believe that proves anything. There are lots of things that
aren't tested by the regression tests, and there's no guarantee that
any you've added cover all bases, either. We always treat user-ID and
security context as a unit; you haven't given any reason why this case
should be handled differently, and I bet it shouldn't.
This patch always handles user-id and security context as a unit.
In case when it was switched, then it shall be always restored.
And, in case when it was not switched, then it shall never be restored.
Was my explanation confusing?
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
On Wed, Jul 4, 2012 at 4:24 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
2012/7/4 Robert Haas <robertmhaas@gmail.com>:
On Tue, Jul 3, 2012 at 12:46 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
My point is that it seems like a bug that the secContext gets restored
in one case and not the other, depending on which user ID was specified
in SET SESSION AUTHORIZATION.Sorry, the above description mention about a case when it does not use
the marker to distinguish a case to switch user-id from a case not to switch.
(I though I was asked the behavior if this logic always switches /
restores ids.)The patch itself works correctly, no regression test failed even though
several tests switches user-id using SET SESSION AUTHORIZATION.I don't believe that proves anything. There are lots of things that
aren't tested by the regression tests, and there's no guarantee that
any you've added cover all bases, either. We always treat user-ID and
security context as a unit; you haven't given any reason why this case
should be handled differently, and I bet it shouldn't.This patch always handles user-id and security context as a unit.
In case when it was switched, then it shall be always restored.
And, in case when it was not switched, then it shall never be restored.Was my explanation confusing?
It's not that your explanation is confusing; it's that it doesn't
match the code.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company