Re: Blocking execution of SECURITY INVOKER
Hi, On 2023-01-13 10:04:19 -0800, Jeff Davis wrote: > However, the normal use case for expressions (whether in a default > expression or check constraint or whatever) is very simple and doesn't > even involve table access. There must be a way to satisfy those simple > cases without opening up a vast attack surface area, and if we do, then > I think my proposal might look useful again. I don't see how. I guess we could try to introduce a classification of "never dangerous" functions (and thus operators). But that seems like a crapton of work and hard to get right. And I think my examples pretty conclusively show that security definer isn't a useful boundary to *reduce* privileges. So the whole idea of preventing only security invoker functions just seems non-viable. I think the combination of a) a setting that restricts evaluation of any non-trusted expressions, independent of the origin b) an easy way to execute arbitrary statements within SECURITY_RESTRICTED_OPERATION is promising though. In later steps We might be able to use a) to offer the option to automatically switch to expression owners in specific places (if the current user has the rights to do that). An alternative to b would be a version SET ROLE that can't be undone. But I think we'd just miss all the other things that are prevented by SECURITY_RESTRICTED_OPERATION. Greetings, Andres Freund
Re: Blocking execution of SECURITY INVOKER
On Fri, 2023-01-13 at 00:16 -0800, Andres Freund wrote: > Just think of set_config(), pg_read_file(), lo_create(), > binary_upgrade_*(), > pg_drop_replication_slot()... Thank you for walking through the details here. I missed it from your first example because it was an extreme case -- a superuser writing an eval() security definer function -- so the answer was to lock such a dangerous function away. But more mild cases are the real problem, because there's a lot of stuff in pg_catalog.*. > If the default values get evaluated, this is arbitrary code exec, > even if it > requires a few contortions. And the same is true for evaluating *any* > expression. Right. However, the normal use case for expressions (whether in a default expression or check constraint or whatever) is very simple and doesn't even involve table access. There must be a way to satisfy those simple cases without opening up a vast attack surface area, and if we do, then I think my proposal might look useful again. -- Jeff Davis PostgreSQL Contributor Team - AWS
Re: Blocking execution of SECURITY INVOKER
On Thu, 2023-01-12 at 19:38 -0800, Andres Freund wrote: > I.e. the default arguments where evaluated with the invoker's > permissions, not > the definer's, despite being controlled by the less privileged user. This is a very interesting case. It also involves tricking the superuser into executing their own function with the attacker's inputs. That part is the same as the other case. What's intriguing here is that it shows the function can be SECURITY INVOKER, and that really means it could be any builtin function as long as the types work out. For example: => create function trick(l pg_lsn = pg_switch_wal()) returns int language plpgsql security definer as $$ begin return 42; end; $$; If the superuser executes that, even though it's a SECURITY DEFINER function owned by an unprivileged user, it will still call pg_switch_wal(). -- Jeff Davis PostgreSQL Contributor Team - AWS
Re: Blocking execution of SECURITY INVOKER
Hi, On 2023-01-12 23:38:50 -0800, Jeff Davis wrote: > On Thu, 2023-01-12 at 19:29 -0800, Andres Freund wrote: > > superuser: > > # CREATE FUNCTION exec_su(p_sql text) RETURNS text LANGUAGE plpgsql > > SECURITY DEFINER AS $$BEGIN RAISE NOTICE 'executing %', p_sql; > > EXECUTE p_sql;RETURN 'p_sql';END;$$; > > # REVOKE ALL ON FUNCTION exec_su FROM PUBLIC ; > > That can be solved by creating the function in a schema where ordinary > users don't have USAGE: > > CREATE TABLE trick_superuser(value text default admin.exec_su('ALTER > USER less_privs SUPERUSER')); > ERROR: permission denied for schema admin Doubtful. Leaving aside the practicalities of using dedicated schemas and enforcing their use, there's plenty functions in pg_catalog that a less privileged user can use to do bad things. Just think of set_config(), pg_read_file(), lo_create(), binary_upgrade_*(), pg_drop_replication_slot()... If the default values get evaluated, this is arbitrary code exec, even if it requires a few contortions. And the same is true for evaluating *any* expression. > > And the admin likely can switch into the user context of > > the less privileged user to perform operations in a safer context. > > How would the admin do that? The malicious UDF can just "RESET SESSION > AUTHORIZATION" to pop back out of the safer context. I thought we had a reasonably convenient way, but now I am not sure anymore. Might have been via a C helper function. It can be hacked together, but this is an area that should be as unhacky as possible. > If there's not a good way to do this safely now, then we should > probably provide one. Yea, particularly because we do have all the infrastructure for it (c.f. SECURITY_LOCAL_USERID_CHANGE / SECURITY_RESTRICTED_OPERATION). Greetings, Andres Freund
Re: Blocking execution of SECURITY INVOKER
Hi, On Thu, 2023-01-12 at 19:29 -0800, Andres Freund wrote: > superuser: > # CREATE FUNCTION exec_su(p_sql text) RETURNS text LANGUAGE plpgsql > SECURITY DEFINER AS $$BEGIN RAISE NOTICE 'executing %', p_sql; > EXECUTE p_sql;RETURN 'p_sql';END;$$; > # REVOKE ALL ON FUNCTION exec_su FROM PUBLIC ; That can be solved by creating the function in a schema where ordinary users don't have USAGE: CREATE TABLE trick_superuser(value text default admin.exec_su('ALTER USER less_privs SUPERUSER')); ERROR: permission denied for schema admin An interesting case, but it looks more like a gotcha (which is solvable with best practices); not a fundamental problem. > The point of the grant system is for a privileged user to safely > allow a less privileged user to perform a safe subset of actions. There is not necessarily a GRANT hierarchy like you describe. The two users can be peers each with comparable privileges that might make grants to each other. > And the admin likely can switch into the user context of > the less privileged user to perform operations in a safer context. How would the admin do that? The malicious UDF can just "RESET SESSION AUTHORIZATION" to pop back out of the safer context. If there's not a good way to do this safely now, then we should probably provide one. > > Regards, -- Jeff Davis PostgreSQL Contributor Team - AWS
Re: Blocking execution of SECURITY INVOKER
On 2023-01-12 19:29:43 -0800, Andres Freund wrote: > Hi, > > On 2023-01-12 18:40:30 -0800, Jeff Davis wrote: > > On Wed, 2023-01-11 at 19:33 -0800, Andres Freund wrote: > > > > > and the > > > privilege check will be done with the rights of the admin in many of > > > these > > > contexts. > > > > Can you explain? > > If the less-privileged user does *not* have execution rights to a security > definer function, but somehow can trick the more-privileged user into calling > the function for them, e.g. by using it as the default expression of a column, > the less-privileged user can escalate to the permissions of the security > definer function. > > superuser: > # CREATE FUNCTION exec_su(p_sql text) RETURNS text LANGUAGE plpgsql SECURITY > DEFINER AS $$BEGIN RAISE NOTICE 'executing %', p_sql; EXECUTE p_sql;RETURN > 'p_sql';END;$$; > # REVOKE ALL ON FUNCTION exec_su FROM PUBLIC ; > > unprivileged user: > $ SELECT exec_su('ALTER USER less_privs SUPERUSER'); > ERROR: 42501: permission denied for function exec_su > $ CREATE TABLE trick_superuser(value text default exec_su('ALTER USER > less_privs SUPERUSER')); > > superuser: > # INSERT INTO trick_superuser DEFAULT VALUES; > NOTICE: 0: executing ALTER USER less_privs SUPERUSER > > > This case would *not* be prevented by your proposed GUC, unless I miss > something major. The superuser trusts itself and thus the exec_su() function. Another reason security definer isn't a way to allow safe execution of lesser privileged code: superuser (andres): # CREATE FUNCTION bleat_whoami() RETURNS text LANGUAGE plpgsql SECURITY INVOKER AS $$BEGIN RAISE NOTICE 'whoami: %', current_user;RETURN current_user;END;$$; # REVOKE ALL ON FUNCTION bleat_whoami FROM PUBLIC; unprivileged user: $ CREATE FUNCTION secdef_with_default(foo text = bleat_whoami()) RETURNS text LANGUAGE plpgsql SECURITY DEFINER AS $$BEGIN RETURN 'secdef_with_default';END;$$; $ SELECT secdef_with_default(); ERROR: 42501: permission denied for function bleat_whoami superuser (andres): # SELECT secdef_with_default(); NOTICE: 0: whoami: andres LOCATION: exec_stmt_raise, pl_exec.c:3893 ┌─┐ │ secdef_with_default │ ├─┤ │ secdef_with_default │ └─┘ (1 row) I.e. the default arguments where evaluated with the invoker's permissions, not the definer's, despite being controlled by the less privileged user. Worsened in this case by the fact that this allowed the less privileged user to call a function they couldn't even call. Greetings, Andres Freund
Re: Blocking execution of SECURITY INVOKER
Hi, On 2023-01-12 18:40:30 -0800, Jeff Davis wrote: > On Wed, 2023-01-11 at 19:33 -0800, Andres Freund wrote: > > > and the > > privilege check will be done with the rights of the admin in many of > > these > > contexts. > > Can you explain? If the less-privileged user does *not* have execution rights to a security definer function, but somehow can trick the more-privileged user into calling the function for them, e.g. by using it as the default expression of a column, the less-privileged user can escalate to the permissions of the security definer function. superuser: # CREATE FUNCTION exec_su(p_sql text) RETURNS text LANGUAGE plpgsql SECURITY DEFINER AS $$BEGIN RAISE NOTICE 'executing %', p_sql; EXECUTE p_sql;RETURN 'p_sql';END;$$; # REVOKE ALL ON FUNCTION exec_su FROM PUBLIC ; unprivileged user: $ SELECT exec_su('ALTER USER less_privs SUPERUSER'); ERROR: 42501: permission denied for function exec_su $ CREATE TABLE trick_superuser(value text default exec_su('ALTER USER less_privs SUPERUSER')); superuser: # INSERT INTO trick_superuser DEFAULT VALUES; NOTICE: 0: executing ALTER USER less_privs SUPERUSER This case would *not* be prevented by your proposed GUC, unless I miss something major. The superuser trusts itself and thus the exec_su() function. > > And encouraging more security definer functions to be used will cause > > a lot of > > other security issues. > > My proposal just gives some user foo a GUC to say "I am not accepting > the risk of eval()ing whatever arbitrary code finds its way in front of > me with all of my privileges". > > If user foo sheds this security burden by setting the GUC, user bar may > then choose to write a trigger function as SECURITY DEFINER so that foo > can access bar's table. But that's the deal the two users struck -- foo > declined the burden, bar accepted it. Why do we want to prevent that > arrangement? Because it afaict doesn't provide any meaningfully increased security guarantees (see above), and opens up new ways of attacking, because while granting execute on a security definer function is low risk, granting execute on security invoker functions is very high risk, but required for triggers etc to work. > Right now, foo *always* has the burden and no opportunity to decline > it, and even a paranoid user can't figure out what code they will be > executing with a given command. That doesn't seem reasonable to me. I agree it's not reasonable - I just don't see the proposal moving the bar. The proposal to not trust any expressions controlled by untrusted users at least allows to prevent execution of code, even if it doesn't provide a way to execute the code in a safe manner. Given that we don't have the former, it seems foolish to shoot for the latter. > > However - I think the concept of more strict ownership checks is a > > good one. I > > just don't think it's right to tie it to SECURITY INVOKER. > > Consider a canonical trigger example like: > https://wiki.postgresql.org/wiki/Audit_trigger or > https://github.com/2ndQuadrant/audit-trigger/blob/master/audit.sql > > How can we make that secure for users that insert into the table with > the trigger if you don't differentiate between SECURITY INVOKER and > SECURITY DEFINER? If you allow neither, then it obviously won't work. > And if you allow both, then the owner of the table can change the > function to SECURITY INVOKER and the definition to be malicious a > millisecond before you insert a tuple. As shown above, triggers are simply not a relevant boundary when a more privileged user accesses a table controlled by a less privileged user. And yes, of course an audit function needs to be security definer. But that's independent of whether it's safe for a more privileged user modify table contents. > I guess we currently say that anyone foolish enough to insert into a > table that they don't own deserves what they get. I agree that we have a problem that we should address. I just don't think your solution works. > That's a weird thing to say when we have such a fine-grained GRANT system > and RLS. That's a non-sequitur imo. Particularly when RLS you'd not allow less-privileged users to create any objects, with the possible exception of temp tables. The point of the grant system is for a privileged user to safely allow a less privileged user to perform a safe subset of actions. That's just a separate angle than allowing safe access for a more privileged user to to objects controlled by a less privileged user. > > I think it'd be quite valuable to have a guc that prevents the > > execution of > > any code that's not directly controlled by the privileged user. Not > > just > > checking function ownership, but also checking ownership of the > > trigger > > definition (i.e. table), check constraints, domain constraints, > > indexes with > > expression columns / partial indexes, etc > > That sounds like a mix of my proposal and Noah's. The way you've > phrased it seems overly strict t
Re: Blocking execution of SECURITY INVOKER
On Wed, 2023-01-11 at 19:33 -0800, Andres Freund wrote: > and the > privilege check will be done with the rights of the admin in many of > these > contexts. Can you explain? > And encouraging more security definer functions to be used will cause > a lot of > other security issues. My proposal just gives some user foo a GUC to say "I am not accepting the risk of eval()ing whatever arbitrary code finds its way in front of me with all of my privileges". If user foo sheds this security burden by setting the GUC, user bar may then choose to write a trigger function as SECURITY DEFINER so that foo can access bar's table. But that's the deal the two users struck -- foo declined the burden, bar accepted it. Why do we want to prevent that arrangement? Right now, foo *always* has the burden and no opportunity to decline it, and even a paranoid user can't figure out what code they will be executing with a given command. That doesn't seem reasonable to me. > However - I think the concept of more strict ownership checks is a > good one. I > just don't think it's right to tie it to SECURITY INVOKER. Consider a canonical trigger example like: https://wiki.postgresql.org/wiki/Audit_trigger or https://github.com/2ndQuadrant/audit-trigger/blob/master/audit.sql How can we make that secure for users that insert into the table with the trigger if you don't differentiate between SECURITY INVOKER and SECURITY DEFINER? If you allow neither, then it obviously won't work. And if you allow both, then the owner of the table can change the function to SECURITY INVOKER and the definition to be malicious a millisecond before you insert a tuple. I guess we currently say that anyone foolish enough to insert into a table that they don't own deserves what they get. That's a weird thing to say when we have such a fine-grained GRANT system and RLS. > I think it'd be quite valuable to have a guc that prevents the > execution of > any code that's not directly controlled by the privileged user. Not > just > checking function ownership, but also checking ownership of the > trigger > definition (i.e. table), check constraints, domain constraints, > indexes with > expression columns / partial indexes, etc That sounds like a mix of my proposal and Noah's. The way you've phrased it seems overly strict though -- do you mean not even execute untrusted expressions? And it seems to cut out maintenance commands, which means it would be hard for administrators to use. I'm OK considering these proposals. Anything that offers some safety. But it seems like both your proposal and Noah's cut out huge amounts of functionality unless you have unqualified trust. > Even a security definer function can mess up > your day when called in the wrong situation, e.g. due to getting > access to the > content of arguments (e.g. a trigger's row contents) I don't see that as a problem. If you're inserting data in a table, you'd expect the owner of the table to see that data and be able to modify it as they see fit. > or preventing an admin's > write from taking effect (by returning the relevant values from a > trigger). I don't see the problem here either. Even if we force the row to be inserted, the table owner could just delete it. > And not ever allowing execution of untrusted code in that situation > IME > doesn't prevent desirable things. I don't understand this statement. > > I think a much more promising path towards that is to add a feature > to logical > replication that changes the execution context to the table owner > while > applying those changes. How is that different from SECURITY DEFINER? > > > And as I said, for 1 and 3 I think it's way preferrable to error out. My proposal does error out for SECURITY INVOKER, so I suppose you're saying we should error out for SECURITY DEFINER as well? In the case of 1, I think that would prevent regular maintenance by an admin. But for use case 3 (extension scripts), I think you're right, erroring on any non-superuser-owned code is probably good. -- Jeff Davis PostgreSQL Contributor Team - AWS
Re: Blocking execution of SECURITY INVOKER
Hi, On 2023-01-11 18:16:32 -0800, Jeff Davis wrote: > Motivation > == > > SECURITY INVOKER is dangerous, particularly for administrators. There > are numerous ways to put code in a place that's likely to be executed: > triggers, views calling functions, logically-replicated tables, casts, > search path and function resolution tricks, etc. If this code is run > with the privileges of the invoker, then it provides an easy path to > privilege escalation. > We've addressed some of these risks, i.e. by offering better ways to > control the search path, and by ignoring SECURITY INVOKER in some > contexts (like maintenance commands). But it still leaves a lot of > risks for administrators who want to do a SELECT or INSERT. And it > limits major use cases, like logical replication (where the > subscription owner must trust all table owners). I'm very skeptical about this framing. On the one hand, you can do a lot of mischief with security definer functions if they get privileged information as well. But more importantly, just because a function is security definer, doesn't mean it's safe to be called with attacker controlled input, and the privilege check will be done with the rights of the admin in many of these contexts. And encouraging more security definer functions to be used will cause a lot of other security issues. However - I think the concept of more strict ownership checks is a good one. I just don't think it's right to tie it to SECURITY INVOKER. I think it'd be quite valuable to have a guc that prevents the execution of any code that's not directly controlled by the privileged user. Not just checking function ownership, but also checking ownership of the trigger definition (i.e. table), check constraints, domain constraints, indexes with expression columns / partial indexes, etc. > Use Cases > = > > 1. If you are a superuser/admin working on a problem interactively, you > can protect yourself against accidentally executing malicious code with > your privileges. In that case I think what's actually desirable is to simply execute no code controlled by untrusted users. Even a security definer function can mess up your day when called in the wrong situation, e.g. due to getting access to the content of arguments (e.g. a trigger's row contents) or preventing an admin's write from taking effect (by returning the relevant values from a trigger). And not ever allowing execution of untrusted code in that situation IME doesn't prevent desirable things. > 2. You can set up logical replication subscriptions into tables owned > by users you don't trust, as long as triggers (if needed) can be safely > written as SECURITY DEFINER. I think a much more promising path towards that is to add a feature to logical replication that changes the execution context to the table owner while applying those changes. Using security definer functions for triggers opens up a significant new attack surface, lots of code that previously didn't need to be safe against any possible privilege escalation, now needs to be. Expanding the scope of what needs to protect against privesc, is a BAD idea. > 3. You can ensure that running an extension script doesn't somehow > execute malicious code with superuser privileges. It's not safe to allow executing secdef code in that context either. If a less privileged user manages to get called in that context you don't want to execute the code, even in a secdef, you want to error out, so the problem can be detected and rectified. > 4. Users can protect themselves from executing malicious code in cases > where: > a. role membership accurately describes the trust relationship > already > b. triggers, views-calling-UDFs, etc., (if any) can be safely written > as SECURITY DEFINER I don't think b) is true very often. And a) seems very problematic as well, as particularly in "pseudo superuser" environments the most feasible way to implement pseudo superusers is to automatically grant group membership to the pseudo superuser. See also e5b8a4c098a. > While that may not be every conceivable use case, it feels very useful > to me. > > Even if you really don't like SECURITY DEFINER, points 1, 3, and 4(a) > seem like wins. And there are a lot of cases where the user simply > doesn't need triggers (etc.). 4 doesn't seem like a win, it's a requirement? And as I said, for 1 and 3 I think it's way preferrable to error out. > Future Work > === > > In some cases, we should consider defaulting (or even forcing) this GUC > to be true, such as in a subscription apply worker. > > This proposal may offer a path to allowing non-superusers to create > event triggers. That'd allow a less-privileged user to completely hobble the admin by erroring out on all actions. Greetings, Andres Freund