242 lines
10 KiB
Diff
242 lines
10 KiB
Diff
From f97ecea1ed7b09c6f1398540a1d72a57eee70c9f Mon Sep 17 00:00:00 2001
|
|
From: Noah Misch <noah@leadboat.com>
|
|
Date: Mon, 9 Nov 2020 07:32:09 -0800
|
|
Subject: [PATCH] In security-restricted operations, block enqueue of at-commit
|
|
user code.
|
|
|
|
Specifically, this blocks DECLARE ... WITH HOLD and firing of deferred
|
|
triggers within index expressions and materialized view queries. An
|
|
attacker having permission to create non-temp objects in at least one
|
|
schema could execute arbitrary SQL functions under the identity of the
|
|
bootstrap superuser. One can work around the vulnerability by disabling
|
|
autovacuum and not manually running ANALYZE, CLUSTER, REINDEX, CREATE
|
|
INDEX, VACUUM FULL, or REFRESH MATERIALIZED VIEW. (Don't restore from
|
|
pg_dump, since it runs some of those commands.) Plain VACUUM (without
|
|
FULL) is safe, and all commands are fine when a trusted user owns the
|
|
target object. Performance may degrade quickly under this workaround,
|
|
however. Back-patch to 9.5 (all supported versions).
|
|
|
|
Reviewed by Robert Haas. Reported by Etienne Stalmans.
|
|
|
|
Security: CVE-2020-25695
|
|
---
|
|
contrib/postgres_fdw/connection.c | 4 +++
|
|
src/backend/access/transam/xact.c | 13 ++++----
|
|
src/backend/commands/portalcmds.c | 5 +++
|
|
src/backend/commands/trigger.c | 12 +++++++
|
|
src/test/regress/expected/privileges.out | 42 ++++++++++++++++++++++++
|
|
src/test/regress/sql/privileges.sql | 34 +++++++++++++++++++
|
|
6 files changed, 104 insertions(+), 6 deletions(-)
|
|
|
|
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
|
|
index 885bd075798c..5dcff3d07624 100644
|
|
--- a/contrib/postgres_fdw/connection.c
|
|
+++ b/contrib/postgres_fdw/connection.c
|
|
@@ -645,6 +645,10 @@ pgfdw_report_error(int elevel, PGresult *res, PGconn *conn,
|
|
|
|
/*
|
|
* pgfdw_xact_callback --- cleanup at main-transaction end.
|
|
+ *
|
|
+ * This runs just late enough that it must not enter user-defined code
|
|
+ * locally. (Entering such code on the remote side is fine. Its remote
|
|
+ * COMMIT TRANSACTION may run deferred triggers.)
|
|
*/
|
|
static void
|
|
pgfdw_xact_callback(XactEvent event, void *arg)
|
|
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
|
|
index 37f31a2c31ea..00cec50e8402 100644
|
|
--- a/src/backend/access/transam/xact.c
|
|
+++ b/src/backend/access/transam/xact.c
|
|
@@ -1994,9 +1994,10 @@ CommitTransaction(void)
|
|
|
|
/*
|
|
* Do pre-commit processing that involves calling user-defined code, such
|
|
- * as triggers. Since closing cursors could queue trigger actions,
|
|
- * triggers could open cursors, etc, we have to keep looping until there's
|
|
- * nothing left to do.
|
|
+ * as triggers. SECURITY_RESTRICTED_OPERATION contexts must not queue an
|
|
+ * action that would run here, because that would bypass the sandbox.
|
|
+ * Since closing cursors could queue trigger actions, triggers could open
|
|
+ * cursors, etc, we have to keep looping until there's nothing left to do.
|
|
*/
|
|
for (;;)
|
|
{
|
|
@@ -2014,9 +2015,6 @@ CommitTransaction(void)
|
|
break;
|
|
}
|
|
|
|
- CallXactCallbacks(is_parallel_worker ? XACT_EVENT_PARALLEL_PRE_COMMIT
|
|
- : XACT_EVENT_PRE_COMMIT);
|
|
-
|
|
/*
|
|
* The remaining actions cannot call any user-defined code, so it's safe
|
|
* to start shutting down within-transaction services. But note that most
|
|
@@ -2024,6 +2022,9 @@ CommitTransaction(void)
|
|
* the transaction-abort path.
|
|
*/
|
|
|
|
+ CallXactCallbacks(is_parallel_worker ? XACT_EVENT_PARALLEL_PRE_COMMIT
|
|
+ : XACT_EVENT_PRE_COMMIT);
|
|
+
|
|
/* If we might have parallel workers, clean them up now. */
|
|
if (IsInParallelMode())
|
|
AtEOXact_Parallel(true);
|
|
diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c
|
|
index 46369cf3dbee..3d01a782da06 100644
|
|
--- a/src/backend/commands/portalcmds.c
|
|
+++ b/src/backend/commands/portalcmds.c
|
|
@@ -27,6 +27,7 @@
|
|
#include "commands/portalcmds.h"
|
|
#include "executor/executor.h"
|
|
#include "executor/tstoreReceiver.h"
|
|
+#include "miscadmin.h"
|
|
#include "rewrite/rewriteHandler.h"
|
|
#include "tcop/pquery.h"
|
|
#include "tcop/tcopprot.h"
|
|
@@ -64,6 +65,10 @@ PerformCursorOpen(DeclareCursorStmt *cstmt, ParamListInfo params,
|
|
*/
|
|
if (!(cstmt->options & CURSOR_OPT_HOLD))
|
|
RequireTransactionChain(isTopLevel, "DECLARE CURSOR");
|
|
+ else if (InSecurityRestrictedOperation())
|
|
+ ereport(ERROR,
|
|
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
|
|
+ errmsg("cannot create a cursor WITH HOLD within security-restricted operation")));
|
|
|
|
/*
|
|
* Parse analysis was done already, but we still have to run the rule
|
|
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
|
|
index f83840625348..9c04eee48422 100644
|
|
--- a/src/backend/commands/trigger.c
|
|
+++ b/src/backend/commands/trigger.c
|
|
@@ -4144,6 +4144,7 @@ afterTriggerMarkEvents(AfterTriggerEventList *events,
|
|
bool immediate_only)
|
|
{
|
|
bool found = false;
|
|
+ bool deferred_found = false;
|
|
AfterTriggerEvent event;
|
|
AfterTriggerEventChunk *chunk;
|
|
|
|
@@ -4179,6 +4180,7 @@ afterTriggerMarkEvents(AfterTriggerEventList *events,
|
|
*/
|
|
if (defer_it && move_list != NULL)
|
|
{
|
|
+ deferred_found = true;
|
|
/* add it to move_list */
|
|
afterTriggerAddEvent(move_list, event, evtshared);
|
|
/* mark original copy "done" so we don't do it again */
|
|
@@ -4186,6 +4188,16 @@ afterTriggerMarkEvents(AfterTriggerEventList *events,
|
|
}
|
|
}
|
|
|
|
+ /*
|
|
+ * We could allow deferred triggers if, before the end of the
|
|
+ * security-restricted operation, we were to verify that a SET CONSTRAINTS
|
|
+ * ... IMMEDIATE has fired all such triggers. For now, don't bother.
|
|
+ */
|
|
+ if (deferred_found && InSecurityRestrictedOperation())
|
|
+ ereport(ERROR,
|
|
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
|
|
+ errmsg("cannot fire deferred trigger within security-restricted operation")));
|
|
+
|
|
return found;
|
|
}
|
|
|
|
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
|
|
index dacc98450514..26ee16a0c370 100644
|
|
--- a/src/test/regress/expected/privileges.out
|
|
+++ b/src/test/regress/expected/privileges.out
|
|
@@ -1253,6 +1253,48 @@ SELECT has_table_privilege('regress_user1', 'atest4', 'SELECT WITH GRANT OPTION'
|
|
t
|
|
(1 row)
|
|
|
|
+-- security-restricted operations
|
|
+\c -
|
|
+CREATE ROLE regress_sro_user;
|
|
+SET SESSION AUTHORIZATION regress_sro_user;
|
|
+CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS
|
|
+ 'GRANT regress_group2 TO regress_sro_user';
|
|
+CREATE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS
|
|
+ 'DECLARE c CURSOR WITH HOLD FOR SELECT unwanted_grant(); SELECT true';
|
|
+-- REFRESH of this MV will queue a GRANT at end of transaction
|
|
+CREATE MATERIALIZED VIEW sro_mv AS SELECT mv_action() WITH NO DATA;
|
|
+REFRESH MATERIALIZED VIEW sro_mv;
|
|
+ERROR: cannot create a cursor WITH HOLD within security-restricted operation
|
|
+CONTEXT: SQL function "mv_action" statement 1
|
|
+\c -
|
|
+REFRESH MATERIALIZED VIEW sro_mv;
|
|
+ERROR: cannot create a cursor WITH HOLD within security-restricted operation
|
|
+CONTEXT: SQL function "mv_action" statement 1
|
|
+SET SESSION AUTHORIZATION regress_sro_user;
|
|
+-- INSERT to this table will queue a GRANT at end of transaction
|
|
+CREATE TABLE sro_trojan_table ();
|
|
+CREATE FUNCTION sro_trojan() RETURNS trigger LANGUAGE plpgsql AS
|
|
+ 'BEGIN PERFORM unwanted_grant(); RETURN NULL; END';
|
|
+CREATE CONSTRAINT TRIGGER t AFTER INSERT ON sro_trojan_table
|
|
+ INITIALLY DEFERRED FOR EACH ROW EXECUTE PROCEDURE sro_trojan();
|
|
+-- Now, REFRESH will issue such an INSERT, queueing the GRANT
|
|
+CREATE OR REPLACE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS
|
|
+ 'INSERT INTO sro_trojan_table DEFAULT VALUES; SELECT true';
|
|
+REFRESH MATERIALIZED VIEW sro_mv;
|
|
+ERROR: cannot fire deferred trigger within security-restricted operation
|
|
+CONTEXT: SQL function "mv_action" statement 1
|
|
+\c -
|
|
+REFRESH MATERIALIZED VIEW sro_mv;
|
|
+ERROR: cannot fire deferred trigger within security-restricted operation
|
|
+CONTEXT: SQL function "mv_action" statement 1
|
|
+BEGIN; SET CONSTRAINTS ALL IMMEDIATE; REFRESH MATERIALIZED VIEW sro_mv; COMMIT;
|
|
+ERROR: must have admin option on role "regress_group2"
|
|
+CONTEXT: SQL function "unwanted_grant" statement 1
|
|
+SQL statement "SELECT unwanted_grant()"
|
|
+PL/pgSQL function sro_trojan() line 1 at PERFORM
|
|
+SQL function "mv_action" statement 1
|
|
+DROP OWNED BY regress_sro_user;
|
|
+DROP ROLE regress_sro_user;
|
|
-- Admin options
|
|
SET SESSION AUTHORIZATION regress_user4;
|
|
CREATE FUNCTION dogrant_ok() RETURNS void LANGUAGE sql SECURITY DEFINER AS
|
|
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
|
|
index 4263315a2d87..f979cccea03f 100644
|
|
--- a/src/test/regress/sql/privileges.sql
|
|
+++ b/src/test/regress/sql/privileges.sql
|
|
@@ -761,6 +761,40 @@ SELECT has_table_privilege('regress_user3', 'atest4', 'SELECT'); -- false
|
|
SELECT has_table_privilege('regress_user1', 'atest4', 'SELECT WITH GRANT OPTION'); -- true
|
|
|
|
|
|
+-- security-restricted operations
|
|
+\c -
|
|
+CREATE ROLE regress_sro_user;
|
|
+
|
|
+SET SESSION AUTHORIZATION regress_sro_user;
|
|
+CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS
|
|
+ 'GRANT regress_group2 TO regress_sro_user';
|
|
+CREATE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS
|
|
+ 'DECLARE c CURSOR WITH HOLD FOR SELECT unwanted_grant(); SELECT true';
|
|
+-- REFRESH of this MV will queue a GRANT at end of transaction
|
|
+CREATE MATERIALIZED VIEW sro_mv AS SELECT mv_action() WITH NO DATA;
|
|
+REFRESH MATERIALIZED VIEW sro_mv;
|
|
+\c -
|
|
+REFRESH MATERIALIZED VIEW sro_mv;
|
|
+
|
|
+SET SESSION AUTHORIZATION regress_sro_user;
|
|
+-- INSERT to this table will queue a GRANT at end of transaction
|
|
+CREATE TABLE sro_trojan_table ();
|
|
+CREATE FUNCTION sro_trojan() RETURNS trigger LANGUAGE plpgsql AS
|
|
+ 'BEGIN PERFORM unwanted_grant(); RETURN NULL; END';
|
|
+CREATE CONSTRAINT TRIGGER t AFTER INSERT ON sro_trojan_table
|
|
+ INITIALLY DEFERRED FOR EACH ROW EXECUTE PROCEDURE sro_trojan();
|
|
+-- Now, REFRESH will issue such an INSERT, queueing the GRANT
|
|
+CREATE OR REPLACE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS
|
|
+ 'INSERT INTO sro_trojan_table DEFAULT VALUES; SELECT true';
|
|
+REFRESH MATERIALIZED VIEW sro_mv;
|
|
+\c -
|
|
+REFRESH MATERIALIZED VIEW sro_mv;
|
|
+BEGIN; SET CONSTRAINTS ALL IMMEDIATE; REFRESH MATERIALIZED VIEW sro_mv; COMMIT;
|
|
+
|
|
+DROP OWNED BY regress_sro_user;
|
|
+DROP ROLE regress_sro_user;
|
|
+
|
|
+
|
|
-- Admin options
|
|
|
|
SET SESSION AUTHORIZATION regress_user4;
|