Thread: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks

Hi all,

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
- 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. Please note as well
that it is possible to implement a patch that keeps compatibility as
well, but I would welcome a debate on the matter. This patch owns also
a good deal to Tom.

I am parking them to the next commit fest for PG11.

Thanks,
-- 
Michael

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

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

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

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

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

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

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

http://postgr.es/m/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

Michael Paquier <michael.paquier@gmail.com> writes:
> 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?

No, I can write it.  But I'm going to wait to see where this debate
settles before expending effort on the docs.
        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

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> 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.

This is where I disagree.  The administrator *is* giving the user
superuser-level access, they're just doing it in a different way from
explicitly saying 'ALTER USER .. WITH SUPERUSER' and that's exactly the
issue that I have.  This isn't a theoretical exploit- the user with
lo_import/lo_export access is able to read and write any file on the
filesystem which the PG OS has access to, including things like
pg_hba.conf in most configurations, the file backing pg_authid, the OS
user's .bashrc, the Kerberos principal, the CA public key, the SSL keys,
tables owned by other users that the in-database role doesn't have
access to, et al.  Further, will we consider these *actual*,
non-theoretical, methods to gain superuser access, or to bypass the
database authorization system, to be security issues or holes to plug?

I'm guessing no, which essentially means that *we* consider access to
lo_import/lo_export to be equivilant to superuser and therefore we're
not going to implement anything to try and prevent the user who has
access to those functions from becoming superuser.  If we aren't willing
to do that, then how can we really say that there's some difference
between access to these functions and being a superuser?

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> I'm guessing no, which essentially means that *we* consider access to
> lo_import/lo_export to be equivilant to superuser and therefore we're
> not going to implement anything to try and prevent the user who has
> access to those functions from becoming superuser.  If we aren't willing
> to do that, then how can we really say that there's some difference
> between access to these functions and being a superuser?

We seem to be talking past each other.  Yes, if a user has malicious
intentions, it's possibly to parlay lo_export into obtaining a superuser
login (I'm less sure that that's necessarily true for lo_import).
That does NOT make it "equivalent", except perhaps in the view of someone
who is only considering blocking malevolent actors.  It does not mean that
there's no value in preventing a task that needs to run lo_export from
being able to accidentally destroy any data in the database.  There's a
range of situations where you are concerned about accidents and errors,
not malicious intent; but your argument ignores those use-cases.

To put it more plainly: your argument is much like saying that a person
who knows a sudo password might as well do everything they ever do as
root.
        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 10:00 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Stephen Frost <sfrost@snowman.net> writes:
>> I'm guessing no, which essentially means that *we* consider access to
>> lo_import/lo_export to be equivilant to superuser and therefore we're
>> not going to implement anything to try and prevent the user who has
>> access to those functions from becoming superuser.  If we aren't willing
>> to do that, then how can we really say that there's some difference
>> between access to these functions and being a superuser?
>
> We seem to be talking past each other.  Yes, if a user has malicious
> intentions, it's possibly to parlay lo_export into obtaining a superuser
> login (I'm less sure that that's necessarily true for lo_import).
> That does NOT make it "equivalent", except perhaps in the view of someone
> who is only considering blocking malevolent actors.  It does not mean that
> there's no value in preventing a task that needs to run lo_export from
> being able to accidentally destroy any data in the database.  There's a
> range of situations where you are concerned about accidents and errors,
> not malicious intent; but your argument ignores those use-cases.

That will not sound much as a surprise as I spawned the original
thread, but like Robert I understand that getting rid of all superuser
checks is a goal that we are trying to reach to allow admins to have
more flexibility in handling permissions to a subset of objects.
Forcing an admin to give full superuser rights to one user willing to
work only on LOs import and export is a wrong concept.
-- 
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:
> That will not sound much as a surprise as I spawned the original
> thread, but like Robert I understand that getting rid of all superuser
> checks is a goal that we are trying to reach to allow admins to have
> more flexibility in handling permissions to a subset of objects.
> Forcing an admin to give full superuser rights to one user willing to
> work only on LOs import and export is a wrong concept.

Right.  I think the question may boil down to how we document this.
The current para reads
   The server-side <function>lo_import</function> and   <function>lo_export</function> functions behave considerably
