Thread: 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
Attachment
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.
>
> 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
Attachment
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], so I'll fix that as well. [1] https://www.postgresql.org/message-id/c6682a25f3f0e9bd520707342219eac5%40oss.nttdata.com Regards, -- Atsushi Torikoshi NTT DATA CORPORATION
On Wed, Jun 09, 2021 at 12:25:51AM +0900, torikoshia wrote: > BTW, I did the same thing in another patch I'm proposing[1], so I'll fix > that as well. Yes, it would be better to be consistent here. -- Michael