Обсуждение: 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

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.

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], 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