On Sat, 2021-10-23 at 20:42 +0000, Bossart, Nathan wrote:
> The predefined roles documentation notes
> that members of pg_signal_backend cannot signal superuser-owned
> backends, but AFAICT pg_log_backend_memory_contexts() has no such
> restriction at the moment. Should we add this?
Added, good catch.
> This is unrelated to this patch, but should we also consider opening
> up pg_reload_conf() and pg_rotate_logfile() to members of
> pg_signal_backend? Those are the other "server signaling functions"
> I
> see in the docs.
Those are actually signalling the postmaster, not an ordinary backend.
Also, those functions are already GRANTable, so I think we should leave
them as-is.
Regards,
Jeff Davis
From 3be296819d6195cbafce72ab085c8430f1e8a502 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Fri, 22 Oct 2021 13:40:35 -0700
Subject: [PATCH] Allow pg_signal_backend members to call
pg_log_backend_memory_contexts().
Discussion: https://postgr.es/m/[email protected]
---
doc/src/sgml/func.sgml | 5 +++--
src/backend/catalog/system_functions.sql | 5 +++++
src/backend/utils/adt/mcxtfuncs.c | 20 ++++++++---------
src/include/catalog/catversion.h | 2 +-
src/test/regress/expected/misc_functions.out | 23 ++++++++++++++++++--
src/test/regress/sql/misc_functions.sql | 15 +++++++++++--
6 files changed, 53 insertions(+), 17 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 5677032cb28..ee8903fe484 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25332,8 +25332,9 @@ SELECT collation for ('foo' COLLATE "de_DE");
(See <xref linkend="runtime-config-logging"/> for more information),
but will not be sent to the client regardless of
<xref linkend="guc-client-min-messages"/>.
- Only superusers can request to log the memory contexts.
- </para></entry>
+ This is also allowed if the calling role has been granted
+ <literal>pg_signal_backend</literal>, however only superusers can
+ cancel superuser backends. </para></entry>
</row>
<row>
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index a416e94d371..2cd5e3fe17b 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -699,6 +699,8 @@ REVOKE EXECUTE ON FUNCTION pg_ls_dir(text) FROM public;
REVOKE EXECUTE ON FUNCTION pg_ls_dir(text,boolean,boolean) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer) FROM PUBLIC;
+
--
-- We also set up some things as accessible to standard roles.
--
@@ -713,6 +715,9 @@ GRANT EXECUTE ON FUNCTION pg_ls_tmpdir() TO pg_monitor;
GRANT EXECUTE ON FUNCTION pg_ls_tmpdir(oid) TO pg_monitor;
+GRANT EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer)
+ TO pg_signal_backend;
+
GRANT pg_read_all_settings TO pg_monitor;
GRANT pg_read_all_stats TO pg_monitor;
diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
index 0d52613bc32..a2f45fb8578 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -162,10 +162,10 @@ pg_get_backend_memory_contexts(PG_FUNCTION_ARGS)
* pg_log_backend_memory_contexts
* Signal a backend process to log its memory contexts.
*
- * Only superusers are allowed to signal to log the memory contexts
- * because allowing any users to issue this request at an unbounded
- * rate would cause lots of log messages and which can lead to
- * denial of service.
+ * Only members of pg_signal_backend are allowed to signal to log the memory
+ * contexts because allowing any users to issue this request at an unbounded
+ * rate would cause lots of log messages and which can lead to denial of
+ * service.
*
* On receipt of this signal, a backend sets the flag in the signal
* handler, which causes the next CHECK_FOR_INTERRUPTS() to log the
@@ -177,12 +177,6 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)
int pid = PG_GETARG_INT32(0);
PGPROC *proc;
- /* Only allow superusers to log memory contexts. */
- if (!superuser())
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a superuser to log memory contexts")));
-
proc = BackendPidGetProc(pid);
/*
@@ -205,6 +199,12 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(false);
}
+ /* Only allow superusers to signal superuser-owned backends. */
+ if (superuser_arg(proc->roleId) && !superuser())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("must be a superuser to log memory contexts of superuser backend")));
+
if (SendProcSignal(pid, PROCSIG_LOG_MEMORY_CONTEXT, proc->backendId) < 0)
{
/* Again, just a warning to allow loops */
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 3253b8751b1..039f6338604 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
*/
/* yyyymmddN */
-#define CATALOG_VERSION_NO 202109101
+#define CATALOG_VERSION_NO 202110230
#endif
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index e845042d38d..a279c164020 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -138,14 +138,33 @@ HINT: No function matches the given name and argument types. You might need to
--
-- Memory contexts are logged and they are not returned to the function.
-- Furthermore, their contents can vary depending on the timing. However,
--- we can at least verify that the code doesn't fail.
+-- we can at least verify that the code doesn't fail, and that the
+-- permissions are set properly.
--
-SELECT * FROM pg_log_backend_memory_contexts(pg_backend_pid());
+SELECT pg_log_backend_memory_contexts(pg_backend_pid());
pg_log_backend_memory_contexts
--------------------------------
t
(1 row)
+CREATE ROLE regress_signal IN ROLE pg_signal_backend;
+CREATE ROLE regress_nosignal;
+SELECT has_function_privilege('regress_signal',
+ 'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- yes
+ has_function_privilege
+------------------------
+ t
+(1 row)
+
+SELECT has_function_privilege('regress_nosignal',
+ 'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- no
+ has_function_privilege
+------------------------
+ f
+(1 row)
+
+DROP ROLE regress_signal;
+DROP ROLE regress_nosignal;
--
-- Test some built-in SRFs
--
diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql
index a398349afc6..0dd8998b62c 100644
--- a/src/test/regress/sql/misc_functions.sql
+++ b/src/test/regress/sql/misc_functions.sql
@@ -35,9 +35,20 @@ SELECT num_nulls();
--
-- Memory contexts are logged and they are not returned to the function.
-- Furthermore, their contents can vary depending on the timing. However,
--- we can at least verify that the code doesn't fail.
+-- we can at least verify that the code doesn't fail, and that the
+-- permissions are set properly.
--
-SELECT * FROM pg_log_backend_memory_contexts(pg_backend_pid());
+
+SELECT pg_log_backend_memory_contexts(pg_backend_pid());
+
+CREATE ROLE regress_signal IN ROLE pg_signal_backend;
+CREATE ROLE regress_nosignal;
+SELECT has_function_privilege('regress_signal',
+ 'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- yes
+SELECT has_function_privilege('regress_nosignal',
+ 'pg_log_backend_memory_contexts(integer)', 'EXECUTE'); -- no
+DROP ROLE regress_signal;
+DROP ROLE regress_nosignal;
--
-- Test some built-in SRFs
--
2.17.1