differently  from their client-side analogs.  These two functions read and write files   in the server's file system,
usingthe permissions of the database's   owning user.  Therefore, their use is restricted to superusers.  In
contrast,the client-side import and export functions read and write files   in the client's file system, using the
permissionsof the client program.   The client-side functions do not require superuser privilege.
 

I think as far as that goes, we can just change to "Therefore, by default
their use is restricted ...".  Then I suggest adding a <caution> para
after that, with wording along the lines of
   It is possible to GRANT use of server-side lo_import and lo_export to   non-superusers, but careful consideration of
thesecurity implications   is required.  A malicious user of such privileges could easily parlay   them into becoming
superuser(for example by rewriting server   configuration files), or could attack the rest of the server's file
systemwithout bothering to obtain database superuser privileges as   such.  Access to roles having such privilege must
thereforebe guarded   just as carefully as access to superuser roles.  Nonetheless, if use   of server-side lo_import
orlo_export is needed for some routine task,   it's safer to use a role of this sort than full superuser privilege,
asthat helps to reduce the risk of damage from accidental errors.
 

We could expand that by mentioning the possibility of wrapper functions,
but it seems long enough already.

Comments?
        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 10:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think as far as that goes, we can just change to "Therefore, by default
> their use is restricted ...".  Then I suggest adding a <caution> para
> after that, with wording along the lines of
>
>     It is possible to GRANT use of server-side lo_import and lo_export to
>     non-superusers, but careful consideration of the security implications
>     is required.  A malicious user of such privileges could easily parlay
>     them into becoming superuser (for example by rewriting server
>     configuration files), or could attack the rest of the server's file
>     system without bothering to obtain database superuser privileges as
>     such.  Access to roles having such privilege must therefore be guarded
>     just as carefully as access to superuser roles.  Nonetheless, if use
>     of server-side lo_import or lo_export is needed for some routine task,
>     it's safer to use a role of this sort than full superuser privilege,
>     as that helps to reduce the risk of damage from accidental errors.

+1.  That seems like great language to me.

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

Michael, Tom,

* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Fri, Nov 10, 2017 at 10:00 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Stephen Frost <sfrost@snowman.net> writes:
> >> I'm guessing no, which essentially means that *we* consider access to
> >> lo_import/lo_export to be equivilant to superuser and therefore we're
> >> not going to implement anything to try and prevent the user who has
> >> access to those functions from becoming superuser.  If we aren't willing
> >> to do that, then how can we really say that there's some difference
> >> between access to these functions and being a superuser?
> >
> > We seem to be talking past each other.  Yes, if a user has malicious
> > intentions, it's possibly to parlay lo_export into obtaining a superuser
> > login (I'm less sure that that's necessarily true for lo_import).
> > That does NOT make it "equivalent", except perhaps in the view of someone
> > who is only considering blocking malevolent actors.  It does not mean that
> > there's no value in preventing a task that needs to run lo_export from
> > being able to accidentally destroy any data in the database.  There's a
> > range of situations where you are concerned about accidents and errors,
> > not malicious intent; but your argument ignores those use-cases.
>
> That will not sound much as a surprise as I spawned the original
> thread, but like Robert I understand that getting rid of all superuser
> checks is a goal that we are trying to reach to allow admins to have
> more flexibility in handling permissions to a subset of objects.
> Forcing an admin to give full superuser rights to one user willing to
> work only on LOs import and export is a wrong concept.

The problem I have with this is that 'full superuser rights' actually
are being granted by this.  You're able to read any file and write any
file on the filesystem that the PG OS user can.  How is that not 'full
superuser rights'?  The concerns about a mistake being made are just as
serious as they are with someone who has superuser rights as someone
could pretty easily end up overwriting the control file or various other
extremely important bits of the system.  This isn't just about what
'blackhats' can do with this.

As I mentioned up-thread, if we want to make it so that non-superusers
can use lo_import/lo_export, then we should do that in a way that
allows the administrator to specify exactly the rights they really want
to give, based on a use-case for them.  There's no use-case for allowing
someone to use lo_export() to overwrite pg_control.  The use-case would
be to allow them to export to a specific directory (or perhaps a set of
directories), or to read from same.  The same is true of COPY.  Further,
I wonder what would happen if we allow this and then someone comes along
and re-proposes the 'CREATE DIRECTORY' concept that I had once proposed
(ideally with things cleaned up and tightened up to avoid there being
issues using it) to address the actual use-case for these functions to
be available to a non-superuser.  We wouldn't be able to change these
functions to start checking the 'directory' rights or we would break
existing non-superuser usage of them.  I imagine we could create
additional functions which check the 'directory' privileges, but that's
hardly ideal, imv.

