Re: [HACKERS] Patch to allow users to kill their own queries
On Fri, Jan 13, 2012 at 14:46, Magnus Hagander mag...@hagander.net wrote: On Fri, Jan 13, 2012 at 14:42, Greg Smith g...@2ndquadrant.com wrote: On 01/03/2012 12:59 PM, Tom Lane wrote: Noah Mischn...@leadboat.com writes: Regarding the other message, avoid composing a translated message from independently-translated parts. Yes. I haven't looked at the patch, but I wonder whether it wouldn't be better to dodge both of these problems by having the subroutine return a success/failure result code, so that the actual ereport can be done at an outer level where the full message (and hint) can be written directly. Between your and Noah's comments I understand the issue and likely direction to sort this out now. Bounced forward to the next CommitFest and back on my plate again. Guess I'll be learning new and exciting things about translation this week. I should say that I have more or less finished this one off, since I figured you were working on more important things. Just a final round of cleanup and commit left, really, so you can take it off your plate again if you want to :-) Finally, to end the bikeshedding, I've applied this patch after another round of fairly extensive changes. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
On 01/03/2012 12:59 PM, Tom Lane wrote: Noah Mischn...@leadboat.com writes: Regarding the other message, avoid composing a translated message from independently-translated parts. Yes. I haven't looked at the patch, but I wonder whether it wouldn't be better to dodge both of these problems by having the subroutine return a success/failure result code, so that the actual ereport can be done at an outer level where the full message (and hint) can be written directly. Between your and Noah's comments I understand the issue and likely direction to sort this out now. Bounced forward to the next CommitFest and back on my plate again. Guess I'll be learning new and exciting things about translation this week. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
On Fri, Jan 13, 2012 at 14:42, Greg Smith g...@2ndquadrant.com wrote: On 01/03/2012 12:59 PM, Tom Lane wrote: Noah Mischn...@leadboat.com writes: Regarding the other message, avoid composing a translated message from independently-translated parts. Yes. I haven't looked at the patch, but I wonder whether it wouldn't be better to dodge both of these problems by having the subroutine return a success/failure result code, so that the actual ereport can be done at an outer level where the full message (and hint) can be written directly. Between your and Noah's comments I understand the issue and likely direction to sort this out now. Bounced forward to the next CommitFest and back on my plate again. Guess I'll be learning new and exciting things about translation this week. I should say that I have more or less finished this one off, since I figured you were working on more important things. Just a final round of cleanup and commit left, really, so you can take it off your plate again if you want to :-) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
On Tue, Dec 20, 2011 at 02:30:08PM +0100, Magnus Hagander wrote: That said - can someone who knows the translation stuff better than me comment on if this is actually going to be translatable, or if it violates too many translation rules? +pg_signal_backend(int pid, int sig, bool allow_same_role, const char *actionstr, const char *hint) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg(must be superuser to %s other server processes, actionstr), + errhint(%s, hint))); + PG_RETURN_BOOL(pg_signal_backend(PG_GETARG_INT32(0), SIGTERM, false, + gettext_noop(terminate), + gettext_noop(You can cancel your own processes with pg_cancel_backend().))); } You need errhint(%s, _(hint)) or errhint(hint) to substitute the translation at runtime; only the printf-pattern string gets an automatic message catalog lookup. Regarding the other message, avoid composing a translated message from independently-translated parts. The translator sees this: #: utils/adt/misc.c:110 #, c-format msgid must be superuser to %s other server processes msgstr #: utils/adt/misc.c:166 msgid terminate msgstr #: utils/adt/misc.c:167 msgid You can cancel your own processes with pg_cancel_backend(). msgstr He'll probably need to read the code to see how the strings go together. If we add an alternative to terminate, not all languages will necessarily have a translation of the outer message that works for both inner fragments. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
Noah Misch n...@leadboat.com writes: Regarding the other message, avoid composing a translated message from independently-translated parts. Yes. I haven't looked at the patch, but I wonder whether it wouldn't be better to dodge both of these problems by having the subroutine return a success/failure result code, so that the actual ereport can be done at an outer level where the full message (and hint) can be written directly. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
On Mon, Dec 19, 2011 at 15:50, Greg Smith g...@2ndquadrant.com wrote:
On 12/18/2011 07:31 AM, Magnus Hagander wrote:
* I restructured the if statements, because I had a hard time
following the comments around that ;) I find this one easier - but I'm
happy to change back if you think your version was more readable.
That looks fine. I highlighted this because I had a feeling there was still
some gain to be had here, just didn't see it myself. This works.
* The error message in pg_signal_backend breaks the abstraction,
because it specifically talks about terminating the other backend -
when it's not supposed to know about that in that function. I think we
either need to get rid of the hint completely, or we need to find a
way to issue it from the caller. Or pass it as a parameter. It's fine
for now since we only have two signals, but we might have more in the
future..
I feel that including a hint in the pg_terminate_backend case is a UI
requirement. If someone has made it as far as discovering that function
exists, tries calling it, and it fails, the friendly thing to do is point
them toward a direction that might work better. Little things like that
make a huge difference in how friendly the software appears to its users;
this is even more true in cases where version improvements actually expand
what can and cannot be done.
My quick and dirty side thinks that just documenting the potential future
issues would be enough:
Due to the limited number of callers of this function, the hint message
here can be certain that pg_terminate_backend provides the only path to
reach this point. If more callers to pg_signal_backend appear, a more
generic hint mechanism might be needed here.
If you must have this more generic mechanism available, I would accept
re-factoring to provide it instead. What I wouldn't want to live with is a
commit of this where the hint goes away completely. It's taken a long time
chopping the specification to get this feature sorted out; we might as well
make what's left be the best it can be now.
How about something like this - passing it in as a parameter?
That said - can someone who knows the translation stuff better than me
comment on if this is actually going to be translatable, or if it
violates too many translation rules?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e7f7fe0..cf77586 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -14244,8 +14244,8 @@ SELECT set_config('log_statement_stats', 'off', false);
para
The functions shown in xref
linkend=functions-admin-signal-table send control signals to
-other server processes. Use of these functions is restricted
-to superusers.
+other server processes. Use of these functions is usually restricted
+to superusers, with noted exceptions.
/para
table id=functions-admin-signal-table
@@ -14262,7 +14262,10 @@ SELECT set_config('log_statement_stats', 'off', false);
literalfunctionpg_cancel_backend(parameterpid/parameter typeint/)/function/literal
/entry
entrytypeboolean/type/entry
- entryCancel a backend's current query/entry
+ entryCancel a backend's current query. You can execute this against
+another backend that has exactly the same role as the user calling the
+function. In all other cases, you must be a superuser.
+/entry
/row
row
entry
@@ -14304,6 +14307,10 @@ SELECT set_config('log_statement_stats', 'off', false);
commandpostgres/command processes on the server (using
applicationps/ on Unix or the applicationTask
Manager/ on productnameWindows/).
+For the less restrictive functionpg_cancel_backend/, the role of an
+active backend can be found from
+the structfieldusename/structfield column of the
+structnamepg_stat_activity/structname view.
/para
para
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 7a2e0c8..45520b6 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -30,6 +30,7 @@
#include postmaster/syslogger.h
#include storage/fd.h
#include storage/pmsignal.h
+#include storage/proc.h
#include storage/procarray.h
#include tcop/tcopprot.h
#include utils/builtins.h
@@ -70,15 +71,45 @@ current_query(PG_FUNCTION_ARGS)
}
/*
- * Functions to send signals to other backends.
+ * Send a signal to another backend.
+ * If allow_same_role is false, actionstr must be set to a string
+ * indicating what the signal does, to be inserted in the error message, and
+ * hint should be set to a hint to be sent along with this message.
*/
static bool
-pg_signal_backend(int pid, int sig)
+pg_signal_backend(int pid, int sig, bool allow_same_role, const char *actionstr, const char *hint)
{
+ PGPROC *proc;
+
if (!superuser())
- ereport(ERROR,
-
Re: [HACKERS] Patch to allow users to kill their own queries
On 12/18/2011 07:31 AM, Magnus Hagander wrote: * I restructured the if statements, because I had a hard time following the comments around that ;) I find this one easier - but I'm happy to change back if you think your version was more readable. That looks fine. I highlighted this because I had a feeling there was still some gain to be had here, just didn't see it myself. This works. * The error message in pg_signal_backend breaks the abstraction, because it specifically talks about terminating the other backend - when it's not supposed to know about that in that function. I think we either need to get rid of the hint completely, or we need to find a way to issue it from the caller. Or pass it as a parameter. It's fine for now since we only have two signals, but we might have more in the future.. I feel that including a hint in the pg_terminate_backend case is a UI requirement. If someone has made it as far as discovering that function exists, tries calling it, and it fails, the friendly thing to do is point them toward a direction that might work better. Little things like that make a huge difference in how friendly the software appears to its users; this is even more true in cases where version improvements actually expand what can and cannot be done. My quick and dirty side thinks that just documenting the potential future issues would be enough: Due to the limited number of callers of this function, the hint message here can be certain that pg_terminate_backend provides the only path to reach this point. If more callers to pg_signal_backend appear, a more generic hint mechanism might be needed here. If you must have this more generic mechanism available, I would accept re-factoring to provide it instead. What I wouldn't want to live with is a commit of this where the hint goes away completely. It's taken a long time chopping the specification to get this feature sorted out; we might as well make what's left be the best it can be now. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
On Fri, Dec 16, 2011 at 13:31, Greg Smith g...@2ndquadrant.com wrote:
On 12/14/2011 05:24 AM, Magnus Hagander wrote:
How about passing a parameter to pg_signal_backend? Making
pg_signal_backend(int pid, int sig, bool allow_samerole)?
That works, got rid of the parts I didn't like and allowed some useful minor
restructuring. I also made the HINT better and match style guidelines:
gsmith= select pg_terminate_backend(21205);
ERROR: must be superuser to terminate other server processes
HINT: You can cancel your own processes with pg_cancel_backend().
gsmith= select pg_cancel_backend(21205);
pg_cancel_backend
---
t
New rev attached and pushed to
https://github.com/greg2ndQuadrant/postgres/tree/cancel-backend (which is
*not* the same branch as I used last time; don't ask why, long story)
I considered some additional ways to restructure the checks that could
remove a further line or two from the logic here, but they all made the
result seem less readable to me. And this is not a high performance code
path. I may have gone a bit too far with the comment additions though, so
feel free to trim that back. It kept feeling weird to me that none of the
individual signaling functions had their own intro comments. I added all
those.
I also wrote up a commentary on the PID wraparound race condition
possibility Josh brought up. Some research shows that pid assignment on
some systems is made more secure by assigning new ones randomly. That seems
like it would make it possible to have a pid get reused much faster than on
the usual sort of system that does sequential assignment and wraparound. A
reuse collision still seems extremely unlikely though. With the new
comments, at least a future someone who speculates on this will know how
much thinking went into the current implementation: enough to notice, not
enough to see anything worth doing about it. Maybe that's just wasted lines
of text?
With so little grief on the last round, I'm going to guess this one will
just get picked up by Magnus to commit next. Marking accordingly and moved
to the current CommitFest.
I was going to, but I noticed a few things:
* I restructured the if statements, because I had a hard time
following the comments around that ;) I find this one easier - but I'm
happy to change back if you think your version was more readable.
* The error message in pg_signal_backend breaks the abstraction,
because it specifically talks about terminating the other backend -
when it's not supposed to know about that in that function. I think we
either need to get rid of the hint completely, or we need to find a
way to issue it from the caller. Or pass it as a parameter. It's fine
for now since we only have two signals, but we might have more in the
future..
* I gave it a run of pgindent ;)
In the attached updated patch I've just removed the HINT and changed
the reference fromterminate to signal. But I'd like your input
onthat before I commit :-)
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e7f7fe0..cf77586 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -14244,8 +14244,8 @@ SELECT set_config('log_statement_stats', 'off', false);
para
The functions shown in xref
linkend=functions-admin-signal-table send control signals to
-other server processes. Use of these functions is restricted
-to superusers.
+other server processes. Use of these functions is usually restricted
+to superusers, with noted exceptions.
/para
table id=functions-admin-signal-table
@@ -14262,7 +14262,10 @@ SELECT set_config('log_statement_stats', 'off', false);
literalfunctionpg_cancel_backend(parameterpid/parameter typeint/)/function/literal
/entry
entrytypeboolean/type/entry
- entryCancel a backend's current query/entry
+ entryCancel a backend's current query. You can execute this against
+another backend that has exactly the same role as the user calling the
+function. In all other cases, you must be a superuser.
+/entry
/row
row
entry
@@ -14304,6 +14307,10 @@ SELECT set_config('log_statement_stats', 'off', false);
commandpostgres/command processes on the server (using
applicationps/ on Unix or the applicationTask
Manager/ on productnameWindows/).
+For the less restrictive functionpg_cancel_backend/, the role of an
+active backend can be found from
+the structfieldusename/structfield column of the
+structnamepg_stat_activity/structname view.
/para
para
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 7a2e0c8..d7f2435 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -30,6 +30,7 @@
#include postmaster/syslogger.h
#include storage/fd.h
#include storage/pmsignal.h
Re: [HACKERS] Patch to allow users to kill their own queries
On Sat, Dec 17, 2011 at 11:58 PM, Tom Lane t...@sss.pgh.pa.us wrote: I think this argument is bogus: if this is a real issue, then no use of kill() anytime, by anyone, is safe. In practice I believe that Unix systems avoid recycling PIDs right away so as to offer some protection. I'm not sure they do anything more sophisticated than cycling through a sufficiently-large PID space, but whether it's that or something else, I guess it must be adequate or they'd have enlarged the space... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
Robert Haas robertmh...@gmail.com writes: On Fri, Dec 16, 2011 at 1:21 AM, Greg Smith g...@2ndquadrant.com wrote: ... If you assume someone can run through all the PIDs between those checks and the kill, the system is already broken that way. From a theoretical point of view, I believe it to be slightly different. If a superuser sends a kill, they will certainly be authorized to kill whatever they end up killing, because they are authorized to kill anything. On the other hand, the proposed patch would potentially result - in the extremely unlikely event of a super-fast PID wraparound - in someone cancelling a query they otherwise wouldn't have been able to cancel. In practice, the chances of this seem fairly remote. I think this argument is bogus: if this is a real issue, then no use of kill() anytime, by anyone, is safe. In practice I believe that Unix systems avoid recycling PIDs right away so as to offer some protection. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
On 12/14/2011 05:24 AM, Magnus Hagander wrote:
How about passing a parameter to pg_signal_backend? Making
pg_signal_backend(int pid, int sig, bool allow_samerole)?
That works, got rid of the parts I didn't like and allowed some useful
minor restructuring. I also made the HINT better and match style
guidelines:
gsmith= select pg_terminate_backend(21205);
ERROR: must be superuser to terminate other server processes
HINT: You can cancel your own processes with pg_cancel_backend().
gsmith= select pg_cancel_backend(21205);
pg_cancel_backend
---
t
New rev attached and pushed to
https://github.com/greg2ndQuadrant/postgres/tree/cancel-backend (which
is *not* the same branch as I used last time; don't ask why, long story)
I considered some additional ways to restructure the checks that could
remove a further line or two from the logic here, but they all made the
result seem less readable to me. And this is not a high performance
code path. I may have gone a bit too far with the comment additions
though, so feel free to trim that back. It kept feeling weird to me
that none of the individual signaling functions had their own intro
comments. I added all those.
I also wrote up a commentary on the PID wraparound race condition
possibility Josh brought up. Some research shows that pid assignment on
some systems is made more secure by assigning new ones randomly. That
seems like it would make it possible to have a pid get reused much
faster than on the usual sort of system that does sequential assignment
and wraparound. A reuse collision still seems extremely unlikely
though. With the new comments, at least a future someone who speculates
on this will know how much thinking went into the current
implementation: enough to notice, not enough to see anything worth
doing about it. Maybe that's just wasted lines of text?
With so little grief on the last round, I'm going to guess this one will
just get picked up by Magnus to commit next. Marking accordingly and
moved to the current CommitFest.
--
Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e7f7fe0..cf77586 100644
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*** SELECT set_config('log_statement_stats',
*** 14244,14251
para
The functions shown in xref
linkend=functions-admin-signal-table send control signals to
! other server processes. Use of these functions is restricted
! to superusers.
/para
table id=functions-admin-signal-table
--- 14244,14251
para
The functions shown in xref
linkend=functions-admin-signal-table send control signals to
! other server processes. Use of these functions is usually restricted
! to superusers, with noted exceptions.
/para
table id=functions-admin-signal-table
*** SELECT set_config('log_statement_stats',
*** 14262,14268
literalfunctionpg_cancel_backend(parameterpid/parameter typeint/)/function/literal
/entry
entrytypeboolean/type/entry
!entryCancel a backend's current query/entry
/row
row
entry
--- 14262,14271
literalfunctionpg_cancel_backend(parameterpid/parameter typeint/)/function/literal
/entry
entrytypeboolean/type/entry
!entryCancel a backend's current query. You can execute this against
! another backend that has exactly the same role as the user calling the
! function. In all other cases, you must be a superuser.
! /entry
/row
row
entry
*** SELECT set_config('log_statement_stats',
*** 14304,14309
--- 14307,14316
commandpostgres/command processes on the server (using
applicationps/ on Unix or the applicationTask
Manager/ on productnameWindows/).
+ For the less restrictive functionpg_cancel_backend/, the role of an
+ active backend can be found from
+ the structfieldusename/structfield column of the
+ structnamepg_stat_activity/structname view.
/para
para
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 7a2e0c8..1b7b75b 100644
*** a/src/backend/utils/adt/misc.c
--- b/src/backend/utils/adt/misc.c
***
*** 30,35
--- 30,36
#include postmaster/syslogger.h
#include storage/fd.h
#include storage/pmsignal.h
+ #include storage/proc.h
#include storage/procarray.h
#include tcop/tcopprot.h
#include utils/builtins.h
*** current_query(PG_FUNCTION_ARGS)
*** 70,84
}
/*
! * Functions to send signals to other backends.
*/
static bool
! pg_signal_backend(int pid, int sig)
{
! if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! (errmsg(must be superuser
Re: [HACKERS] Patch to allow users to kill their own queries
On Fri, Dec 16, 2011 at 1:21 AM, Greg Smith g...@2ndquadrant.com wrote: This is a problem with the existing code though, and the proposed changes don't materially alter that; there's just another quick check in one path through. Right now we check if someone is superuser, then if it's a backend PID, then we send the signal. If you assume someone can run through all the PIDs between those checks and the kill, the system is already broken that way. From a theoretical point of view, I believe it to be slightly different. If a superuser sends a kill, they will certainly be authorized to kill whatever they end up killing, because they are authorized to kill anything. On the other hand, the proposed patch would potentially result - in the extremely unlikely event of a super-fast PID wraparound - in someone cancelling a query they otherwise wouldn't have been able to cancel. In practice, the chances of this seem fairly remote. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
On 12/16/2011 08:42 AM, Robert Haas wrote: the proposed patch would potentially result - in the extremely unlikely event of a super-fast PID wraparound - in someone cancelling a query they otherwise wouldn't have been able to cancel. So how might this get exploited? -Attach a debugger and put a breakpoint between the check and the kill -Fork processes to get close to your target -Wait for a process you want to mess with to appear at the PID you're waiting for. If you miss it, repeat fork bomb and try again. -Resume the debugger to kill the other user's process If I had enough access to launch this sort of attack, I think I'd find mayhem elsewhere more a more profitable effort. Crazy queries, work_mem abuse, massive temp file generation, trying to get the OOM killer involved; seems like there's bigger holes than this already. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
On Friday, December 16, 2011, Robert Haas wrote: On Fri, Dec 16, 2011 at 1:21 AM, Greg Smith g...@2ndquadrant.comjavascript:; wrote: This is a problem with the existing code though, and the proposed changes don't materially alter that; there's just another quick check in one path through. Right now we check if someone is superuser, then if it's a backend PID, then we send the signal. If you assume someone can run through all the PIDs between those checks and the kill, the system is already broken that way. From a theoretical point of view, I believe it to be slightly different. If a superuser sends a kill, they will certainly be authorized to kill whatever they end up killing, because they are authorized to kill anything. On the other hand, the proposed patch Not necessarily. What if it's recycled as a backend in a different postgres installation. Or just a cronjob or shell running as the same user? Sure, you can argue that the superuser can destroy anything he wants - but in that case, why do we have a check for this at all in the first place? I think we can safely say that any OS that actually manages to recycle the PID in the short time it takes to get between those instructions is so broken we don't need to care about that.
Re: [HACKERS] Patch to allow users to kill their own queries
On Friday, December 16, 2011, Greg Smith wrote: On 12/16/2011 08:42 AM, Robert Haas wrote: the proposed patch would potentially result - in the extremely unlikely event of a super-fast PID wraparound - in someone cancelling a query they otherwise wouldn't have been able to cancel. So how might this get exploited? -Attach a debugger and put a breakpoint between the check and the kill Once you've attached a debugger, you've already won. You can inject arbitrary instructions at this point, no? -Fork processes to get close to your target -Wait for a process you want to mess with to appear at the PID you're waiting for. If you miss it, repeat fork bomb and try again. -Resume the debugger to kill the other user's process If I had enough access to launch this sort of attack, I think I'd find mayhem elsewhere more a more profitable effort. Crazy queries, work_mem abuse, massive temp file generation, trying to get the OOM killer involved; seems like there's bigger holes than this already. killall -9 postgres is even easier. //Magnus
Re: [HACKERS] Patch to allow users to kill their own queries
Excerpts from Greg Smith's message of vie dic 16 11:19:52 -0300 2011: On 12/16/2011 08:42 AM, Robert Haas wrote: the proposed patch would potentially result - in the extremely unlikely event of a super-fast PID wraparound - in someone cancelling a query they otherwise wouldn't have been able to cancel. So how might this get exploited? -Attach a debugger and put a breakpoint between the check and the kill If you can attach a debugger to a backend, you already have the credentials to the user running postmaster, which we assume to be a game over condition. I guess a more interesting exploitation would be to do this until the killing backend randomly wins the race condition against a dying backend and a new one starting up; this would require very rapid reuse of the PID, plus very rapid startup of the new proces. I think we already assume that this is not the case in other code paths, even on systems that randomly (instead of sequentially) assign PIDs. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
On Tue, Dec 13, 2011 at 5:59 AM, Greg Smith g...@2ndquadrant.com wrote: Same-user cancels, but not termination. Only this, and nothing more. +1 from me on this approach. I think enough people have clamored for this simple approach which solves the common-case. There's one obvious and questionable design decision I made to highlight. Right now the only consumers of pg_signal_backend are the cancel and terminate calls. What I did was make pg_signal_backend more permissive, adding the idea that role equivalence = allowed, and therefore granting that to anything else that might call it. And then I put a stricter check on termination. This results in a redundant check of superuser on the termination check, and the potential for mis-using pg_signal_backend. . . I think that's OK, it should be easy enough to reorganize if more callers to pg_signal_backend() happen to come along. The only niggling concern I have about this patch (and, I think, all similar ones proposed) is the possible race condition between the permissions checking and the actual call of kill() inside pg_signal_backend(). I fooled around with this by using gdb to attach to Backend #1, stuck a breakpoint right before the call to: if (kill(-pid, sig)) and ran a pg_cancel_backend() of a same-role Backend #2. Then, with the permissions checking passed, I exited Backend #2 and used a script to call fork() in a loop until my system PIDs had wrapped around to a few PIDs before the one Backend #2 had held. Then, I opened a few superuser connections until I had one with the same PID which Backend #2 previously had. After I released the breakpoint in gdb on Backend #1, it happily sent a SIGINT to my superuser backend. I notice that BackendPidGetProc() has the comment: ... Note that it is up to the caller to be sure that the question remains meaningful for long enough for the answer to be used ... So someone has mulled this over before. It took my script maybe 15-20 minutes to cause a wraparound by running fork() in a loop, and that wraparound would somehow have to occur within the short execution of pg_signal_backend(). I'm not super worried about the patch from a security perspective, just thought I'd point this out. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
On 12/15/2011 07:36 PM, Josh Kupershmidt wrote: The only niggling concern I have about this patch (and, I think, all similar ones proposed) is the possible race condition between the permissions checking and the actual call of kill() inside pg_signal_backend(). This is a problem with the existing code though, and the proposed changes don't materially alter that; there's just another quick check in one path through. Right now we check if someone is superuser, then if it's a backend PID, then we send the signal. If you assume someone can run through all the PIDs between those checks and the kill, the system is already broken that way. I notice that BackendPidGetProc() has the comment: ... Note that it is up to the caller to be sure that the question remains meaningful for long enough for the answer to be used ... So someone has mulled this over before. It took my script maybe 15-20 minutes to cause a wraparound by running fork() in a loop, and that wraparound would somehow have to occur within the short execution of pg_signal_backend(). Right, the the possibility you're raising is the obvious flaw with this approach. Currently the only consumers of BackendPidGetProc or IsBackendPid are these cancel/terminate functions. As you measured, running through the PIDs fast enough to outrace any of that code is unlikely. I'm sure a lot of other UNIX apps would break, too, if that ever became untrue. The sort of thing that comment is warning is that you wouldn't want to do something like grab a PID, vacuum a table, and then assume that PID was still valid. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
On Tue, Dec 13, 2011 at 11:59, Greg Smith g...@2ndquadrant.com wrote: The submission from Edward Muller I'm replying to is quite similar to what the other raging discussion here decided was the right level to target. There was one last year from Josh Kupershmidt with similar goals: http://archives.postgresql.org/pgsql-admin/2010-02/msg00052.php A good place to start is the concise summary of the new specification goal that Tom made in the other thread: If allowing same-user cancels is enough to solve 95% or 99% of the real-world use cases, let's just do that. Same-user cancels, but not termination. Only this, and nothing more. Good. Appropriate credits here would go Josh Kupershmidt, Edward Muller, and then myself; everyone did an equally useful chunk of this in that order. It's all packaged up for useful gitsumption at https://github.com/greg2ndQuadrant/postgres/tree/cancel_backend too. I attached it to the next CommitFest: https://commitfest.postgresql.org/action/patch_view?id=722 but would enjoy seeing a stake finally put through its evil heart before then, as I don't think there's much left to do now. test= select pg_terminate_backend(28154); ERROR: must be superuser to terminate other server processes HINT: you can use pg_cancel_backend() on your own processes That HINT sounds a bit weird to me. you can cancel your own queries using pg_cancel_backend() instead or something like that? Victory over the evil sleeping backend is complete, without a superuser in sight. \o/ There's one obvious and questionable design decision I made to highlight. Right now the only consumers of pg_signal_backend are the cancel and terminate calls. What I did was make pg_signal_backend more permissive, adding the idea that role equivalence = allowed, and therefore granting that to anything else that might call it. And then I put a stricter check on termination. This results in a redundant check of superuser on the termination check, and the potential for mis-using pg_signal_backend. I documented all that and liked the result; it feels better to me to have pg_signal_backend provide an API that is more flexible here. Pushback to structure this differently is certainly possible though, and I'm happy to iterate the patch to address that. It might drift back toward something closer to Josh's original design. How about passing a parameter to pg_signal_backend? Making pg_signal_backend(int pid, int sig, bool allow_samerole)? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
On 12/14/2011 05:24 AM, Magnus Hagander wrote: How about passing a parameter to pg_signal_backend? Making pg_signal_backend(int pid, int sig, bool allow_samerole)? That sounds like it will result in less code, and make the API I was documenting be obvious from the parameters instead. I'll refactor to do that, tweak the hint message, and resubmit. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
Magnus Hagander mag...@hagander.net writes: On Tue, Dec 13, 2011 at 11:59, Greg Smith g...@2ndquadrant.com wrote: HINT: you can use pg_cancel_backend() on your own processes That HINT sounds a bit weird to me. you can cancel your own queries using pg_cancel_backend() instead or something like that? Doesn't follow message style guidelines either ;-). Hints should be complete sentences, with initial cap and ending period. http://www.postgresql.org/docs/devel/static/error-style-guide.html regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
The submission from Edward Muller I'm replying to is quite similar to
what the other raging discussion here decided was the right level to
target. There was one last year from Josh Kupershmidt with similar
goals: http://archives.postgresql.org/pgsql-admin/2010-02/msg00052.php
A good place to start is the concise summary of the new specification
goal that Tom made in the other thread:
If allowing same-user cancels is enough to solve 95% or 99% of the
real-world use cases, let's just do that.
Same-user cancels, but not termination. Only this, and nothing more.
Relative to that goal, Ed's patch was too permissive for termination,
and since he's new to this code it didn't check all the error conditions
possible here. Josh's patch had many of the right error checks, but it
was more code than I liked for his slightly different permissions
change. And its attempts to be helpful leaked role information. (That
may have been just debugging debris left for review purposes) I mashed
the best bits of both together, tried to simplify the result, then
commented heavily upon the race conditions and design decisions the code
reflects. Far as I can tell the patch is feature complete, including
documentation.
Appropriate credits here would go Josh Kupershmidt, Edward Muller, and
then myself; everyone did an equally useful chunk of this in that
order. It's all packaged up for useful gitsumption at
https://github.com/greg2ndQuadrant/postgres/tree/cancel_backend too. I
attached it to the next CommitFest:
https://commitfest.postgresql.org/action/patch_view?id=722 but would
enjoy seeing a stake finally put through its evil heart before then, as
I don't think there's much left to do now.
To demo I start with a limited user and a crazy, must be stopped backend:
$ createuser test
Shall the new role be a superuser? (y/n) n
Shall the new role be allowed to create databases? (y/n) n
Shall the new role be allowed to create more new roles? (y/n) n
$ psql -U test
test= select pg_sleep(100);
Begin another session, find and try to terminate; get rejected with a
hint, then follow it to cancel:
test= select procpid,usename,current_query from pg_stat_activity;
-[ RECORD 1 ]-+
procpid | 28154
usename | test
current_query | select pg_sleep(100);
test= select pg_terminate_backend(28154);
ERROR: must be superuser to terminate other server processes
HINT: you can use pg_cancel_backend() on your own processes
test= select pg_cancel_backend(28154);
-[ RECORD 1 ]-+--
pg_cancel_backend | t
And then this is shown on the first one:
test= select pg_sleep(100);
ERROR: canceling statement due to user request
Victory over the evil sleeping backend is complete, without a superuser
in sight.
There's one obvious and questionable design decision I made to
highlight. Right now the only consumers of pg_signal_backend are the
cancel and terminate calls. What I did was make pg_signal_backend more
permissive, adding the idea that role equivalence = allowed, and
therefore granting that to anything else that might call it. And then I
put a stricter check on termination. This results in a redundant check
of superuser on the termination check, and the potential for mis-using
pg_signal_backend. I documented all that and liked the result; it feels
better to me to have pg_signal_backend provide an API that is more
flexible here. Pushback to structure this differently is certainly
possible though, and I'm happy to iterate the patch to address that. It
might drift back toward something closer to Josh's original design.
--
Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e7f7fe0..f145c3f 100644
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*** SELECT set_config('log_statement_stats',
*** 14244,14251
para
The functions shown in xref
linkend=functions-admin-signal-table send control signals to
! other server processes. Use of these functions is restricted
! to superusers.
/para
table id=functions-admin-signal-table
--- 14244,14251
para
The functions shown in xref
linkend=functions-admin-signal-table send control signals to
! other server processes. Use of these functions is usually restricted
! to superusers, with noted exceptions.
/para
table id=functions-admin-signal-table
*** SELECT set_config('log_statement_stats',
*** 14262,14268
literalfunctionpg_cancel_backend(parameterpid/parameter typeint/)/function/literal
/entry
entrytypeboolean/type/entry
!entryCancel a backend's current query/entry
/row
row
entry
--- 14262,14271
Re: [HACKERS] Patch to allow users to kill their own queries
On 11/16/2011 01:28 PM, Daniel Farina wrote: As it would turn out, a patch for this has already been submitted: http://archives.postgresql.org/pgsql-hackers/2011-10/msg1.php There was some wrangling on whether it needs to be extended to be useful, but for our purposes the formulation already posted already captures vital value for us, and in that form appears to be fairly uncontentious. I have moved it to the current commitfest, with a comment linking to the 'please revive this patch' thread whereby a second glance at what to do about this was conducted. Yeah, that one got a raw deal; it should be in the *current* CommitFest--you had it in the next one. I'll join the chorus to just allow people to fire just the pg_cancel_backend pea shooter foot gun targeted only at their own feet, make most users happy, and punt anything more complicated off as troublesome relative to its benefit. That's what Daniel has said, what his co-worker Edward also implemented, what Noah thought was good enough given other security mechanisms in place, and what Tom thinks is reasonable too. This I feel is important, so I'm going to add myself as the next reviewer and include it in my testing run tomorrow. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
Edward Muller edw...@heroku.com wrote: Looking for comments ... https://gist.github.com/be937d3a7a5323c73b6e We'd like to get this, or something like it, into 9.2 If you want it to be seriously considered, you should post the patch to this list, which makes it part of the permanent archives and indicates your willingness to place the code under the PostgreSQL license. http://wiki.postgresql.org/wiki/Submitting_a_Patch#Patch_submission Unfortunately, you're a day late in terms of getting it reviewed in the just-started CommitFest, but another will start in two months. https://commitfest.postgresql.org/action/commitfest_view/open Of course, you're free to talk about it until then, and perhaps someone will take a look at it before the next CF; but a lot of attention will be focused on the patches submitted by the start of the current CF. Any chance you could help review patches in the CF in progress, to help get others' time freed up sooner?: https://commitfest.postgresql.org/action/commitfest_view/inprogress It's a good way to become familiar with the process, so that you can better move your own patch along. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
On Wed, Nov 16, 2011 at 12:06 PM, Kevin Grittner kevin.gritt...@wicourts.gov wrote: Edward Muller edw...@heroku.com wrote: Looking for comments ... https://gist.github.com/be937d3a7a5323c73b6e We'd like to get this, or something like it, into 9.2 If you want it to be seriously considered, you should post the patch to this list, which makes it part of the permanent archives and indicates your willingness to place the code under the PostgreSQL license. inline http://wiki.postgresql.org/wiki/Submitting_a_Patch#Patch_submission I think this was done right. [snip] Any chance you could help review patches in the CF in progress, to help get others' time freed up sooner?: https://commitfest.postgresql.org/action/commitfest_view/inprogress Well, I'm totally new to the code base, I don't write in *C* very often (probably obvious from my patch). Maybe over time It's a good way to become familiar with the process, so that you can better move your own patch along. -Kevin user_signal.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to allow users to kill their own queries
On Wed, Nov 16, 2011 at 12:06 PM, Kevin Grittner kevin.gritt...@wicourts.gov wrote: Edward Muller edw...@heroku.com wrote: Looking for comments ... https://gist.github.com/be937d3a7a5323c73b6e We'd like to get this, or something like it, into 9.2 On Wed, Nov 16, 2011 at 12:06 PM, Kevin Grittner kevin.gritt...@wicourts.gov wrote: Edward Muller edw...@heroku.com wrote: Looking for comments ... https://gist.github.com/be937d3a7a5323c73b6e We'd like to get this, or something like it, into 9.2 As it would turn out, a patch for this has already been submitted: http://archives.postgresql.org/pgsql-hackers/2011-10/msg1.php There was some wrangling on whether it needs to be extended to be useful, but for our purposes the formulation already posted already captures vital value for us, and in that form appears to be fairly uncontentious. I have moved it to the current commitfest, with a comment linking to the 'please revive this patch' thread whereby a second glance at what to do about this was conducted. The link follows: https://commitfest.postgresql.org/action/patch_view?id=541 If you want it to be seriously considered, you should post the patch to this list, which makes it part of the permanent archives and indicates your willingness to place the code under the PostgreSQL license. Although technical mailing lists are not primarily a place of reflection and sensitivity, I do think that wording addressed to a new participant could have been kinder. Perhaps, Unfortunately we cannot accept or even read your patch because of licensing concerns, would you please follow the following patch submission guidelines? link. -- fdr -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
