On Fri, Mar 24, 2023 at 8:02 PM Jeff Davis <pg...@j-davis.com> wrote: > Without a reasonable example, we should probably be on some kind of > path to disallowing crazy stuff in triggers that poses only risks and > no benefits. Not the job of this patch, but perhaps it can be seen as a > step in that direction?
Possibly, but it's a little harder to say what's crazy in a trigger than in some other contexts. I feel like it would be fine to say that your index expression should probably not be doing "ALTER USER somebody SUPERUSER" really ever, but it's not quite so clear in case of a trigger. And the stuff that's prevented by SECURITY_RESTRICTED_OPERATION isn't that sort of thing, but has rather to do with stuff that messes with the session state, maybe leaving booby-traps behind for the next user. For instance, if user A runs some code as user B and user B closes a cursor opened by A and opens a new one with the same name, that has a rather good chance of making A do something they didn't intend to do. SECURITY_RESTRICTED_OPERATION is aimed at preventing that sort of attack. > > I am not thrilled with this either, but if you can arrange to run > > code > > as a certain user without the ability to SET ROLE to that user then > > there is, by definition, a potential privilege escalation. > > I don't think that's "by definition". > > The code is being executed with the privileges of the person who wrote > it. I don't see anything inherently insecure about that. There could be > incidental or practical risks, but it's a pretty reasonable thing to do > at a high level. Not really. My home directory on my laptop is full of Perl and sh scripts that I wouldn't want someone else to execute as me. They don't have any defenses against malicious use because I don't expect anyone else has access to run them, especially as me. If someone got access to run them as me, they'd compromise my laptop account in no time at all. I don't see any reason to believe the situation is any different inside of a database. People have no reason to harden code unless someone else is going to have access to run it. > > I don't > > think we can just dismiss that as a non-issue. If the ability of one > > user to potentially become some other user weren't a problem, we > > wouldn't need any patch at all. Imagine for example that the table > > owner has a trigger which doesn't sanitize search_path. The > > subscription owner can potentially leverage that to get the table > > owner's privileges. > > Can you explain? Couldn't we control the subscription process's search > path? There's no place in the code right now where when we switch user identities we also change the search_path. There is nothing to prevent us from writing such code, but we have no place from which to obtain a search_path that will cause the called code to behave properly. We don't have access to the search_path that would prevail at the time the target user logged in, and even if we did, we don't know that that search_path is secure. We do know that an empty search_path is secure, but it's probably also going to cause any code we run to fail, unless that code schema-qualifies all references outside of pg_catalog, or unless it sets search_path itself. search_path also isn't necessarily the only problem. As a hypothetical example, suppose I create a table with one text column, revoke all public access to that table, and then create an on-insert trigger that executes as an SQL command any text value inserted into the table. This is perfectly secure as long as I'm the only one who can access the table, but if someone else gets access to insert things into that table using my credentials then they can easily break into my account. Real examples aren't necessarily that dramatically bad, but that doesn't mean they don't exist. > The benefit of delegating to a non-superuser is to contain the risk if > that account is compromised. Allowing SET ROLE on tons of accounts > diminishes that benefit, a lot. Well, I continue to feel that if you can compromise the subscription owner's account, we haven't really fixed anything yet. I mean, it used to be that autovacuum could compromise the superuser's account, and we fixed that. If we find more ways for that same thing to happen, we would presumably fix those too. We would never accept a situation where autovacuum can compromise the superuser's account. And we shouldn't accept a situation where either the table owner can compromise the subscription owner's account, either. And similarly nobody ever proposed that that issue should be fixed by running the autovacuum worker process as some kind of low-privileged user that we created specially for that purpose. We just ... fixed it so that no compromise was possible. And I think that's also the right approach here. I do agree with you that allowing SET ROLE on tons of accounts is not great, though. I don't really think it matters very much today, because basically all subscriptions today are owned by superusers and can do everything anyway. But if you imagine that a lot of subscriptions are going to be owned by less-privileged users, then it's a lot less nice. I think it has the strength of at least being honest about what the problem is. It makes it clear right on the tin that the subscription owner is going to be able to get into the table owner's accounts, in a way that people shouldn't really miss noticing. And if they notice it, then they're at least aware of it, which is something. It would be better still if we could prevent the subscription owner from hacking into the table owner's account. Then, we could very sensibly remove the SET ROLE requirement and check something weaker instead, and that would be fantastic. Since I didn't see how to engineer that, I did this. -- Robert Haas EDB: http://www.enterprisedb.com
