[HACKERS] Re: pg_terminate_backend and pg_cancel_backend by not administrator user
On Fri, Jul 01, 2011 at 07:31:30PM +0200, Torello Querci wrote: > 2011/6/2 Noah Misch : > > Having thought about this some more, I do now see a risk. ?Currently, a > > SECURITY > > DEFINER function (actually any function, but that's where it matters) can > > trap > > query_canceled. ?By doing so, the author can ensure that only superusers and > > crashes may halt the function during a section protected in this way. ?One > > might > > use it to guard a series of updates made over dblink. > > ?pg_terminate_backend() > > breaks this protection. ?I've never designed something this way; it only > > suffices when you merely sort-of-care about transactional integrity. > > ?Perhaps > > it's an acceptable loss for this feature? > > > >> And if so, is this patch a good first step on that path? > > > > Understand that the pg_terminate_backend() is able to kill process > that need not to be killed. > I suppose that looking inside the internal postgreql table in order to > not allow a normal db owner to kill a superuser connection can avoid > this problem? Checking whether a session is authenticated to a superuser is not necessary or sufficient to close the hazard I described above. My inclination is to just say that the hazard is acceptable, and we should not worry about it. No database owner should be allowed to kill processes like the bgwriter or the stats collector. Since they do not connect to databases or operate as an authenticated user, none of the proposed tests would open up ways to kill them. > If I am the database owner I need to be able to manage my DB. Ok for > superuser connection (and internal administrative process like > autovacuum) > I am the developer, not the DBA, so sometimes, when I wrong something, > I need to kill my session if I wrong something > > Can we suppose, in a more generic case, that an user can kill > connection only from the same user even if this is not the database > owner? Yes. Modulo concerns I described above, database owners should be allowed to cancel or terminate any backend connected to their databases, and any user should be able to cancel or terminate backends authenticated to themselves. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: pg_terminate_backend and pg_cancel_backend by not administrator user
On Wed, Jun 01, 2011 at 10:26:34PM -0400, Josh Kupershmidt wrote: > On Wed, Jun 1, 2011 at 5:55 PM, Noah Misch wrote: > > On Sun, May 29, 2011 at 10:56:02AM -0400, Josh Kupershmidt wrote: > >> Looking around, I see there were real problems[1] with sending SIGTERM > >> to individual backends back in 2005 or so, and pg_terminate_backend() > >> was only deemed safe enough to put in for 8.4 [2]. So expanding > >> pg_terminate_backend() privileges does make me a tad nervous. > > > > The documentation for the CREATE USER flag would boil down to "omit this > > flag > > only if you're worried about undiscovered PostgreSQL bugs in this area". > > ?I'd > > echo Tom's sentiment from the first thread, "In any case I think we have to > > solve it, not create new mechanisms to try to ignore it." > > I do agree with Tom's sentiment from that thread. But, if we are > confident that pg_terminate_backend() is safe enough to relax > permissions on, then I take it you agree we should plan to extend this > power to all users? Yes; that's what I was trying to say. Having thought about this some more, I do now see a risk. Currently, a SECURITY DEFINER function (actually any function, but that's where it matters) can trap query_canceled. By doing so, the author can ensure that only superusers and crashes may halt the function during a section protected in this way. One might use it to guard a series of updates made over dblink. pg_terminate_backend() breaks this protection. I've never designed something this way; it only suffices when you merely sort-of-care about transactional integrity. Perhaps it's an acceptable loss for this feature? > And if so, is this patch a good first step on that path? Yes. > >> Reading through those old threads made me realize this patch would > >> give database owners the ability to kill off autovacuum workers. Seems > >> like we'd want to restrict that power to superusers. > > > > Would we? ?Any old user can already stifle VACUUM by holding a transaction > > open. > > This is true, though it's possible we might at some point want a > backend process which really shouldn't be killable by non-superusers > (if vacuum/autovacuum isn't one already.) Actually, I could easily > imagine a superuser running an important query on a database getting > peeved if a non-superuser were allowed to cancel/terminate his > queries. That's really a different level of user isolation than we have. If your important query runs on a database owned by someone else, calls functions owned by someone else, or reads tables owned by someone else, you're substantially at the mercy of those object owners. That situation probably is unsatisfactory to some folks. Adding the possibility that a database owner could cancel your query seems like an extension of that codependency more than a new exposure. Thanks, nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: pg_terminate_backend and pg_cancel_backend by not administrator user
On Sun, May 29, 2011 at 10:56:02AM -0400, Josh Kupershmidt wrote: > On Sun, May 29, 2011 at 5:04 AM, Noah Misch wrote: > > What risks arise from unconditionally allowing these calls for the same > > user's > > backends? ?`pg_cancel_backend' ought to be safe enough; the user always has > > access to the standard cancellation protocol, making the SQL interface a > > mere > > convenience (albeit a compelling one). ?`pg_terminate_backend' does open up > > access to a new behavior, but no concrete risks come to mind. > > Looking around, I see there were real problems[1] with sending SIGTERM > to individual backends back in 2005 or so, and pg_terminate_backend() > was only deemed safe enough to put in for 8.4 [2]. So expanding > pg_terminate_backend() privileges does make me a tad nervous. The documentation for the CREATE USER flag would boil down to "omit this flag only if you're worried about undiscovered PostgreSQL bugs in this area". I'd echo Tom's sentiment from the first thread, "In any case I think we have to solve it, not create new mechanisms to try to ignore it." > Reading through those old threads made me realize this patch would > give database owners the ability to kill off autovacuum workers. Seems > like we'd want to restrict that power to superusers. Would we? Any old user can already stifle VACUUM by holding a transaction open. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: pg_terminate_backend and pg_cancel_backend by not administrator user
On Sat, May 28, 2011 at 01:44:20PM -0400, Josh Kupershmidt wrote:
> Anssi and I posted some initial feedback on the patch's goals earlier.
> I would like to ultimately see users have the capability to
> pg_cancel_backend() their own queries. But I could at least conceive
> of others not wanting this behavior enabled by default. So perhaps
> this patch's approach of granting extra privs to the database owner
> could work as a first attempt. And maybe a later version could
> introduce a GUC allowing the DBA to control whether users can
> cancel/terminate their backends, or we could instead have an option
> flag to CREATE/ALTER ROLE, allowing per-user configuration.
What risks arise from unconditionally allowing these calls for the same user's
backends? `pg_cancel_backend' ought to be safe enough; the user always has
access to the standard cancellation protocol, making the SQL interface a mere
convenience (albeit a compelling one). `pg_terminate_backend' does open up
access to a new behavior, but no concrete risks come to mind.
On the other hand, this *would* be substantial new authority for database
owners. Seems like a reasonable authority to grant, though.
> It would be helpful to hear from others whether this patch's goals
> would work as a first pass at this problem, so that Torello doesn't
> waste time on a doomed approach. Also, it might be helpful to add an
> entry on the Todo list for 'allow non-superusers to use
> pg_cancel_backend()', in case this patch gets sunk.
>
> Now, a few technical comments about the patch:
> 1.) This bit looks dangerous:
> +backend = pgstat_fetch_stat_beentry(i);
> +if (backend->st_procpid == pid) {
>
> Since pgstat_fetch_stat_beentry() might return NULL.
I think you want BackendPidGetProc().
Thanks,
nm
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