I'm disappointed to apparently be in the minority on this, but that
seems to be the case unless someone else wishes to comment.  While I
appreciate the discussion, I'm certainly no more convinced today than I
was when I first saw this go in that allowing these functions to be
granted to non-superusers does anything but effectively make them into
superusers who aren't explicitly identified as superusers.

Thanks!

Stephen

On Sat, Nov 11, 2017 at 12:50 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Nov 10, 2017 at 10:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think as far as that goes, we can just change to "Therefore, by default
>> their use is restricted ...".  Then I suggest adding a <caution> para
>> after that, with wording along the lines of
>>
>>     It is possible to GRANT use of server-side lo_import and lo_export to
>>     non-superusers, but careful consideration of the security implications
>>     is required.  A malicious user of such privileges could easily parlay
>>     them into becoming superuser (for example by rewriting server
>>     configuration files), or could attack the rest of the server's file
>>     system without bothering to obtain database superuser privileges as
>>     such.  Access to roles having such privilege must therefore be guarded
>>     just as carefully as access to superuser roles.  Nonetheless, if use
>>     of server-side lo_import or lo_export is needed for some routine task,
>>     it's safer to use a role of this sort than full superuser privilege,
>>     as that helps to reduce the risk of damage from accidental errors.
>
> +1.  That seems like great language to me.

+1. Not convinced that mentioning wrappers is worth the complication.
Experienced admins likely already know such matters.
-- 
Michael


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

Stephen Frost <sfrost@snowman.net> writes:
> * Michael Paquier (michael.paquier@gmail.com) wrote:
>> Forcing an admin to give full superuser rights to one user willing to
>> work only on LOs import and export is a wrong concept.

> The problem I have with this is that 'full superuser rights' actually
> are being granted by this.  You're able to read any file and write any
> file on the filesystem that the PG OS user can.  How is that not 'full
> superuser rights'?

It doesn't cause, say, "DELETE FROM pg_proc;" to succeed.

You're still not getting the point that this is for people who want
self-imposed restrictions.  Sure, you can't give out lo_export privilege
to someone you would not trust with superuser.  But somebody who needs
lo_export, and is trustworthy enough to have it, may still wish to do
the inside-the-database part of their work with less than full superuser,
just as a safety measure.  It's the *exact same* reason why cautious
people use "sudo" rather than just running as root all the time.

> As I mentioned up-thread, if we want to make it so that non-superusers
> can use lo_import/lo_export, then we should do that in a way that
> allows the administrator to specify exactly the rights they really want
> to give, based on a use-case for them.

Our current answer for that is wrapper functions.  This patch does not
make that answer any less applicable than before.

> I wonder what would happen if we allow this and then someone comes along
> and re-proposes the 'CREATE DIRECTORY' concept that I had once proposed
> (ideally with things cleaned up and tightened up to avoid there being
> issues using it) to address the actual use-case for these functions to
> be available to a non-superuser.  We wouldn't be able to change these
> functions to start checking the 'directory' rights or we would break
> existing non-superuser usage of them.

This is a straw man argument --- if we tighten up the function to check
this as-yet-nonexistent feature, how is that not breaking existing
use-cases anyway?
        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 Nov 10, 2017, at 3:58 PM, Stephen Frost <sfrost@snowman.net> wrote:
>
> Michael, Tom,
>
> * Michael Paquier (michael.paquier@gmail.com) wrote:
>> On Fri, Nov 10, 2017 at 10:00 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Stephen Frost <sfrost@snowman.net> writes:
>>>> I'm guessing no, which essentially means that *we* consider access to
>>>> lo_import/lo_export to be equivilant to superuser and therefore we're
>>>> not going to implement anything to try and prevent the user who has
>>>> access to those functions from becoming superuser.  If we aren't willing
>>>> to do that, then how can we really say that there's some difference
>>>> between access to these functions and being a superuser?
>>>
>>> We seem to be talking past each other.  Yes, if a user has malicious
>>> intentions, it's possibly to parlay lo_export into obtaining a superuser
>>> login (I'm less sure that that's necessarily true for lo_import).
>>> That does NOT make it "equivalent", except perhaps in the view of someone
>>> who is only considering blocking malevolent actors.  It does not mean that
>>> there's no value in preventing a task that needs to run lo_export from
>>> being able to accidentally destroy any data in the database.  There's a
>>> range of situations where you are concerned about accidents and errors,
>>> not malicious intent; but your argument ignores those use-cases.
>>
>> That will not sound much as a surprise as I spawned the original
>> thread, but like Robert I understand that getting rid of all superuser
>> checks is a goal that we are trying to reach to allow admins to have
>> more flexibility in handling permissions to a subset of objects.
>> Forcing an admin to give full superuser rights to one user willing to
>> work only on LOs import and export is a wrong concept.
>
> The problem I have with this is that 'full superuser rights' actually
> are being granted by this.  You're able to read any file and write any
> file on the filesystem that the PG OS user can.  How is that not 'full
> superuser rights'?  The concerns about a mistake being made are just as
> serious as they are with someone who has superuser rights as someone
> could pretty easily end up overwriting the control file or various other
> extremely important bits of the system.  This isn't just about what
> 'blackhats' can do with this.
>
> As I mentioned up-thread, if we want to make it so that non-superusers
> can use lo_import/lo_export, then we should do that in a way that
> allows the administrator to specify exactly the rights they really want
> to give, based on a use-case for them.  There's no use-case for allowing
> someone to use lo_export() to overwrite pg_control.  The use-case would
> be to allow them to export to a specific directory (or perhaps a set of
> directories), or to read from same.  The same is true of COPY.  Further,
> I wonder what would happen if we allow this and then someone comes along
> and re-proposes the 'CREATE DIRECTORY' concept that I had once proposed
> (ideally with things cleaned up and tightened up to avoid there being
> issues using it) to address the actual use-case for these functions to
> be available to a non-superuser.  We wouldn't be able to change these
> functions to start checking the 'directory' rights or we would break
> existing non-superuser usage of them.  I imagine we could create
> additional functions which check the 'directory' privileges, but that's
> hardly ideal, imv.
>
> I'm disappointed to apparently be in the minority on this, but that
> seems to be the case unless someone else wishes to comment.  While I
> appreciate the discussion, I'm certainly no more convinced today than I
> was when I first saw this go in that allowing these functions to be
> granted to non-superusers does anything but effectively make them into
> superusers who aren't explicitly identified as superusers.

Just to help understand your concern, and not to propose actual features,
would it help if there were

(1) a function, perhaps pg_catalog.pg_exploiters(), which would return all
roles that have been granted exploitable privileges, such that it would be
easier to identify all superusers, including those whose superuserishness
derives from a known export, and

(2) a syntax change for GRANT that would require an extra token, so
that you'd have to write something like
GRANT EXPLOITABLE lo_export TO trusted_user_foo

so that you couldn't unknowingly grant a dangerous privilege.

Or is there more to it than that?

mark







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

On Sat, Nov 11, 2017 at 9:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Sat, Nov 11, 2017 at 12:50 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Fri, Nov 10, 2017 at 10:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I think as far as that goes, we can just change to "Therefore, by default
>>> their use is restricted ...".  Then I suggest adding a <caution> para
>>> after that, with wording along the lines of
>>>
>>>     It is possible to GRANT use of server-side lo_import and lo_export to
>>>     non-superusers, but careful consideration of the security implications
>>>     is required.  A malicious user of such privileges could easily parlay
>>>     them into becoming superuser (for example by rewriting server
>>>     configuration files), or could attack the rest of the server's file
>>>     system without bothering to obtain database superuser privileges as
>>>     such.  Access to roles having such privilege must therefore be guarded
>>>     just as carefully as access to superuser roles.  Nonetheless, if use
>>>     of server-side lo_import or lo_export is needed for some routine task,
>>>     it's safer to use a role of this sort than full superuser privilege,
>>>     as that helps to reduce the risk of damage from accidental errors.
>>
>> +1.  That seems like great language to me.
>
> +1. Not convinced that mentioning wrappers is worth the complication.
> Experienced admins likely already know such matters.

For archives' sake, doc improvements are committed as of 6d77652.
-- 
Michael