Simplify ACL handling for large objects and removal of superuser() checks

On Sun, Aug 13, 2017 at 11:20 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Recent commit 8d98819 has added a missing permissoin check to lo_put()
to make sure that the write permissions of the object are properly set
before writing to a large object. When studying the problem, we bumped
into the fact that it is possible to actually simplify those ACL
checks and replace them by checks when opening the large object. This
makes all the checks now in be-fsstubs.c happen in inv_api.c, which is
where all the large object handling happens at a lower level. This
way, it is also possible to remove the extra logic in place to have
the permission checks happen only once.

At the same time, we have bumped into two things:
- ALLOW_DANGEROUS_LO_FUNCTIONS has been introduced in 1999, so it
could be time to let it go. I have known of no place where this is
actively used.
- lo_import and lo_export on the backend have superuser checks. We
could remove them and replace them with proper REVOKE permissions to
allow administrators to give access to those functions.

Attached is a set of 3 patches:
- 0001 removes ALLOW_DANGEROUS_LO_FUNCTIONS
- 0002 replaces the superuser checks with GRANT permissions

+1 for 0001 and 0002 in general, but I can't help noticing that they
lead to a noticeable worsening of the error messages in the regression
tests.

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

On Tue, Aug 15, 2017 at 10:35 PM, Robert Haas <robertmhaas@gmail.com> wrote:

+1 for 0001 and 0002 in general, but I can't help noticing that they
lead to a noticeable worsening of the error messages in the regression
tests.

As the access restriction gets handled by GRANT in this patch, that's
a price to pay. The verbosity of this message could be kept by
introducing a default role dedicated to LOs. Personally, I am of the
opinion that a default role in this case is not justified for only
those functions. Other opinions are welcome.

I have noticed that 0002 should update lobj.sgml as well, something I
missed. Now the docs say "Therefore, their use is restricted to
superusers." for lo_import and lo_export, but I think that "by
default" should be appended.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On Mon, Aug 14, 2017, Michael Paquier <michael.paquier@gmail.com> wrote:

Attached is a set of 3 patches:

I tried to review the patch and firstly patch applies cleanly without any
noise.

- 0001 removes ALLOW_DANGEROUS_LO_FUNCTIONS

I think it is better to remove "ALLOW_DANGEROUS_LO_FUNCTIONS" from
pg_config_manual.h as well.

- 0002 replaces the superuser checks with GRANT permissions
- 0003 refactors the LO code to improve the ACL handling. Note that
this patch introduces a more debatable change: now there is no
distinction between INV_WRITE and INV_WRITE|INV_READ, as when
INV_WRITE is used INV_READ is implied. The patch as proposed does a
complete split of both permissions to give a system more natural and
more flexible. The current behavior is documented.

@@ -163,22 +150,16 @@ lo_read(int fd, char *buf, int len)
....
+ if ((lobj->flags & IFS_RDLOCK) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("large object descriptor %d was not opened for reading",
+ fd)));

Do we ever reach this error? Because per my understanding

1) if the application didn't call lo_open before lo_read, then file
descriptor validation in the beginning of this function should capture it.

2) if the application called lo_open only with write mode should be able to
read as well per documentation.

Ok, in the inv_open(), IFS_RDLOCK is set only when explicit READ mode is
requested. So, it causes lo_read failure when large-object is opened in
WRITE mode? I think documented behavior is more appropriate and post ACL
checks for select and update permissions in inv_open(), IFS_RDLOCK flag
should be set regardless of mode specified.

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

On Tue, Sep 19, 2017 at 1:24 PM, Vaishnavi Prabakaran
<vaishnaviprabakaran@gmail.com> wrote:

On Mon, Aug 14, 2017, Michael Paquier <michael.paquier@gmail.com> wrote:

Attached is a set of 3 patches:

I tried to review the patch and firstly patch applies cleanly without any
noise.

Thanks for the review, Vaishnavi.

- 0001 removes ALLOW_DANGEROUS_LO_FUNCTIONS

I think it is better to remove "ALLOW_DANGEROUS_LO_FUNCTIONS" from
pg_config_manual.h as well.

Indeed. Fixed.

@@ -163,22 +150,16 @@ lo_read(int fd, char *buf, int len)
....
+ if ((lobj->flags & IFS_RDLOCK) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("large object descriptor %d was not opened for reading",
+ fd)));

Do we ever reach this error? Because per my understanding

