Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)
On Fri, Jul 20, 2012 at 03:39:33PM -0400, Robert Haas wrote: > On Fri, Jul 20, 2012 at 1:52 PM, Tom Lane 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)
On Fri, Jul 20, 2012 at 3:45 PM, Tom Lane wrote: > Robert Haas 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)
Robert Haas 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)
On Fri, Jul 20, 2012 at 1:52 PM, Tom Lane wrote: > Robert Haas writes: >> On Tue, Jun 12, 2012 at 5:12 PM, Tom Lane 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)
Robert Haas writes: > On Tue, Jun 12, 2012 at 5:12 PM, Tom Lane 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)
On Tue, Jun 12, 2012 at 5:12 PM, Tom Lane wrote: > "Kevin Grittner" 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)
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)
Stephen Frost 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
Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)
Tom Lane 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 (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)
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)
Excerpts from Kevin Grittner's message of mar jun 12 17:08:09 -0400 2012: > >Stephen Frost 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 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 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)
"Kevin Grittner" writes: > Tom Lane 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)
>Stephen Frost 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 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)
Stephen Frost 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)
* Tom Lane (t...@sss.pgh.pa.us) wrote: > "Kevin Grittner" 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 wrote: > "Kevin Grittner" 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)
"Kevin Grittner" writes: > Tom Lane 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)
Tom Lane 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)
Robert Haas writes: > On Tue, Jun 12, 2012 at 11:31 AM, Noah Misch 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)
On Tue, Jun 12, 2012 at 11:31 AM, Noah Misch 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)
On Mon, Jun 11, 2012 at 11:03:10PM -0400, Tom Lane wrote: > Noah Misch 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)
Noah Misch 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
[HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)
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 name ( [ [ 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 = HeapTupleGetOid(languageTuple); languageStruct = (Form_pg_language) GETSTRUCT(languageTuple); languageValidator = languageStruct->lanvalidator; ReleaseSysCache(languageTuple); *** *** 1312,1318 AlterFunction(AlterFunctionStmt *stmt) --- 1327,1355 if (volatility_item) procForm->provolatile = interpret_func_volatility(volatility_item); if (strict_item) + { + /* +* C-language functions that expect to be declared STRICT often omit +* NULL argument checks, crashing if they do receive a NULL. To +* protect such functions against less-privileged owners, clearing +* proisstrict requires the authority to define a new function of the +* same language. +*/ + if (!intVal(strict_item->arg)) + { + HeapTuple langTuple; + + langTuple = SearchSysCache1(LANGOID, + PointerGetDatum(procForm->prolang)); + if (!HeapTupleIsValid(langTuple)) + elog(ERROR, "cache lookup failed for language %u", +procForm->prolang); + check_language_usage(langTuple); + ReleaseSysCache(langTuple); + } + procForm->proisstrict = intVal(strict_item->arg); + } if (security_def_item) procForm->prosecdef = intVal(security_def_item->arg); if (leakproof_item) *** *** 1974,2002 ExecuteDoStmt(DoStmt *stmt) (PLTemplateExists(language) ? errhint("Use CREATE LANGUAGE to load the language into the database.") : 0))); codeblock->langOid = HeapTupleGetOid(languageTuple); languageStruct = (Form_pg_language) GETSTRUCT(languageTuple); codeblock->langIsTrusted = languageStruct->lanpltrusted; - if (languageStruct->lanpltrusted) - { - /* if trusted language, need USAGE privilege */ - AclResult aclresult; - - aclresult = pg_language_aclcheck(codeblock->langOid, GetUserId(), -