Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Noah Misch]

On Fri, Jul 20, 2012 at 03:39:33PM -0400, Robert Haas wrote:
 On Fri, Jul 20, 2012 at 1:52 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  And with that too.  The STRICT option is a fairly obvious security
  hazard, but who's to say there are not others?  I think it'd be easier
  to make a case for forbidding a non-superuser from altering *any*
  property of a C function.  I'd rather start from the point of allowing
  only what is clearly safe than disallowing only what is clearly unsafe.
 
 That seems like a fairly drastic overreaction.  Are you going to ban
 renaming it or changing the owner, which are in completely different
 code paths?  Yuck.  Even if you only ban it for the main ALTER
 FUNCTION code path, it seems pretty draconian, because it looks to me
 like nearly everything else that's there is perfectly safe.  I mean,
 assuming the guy who wrote the C code didn't do anything completely
 insane or malicious, setting GUCs or whatever should be perfectly OK.
 Honestly, if you want to change something in the code, I'm not too
 convinced that there's anything better than what Noah proposed
 originally.

How about a compromise of blocking GUC and STRICT changes while allowing
everything else?  We add PGC_USERSET GUCs in most releases.  As long as
non-superuser owners of trusted-language functions can change attached GUC
settings, review for each new GUC really ought to consider that scenario.
That will be easy to forget.  I'm already wary about allowing changes to GUCs
like sql_inheritance and search_path.  By contrast, the list of ALTER FUNCTION
alterations has grown slowly; the last addition before PostgreSQL 9.2 came in
PostgreSQL 8.3.  Anyone implementing a new alteration will be modifying
AlterFunction() and have ample opportunity to notice from surrounding code the
need to identify a suitable permissions check.  Also, like you say, the other
existing alterations are clearly safe.

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Robert Haas]

On Tue, Jun 12, 2012 at 5:12 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Kevin Grittner kevin.gritt...@wicourts.gov writes:
 Rather than trying to enforce this in the
 ALTER FUNCTION implementation, maybe we should just advise that if
 you're going to grant ownership of a C function to a role which
 might accidentally or maliciously allow it to be called with NULL
 input, the C function should return NULL in that case.

 Yeah, the just-code-defensively option is worth considering too.

After rereading this thread, I think I agree with Kevin as well.  In
order to have a problem, you have to (a) be superuser (i.e. be a
person who already has many ways of compromising the security and
integrity of the system), (b) decide to grant ownership of a C
function to a non-superuser, and (c) fail to verify that the function
is coded in a sufficiently defensive fashion to survive whatever that
user might do with it.  So it seems plausible to simply define this
problem as administrator error rather than a failure of security.

Having said that, I do believe that answer is to some extent a
cop-out.  It's practical to define things that way here because the
cost of checking for NULL inputs is pretty darn small.  But suppose
ALTER FUNCTION had a facility to change the type of a function
argument.  Would we then insist that any C function intended to be
used in this way must check that the type of each argument matches the
function's expectation?  Fortunately, we don't have to decide that
right now because ALTER FUNCTION doesn't allow that and probably never
will.  But it would certainly be awkward if we did someday allow that,
because now any C function that is intended to be used in this way has
to include this extra check or be labelled insecure.  Short of
allowing the C code to advertise the function's expectations so that
CREATE/ALTER FUNCTION can cross-check them, I don't see a way out of
that problem because on the flip side, the C code could - not to
put too fine a point on it - be relying on just about anything.  It
could assert that work_mem is less than 1GB (and the user could crash
it by increasing work_mem).  It could crash on any day except Tuesday
(we always do payroll here on Tuesdays, so why would you call it on
any other day?).  It could erase the entire database unless it's
marked immutable.  We would surely blame any of those failure modes on
the person who wrote the C code, and if we make the opposite decision
here, then I think we put ourselves in the awkward position of trying
to adjudicate what is and is not reasonably for third parties to do in
their C code.  I don't really want to go there, but I do think this
points to the need for extreme caution if we ever create any more
function properties that C coders might be tempted to rely on, or
allow any properties that C code may already be relying on to be
changed in new ways.

I'm going to mark this patch as rejected in the CF app, which is not
intended to foreclose debate on what to do about this, but just to
state that there seems to be no consensus to solve this problem in the
manner proposed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Tom Lane]

Robert Haas robertmh...@gmail.com writes:
 On Tue, Jun 12, 2012 at 5:12 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Yeah, the just-code-defensively option is worth considering too.

 After rereading this thread, I think I agree with Kevin as well. ...
 Having said that, I do believe that answer is to some extent a
 cop-out.

I agree with that --- doing nothing at all doesn't seem like the best
option here.

 ... on the flip side, the C code could - not to
 put too fine a point on it - be relying on just about anything.

And with that too.  The STRICT option is a fairly obvious security
hazard, but who's to say there are not others?  I think it'd be easier
to make a case for forbidding a non-superuser from altering *any*
property of a C function.  I'd rather start from the point of allowing
only what is clearly safe than disallowing only what is clearly unsafe.

Taking a step or two back, I think that the real use-case we should
be considering here is allowing non-superusers to own (or at least
install) extensions that contain C functions.  We would probably want
the non-superuser to be able to drop the extension again, maybe
ALTER EXTENSION SET SCHEMA, maybe ALTER EXTENSION OWNER ... and likely
not too darn much else.  Fooling with any of the contained objects
doesn't seem like something we want to permit, since it's likely that
something like a datatype is going to have dependencies on not just
specific objects' properties but their interrelationships.

One possible approach to that is to say that the nominal owner of such
an extension only owns the extension itself, and ownership of the
contained objects is held by, say, the bootstrap superuser.  There are
other ways too of course, but this way would bypass the problem of
figuring out how to restrict what an object's nominal owner can do
to it.

regards, tom lane

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Robert Haas]

On Fri, Jul 20, 2012 at 1:52 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Tue, Jun 12, 2012 at 5:12 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Yeah, the just-code-defensively option is worth considering too.

 After rereading this thread, I think I agree with Kevin as well. ...
 Having said that, I do believe that answer is to some extent a
 cop-out.

 I agree with that --- doing nothing at all doesn't seem like the best
 option here.

 ... on the flip side, the C code could - not to
 put too fine a point on it - be relying on just about anything.

 And with that too.  The STRICT option is a fairly obvious security
 hazard, but who's to say there are not others?  I think it'd be easier
 to make a case for forbidding a non-superuser from altering *any*
 property of a C function.  I'd rather start from the point of allowing
 only what is clearly safe than disallowing only what is clearly unsafe.

That seems like a fairly drastic overreaction.  Are you going to ban
renaming it or changing the owner, which are in completely different
code paths?  Yuck.  Even if you only ban it for the main ALTER
FUNCTION code path, it seems pretty draconian, because it looks to me
like nearly everything else that's there is perfectly safe.  I mean,
assuming the guy who wrote the C code didn't do anything completely
insane or malicious, setting GUCs or whatever should be perfectly OK.
Honestly, if you want to change something in the code, I'm not too
convinced that there's anything better than what Noah proposed
originally.

 Taking a step or two back, I think that the real use-case we should
 be considering here is allowing non-superusers to own (or at least
 install) extensions that contain C functions.  We would probably want
 the non-superuser to be able to drop the extension again, maybe
 ALTER EXTENSION SET SCHEMA, maybe ALTER EXTENSION OWNER ... and likely
 not too darn much else.  Fooling with any of the contained objects
 doesn't seem like something we want to permit, since it's likely that
 something like a datatype is going to have dependencies on not just
 specific objects' properties but their interrelationships.

Moreover, it breaks dump-and-restore.

 One possible approach to that is to say that the nominal owner of such
 an extension only owns the extension itself, and ownership of the
 contained objects is held by, say, the bootstrap superuser.  There are
 other ways too of course, but this way would bypass the problem of
 figuring out how to restrict what an object's nominal owner can do
 to it.

I don't particularly care for that solution; it seems like a kludge.
I've kind of wondered whether we ought to have checks in all the ALTER
routines that spit up if you try to ALTER an extension member from any
place other than an extension upgrade script...  but that still
wouldn't prevent the extension owner from dropping the members out of
the extension and then modifying them afterwards.  I'm not sure we
want to prevent that in general, but maybe there could be some
locked-down mode that has that effect.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Tom Lane]

Robert Haas robertmh...@gmail.com writes:
 I don't particularly care for that solution; it seems like a kludge.
 I've kind of wondered whether we ought to have checks in all the ALTER
 routines that spit up if you try to ALTER an extension member from any
 place other than an extension upgrade script...  but that still
 wouldn't prevent the extension owner from dropping the members out of
 the extension and then modifying them afterwards.  I'm not sure we
 want to prevent that in general, but maybe there could be some
 locked-down mode that has that effect.

Right, I wasn't too clear about that, but I meant that we'd have some
sort of locked-down state for an extension that would forbid fooling
with its contents.  For development purposes, or for anybody that knows
what they're doing, adding/subtracting/modifying member objects is
mighty handy.  But a non-superuser who's loaded an extension that
contains C functions ought not have those privileges for it.

regards, tom lane

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Robert Haas]

On Fri, Jul 20, 2012 at 3:45 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 I don't particularly care for that solution; it seems like a kludge.
 I've kind of wondered whether we ought to have checks in all the ALTER
 routines that spit up if you try to ALTER an extension member from any
 place other than an extension upgrade script...  but that still
 wouldn't prevent the extension owner from dropping the members out of
 the extension and then modifying them afterwards.  I'm not sure we
 want to prevent that in general, but maybe there could be some
 locked-down mode that has that effect.

 Right, I wasn't too clear about that, but I meant that we'd have some
 sort of locked-down state for an extension that would forbid fooling
 with its contents.  For development purposes, or for anybody that knows
 what they're doing, adding/subtracting/modifying member objects is
 mighty handy.  But a non-superuser who's loaded an extension that
 contains C functions ought not have those privileges for it.

I could see having such a mode.  I'm not sure that it would eliminate
people's desire to manually give away functions, though.  In fact,
thinking about a couple of our customers, I'm pretty sure it wouldn't.
 Now whether it's a good idea is another question, but...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Noah Misch]

On Tue, Jun 12, 2012 at 03:13:26PM -0400, Tom Lane wrote:
 Even if I accepted that premise, this patch is a pretty bad
 implementation of it, because it restricts cases that there is no
 reason to think are unsafe.
 
 A less bizarre and considerably more future-proof restriction, IMO,
 would simply refuse any attempt to give ownership of a C function
 to a non-superuser.

That just moves the collateral damage to a different set of victims.

Hardcoding the list of vulnerable languages isn't so future-proof.  I'd
otherwise agree in principle if we were designing a system from scratch, but
it doesn't fit with the need to harmoniously protect existing systems.  Adding
this restriction now would make some existing databases fail to restore from
dumps.  That is grave enough at any juncture, let alone for a backpatched fix.

On Tue, Jun 12, 2012 at 04:14:48PM -0400, Tom Lane wrote:
 Basically, if we go down the road Noah is proposing, I foresee a steady
 stream of security bugs and ensuing random restrictions on what function
 owners can do.  I do not like that future.

Then let's have every ALTER FUNCTION require the same language usage check as
CREATE FUNCTION.  Or, if you insist, do so only for the hardcoded cases of
INTERNALlanguageId and ClanguageId, and document that no third-party PL may
rely on STRICT to the extent they do.  This of course forbids more than
necessary but still strictly less than your proposal of blocking the original
ownership change.

nm

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Noah Misch]

On Mon, Jun 11, 2012 at 11:03:10PM -0400, Tom Lane wrote:
 Noah Misch n...@leadboat.com writes:
  CREATE FUNCTION + ALTER FUNCTION OWNER TO is useful for creating another
  user's untrusted-language SECURITY DEFINER function.  ALTER FUNCTION 
  CALLED ON
  NULL INPUT ought to require that the user be eligible to redefine the 
  function
  completely.
 
  Here's a patch implementing that restriction.  To clarify, I see no need to
  repeat *all* the CREATE-time checks; for example, there's no need to recheck
  permission to use the return type.  The language usage check is enough.
 
 This seems bizarre and largely unnecessary.  As you stated to begin
 with, granting ownership of a function implies some degree of trust.

Yes, but I would never expect that level of trust to include access to crash
the server as a consequence of the function's reliance on STRICT.

 I do not want to get into the business of parsing exactly which variants
 of ALTER FUNCTION ought to be considered safe.

Fair concern.

 And I definitely don't
 want to add a check that enforces restrictions against cases that have
 got nothing whatever to do with C-language functions, as this patch
 does.

We don't have a principled basis for assuming that this hazard cannot apply to
third-party untrusted languages.  We could add another pg_language flag to
make the distinction for languages like plperlu.  They're untrusted by virtue
of granted access beyond the database, but no mismatch between the function
definition and the function implementation can crash the server or similar.
Adding such a thing at this point seems excessive to me.

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Robert Haas]

On Tue, Jun 12, 2012 at 11:31 AM, Noah Misch n...@leadboat.com wrote:
  Here's a patch implementing that restriction.  To clarify, I see no need to
  repeat *all* the CREATE-time checks; for example, there's no need to 
  recheck
  permission to use the return type.  The language usage check is enough.

 This seems bizarre and largely unnecessary.  As you stated to begin
 with, granting ownership of a function implies some degree of trust.

 Yes, but I would never expect that level of trust to include access to crash
 the server as a consequence of the function's reliance on STRICT.

+1.  Crashes are bad.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Tom Lane]

Robert Haas robertmh...@gmail.com writes:
 On Tue, Jun 12, 2012 at 11:31 AM, Noah Misch n...@leadboat.com wrote:
 This seems bizarre and largely unnecessary.  As you stated to begin
 with, granting ownership of a function implies some degree of trust.

 Yes, but I would never expect that level of trust to include access to crash
 the server as a consequence of the function's reliance on STRICT.

 +1.  Crashes are bad.

C functions, by definition, carry a risk of crashing the server.
I cannot fathom the reasoning why we should consider that granting
ownership of one to an untrustworthy user is ever a good idea, let alone
something we promise to protect you from any bad consequences of.

Even if I accepted that premise, this patch is a pretty bad
implementation of it, because it restricts cases that there is no
reason to think are unsafe.

A less bizarre and considerably more future-proof restriction, IMO,
would simply refuse any attempt to give ownership of a C function
to a non-superuser.

regards, tom lane

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Kevin Grittner]

Tom Lane t...@sss.pgh.pa.us wrote:
 
 A less bizarre and considerably more future-proof restriction,
 IMO, would simply refuse any attempt to give ownership of a C
 function to a non-superuser.
 
We have C replication trigger functions where this would be a bad
thing.  They can't work properly as SECURITY INVOKER, and I see it
as a big step backwards in security to make the only other option
SECURITY DEFINER with a superuser as the owner.  It's not too hard
to come up with other use cases where you want to grant one class of
users rights to do something only through a certain function, not
directly.
 
So there is clearly a need to support ownership of functions,
including C functions, by users who are effectively at an
intermediate level of trust.  We could conceivably use the
database owner for that role, but that seem unnecessarily limiting.
 
-Kevin

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Tom Lane]

Kevin Grittner kevin.gritt...@wicourts.gov writes:
 Tom Lane t...@sss.pgh.pa.us wrote:
 A less bizarre and considerably more future-proof restriction,
 IMO, would simply refuse any attempt to give ownership of a C
 function to a non-superuser.
 
 We have C replication trigger functions where this would be a bad
 thing.  They can't work properly as SECURITY INVOKER, and I see it
 as a big step backwards in security to make the only other option
 SECURITY DEFINER with a superuser as the owner.

Could you provide more details about that?  If nothing else, this
could be handled with a non-C wrapper function, but I'm not clear
on the generality of the use-case.

 It's not too hard
 to come up with other use cases where you want to grant one class of
 users rights to do something only through a certain function, not
 directly.

Generally I'd imagine that that has something to do with permission
to call the function, not with who owns it.

Basically, if we go down the road Noah is proposing, I foresee a steady
stream of security bugs and ensuing random restrictions on what function
owners can do.  I do not like that future.

regards, tom lane

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Kevin Grittner]

Tom Lane t...@sss.pgh.pa.us wrote:
 Kevin Grittner kevin.gritt...@wicourts.gov writes:
 
 We have C replication trigger functions where this would be a bad
 thing.  They can't work properly as SECURITY INVOKER, and I see
 it as a big step backwards in security to make the only other
 option SECURITY DEFINER with a superuser as the owner.
 
 Could you provide more details about that?
 
We have a capture_replication_data() trigger function that we attach
to each table which is to be replicated as the first AFTER EACH ROW
trigger for INSERT OR UPDATE OR DELETE.  It records the data
involved in the primitive operation against the row for logical
replication at the row level.  We don't want users to be able to
modify or even view the captured data in the replication tables
except through this function.  (It's actually a bit more complicated
than that because of transaction metadata, but the overall concept
is the same.)
 
We currently use the database owner for the owner of these SECURITY
DEFINER functions, but it seems to me that there could be legitimate
reasons to create a special user with more limited rights than the
database owner in some cases -- just to ensure that a mistake in the
coding of a function doesn't open up an unnecessarily large security
hole.
 
 If nothing else, this could be handled with a non-C wrapper
 function, but I'm not clear on the generality of the use-case.
 
I'm not so sure that this would work for a generalized trigger
function that can be attached to any table like this.
 
 It's not too hard to come up with other use cases where you want
 to grant one class of users rights to do something only through a
 certain function, not directly.
 
 Generally I'd imagine that that has something to do with
 permission to call the function, not with who owns it.
 
Well, it's a matter of fail-safe techniques.  You grant execute
permission for the function to a the role(s) which should be allowed
to do it only through the function.  But do you then necessarily
want the function to execute with unlimited rights, or with the most
restricted set of rights which allows it to perform the intended
function?
 
 Basically, if we go down the road Noah is proposing, I foresee a
 steady stream of security bugs and ensuing random restrictions on
 what function owners can do.  I do not like that future.
 
I do see your point, but I don't like the solution you proposed.
 
As I understand it, the problem Noah is trying to address is that if
we created a replication_manager role for owning these functions,
instead of using the database owner, that role could alter a C
function which isn't coded to handle NULL input to allow it to be
called on NULL input anyway.  Is that right?
 
The first solution which comes to mind for me is to allow a C
function to be compiled with that limitation -- so that *nobody*
could set the wrong option for it.  Then I realize that you already
can test for this in a C function and return NULL if any inputs are
NULL if you want to.  Rather than trying to enforce this in the
ALTER FUNCTION implementation, maybe we should just advise that if
you're going to grant ownership of a C function to a role which
might accidentally or maliciously allow it to be called with NULL
input, the C function should return NULL in that case.
 
-Kevin

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Kevin Grittner kevin.gritt...@wicourts.gov writes:
  It's not too hard
  to come up with other use cases where you want to grant one class of
  users rights to do something only through a certain function, not
  directly.
 
 Generally I'd imagine that that has something to do with permission
 to call the function, not with who owns it.

What I believe Kevin is getting at here is this:

There's no way to say run this function as user X except by making it
SECURITY DEFINER and owned by the user you want the function to run as.

I believe everyone agrees that only a superuser should be allowed
CHANGE a C-language function.  Unfortunately, being the 'OWNER' conveys
more than just the ability to change the function.

If we had an independent way to have the function run as a specific
user, where that user DIDN'T own the function, I think Kevin's use case
would be satisfied.

I'm not sure if it's be reasonable for a C-language function to just go
ahead and decide to change the user it's running as in the database..
I have to admit that I don't think I've ever tried to do that.

 Basically, if we go down the road Noah is proposing, I foresee a steady
 stream of security bugs and ensuing random restrictions on what function
 owners can do.  I do not like that future.

I agree that we don't want to have to police what a function owner can
do to a function, and therefore untrusted language functions should only
be owned by superusers, but I feel that means we need to look at what
function ownership currently implies and allows and consider if those
operations should be broken out and made independently grantable.  When
it comes to 'SECURITY DEFINER' and it's relationship to 'OWNER', I think
we have to provide some kind of solution that doesn't require those
functions to be run as superuser simply because the function has to be
owned by a superuser.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Tom Lane]

Stephen Frost sfr...@snowman.net writes:
 What I believe Kevin is getting at here is this:

 There's no way to say run this function as user X except by making it
 SECURITY DEFINER and owned by the user you want the function to run as.

 If we had an independent way to have the function run as a specific
 user, where that user DIDN'T own the function, I think Kevin's use case
 would be satisfied.

Interesting thought.  I'm not exactly sure who should be allowed to
apply the RUN AS other-user option to a function, but I can see the
possible value of separating the right to modify the function's
definition from the user the function runs as.  Kevin, does this seem
like it would address your concern?

regards, tom lane

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Kevin Grittner]

Stephen Frost sfr...@snowman.net wrote: 
 
 If we had an independent way to have the function run as a
 specific user, where that user DIDN'T own the function, I think
 Kevin's use case would be satisfied.
 
I agree.  I'm not sure quite what that would look like, but maybe
SECURITY ROLE rolename or some such could be an alternative to
SECURITY INVOKER and SECURITY DEFINER.  (I haven't looked to see
what the standard has here.)
 
-Kevin

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Tom Lane]

Kevin Grittner kevin.gritt...@wicourts.gov writes:
 Tom Lane t...@sss.pgh.pa.us wrote:
 Could you provide more details about that?
 
 We have a capture_replication_data() trigger function that we attach
 to each table which is to be replicated as the first AFTER EACH ROW
 trigger for INSERT OR UPDATE OR DELETE.  It records the data
 involved in the primitive operation against the row for logical
 replication at the row level.  We don't want users to be able to
 modify or even view the captured data in the replication tables
 except through this function.  (It's actually a bit more complicated
 than that because of transaction metadata, but the overall concept
 is the same.)
 
 We currently use the database owner for the owner of these SECURITY
 DEFINER functions, but it seems to me that there could be legitimate
 reasons to create a special user with more limited rights than the
 database owner in some cases -- just to ensure that a mistake in the
 coding of a function doesn't open up an unnecessarily large security
 hole.

I'm not entirely following here.  If the function is coded in C, then
it can pretty much do what it pleases independently of what user ID
is thought to be executing it at the SQL level.  That would really only
matter if you were doing some SQL stuff via the SPI interface --- and
if that's the case, couldn't the C function set the appropriate role
to use for itself, anyway?  (In other words, it's not that hard to build
a RUN AS other-user feature into a C function, even without any support
from the rest of the system.)

 Rather than trying to enforce this in the
 ALTER FUNCTION implementation, maybe we should just advise that if
 you're going to grant ownership of a C function to a role which
 might accidentally or maliciously allow it to be called with NULL
 input, the C function should return NULL in that case.

Yeah, the just-code-defensively option is worth considering too.

regards, tom lane

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Alvaro Herrera]

Excerpts from Kevin Grittner's message of mar jun 12 17:08:09 -0400 2012:
 Stephen Frost sfr...@snowman.net wrote: 
  
  If we had an independent way to have the function run as a
  specific user, where that user DIDN'T own the function, I think
  Kevin's use case would be satisfied.
  
 I agree.  I'm not sure quite what that would look like, but maybe
 SECURITY ROLE rolename or some such could be an alternative to
 SECURITY INVOKER and SECURITY DEFINER.  (I haven't looked to see
 what the standard has here.)

We talked briefly about this a year ago:
http://wiki.postgresql.org/wiki/PgCon_2011_Developer_Meeting#Authorization_Issues
(Not quite the same thing, but it's closely related).

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
 I'm not exactly sure who should be allowed to
 apply the RUN AS other-user option to a function, but I can see the
 possible value of separating the right to modify the function's
 definition from the user the function runs as.

When it comes to 'who can set it'- my first reaction is the owner.
The next question is- what rights does the owner have to have on the
other-user role, and I would suggest membership.  This could be
extremely useful for non-C functions as well, consider this:

I'm Bob.  I have an 'audit' role which is granted to me.  I'd like to
create a function that runs as 'audit' (which has various rights granted
to it which are less than the rights of 'Bob'), but which only I can
modify.  If I've been granted the 'audit' role, then I can create a
function which is owned by 'audit' (set role audit; create function
...), and I could make it security definer, therefore I should be able
to create a function which is owned by me and runs as 'audit'.

Writing this a bit off-the-cuff, so apologies if there are obvious flaws
in this logic. :)

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
 (In other words, it's not that hard to build
 a RUN AS other-user feature into a C function, even without any support
 from the rest of the system.)

I was considering this and a bit concerned about what would happen if
the C function actually did this and if we'd clean things up properly at
the end or if the function would be required to handle that clean-up
(if it was written as SECUURITY INVOKER, which is what's being suggested
here)...

In general, I'd certainly rather have the database handle that cleanly
and consistently than expect my function to clean up after itself.

Alvaro's point about the discussion of a stack of roles is certainly
something else to consider, though I feel that the 'run-as' option is
pretty straight-forward and could be done more-or-less identically to
how we do secuirty definer now, it's just changing where we get the role
to change to before running the function.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Kevin Grittner]

Tom Lane t...@sss.pgh.pa.us wrote:
 
 I'm not entirely following here.  If the function is coded in C,
 then it can pretty much do what it pleases independently of what
 user ID is thought to be executing it at the SQL level.  That
 would really only matter if you were doing some SQL stuff via the
 SPI interface
 
Yeah, there is a lot of SPI through cached prepared statements.
 
 if that's the case, couldn't the C function set the appropriate
 role to use for itself, anyway?
 
I suppose it could, though I never really thought about that aspect
of the issue before.
 
 (In other words, it's not that hard to build a RUN AS other-user
 feature into a C function, even without any support from the rest
 of the system.)
 
Similar to what Stephen says in his post that came through while I
was typing this, I would be less comfortable with this being a
hand-rolled feature of individual C functions than having some more
systematic way to handle it.  Even if there is the possibility that
someone could subvert the more systematic way of doing things with
clever C code, I think the systematic approach reduces risk from
error and would tend to make hostile code in C functions stand out
more.  And having the information at the catalog level would make
the intended usages easier to manage.
 
-Kevin

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

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Tom Lane]

Stephen Frost sfr...@snowman.net writes:
 * Tom Lane (t...@sss.pgh.pa.us) wrote:
 (In other words, it's not that hard to build
 a RUN AS other-user feature into a C function, even without any support
 from the rest of the system.)

 I was considering this and a bit concerned about what would happen if
 the C function actually did this and if we'd clean things up properly at
 the end or if the function would be required to handle that clean-up
 (if it was written as SECUURITY INVOKER, which is what's being suggested
 here)...

It would have to remember to restore the previous role on normal exit,
but I believe that the system would take care of cleanup if an error is
thrown.  Looking at fmgr_security_definer, there are just a couple of
lines involved with changing the active role.  (There's a boatload of
*other* crap that's been shoved into that function over time, but the
part of it that actually does what it's named for is pretty darn small.)

 Alvaro's point about the discussion of a stack of roles is certainly
 something else to consider, though I feel that the 'run-as' option is
 pretty straight-forward and could be done more-or-less identically to
 how we do secuirty definer now, it's just changing where we get the role
 to change to before running the function.

Yes, it would surely not be much more code, just a bit added here:

if (procedureStruct-prosecdef)
fcache-userid = procedureStruct-proowner;

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

[HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Noah Misch]

On Wed, May 30, 2012 at 07:34:16PM -0400, Noah Misch wrote:
 ALTER FUNCTION OWNER TO on a C-language function conveys more trust than
 meets the eye:
 
   BEGIN;
   CREATE ROLE alice;
   CREATE FUNCTION mylen(text) RETURNS integer LANGUAGE internal IMMUTABLE 
 STRICT AS 'textlen';
   ALTER FUNCTION mylen(text) OWNER TO alice;
   COMMIT;
 
   SET SESSION AUTHORIZATION alice;
   ALTER FUNCTION mylen(text) CALLED ON NULL INPUT;
   SELECT mylen(NULL); -- SIGSEGV
 
 CREATE FUNCTION + ALTER FUNCTION OWNER TO is useful for creating another
 user's untrusted-language SECURITY DEFINER function.  ALTER FUNCTION CALLED ON
 NULL INPUT ought to require that the user be eligible to redefine the function
 completely.

Here's a patch implementing that restriction.  To clarify, I see no need to
repeat *all* the CREATE-time checks; for example, there's no need to recheck
permission to use the return type.  The language usage check is enough.

I didn't feel the need to memorialize a test like the above in an actual
regression test, but that's the one I used to verify the change.

Considering the crash potential, I'd recommend backpatching this.

Thanks,
nm
diff --git a/doc/src/sgml/ref/alter_function.sgml 
b/doc/src/sgml/ref/alter_function.sgml
index 013b6f8..cfc2a69 100644
*** a/doc/src/sgml/ref/alter_function.sgml
--- b/doc/src/sgml/ref/alter_function.sgml
***
*** 54,59  ALTER FUNCTION replaceablename/replaceable ( [ [ 
replaceable class=paramet
--- 54,61 
  
para
 You must own the function to use commandALTER FUNCTION/.
+Marking a function literalCALLED ON NULL INPUT/ also requires
+permission to use the function's language when creating new functions.
 To change a function's schema, you must also have literalCREATE/
 privilege on the new schema.
 To alter the owner, you must also be a direct or indirect member of the new
diff --git a/src/backend/commands/functioncindex 13e30f4..b40bcdf 100644
*** a/src/backend/commands/functioncmds.c
--- b/src/backend/commands/functioncmds.c
***
*** 70,75  static void AlterFunctionOwner_internal(Relation rel, HeapTuple 
tup,
--- 70,107 
  
  
  /*
+  * May the current user create functions of a given language?
+  */
+ static void
+ check_language_usage(HeapTuple languageTuple)
+ {
+   Oid languageOid;
+   Form_pg_language languageStruct;
+ 
+   languageOid = HeapTupleGetOid(languageTuple);
+   languageStruct = (Form_pg_language) GETSTRUCT(languageTuple);
+ 
+   if (languageStruct-lanpltrusted)
+   {
+   /* if trusted language, need USAGE privilege */
+   AclResult   aclresult;
+ 
+   aclresult = pg_language_aclcheck(languageOid, GetUserId(), 
ACL_USAGE);
+   if (aclresult != ACLCHECK_OK)
+   aclcheck_error(aclresult, ACL_KIND_LANGUAGE,
+  
NameStr(languageStruct-lanname));
+   }
+   else
+   {
+   /* if untrusted language, must be superuser */
+   if (!superuser())
+   aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_LANGUAGE,
+  
NameStr(languageStruct-lanname));
+   }
+ }
+ 
+ 
+ /*
   * Examine the RETURNS clause of the CREATE FUNCTION statement
   * and return information about it as *prorettype_p and *returnsSet.
   *
***
*** 864,890  CreateFunction(CreateFunctionStmt *stmt, const char 
*queryString)
 (PLTemplateExists(language) ?
  errhint(Use CREATE LANGUAGE to load the 
language into the database.) : 0)));
  
languageOid = HeapTupleGetOid(languageTuple);
languageStruct = (Form_pg_language) GETSTRUCT(languageTuple);
- 
-   if (languageStruct-lanpltrusted)
-   {
-   /* if trusted language, need USAGE privilege */
-   AclResult   aclresult;
- 
-   aclresult = pg_language_aclcheck(languageOid, GetUserId(), 
ACL_USAGE);
-   if (aclresult != ACLCHECK_OK)
-   aclcheck_error(aclresult, ACL_KIND_LANGUAGE,
-  
NameStr(languageStruct-lanname));
-   }
-   else
-   {
-   /* if untrusted language, must be superuser */
-   if (!superuser())
-   aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_LANGUAGE,
-  
NameStr(languageStruct-lanname));
-   }
- 
languageValidator = languageStruct-lanvalidator;
  
ReleaseSysCache(languageTuple);
--- 896,905 
 (PLTemplateExists(language) ?
  errhint(Use CREATE LANGUAGE to load the 
language into the database.) : 0)));
  
+   check_language_usage(languageTuple);
+ 
languageOid = 

Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

[Tom Lane]

Noah Misch n...@leadboat.com writes:
 CREATE FUNCTION + ALTER FUNCTION OWNER TO is useful for creating another
 user's untrusted-language SECURITY DEFINER function.  ALTER FUNCTION CALLED 
 ON
 NULL INPUT ought to require that the user be eligible to redefine the 
 function
 completely.

 Here's a patch implementing that restriction.  To clarify, I see no need to
 repeat *all* the CREATE-time checks; for example, there's no need to recheck
 permission to use the return type.  The language usage check is enough.

This seems bizarre and largely unnecessary.  As you stated to begin
with, granting ownership of a function implies some degree of trust.
I do not want to get into the business of parsing exactly which variants
of ALTER FUNCTION ought to be considered safe.  And I definitely don't
want to add a check that enforces restrictions against cases that have
got nothing whatever to do with C-language functions, as this patch
does.

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