This error can be reached, and it is part of the regression tests. One
query which passed previously is now failing:
+SELECT loread(lo_open(1001, x'20000'::int), 32);   -- fail, wrong mode
+ERROR:  large object descriptor 0 was not opened for reading

I think documented behavior is more appropriate and post ACL
checks for select and update permissions in inv_open(), IFS_RDLOCK flag
should be set regardless of mode specified.

OK, so that's one vote for keeping the existing API behavior with
write implying read. Thanks for the input. At least I can see no
complains about patches 1 and 2 :)
--
Michael

Hi,

On Tue, Sep 19, 2017 at 5:12 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

@@ -163,22 +150,16 @@ lo_read(int fd, char *buf, int len)
....
+ if ((lobj->flags & IFS_RDLOCK) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("large object descriptor %d was not opened for reading",
+ fd)));

Do we ever reach this error? Because per my understanding

This error can be reached, and it is part of the regression tests. One
query which passed previously is now failing:
+SELECT loread(lo_open(1001, x'20000'::int), 32);   -- fail, wrong mode
+ERROR:  large object descriptor 0 was not opened for reading

Yes, I did realize on further reading the patch and what led to the
confusion is that in the 3rd patch , updated documentation(copied below)
still says that reading from a descriptor opened with INV_WRITE is
possible. I think we need some correction here to reflect the modified code
behavior.

+     or other transactions.  Reading from a descriptor opened with
+     <symbol>INV_WRITE</symbol> or <symbol>INV_READ</> <literal>|</>
+     <symbol>INV_WRITE</symbol> returns data that reflects all writes of
+     other committed transactions as well as writes of the current
+     transaction.

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

On Tue, Sep 26, 2017 at 9:04 AM, Vaishnavi Prabakaran
<vaishnaviprabakaran@gmail.com> wrote:

Yes, I did realize on further reading the patch and what led to the
confusion is that in the 3rd patch , updated documentation(copied below)
still says that reading from a descriptor opened with INV_WRITE is possible.
I think we need some correction here to reflect the modified code behavior.

+     or other transactions.  Reading from a descriptor opened with
+     <symbol>INV_WRITE</symbol> or <symbol>INV_READ</> <literal>|</>
+     <symbol>INV_WRITE</symbol> returns data that reflects all writes of
+     other committed transactions as well as writes of the current
+     transaction.

Indeed, you are right. There is an error here. This should read as
"INV_READ | INV_WRITE" only. Using "INV_WRITE" implies that reads
cannot happen.
--
Michael

On Tue, Sep 26, 2017 at 11:45 AM, Michael Paquier <michael.paquier@gmail.com

wrote:

On Tue, Sep 26, 2017 at 9:04 AM, Vaishnavi Prabakaran
<vaishnaviprabakaran@gmail.com> wrote:

Yes, I did realize on further reading the patch and what led to the
confusion is that in the 3rd patch , updated documentation(copied below)
still says that reading from a descriptor opened with INV_WRITE is

possible.

I think we need some correction here to reflect the modified code

behavior.

+     or other transactions.  Reading from a descriptor opened with
+     <symbol>INV_WRITE</symbol> or <symbol>INV_READ</> <literal>|</>
+     <symbol>INV_WRITE</symbol> returns data that reflects all writes of
+     other committed transactions as well as writes of the current
+     transaction.

Indeed, you are right. There is an error here. This should read as
"INV_READ | INV_WRITE" only. Using "INV_WRITE" implies that reads
cannot happen.

Thanks for correcting.

I moved the cf entry to "ready for committer", and though my vote is
for keeping
the existing API behavior with write implying read, I let the committer
decide whether the following behavior change is Ok or not.
"Reading from a large-object descriptor opened with INV_WRITE is NOT
possible"

Thanks & Regards,
Vaishnavi
Fujitsu Australia.

On Tue, Sep 26, 2017 at 11:42 AM, Vaishnavi Prabakaran
<vaishnaviprabakaran@gmail.com> wrote:

I moved the cf entry to "ready for committer", and though my vote is for
keeping the existing API behavior with write implying read, I let the
committer decide whether the following behavior change is Ok or not.
"Reading from a large-object descriptor opened with INV_WRITE is NOT
possible"

Thanks for the review!
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier <michael.paquier@gmail.com> writes:

On Tue, Sep 26, 2017 at 11:42 AM, Vaishnavi Prabakaran
<vaishnaviprabakaran@gmail.com> wrote:

I moved the cf entry to "ready for committer", and though my vote is for
keeping the existing API behavior with write implying read, I let the
committer decide whether the following behavior change is Ok or not.
"Reading from a large-object descriptor opened with INV_WRITE is NOT
possible"

Thanks for the review!

After chewing on this some more, I'm inclined to agree that we should
not change the behavior of the read/write flags. There's been no
field requests for a true-write-only mode, so I think we're much more
likely to get complaints from users whose code we broke than plaudits
from people who think the change is helpful.

I believe it would be easy enough to adjust the patch so that we can
still have the refactoring benefits; we'd just need the bit that
translates from external to internal flags to go more like

if (flags & INV_WRITE)
descflags |= IFS_WRLOCK | IFS_RDLOCK;
if (flags & INV_READ)
descflags |= IFS_RDLOCK;

(Preferably with a comment about why it's like this.)

Another idea would be to invent a new external flag bit "INV_WRITE_ONLY",
so that people who wanted true write-only could get it, without breaking
backwards-compatible behavior. But I'm inclined to wait for some field
demand to show up before adding even that little bit of complication.

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

On Thu, Nov 9, 2017 at 6:05 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Michael Paquier <michael.paquier@gmail.com> writes:

On Tue, Sep 26, 2017 at 11:42 AM, Vaishnavi Prabakaran
<vaishnaviprabakaran@gmail.com> wrote:

I moved the cf entry to "ready for committer", and though my vote is for
keeping the existing API behavior with write implying read, I let the
committer decide whether the following behavior change is Ok or not.
"Reading from a large-object descriptor opened with INV_WRITE is NOT
possible"

Thanks for the review!

After chewing on this some more, I'm inclined to agree that we should
not change the behavior of the read/write flags. There's been no
field requests for a true-write-only mode, so I think we're much more
likely to get complaints from users whose code we broke than plaudits
from people who think the change is helpful.

Thanks for the input. Then that's two people favoring no changes.

I believe it would be easy enough to adjust the patch so that we can
still have the refactoring benefits; we'd just need the bit that
translates from external to internal flags to go more like

if (flags & INV_WRITE)
descflags |= IFS_WRLOCK | IFS_RDLOCK;
if (flags & INV_READ)
descflags |= IFS_RDLOCK;

(Preferably with a comment about why it's like this.)

Sure, I have updated the patch with this comment:
+   /*
+    * Historically, no difference is made between (INV_WRITE) and
+    * (INV_WRITE | INV_READ), the caller being allowed to read the large
+    * object descriptor in either case.
+    */
Of course please feel free to reword if you find something better-suited.

Another idea would be to invent a new external flag bit "INV_WRITE_ONLY",
so that people who wanted true write-only could get it, without breaking
backwards-compatible behavior. But I'm inclined to wait for some field
demand to show up before adding even that little bit of complication.

Demand that may never show up, and the current behavior on HEAD does
not receive any complains either. I am keeping the patch simple for
now. That's less aspirin needed for everybody.
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:

On Thu, Nov 9, 2017 at 6:05 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Another idea would be to invent a new external flag bit "INV_WRITE_ONLY",
so that people who wanted true write-only could get it, without breaking
backwards-compatible behavior. But I'm inclined to wait for some field
demand to show up before adding even that little bit of complication.

Demand that may never show up, and the current behavior on HEAD does
not receive any complains either. I am keeping the patch simple for
now. That's less aspirin needed for everybody.

Looks good to me, pushed.

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

Tom, Michael,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Michael Paquier <michael.paquier@gmail.com> writes:

On Thu, Nov 9, 2017 at 6:05 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Another idea would be to invent a new external flag bit "INV_WRITE_ONLY",
so that people who wanted true write-only could get it, without breaking
backwards-compatible behavior. But I'm inclined to wait for some field
demand to show up before adding even that little bit of complication.

Demand that may never show up, and the current behavior on HEAD does
not receive any complains either. I am keeping the patch simple for
now. That's less aspirin needed for everybody.

Looks good to me, pushed.

While we have been working to reduce the number of superuser() checks in
the backend in favor of having the ability to GRANT explicit rights, one
of the guideing principles has always been that capabilities which can
be used to gain superuser rights with little effort should remain
restricted to the superuser, which is why the lo_import/lo_export hadn't
been under consideration for superuser-check removal in the analysis I
provided previously.

As such, I'm not particularly thrilled to see those checks simply just
removed. If we are going to say that it's acceptable to allow
non-superusers arbitrary access to files on the filesystem as the OS
user then we would also be removing similar checks from adminpack,
something I've also been against quite clearly in past discussions.

This also didn't update the documentation regarding these functions
which clearly states that superuser is required. If we are going to
allow non-superusers access to these functions, we certainly need to
remove the claim stating that we don't do that and further we must make
it abundantly clear that these functions are extremely dangerous and
could be trivially used to make someone who has access to them into a
superuser.

I continue to feel that this is something worthy of serious
consideration and discussion, and not something to be done because we
happen to be modifying the code in this area. I'm tempted to suggest we
should have another role attribute or some other additional check when
it comes to functions like these.

The right way to allow users to be able to pull in data off the
filesystem, imv, would be by providing a way to define specific
locations on the filesystem which users can be granted access to import
data from, as I once wrote a patch to do. That's certainly quite tricky
to get correct, but that's much better than allowing non-superusers
arbitrary access, which is what this does and what users may start using
if we continue to remove these restrictions without providing a better
option.

Thanks!

Stephen

On Thu, Nov 9, 2017 at 1:16 PM, Stephen Frost <sfrost@snowman.net> wrote:

While we have been working to reduce the number of superuser() checks in
the backend in favor of having the ability to GRANT explicit rights, one
of the guideing principles has always been that capabilities which can
be used to gain superuser rights with little effort should remain
restricted to the superuser, which is why the lo_import/lo_export hadn't
been under consideration for superuser-check removal in the analysis I
provided previously.

I disagree that that is, or should be, a guiding principle. I'm not
sure that anyone other than you and your fellow employees at Crunchy
has ever agreed that this is some kind of principle. You make it
sound like there's a consensus about this, but I think there isn't.

I think our guiding principle should be to get rid of ALL of the
hard-coded superuser checks and let people GRANT what they want. If
they grant a permission that results in somebody escalating to
superuser, they get to keep both pieces. Such risks might be worth
documenting, but we shouldn't obstruct people from doing it.

In the same way, Linux will not prevent you from making a binary
setuid regardless of what the binary does. If you make a binary
setuid root that lets someone hack root, that's your fault, not the
operating system.

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

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Thu, Nov 9, 2017 at 1:16 PM, Stephen Frost <sfrost@snowman.net> wrote:

While we have been working to reduce the number of superuser() checks in
the backend in favor of having the ability to GRANT explicit rights, one
of the guideing principles has always been that capabilities which can
be used to gain superuser rights with little effort should remain
restricted to the superuser, which is why the lo_import/lo_export hadn't
been under consideration for superuser-check removal in the analysis I
provided previously.

I disagree that that is, or should be, a guiding principle.

It was what I was using as the basis of the work which I did in this
area and, at least at that time, there seemed to be little issue with
that.

I'm not
sure that anyone other than you and your fellow employees at Crunchy
has ever agreed that this is some kind of principle. You make it
sound like there's a consensus about this, but I think there isn't.

It's unclear to me why you're bringing up employers in this discussion,
particularly since Tom is the one who just moved things in the direction
that you're evidently advocating for. Clearly there isn't consensus if
you and others disagree. Things certainly can change over time as well,
but if we're going to make a change here then we should make it
willfully, plainly, and consistently.

I think our guiding principle should be to get rid of ALL of the
hard-coded superuser checks and let people GRANT what they want. If
they grant a permission that results in somebody escalating to
superuser, they get to keep both pieces. Such risks might be worth
documenting, but we shouldn't obstruct people from doing it.

I would certainly like to see the many additional hard-coded superuser
checks introduced into PG10 removed and replaced with more fine-grained
GRANT-based privileges (either as additional GRANT rights or perhaps
through the default roles system). What I dislike about allowing
GRANT's of a privilege which allows someone to be superuser is that it
makes it *look* like you're only GRANT'ing some subset of reasonable
rights when, in reality, you're actually GRANT'ing a great deal more.

This is not unlike the discussions we've had in the past around allowing
non-owners of a table to modify properties of a table, where the
argument has been successfully and clearly made that if you make the
ability to change the table a GRANT'able right, then that can be used to
become the role which owns the table without much difficulty, and so
there isn't really a point in making that right independently
GRANT'able. We have some of that explicitly GRANT'able today due to
requirements of the spec, but that's outside of our control.

In the same way, Linux will not prevent you from making a binary
setuid regardless of what the binary does. If you make a binary
setuid root that lets someone hack root, that's your fault, not the
operating system.

This is actually one of the things that SELinux is intended to address,
so I don't agree that it's quite this cut-and-dry.

Thanks!

Stephen

On Thu, Nov 9, 2017 at 1:52 PM, Stephen Frost <sfrost@snowman.net> wrote:

I disagree that that is, or should be, a guiding principle.

It was what I was using as the basis of the work which I did in this
area and, at least at that time, there seemed to be little issue with
that.

Well, there actually kind of was. It came up here:

/messages/by-id/CA+TgmoY6nE5n4Jc5aWxSer2g2GDgR4oMf7EdCeXamVPF_JqUzQ@mail.gmail.com

I mis-remembered who was on which side of the debate, though, hence
the comment about employers. But never mind about that, since I was
wrong. Sorry for not checking that more carefully before.

This is not unlike the discussions we've had in the past around allowing
non-owners of a table to modify properties of a table, where the
argument has been successfully and clearly made that if you make the
ability to change the table a GRANT'able right, then that can be used to
become the role which owns the table without much difficulty, and so
there isn't really a point in making that right independently
GRANT'able. We have some of that explicitly GRANT'able today due to
requirements of the spec, but that's outside of our control.

I don't think it's quite the same thing. I wouldn't go out of my way
to invent grantable table rights that just let you usurp the table
owner's permissions, but I still think it's better to have a uniform
rule that functions we ship don't contain hard-coded superuser checks.
One problem is that which functions allow an escalation to superuser
that is easy enough or reliable enough may not actually be a very
bright line in all cases. But more than that, I think it's a
legitimate decision to grant to a user a right that COULD lead to a
superuser escalation and trust them not to use that way, or prevent
them from using it that way by some means not known to the database
system (e.g. route all queries through pgbouncer and send them to a
teletype; if a breach is detected, go to the teletype room, read the
paper contained therein, and decide who to fire/imprison/execute at
gunpoint). It shouldn't be our job to decide that granting a certain
right is NEVER ok.

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

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Thu, Nov 9, 2017 at 1:52 PM, Stephen Frost <sfrost@snowman.net> wrote:

This is not unlike the discussions we've had in the past around allowing
non-owners of a table to modify properties of a table, where the
argument has been successfully and clearly made that if you make the
ability to change the table a GRANT'able right, then that can be used to
become the role which owns the table without much difficulty, and so
there isn't really a point in making that right independently
GRANT'able. We have some of that explicitly GRANT'able today due to
requirements of the spec, but that's outside of our control.

I don't think it's quite the same thing. I wouldn't go out of my way
to invent grantable table rights that just let you usurp the table
owner's permissions, but I still think it's better to have a uniform
rule that functions we ship don't contain hard-coded superuser checks.

Just to be clear, we should certainly be thinking about more than just
functions but also about things like publications and subscriptions and
other bits of the system. I haven't done a detailed analysis quite yet,
but I'm reasonably confident that a number of things in this last
release cycle have resulted in quite a few additional explicit superuser
checks, which I do think is an issue to be addressed.

One problem is that which functions allow an escalation to superuser
that is easy enough or reliable enough may not actually be a very
bright line in all cases. But more than that, I think it's a
legitimate decision to grant to a user a right that COULD lead to a
superuser escalation and trust them not to use that way, or prevent
them from using it that way by some means not known to the database
system (e.g. route all queries through pgbouncer and send them to a
teletype; if a breach is detected, go to the teletype room, read the
paper contained therein, and decide who to fire/imprison/execute at
gunpoint). It shouldn't be our job to decide that granting a certain
right is NEVER ok.

I agree that it may not be obvious which cases lead to a relatively easy
way to obtain superuser and which don't, and that's actually why I'd
much rather it be something that we're considering and looking out for
because, frankly, we're in a much better position generally to realize
those cases than our users are. Further, I agree entirely that we
shouldn't be deciding that certain capabilities are never allowed to be
given to a user- but that's why superuser *exists* and why it's possible
to give superuser access to multiple users. The question here is if
it's actually sensible for us to make certain actions be grantable to
non-superusers which allow that role to become, or to essentially be, a
superuser. What that does, just as it does in the table case, from my
viewpoint at least, is make it *look* to admins like they're grant'ing
something less than superuser when, really, they're actually giving the
user superuser-level access. That violates the POLA because the admin
didn't write 'ALTER USER joe WITH SUPERUSER', they just wrote 'GRANT
EXECUTE ON lo_import() TO joe;'.

We can certainly argue about if the admin should have just known that
lo_import()/lo_export() or similar functions were equivilant to
grant'ing a user superuser access, but it's not entirely clear to me
that it's actually advantageous to go out of our way to provide multiple
ways for superuser-level access to be grant'ed out to users. Let's
provide one very clear way to do that and strive to avoid building in
others, either intentionally or unintentionally.

Thanks!

Stephen

On Thu, Nov 9, 2017 at 2:56 PM, Stephen Frost <sfrost@snowman.net> wrote:

I agree that it may not be obvious which cases lead to a relatively easy
way to obtain superuser and which don't, and that's actually why I'd
much rather it be something that we're considering and looking out for
because, frankly, we're in a much better position generally to realize
those cases than our users are.

I disagree. It's flattering to imagine that PostgreSQL developers, as
a class, are smarter than PostgreSQL users, but it doesn't match my
observations.

Further, I agree entirely that we
shouldn't be deciding that certain capabilities are never allowed to be
given to a user- but that's why superuser *exists* and why it's possible
to give superuser access to multiple users. The question here is if
it's actually sensible for us to make certain actions be grantable to
non-superusers which allow that role to become, or to essentially be, a
superuser. What that does, just as it does in the table case, from my
viewpoint at least, is make it *look* to admins like they're grant'ing
something less than superuser when, really, they're actually giving the
user superuser-level access. That violates the POLA because the admin
didn't write 'ALTER USER joe WITH SUPERUSER', they just wrote 'GRANT
EXECUTE ON lo_import() TO joe;'.

This is exactly the kind of thing that I'm talking about. Forcing an
administrator to hand out full superuser access instead of being able
to give just lo_import() makes life difficult for users for no good
reason. The existence of a theoretically-exploitable security
vulnerability is not tantamount to really having access, especially on
a system with a secured logging facility.

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

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Nov 9, 2017 at 2:56 PM, Stephen Frost <sfrost@snowman.net> wrote:

Further, I agree entirely that we
shouldn't be deciding that certain capabilities are never allowed to be
given to a user- but that's why superuser *exists* and why it's possible
to give superuser access to multiple users. The question here is if
it's actually sensible for us to make certain actions be grantable to
non-superusers which allow that role to become, or to essentially be, a
superuser. What that does, just as it does in the table case, from my
viewpoint at least, is make it *look* to admins like they're grant'ing
something less than superuser when, really, they're actually giving the
user superuser-level access. That violates the POLA because the admin
didn't write 'ALTER USER joe WITH SUPERUSER', they just wrote 'GRANT
EXECUTE ON lo_import() TO joe;'.

This is exactly the kind of thing that I'm talking about. Forcing an
administrator to hand out full superuser access instead of being able
to give just lo_import() makes life difficult for users for no good
reason. The existence of a theoretically-exploitable security
vulnerability is not tantamount to really having access, especially on
a system with a secured logging facility.

Exactly. I think that Stephen's argument depends on what a black-hat
privilege recipient could theoretically do, but fails to consider what's
useful for white-hat users. One of the standard rules for careful admins
is to do as little as possible as root/superuser. If you have a situation
where it's necessary to use, say, lo_import as part of a routine data
import task, then the only alternative previously was to do that task as
superuser. That is not an improvement, by any stretch of the imagination,
over granting lo_import privileges to some otherwise-vanilla role that can
run the data import task.

We've previously discussed workarounds such as creating SECURITY DEFINER
wrapper functions, but I don't think that approach does much to change the
terms of the debate: it'd still be better if the wrapper function itself
didn't need full superuser.

I did miss the need to fix the docs, and am happy to put in some strong
wording about the security hazards of these functions while fixing the
docs. But I do not think that leaving them with hardwired superuser
checks is an improvement over being able to control them with GRANT.

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

On Fri, Nov 10, 2017 at 7:05 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I did miss the need to fix the docs, and am happy to put in some strong
wording about the security hazards of these functions while fixing the
docs. But I do not think that leaving them with hardwired superuser
checks is an improvement over being able to control them with GRANT.

Sorry about that. lobj.sgml indeed mentions superusers. Do you need a patch?
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers