Re: pg_signal_backend() asymmetry

Hi all,

I have one nitpick related to the recent changes for
pg_cancel_backend() and pg_terminate_backend(). If you use these
functions as an unprivileged user, and try to signal a nonexistent
PID, you get:

ERROR: must be superuser or have the same role to cancel queries
running in other server processes

Whereas if you do the same thing as a superuser, you get:

WARNING: PID 123 is not a PostgreSQL server process
pg_cancel_backend
-------------------
f
(1 row)

The comment above the WARNING generated for the latter case says:

/*
* This is just a warning so a loop-through-resultset
will not abort
* if one backend terminated on its own during the run
*/

which is nice, but wouldn't unprivileged users want the same behavior?
Not to mention, the ERROR above is misleading, as it claims the
nonexistent PID really belongs to another user.

A simple fix is attached. The existing code called both
BackendPidGetProc(pid) and IsBackendPid(pid) while checking a
non-superuser's permissions, which really means two separate calls to
BackendPidGetProc(pid). I simplified that to down to a single
BackendPidGetProc(pid) call.

Josh

On Wed, Jun 27, 2012 at 5:38 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> Hi all,
>
> I have one nitpick related to the recent changes for
> pg_cancel_backend() and pg_terminate_backend(). If you use these
> functions as an unprivileged user, and try to signal a nonexistent
> PID, you get:

I think the goal there is to avoid leakage of the knowledge or
non-knowledge of a given PID existing once it is deemed out of
Postgres' control. Although I don't have a specific attack vector in
mind for when one knows a PID exists a-priori, it does seem like an
unnecessary admission on the behalf of other programs.

Also, in pg_cancel_backend et al, PID really means "database session",
but as-is the marrying of PID and session is one of convenience, so I
think any message that communicates more than "that database session
does not exist" is superfluous anyhow. Perhaps there is a better
wording for the time being that doesn't implicate the existence or
non-existence of the PID?

--
fdr

On Thu, Jun 28, 2012 at 10:36 AM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
> On Wed, Jun 27, 2012 at 5:38 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
>> Hi all,
>>
>> I have one nitpick related to the recent changes for
>> pg_cancel_backend() and pg_terminate_backend(). If you use these
>> functions as an unprivileged user, and try to signal a nonexistent
>> PID, you get:
>
> I think the goal there is to avoid leakage of the knowledge or
> non-knowledge of a given PID existing once it is deemed out of
> Postgres' control.  Although I don't have a specific attack vector in
> mind for when one knows a PID exists a-priori, it does seem like an
> unnecessary admission on the behalf of other programs.

Well, there are two sides to it - one is the message, the other is if
it should be ERROR or WARNING. My reading is that Josh is suggesting
we make them WARNING in both cases, right?

It should be possible to make it a WARNING without leaking information
in the error message..

> Also, in pg_cancel_backend et al, PID really means "database session",
> but as-is the marrying of PID and session is one of convenience, so I
> think any message that communicates more than "that database session
> does not exist" is superfluous anyhow.  Perhaps there is a better
> wording for the time being that doesn't implicate the existence or
> non-existence of the PID?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Thu, Jun 28, 2012 at 01:36:49AM -0700, Daniel Farina wrote:
> On Wed, Jun 27, 2012 at 5:38 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> > I have one nitpick related to the recent changes for
> > pg_cancel_backend() and pg_terminate_backend(). If you use these
> > functions as an unprivileged user, and try to signal a nonexistent
> > PID, you get:
>
> I think the goal there is to avoid leakage of the knowledge or
> non-knowledge of a given PID existing once it is deemed out of
> Postgres' control. Although I don't have a specific attack vector in
> mind for when one knows a PID exists a-priori, it does seem like an
> unnecessary admission on the behalf of other programs.

I think it was just an oversight. I agree that these functions have no
business helping users probe for live non-PostgreSQL PIDs on the server, but
they don't do so and Josh's patch won't change that. I recommend committing
the patch. Users will be able to probe for live PostgreSQL PIDs, but
pg_stat_activity already provides those.

> Also, in pg_cancel_backend et al, PID really means "database session",
> but as-is the marrying of PID and session is one of convenience, so I
> think any message that communicates more than "that database session
> does not exist" is superfluous anyhow. Perhaps there is a better
> wording for the time being that doesn't implicate the existence or
> non-existence of the PID?

Perhaps, though I'm not coming up with anything. The message isn't wrong; the
value is a PID independent of whether some process has that PID.

Thanks,
nm

On Thu, Jun 28, 2012 at 6:48 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Thu, Jun 28, 2012 at 01:36:49AM -0700, Daniel Farina wrote:
>> On Wed, Jun 27, 2012 at 5:38 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
>> > I have one nitpick related to the recent changes for
>> > pg_cancel_backend() and pg_terminate_backend(). If you use these
>> > functions as an unprivileged user, and try to signal a nonexistent
>> > PID, you get:
>>
>> I think the goal there is to avoid leakage of the knowledge or
>> non-knowledge of a given PID existing once it is deemed out of
>> Postgres' control.  Although I don't have a specific attack vector in
>> mind for when one knows a PID exists a-priori, it does seem like an
>> unnecessary admission on the behalf of other programs.
>
> I think it was just an oversight.  I agree that these functions have no
> business helping users probe for live non-PostgreSQL PIDs on the server, but
> they don't do so and Josh's patch won't change that.  I recommend committing
> the patch.  Users will be able to probe for live PostgreSQL PIDs, but
> pg_stat_activity already provides those.

Yeah, I figured that since pg_stat_activity already shows users PIDs,
there should be no need to have pg_signal_backend() lie about whether
a PID was a valid backend or not. And if for some reason we did want
to lie, we should give an accurate message like "not valid backend, or
must be superuser or have the same role...".

I noticed that I neglected to update the comment which was in the
non-superuser case: updated version attached.

On Thu, Jun 28, 2012 at 5:22 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> Well, there are two sides to it - one is the message, the other is if
> it should be ERROR or WARNING. My reading is that Josh is suggesting
> we make them WARNING in both cases, right?

Maybe I should have said I had "a few" nitpicks instead of just "one"
:-) A more detailed summary of the little issues I'm aiming to fix:

1a.) SIGNAL_BACKEND_NOPERMISSION being used for both invalid PID and
role mismatch, causing the "must be superuser or have ..." message to
be printed in both cases for non-superusers

1b.) Since SIGNAL_BACKEND_NOPERMISSION is used for both duties, you'll
get an ERROR instead of just a WARNING during plausibly-normal usage.
For example, if you're running
SELECT pg_cancel_backend(pid)
FROM pg_stat_activity
WHERE usename = CURRENT_USER AND
pid != pg_backend_pid();

you might be peeved if it ERROR'ed out with the bogus message claiming
the process was owned by someone else, when the backend has just
exited on its own. So even if we thought it was worth hiding to
non-superusers whether the PID is invalid or belongs to someone else,
I think the message should just be at WARNING level.

2a.) Existing code uses two calls to BackendPidGetProc() for
non-superusers in the error-free case.

2b.) I think the comment "the check for whether it's a backend process
is below" is a little misleading, since IsBackendPid() is just
checking whether the return of BackendPidGetProc() is NULL, which has
already been done.

3.) grammar fix for comment ("on it's own" -> "on its own")

Josh

I'm marking this patch Ready for Committer. I suggest backpatching it to 9.2;
the patch corrects an oversight in 9.2 changes. There's more compatibility
value in backpatching than in retaining distinct behavior for 9.2 only.

On Thu, Jun 28, 2012 at 09:32:41AM -0700, Josh Kupershmidt wrote:
> ! if (!superuser())
> {
> /*
> ! * Since the user is not superuser, check for matching roles.
> */
> ! if (proc->roleId != GetUserId())
> ! return SIGNAL_BACKEND_NOPERMISSION;
> }

I would have collapsed the conditionals and deleted the obvious comment:

if (!(superuser() || proc->roleId == GetUserId()))
return SIGNAL_BACKEND_NOPERMISSION;

The committer can do that if desired; no need for another version.

Thanks,
nm

Excerpts from Noah Misch's message of mié sep 26 17:54:43 -0300 2012:
> I'm marking this patch Ready for Committer. I suggest backpatching it to 9.2;
> the patch corrects an oversight in 9.2 changes. There's more compatibility
> value in backpatching than in retaining distinct behavior for 9.2 only.
>
> On Thu, Jun 28, 2012 at 09:32:41AM -0700, Josh Kupershmidt wrote:
> > ! if (!superuser())
> > {
> > /*
> > ! * Since the user is not superuser, check for matching roles.
> > */
> > ! if (proc->roleId != GetUserId())
> > ! return SIGNAL_BACKEND_NOPERMISSION;
> > }
>
> I would have collapsed the conditionals and deleted the obvious comment:
>
> if (!(superuser() || proc->roleId == GetUserId()))
> return SIGNAL_BACKEND_NOPERMISSION;
>
> The committer can do that if desired; no need for another version.

Thanks, I pushed to HEAD and 9.2 with the suggested tweaks.

Documentation doesn't specify the behavior of the non-superuser case
when there's trouble, so I too think the behavior change in 9.2 is okay.
I am less sure about it needing documenting.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services