Misplaced superuser check in pg_log_backend_memory_contexts()
Hi all,
While reading the code of pg_log_backend_memory_contexts(), I have
been surprised to see that the code would attempt to look at a PROC
entry based on the given input PID *before* checking if the function
has been called by a superuser. This does not strike me as a good
idea as this allows any users to call this function and to take
ProcArrayLock in shared mode, freely.
It seems to me that we had better check for a superuser at the
beginning of the function, like in the attached.
Thanks,
--
Michael
Attachments:
log-backend-superuser.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
index 2984768d19..0d52613bc3 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -175,7 +175,15 @@ Datum
pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)
{
int pid = PG_GETARG_INT32(0);
- PGPROC *proc = BackendPidGetProc(pid);
+ 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);
/*
* BackendPidGetProc returns NULL if the pid isn't valid; but by the time
@@ -197,12 +205,6 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(false);
}
- /* Only allow superusers to log memory contexts. */
- if (!superuser())
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a superuser to log memory contexts")));
-
if (SendProcSignal(pid, PROCSIG_LOG_MEMORY_CONTEXT, proc->backendId) < 0)
{
/* Again, just a warning to allow loops */
On Sun, Jun 06, 2021 at 03:53:10PM +0900, Michael Paquier wrote:
While reading the code of pg_log_backend_memory_contexts(), I have
been surprised to see that the code would attempt to look at a PROC
entry based on the given input PID *before* checking if the function
has been called by a superuser. This does not strike me as a good
idea as this allows any users to call this function and to take
ProcArrayLock in shared mode, freely.
It doesn't seem like a huge problem as at least GetSnapshotData also acquires
ProcArrayLock in shared mode. Knowing if a specific pid is a postgres backend
or not isn't privileged information either, and anyone can check that using
pg_stat_activity as an unprivileged user (which will also acquire ProcArrayLock
in shared mode).
It seems to me that we had better check for a superuser at the
beginning of the function, like in the attached.
However +1 for the patch, as it seems more consistent to always get a
permission failure if you're not a superuser.
On Sun, Jun 6, 2021 at 12:23 PM Michael Paquier <michael@paquier.xyz> wrote:
Hi all,
While reading the code of pg_log_backend_memory_contexts(), I have
been surprised to see that the code would attempt to look at a PROC
entry based on the given input PID *before* checking if the function
has been called by a superuser. This does not strike me as a good
idea as this allows any users to call this function and to take
ProcArrayLock in shared mode, freely.It seems to me that we had better check for a superuser at the
beginning of the function, like in the attached.
pg_signal_backend still locks ProcArrayLock in shared mode first and then
checks for the superuser permissions. Of course, it does that for the
roleId i.e. superuser_arg(proc->roleId), but there's also superuser() check.
With Regards,
Bharath Rupireddy.
Julien Rouhaud <rjuju123@gmail.com> writes:
On Sun, Jun 06, 2021 at 03:53:10PM +0900, Michael Paquier wrote:
It seems to me that we had better check for a superuser at the
beginning of the function, like in the attached.
However +1 for the patch, as it seems more consistent to always get a
permission failure if you're not a superuser.
Yeah, it's just weird if such a check is not the first thing
in the function. Even if you can convince yourself that the
actions taken before that don't create any security issue today,
it's not hard to imagine that innocent future code rearrangements
could break that argument. What's the value of postponing the
check anyway?
regards, tom lane
On Sun, Jun 06, 2021 at 11:13:40AM -0400, Tom Lane wrote:
Julien Rouhaud <rjuju123@gmail.com> writes:
However +1 for the patch, as it seems more consistent to always get a
permission failure if you're not a superuser.Yeah, it's just weird if such a check is not the first thing
in the function. Even if you can convince yourself that the
actions taken before that don't create any security issue today,
it's not hard to imagine that innocent future code rearrangements
could break that argument. What's the value of postponing the
check anyway?
Thanks for the input, I have applied the patch.
--
Michael
On 2021/06/08 11:49, Michael Paquier wrote:
On Sun, Jun 06, 2021 at 11:13:40AM -0400, Tom Lane wrote:
Julien Rouhaud <rjuju123@gmail.com> writes:
However +1 for the patch, as it seems more consistent to always get a
permission failure if you're not a superuser.Yeah, it's just weird if such a check is not the first thing
in the function. Even if you can convince yourself that the
actions taken before that don't create any security issue today,
it's not hard to imagine that innocent future code rearrangements
could break that argument. What's the value of postponing the
check anyway?Thanks for the input, I have applied the patch.
Thanks a lot!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2021-06-08 11:49, Michael Paquier wrote:
On Sun, Jun 06, 2021 at 11:13:40AM -0400, Tom Lane wrote:
Julien Rouhaud <rjuju123@gmail.com> writes:
However +1 for the patch, as it seems more consistent to always get a
permission failure if you're not a superuser.Yeah, it's just weird if such a check is not the first thing
in the function. Even if you can convince yourself that the
actions taken before that don't create any security issue today,
it's not hard to imagine that innocent future code rearrangements
could break that argument. What's the value of postponing the
check anyway?Thanks for the input, I have applied the patch.
Thanks for your modification!
BTW, I did the same thing in another patch I'm proposing[1]/messages/by-id/c6682a25f3f0e9bd520707342219eac5@oss.nttdata.com, so I'll fix
that as well.
[1]: /messages/by-id/c6682a25f3f0e9bd520707342219eac5@oss.nttdata.com
/messages/by-id/c6682a25f3f0e9bd520707342219eac5@oss.nttdata.com
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION