Horton

Thread

Commits

  1. Reorder superuser check in pg_log_backend_memory_contexts()

  1. Misplaced superuser check in pg_log_backend_memory_contexts()

    Michael Paquier <michael@paquier.xyz> — 2021-06-06T06:53:10Z

    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

  2. Re: Misplaced superuser check in pg_log_backend_memory_contexts()

    Julien Rouhaud <rjuju123@gmail.com> — 2021-06-06T07:13:12Z

    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.

  3. Re: Misplaced superuser check in pg_log_backend_memory_contexts()

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-06-06T13:33:46Z

    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.

  4. Re: Misplaced superuser check in pg_log_backend_memory_contexts()

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-06T15:13:40Z

    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

  5. Re: Misplaced superuser check in pg_log_backend_memory_contexts()

    Michael Paquier <michael@paquier.xyz> — 2021-06-08T02:49:06Z

    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

  6. Re: Misplaced superuser check in pg_log_backend_memory_contexts()

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-06-08T14:30:00Z

    
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

  7. Re: Misplaced superuser check in pg_log_backend_memory_contexts()

    torikoshia <torikoshia@oss.nttdata.com> — 2021-06-08T15:25:51Z

    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

  8. Re: Misplaced superuser check in pg_log_backend_memory_contexts()

    Michael Paquier <michael@paquier.xyz> — 2021-06-09T01:37:53Z

    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