Thread: [HACKERS] pg_ls_dir & friends still have a hard-coded superuser check
Commit 1574783b4ced0356fbc626af1a1a469faa6b41e1 gratifyingly removed hard-coded superuser checks from assorted functions, which makes it possible to GRANT EXECUTE ON FUNCTION pg_catalog.whatever() to unprivileged users if the DBA so desires. However, the functions in genfile.c still have hard-coded checks: pg_read_file(), pg_read_binary_file(), pg_stat_file(), and pg_ls_dir(). I think those functions ought to get the same treatment that the commit mentioned above gave to a bunch of others. Obviously, there's some risk of DBAs doing stupid things there, but stupidity is hard to prevent in a general way and nanny-ism is annoying. The use case I have in mind is a monitoring tool that needs access to one more of those functions -- in keeping with the principle of least privilege, it's much better to give the monitoring user only the privileges which it actually needs than to make it a superuser. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
I tend to agree. But in the past when this came up people pointed out you could equally do things this way and still grant all the access you wanted using SECURITY DEFINER. Arguably that's a better approach because then instead of auditing the entire monitor script you only need to audit this one wrapper function, pg_ls_monitor_dir() which just calls pg_ls_dir() on this one directory.
On Wed, Jan 25, 2017 at 11:30 AM, Greg Stark <stark@mit.edu> wrote: > I tend to agree. But in the past when this came up people pointed out > you could equally do things this way and still grant all the access > you wanted using SECURITY DEFINER. Arguably that's a better approach > because then instead of auditing the entire monitor script you only > need to audit this one wrapper function, pg_ls_monitor_dir() which > just calls pg_ls_dir() on this one directory. I agree that can be done, but it's nicer if you can use the same SQL all the time. With that solution, you need one SQL query to run when you've got superuser privileges and a different SQL query to run when you are running without superuser privileges but somebody's run the create-security-definer-wrappers-for-me script. That's a deployment nuisance if you want to support both configurations. Also, the same argument could be made about removing the built-in superuser check from ANY function, and we've already rejected that argument for a bunch of other functions. If we say that argument is valid for some functions but not others, then we've got to decide for which ones it's valid and for which ones it isn't, and consensus will not be forthcoming. I take the position that hard-coded superuser checks stink in general, and I'm grateful to Stephen for his work making dump/restore work properly on system catalog permissions so that we can support better alternatives. I'm not asking for anything more than that we apply that same policy here as we have in other cases. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > The use case I have in mind is > a monitoring tool that needs access to one more of those functions -- > in keeping with the principle of least privilege, it's much better to > give the monitoring user only the privileges which it actually needs > than to make it a superuser. That isn't what you're doing with those functions though, you're giving the monitoring tool superuser-level rights but trying to pretend like you're not. That's not really how good security works. I am entirely in agreement with providing a way to give monitoring tools more information, but that should be done through proper design and consideration of just what info they actually need (as well as what a useful format for it is). On my plate for a long time has been to add a function to return how much WAL is remaining in pg_wal for a monitoring system to be able to report on. That could be done with something like pg_ls_dir, but that's a rather hokey way to get it, and it'd be a lot nicer to just have a function which returns it, or maybe one that returns the oldest WAL position available on the system and what the current position is, which I think we might actually have. In other words, please actually outline a use-case, and let's design a proper solution. Thanks! Stephen
Greg, * Greg Stark (stark@mit.edu) wrote: > I tend to agree. But in the past when this came up people pointed out > you could equally do things this way and still grant all the access > you wanted using SECURITY DEFINER. Arguably that's a better approach > because then instead of auditing the entire monitor script you only > need to audit this one wrapper function, pg_ls_monitor_dir() which > just calls pg_ls_dir() on this one directory. I'm not a fan of SECURITY DEFINER functions for this sort of thing and don't like the suggestion of simply wrapping functions that provide superuser-level access in a security definer function and then saying that giving someone access to that function isn't giving them superuser, because that's just false. Thanks! Stephen
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > Also, the same argument could be made about removing the built-in > superuser check from ANY function, and we've already rejected that > argument for a bunch of other functions. If we say that argument is > valid for some functions but not others, then we've got to decide for > which ones it's valid and for which ones it isn't, and consensus will > not be forthcoming. I take the position that hard-coded superuser > checks stink in general, and I'm grateful to Stephen for his work > making dump/restore work properly on system catalog permissions so > that we can support better alternatives. I'm not asking for anything > more than that we apply that same policy here as we have in other > cases. I went over *every* superuser check in the system when I did that work, wrote up a long email about why I made the decisions that I did, posted it here, had follow-on discussions, all of which lead to the patch which ended up going in. I am not anxious to revisit that decision and certainly not based on an argument that, so far, boils down to "I think a monitoring system might be able to use this function that allows it to read pg_authid directly, so we should drop the superuser() check in it." Thanks! Stephen
On Wed, Jan 25, 2017 at 2:08 PM, Stephen Frost <sfrost@snowman.net> wrote: > That isn't what you're doing with those functions though, you're giving > the monitoring tool superuser-level rights but trying to pretend like > you're not. Uh, how so? None of those functions can be used to escalate to superuser privileges. I am trying to give SOME superuser privileges and not others. That IS how good security works. I don't really think it's necessary to outline the use case more than I have already. It's perfectly reasonable to want a monitoring tool to have access to pg_ls_dir() - for example, you could use that to monitor for relation files orphaned by a previous crash. Also, as mentioned above, I don't think this should have to be litigated for every single function individually. If it's a good idea for a non-superuser to be able to pg_start_backup() and pg_stop_backup(), the person doing that has access to read every byte of data in the filesystem; if they don't, there's no point in giving them access to run those functions. Access to just pg_ls_dir(), for example, can't be any more dangerous than that. Indeed, I'd argue that it's a heck of a lot LESS dangerous. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Jan 25, 2017 at 2:13 PM, Stephen Frost <sfrost@snowman.net> wrote: > I went over *every* superuser check in the system when I did that work, > wrote up a long email about why I made the decisions that I did, posted > it here, had follow-on discussions, all of which lead to the patch which > ended up going in. Link to that email? I went back and looked at that thread and didn't see anything that looked like a general policy statement to me. But I may have missed it. > I am not anxious to revisit that decision and certainly not based on > an argument that, so far, boils down to "I think a monitoring system > might be able to use this function that allows it to read pg_authid > directly, so we should drop the superuser() check in it." Well, I'm not eager to revisit all the decisions you'd like to overturn either, but we'll just both have to cope. I assume we're both coming at these issues with the intention of making PostgreSQL better; the fact that we don't always agree on everything is probably inevitable. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Wed, Jan 25, 2017 at 2:13 PM, Stephen Frost <sfrost@snowman.net> wrote: > > I went over *every* superuser check in the system when I did that work, > > wrote up a long email about why I made the decisions that I did, posted > > it here, had follow-on discussions, all of which lead to the patch which > > ended up going in. > > Link to that email? I went back and looked at that thread and didn't > see anything that looked like a general policy statement to me. But I > may have missed it. Not sure which thread you were looking at, but this one: https://www.postgresql.org/message-id/20141015052259.GG28859%40tamriel.snowman.net Has a review of all superuser checks in the backend, as noted in the first paragraph ("shown below in a review of the existing superuser checks in the backend"). Later on in that thread, at least in: https://www.postgresql.org/message-id/20160106161302.GP3685%40tamriel.snowman.net In an email to you and Noah: ---------------- The general approach which I've been using for the default roles is that they should grant rights which aren't able to be used to trivially make oneself a superuser. ---------------- My recollection is saying that about 10 times during that period of time, though perhaps I am exaggurating due to it being a rather painful process to get through. > I assume we're > both coming at these issues with the intention of making PostgreSQL > better; Always. > the fact that we don't always agree on everything is probably > inevitable. Agreed. Thanks! Stephen
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Wed, Jan 25, 2017 at 2:08 PM, Stephen Frost <sfrost@snowman.net> wrote: > > That isn't what you're doing with those functions though, you're giving > > the monitoring tool superuser-level rights but trying to pretend like > > you're not. > > Uh, how so? None of those functions can be used to escalate to > superuser privileges. I am trying to give SOME superuser privileges > and not others. That IS how good security works. Your email is 'pg_ls_dir & friends', which I took to imply at *least* pg_read_file() and pg_read_binary_file(), and it's not unreasonable to think you may have meant everything in adminpack, which also includes pg_file_write(), pg_file_rename() and pg_file_unlink(). > I don't really think it's necessary to outline the use case more than > I have already. It's perfectly reasonable to want a monitoring tool > to have access to pg_ls_dir() - for example, you could use that to > monitor for relation files orphaned by a previous crash. Sure. What I believe would be better would be a function in PG that allows you to know about all of the relation files which were orphaned from a previous crash, and perhaps even one to go clean all of those up. I certainly would have no issue with both of those being available to a non-superuser. Sure, you could do the same with pg_ls_dir(), various complex bits of SQL to figure out what's been orphaned, and pg_file_unlink() to handle the nasty part of unlinking the files, but you could also use pg_ls_dir() to look at the files in PG's home directory, or pg_file_unlink() to remove the PG user's .bashrc. Does the monitoring tool need to be able to see the files in PG's root directory, or to be able to unlink the PG user's .bashrc? No. > Also, as > mentioned above, I don't think this should have to be litigated for > every single function individually. If it's a good idea for a > non-superuser to be able to pg_start_backup() and pg_stop_backup(), > the person doing that has access to read every byte of data in the > filesystem; if they don't, there's no point in giving them access to > run those functions. Access to just pg_ls_dir(), for example, can't > be any more dangerous than that. Indeed, I'd argue that it's a heck > of a lot LESS dangerous. It wasn't litigated for every single function. A reivew of all cases was performed and a very lengthy discussion held about how to give non-superusers access to the functions which made sense was done, with the resulting work finally being accepted and included into 9.6. Further, as discussed and explained on the thread I just linked you to, the assumption that a user who can call pg_start/stop_backup() has access to read every byte on the filesystem isn't correct. Am I sure that someone who has pg_ls_dir() could get superuser access on the box? No, but I am sure that, in my opinion, it's frankly a pretty dirty hack to use pg_ls_dir() in a monitoring tool and we should provide better ways to monitor PG to our users. Also, am I sure that we shouldn't ever give a user the ability to arbitrairly list directories on the filesystem? No, but this isn't a good justification for that change. If we *are* going to go down the road of giving users filesystem-like access of any kind then I would *much* rather look at the approach that I outlined before and that other database systems have, which is to have a way to say "user X can read, write, list, do whatever, with files in directory /blah/blah". Perhaps with sub-directory abilities too. Nannyism, if we had that ability, would be to say "you can set DIRECTORY to anything but PGDATA." or "you can't set DIRECTORY to be /". I don't think it's nannyism to keep things like pg_read_file and pg_file_write as superuser-only. Thanks! Stephen
On Wed, Jan 25, 2017 at 3:17 PM, Stephen Frost <sfrost@snowman.net> wrote: > Your email is 'pg_ls_dir & friends', which I took to imply at *least* > pg_read_file() and pg_read_binary_file(), and it's not unreasonable to > think you may have meant everything in adminpack, which also includes > pg_file_write(), pg_file_rename() and pg_file_unlink(). Well, I was talking about genfile.c, which doesn't contain those functions, but for the record I'm in favor of extirpating every single hard-coded superuser check from the backend without exception. Preventing people from calling functions by denying the ability to meaningfully GRANT EXECUTE on those functions doesn't actually stop them from delegating those functions to non-superusers. It either (a) causes them to give superuser privileges to accounts that otherwise would have had lesser privileges or (b) forces them to use wrapper functions to grant access rather than doing it directly or (c) some other dirty hack. If they pick (a), security is worse; if they pick (b) or (c), you haven't prevented them from doing what they wanted to do anyway. You've just made it annoying and inconvenient. Both EnterpriseDB's PEM and check_postgres.pl currently have a bunch of things that don't work unless you run as superuser. I think we should be trying to provide ways of reducing those lists. If I can't get agreement on method (a), I'm going to go look for ways of doing (b) or (c), which is more work and uglier but if I can't get consensus here then oh well. >> I don't really think it's necessary to outline the use case more than >> I have already. It's perfectly reasonable to want a monitoring tool >> to have access to pg_ls_dir() - for example, you could use that to >> monitor for relation files orphaned by a previous crash. > > Sure. What I believe would be better would be a function in PG that > allows you to know about all of the relation files which were orphaned > from a previous crash, and perhaps even one to go clean all of those up. > I certainly would have no issue with both of those being available to a > non-superuser. > > Sure, you could do the same with pg_ls_dir(), various complex bits of > SQL to figure out what's been orphaned, and pg_file_unlink() to handle > the nasty part of unlinking the files, but you could also use > pg_ls_dir() to look at the files in PG's home directory, or > pg_file_unlink() to remove the PG user's .bashrc. > > Does the monitoring tool need to be able to see the files in PG's root > directory, or to be able to unlink the PG user's .bashrc? No. I do not accept your authority to determine what monitoring tools need to or should do. Monitoring tools that use pg_ls_dir are facts on the ground, and your disapprobation doesn't change that at all. It just obstructs moving to a saner permissions framework. I agree that pg_file_unlink() is an unlikely need for a monitoring tool, but (1) conflating pg_ls_dir with pg_file_unlink is a bit unfair and (2) there's no actual point whatsoever in trying to restrict to whom the superuser can give permissions, because the superuser has numerous ways of working around any such restriction. > I don't think it's nannyism to keep things like pg_read_file and > pg_file_write as superuser-only. I entirely disagree. It is for the DBA to decide to whom and for what purpose he wishes to give permissions. And if somebody writes a tool -- monitoring or otherwise -- that needs those permissions, the DBA should be empowered to give those permissions and no more, and should be empowered to do that in a nice, clean way without a lot of hassle. The idea that you or I would try to decide which functions a user is allowed to want to grant access to and which ones they are not allowed to want to grant access to seems laughable to me. We're not smart enough to foresee every possible use case, and we shouldn't try. Even for a function that theoretically allows a privilege escalation to superuser (like pg_write_file), it's got to be better to give a user account permission to use that one function than to just give up and give that account superuser privileges, because escalating privileges with that blunt instrument would take more work than a casual hacker would be willing to put in. For a function like the one that this thread actually started out being about, like pg_ls_dir, it's vastly better. You mention that you're not sure that pg_ls_dir() would allow a privilege escalation to superuser, but I think we both know that there is probably no such escalation. Why you'd rather have people running check_postgres.pl's wal_files check under an actual superuser account rather than an account that only has rights to do pg_ls_dir is beyond me. Your response will no doubt be that there should otherwise be some other way to write that check that doesn't use pg_ls_dir. Maybe. But the thing is that pg_ls_dir is a general tool. You can use it to do a lot of things, and you can write a monitoring probe that does those things without having to wait for a new release of PostgreSQL. So your argument boils down to saying that when somebody wants to monitor something that could be checked using pg_ls_dir(), they shouldn't do write a query using pg_ls_dir(). Instead, they should submit a new patch to core that adds a special-purpose function implementing the exact functionality they need, get consensus on it, get somebody to commit it, wait for a new PostgreSQL release to come out, wait for the users who might want the probe to upgrade to that new release, and THEN they can do the monitoring they want to do. Now, I say that's ridiculous. What's actually going to happen is that the tool author is going to say "this probe requires superuser" and move on. The fact that check_postgres.pl has done exactly that for about 5 different probes -- not all because of pg_ls_dir, but one of them is for that reason -- suggests that this will in fact be a typical reaction whenever a fine-grained privilege is not available to be granted. We can decide to alleviate that pain or we can decide against it, but telling people "don't do that" is not going to work. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Wed, Jan 25, 2017 at 3:17 PM, Stephen Frost <sfrost@snowman.net> wrote: > > Your email is 'pg_ls_dir & friends', which I took to imply at *least* > > pg_read_file() and pg_read_binary_file(), and it's not unreasonable to > > think you may have meant everything in adminpack, which also includes > > pg_file_write(), pg_file_rename() and pg_file_unlink(). > > Well, I was talking about genfile.c, which doesn't contain those > functions, but for the record I'm in favor of extirpating every single > hard-coded superuser check from the backend without exception. Then it was correct for me to assume that's what you meant, and means my reaction and response are on-point. > Preventing people from calling functions by denying the ability to > meaningfully GRANT EXECUTE on those functions doesn't actually stop > them from delegating those functions to non-superusers. It either (a) > causes them to give superuser privileges to accounts that otherwise > would have had lesser privileges or (b) forces them to use wrapper > functions to grant access rather than doing it directly or (c) some > other dirty hack. If they pick (a), security is worse; if they pick > (b) or (c), you haven't prevented them from doing what they wanted to > do anyway. You've just made it annoying and inconvenient. The notion that security is 'worse' under (a) is flawed- it's no different. With regard to 'b', if their wrapper function is sufficiently careful to ensure that the caller isn't able to do anything which would increase the caller's level to that of superuser, then security is improved. If the wrapper simply turns around can calls the underlying function, then it's no different from '(a)'. I am suggesting that we shouldn't make it look like there are distinctions when there is actually no difference. That is a good thing for our users because it keeps them informed about what they're actually doing when they grant access. > Both EnterpriseDB's PEM and check_postgres.pl currently have a bunch > of things that don't work unless you run as superuser. I think we > should be trying to provide ways of reducing those lists. If I can't > get agreement on method (a), I'm going to go look for ways of doing > (b) or (c), which is more work and uglier but if I can't get consensus > here then oh well. I've commented on here and spoken numerous times about exactly that goal of reducing the checks in check_postgres.pl which require superuser. You're not actually doing that and nothing you've outlined in here so far makes me believe you see how having pg_write_file() access is equivilant to giving someone superuser, and that concerns me. As someone who has contributed code and committed code back to check_postgres.pl, I would be against making changes there which install security definer functions to give the monitoring user superuser-level access, and I believe Greg S-M would feel the same way considering that he and I have discussed exactly that in the past. I don't mean to speak for him and perhaps his opinion has changed, but it seems unlikely to me. If the DBA wants to give the monitoring user superuser-level access to run the superuser-requiring checks that check_postgres.pl has, they're welcome to do so, but they'll be making an informed decision that they have weighed the risk of their monitoring user being compromised against the value of that additional monitoring, which is an entirely appropriate and reasonable decision for them to make. > I do not accept your authority to determine what monitoring tools need > to or should do. Monitoring tools that use pg_ls_dir are facts on the > ground, and your disapprobation doesn't change that at all. It just > obstructs moving to a saner permissions framework. Allowing GRANT to be used to give access to pg_write_file and friends is not a 'permissions framework'. Further, I am not claiming authority over what monitoring tools should need to do or not do, and a great many people run their monitoring tools as superuser. I am not trying to take away their ability to do so. The way to make progress here is not, however, to just decide that all those superuser() checks we put in place were silly and should be removed, it's to provide better ways to monitor PG which provide exactly the monitoring information needed in a useful and sensible way. I understand the allure of just removing a few lines of code to make things "easier" or "faster" or "better", but I don't think it's a good idea to remove these superuser checks, nor do I think it's a good idea to remove our WAL CRCs even if it'd help some of my clients, nor do I think it's a good idea to have checksums disabled by default, or to rip out all of WAL as being "unnecessary" because we have ZFS and it should handle all things data and we could just fsync all of the heap files on commit and be done with it. Admittedly, you're not argueing for half of what I just mentioned, but I'm not arguing that DBAs shouldn't be able to have their monitoring users be superusers either, I'm pointing out that removing those checks makes any user who is GRANT access to the functions the equivilant of a superuser, potentially without the DBA realizing it, and that is an all-together bad thing. If a DBA sees the reasoning behind not making a monitoring user a superuser, we shouldn't be trying to pull the wool over their eyes or sprinkling a little GRANT dust over in the corner that turns the monitoring user into an account with superuser-level access, we should accept that their decision is to not have the monitoring user have superuser rights and ensure that the database permission system doesn't give it to them. > > I don't think it's nannyism to keep things like pg_read_file and > > pg_file_write as superuser-only. > > I entirely disagree. It is for the DBA to decide to whom and for what > purpose he wishes to give permissions. And if somebody writes a tool > -- monitoring or otherwise -- that needs those permissions, the DBA > should be empowered to give those permissions and no more, and should > be empowered to do that in a nice, clean way without a lot of hassle. The DBA is well within their rights and abilities to do so with a simple comamnd: ALTER ROLE. I am not suggesting that we take their right or ability to do so away from them. > Why you'd rather have people > running check_postgres.pl's wal_files check under an actual superuser > account rather than an account that only has rights to do pg_ls_dir is > beyond me. I don't want people to run the wal_files check as a superuser, as I've said before, even on this thread, there should be a better answer. I do not view removing the superuser() check, while appealing and appearing to be simple, to be that 'better answer'. > Your response will no doubt be that there should otherwise be some > other way to write that check that doesn't use pg_ls_dir. Maybe. But > the thing is that pg_ls_dir is a general tool. You can use it to do a > lot of things, and you can write a monitoring probe that does those > things without having to wait for a new release of PostgreSQL. With pg_write_file(), pg_read_file(), and friends, there's a whole lot of stuff we could punt on and say "user, you figure out how to make these work for you, we don't feel like doing anything more." > So > your argument boils down to saying that when somebody wants to monitor > something that could be checked using pg_ls_dir(), they shouldn't do > write a query using pg_ls_dir(). No, I'm saying that we should provide something better than pg_ls_dir() for them to use. If they want to use pg_ls_dir(), then they can go for it, but I really do *not* think the answer to "what's the best way to see how much WAL space PG is using?" is "oh, just use pg_ls_dir()." Consdier if we had such a neat feature today to return the number of WAL files- assuming we named it correctly, we wouldn't have people complaining about the fact that we're about to change pg_xlog to pg_wal. Generally speaking, I'd really like external tools, monitoring systems, etc, to know less, not more, about our on-disk data layout and structures, where that's possible. > Instead, they should submit a new > patch to core that adds a special-purpose function implementing the > exact functionality they need, get consensus on it, get somebody to > commit it, wait for a new PostgreSQL release to come out, wait for the > users who might want the probe to upgrade to that new release, and > THEN they can do the monitoring they want to do. I sure do think that we should add new monitoring capabilities to PG and that we absolutely should *not* start telling people who want to implement new monitoring features that "oh, well, users can just use pg_read_binary_file() directly to get that info from pg_controldata, why would we need to write a function for that?" From my perspective, at least, that's about the same level as what you're asking to do with pg_ls_dir(). If your goal is to give users the ability to query how many WAL files are in pg_wal, I don't doubt that you could get it implemented and into PG10 with a lot more ease than arguing about pg_ls_dir() with me, or anyone else who wants to chime in on this, and users would have it at just the same time as they would a hack to pg_ls_dir(), and they wouldn't be giving their monitoring user the ability to look at what files the DBA has his home directory on the database server. > Now, I say that's > ridiculous. What's actually going to happen is that the tool author > is going to say "this probe requires superuser" and move on. Sure, and that's a fair response. > The fact > that check_postgres.pl has done exactly that for about 5 different > probes -- not all because of pg_ls_dir, but one of them is for that > reason -- suggests that this will in fact be a typical reaction > whenever a fine-grained privilege is not available to be granted. We > can decide to alleviate that pain or we can decide against it, but > telling people "don't do that" is not going to work. I am all in favor of alleviating that pain, by providing a proper solution which is actually well thought out and designed to answer the question. Removing the check from pg_ls_dir() isn't any of that. Thanks! Stephen
Hi, On 2017-01-25 16:52:38 -0500, Stephen Frost wrote: > * Robert Haas (robertmhaas@gmail.com) wrote: > > Preventing people from calling functions by denying the ability to > > meaningfully GRANT EXECUTE on those functions doesn't actually stop > > them from delegating those functions to non-superusers. It either (a) > > causes them to give superuser privileges to accounts that otherwise > > would have had lesser privileges or (b) forces them to use wrapper > > functions to grant access rather than doing it directly or (c) some > > other dirty hack. If they pick (a), security is worse; if they pick > > (b) or (c), you haven't prevented them from doing what they wanted to > > do anyway. You've just made it annoying and inconvenient. > > The notion that security is 'worse' under (a) is flawed- it's no > different. Huh? Obviously that's nonesense, given the pg_ls_dir example. > With regard to 'b', if their wrapper function is > sufficiently careful to ensure that the caller isn't able to do anything > which would increase the caller's level to that of superuser, then > security is improved. Given how complex "sufficiently careful" is for security definer UDFs, in comparison to estimating the security of granting to a function like pg_ls_dir (or pretty much any other that doesn't call out to SQL level stuff like operators, output functions, etc), I don't understand this. > If the wrapper simply turns around can calls the underlying function, > then it's no different from '(a)'. Except for stuff like search path. > I am suggesting that we shouldn't make it look like there are > distinctions when there is actually no difference. That is a good thing > for our users because it keeps them informed about what they're actually > doing when they grant access. This position doesn't make a lick of sense to me. There's simply no benefit at all in requiring to create wrapper functions, over allowing to grant to non-superuser. Both is possible, secdef is a lot harder to get right. And you already heavily started down the path of removing superuser() type checks - you're just arguing to make it more or less randomly less consistent. > I've commented on here and spoken numerous times about exactly that goal > of reducing the checks in check_postgres.pl which require superuser. > You're not actually doing that and nothing you've outlined in here so > far makes me believe you see how having pg_write_file() access is > equivilant to giving someone superuser, and that concerns me. That's the users responsibility, and Robert didnt' really suggest granting pg_write_file() permissions, so this seems to be a straw man. Andres
Andres, * Andres Freund (andres@anarazel.de) wrote: > On 2017-01-25 16:52:38 -0500, Stephen Frost wrote: > > * Robert Haas (robertmhaas@gmail.com) wrote: > > > Preventing people from calling functions by denying the ability to > > > meaningfully GRANT EXECUTE on those functions doesn't actually stop > > > them from delegating those functions to non-superusers. It either (a) > > > causes them to give superuser privileges to accounts that otherwise > > > would have had lesser privileges or (b) forces them to use wrapper > > > functions to grant access rather than doing it directly or (c) some > > > other dirty hack. If they pick (a), security is worse; if they pick > > > (b) or (c), you haven't prevented them from doing what they wanted to > > > do anyway. You've just made it annoying and inconvenient. > > > > The notion that security is 'worse' under (a) is flawed- it's no > > different. > > Huh? Obviously that's nonesense, given the pg_ls_dir example. Robert's made it clear that he'd like to have a blanket rule that we don't have superuser checks in these code paths if they can be GRANT'd at the database level, which goes beyond pg_ls_dir. If the question was only about pg_ls_dir, then I still wouldn't be for it, because, as the bits you didn't quote discussed, it encourages users and 3rd party tool authors to base more things off of pg_ls_dir to look into the way PG stores data on disk, and affords more access than the monitoring user has any need for, none of which are good, imv. It also discourages people from implementing proper solutions which you can 'just use pg_ls_dir()', which I also don't agree with. If you really want to do an ls, go talk to the OS. ACLs are possible to provide that with more granularity than what would be available through pg_ls_dir(). We aren't in the "give a user the ability to do an ls" business, frankly. > > With regard to 'b', if their wrapper function is > > sufficiently careful to ensure that the caller isn't able to do anything > > which would increase the caller's level to that of superuser, then > > security is improved. > > Given how complex "sufficiently careful" is for security definer UDFs, > in comparison to estimating the security of granting to a function like > pg_ls_dir (or pretty much any other that doesn't call out to SQL level > stuff like operators, output functions, etc), I don't understand this. If you're implying that security definer UDFs are hard to write and get correct, then I agree with you there. I was affording the benefit of the doubt to that proposed approach. > > If the wrapper simply turns around can calls the underlying function, > > then it's no different from '(a)'. > > Except for stuff like search path. If you consider '(a)' to be the same as superuser, which I was postulating, then this doesn't strike me as making terribly much difference. > > I am suggesting that we shouldn't make it look like there are > > distinctions when there is actually no difference. That is a good thing > > for our users because it keeps them informed about what they're actually > > doing when they grant access. > > This position doesn't make a lick of sense to me. There's simply no > benefit at all in requiring to create wrapper functions, over allowing > to grant to non-superuser. Both is possible, secdef is a lot harder to > get right. And you already heavily started down the path of removing > superuser() type checks - you're just arguing to make it more or less > randomly less consistent. I find this bizarre considering I went through a detailed effort to go look at every superuser check in the system and discussed, on this list, the reasoning behind each and every one of them. I do generally consider arbitrary access to syscalls via the database to be a privilege which really only the superuser should have. > > I've commented on here and spoken numerous times about exactly that goal > > of reducing the checks in check_postgres.pl which require superuser. > > You're not actually doing that and nothing you've outlined in here so > > far makes me believe you see how having pg_write_file() access is > > equivilant to giving someone superuser, and that concerns me. > > That's the users responsibility, and Robert didnt' really suggest > granting pg_write_file() permissions, so this seems to be a straw man. He was not arguing for only pg_ls_dir(), but for checks in all "friends", which he later clarified to include pg_write_file(). Thanks! Stephen
On 2017-01-25 18:04:09 -0500, Stephen Frost wrote: > Andres, > > * Andres Freund (andres@anarazel.de) wrote: > > On 2017-01-25 16:52:38 -0500, Stephen Frost wrote: > > > * Robert Haas (robertmhaas@gmail.com) wrote: > > > > Preventing people from calling functions by denying the ability to > > > > meaningfully GRANT EXECUTE on those functions doesn't actually stop > > > > them from delegating those functions to non-superusers. It either (a) > > > > causes them to give superuser privileges to accounts that otherwise > > > > would have had lesser privileges or (b) forces them to use wrapper > > > > functions to grant access rather than doing it directly or (c) some > > > > other dirty hack. If they pick (a), security is worse; if they pick > > > > (b) or (c), you haven't prevented them from doing what they wanted to > > > > do anyway. You've just made it annoying and inconvenient. > > > > > > The notion that security is 'worse' under (a) is flawed- it's no > > > different. > > > > Huh? Obviously that's nonesense, given the pg_ls_dir example. > > Robert's made it clear that he'd like to have a blanket rule that we > don't have superuser checks in these code paths if they can be GRANT'd > at the database level, which goes beyond pg_ls_dir. That seems right to me. I don't see much benefit for the superuser() style checks, with a few exceptions. Granting by default is obviously an entirely different question. > If the question was only about pg_ls_dir, then I still wouldn't be for > it, because, as the bits you didn't quote discussed, it encourages users > and 3rd party tool authors to base more things off of pg_ls_dir to > look into the way PG stores data on disk, and affords more access than > the monitoring user has any need for, none of which are good, imv. It > also discourages people from implementing proper solutions which you can > 'just use pg_ls_dir()', which I also don't agree with. In other words, you're trying to force people to do stuff your preferred way, instead of allowing them to get things done is a reasonable manner. > If you really want to do an ls, go talk to the OS. ACLs are possible to > provide that with more granularity than what would be available through > pg_ls_dir(). We aren't in the "give a user the ability to do an ls" > business, frankly. Wut. > > > I am suggesting that we shouldn't make it look like there are > > > distinctions when there is actually no difference. That is a good thing > > > for our users because it keeps them informed about what they're actually > > > doing when they grant access. > > > > This position doesn't make a lick of sense to me. There's simply no > > benefit at all in requiring to create wrapper functions, over allowing > > to grant to non-superuser. Both is possible, secdef is a lot harder to > > get right. And you already heavily started down the path of removing > > superuser() type checks - you're just arguing to make it more or less > > randomly less consistent. > > I find this bizarre considering I went through a detailed effort to go > look at every superuser check in the system and discussed, on this list, > the reasoning behind each and every one of them. I do generally > consider arbitrary access to syscalls via the database to be a privilege > which really only the superuser should have. Just because you argued doesn't mean I agree. Greetings, Andres Freund
Andres, * Andres Freund (andres@anarazel.de) wrote: > On 2017-01-25 18:04:09 -0500, Stephen Frost wrote: > > Robert's made it clear that he'd like to have a blanket rule that we > > don't have superuser checks in these code paths if they can be GRANT'd > > at the database level, which goes beyond pg_ls_dir. > > That seems right to me. I don't see much benefit for the superuser() > style checks, with a few exceptions. Granting by default is obviously > an entirely different question. Well, for my part at least, I disagree. Superuser is a very different animal, imv, than privileges which can be GRANT'd, and I feel that's an altogether good thing. > In other words, you're trying to force people to do stuff your preferred > way, instead of allowing them to get things done is a reasonable manner. Apparently we disagree about what is a 'reasonable manner'. Thanks! Stephen
On Wed, Jan 25, 2017 at 8:22 PM, Stephen Frost <sfrost@snowman.net> wrote: > Apparently we disagree about what is a 'reasonable manner'. Yes. I think that a "reasonable manner" should mean "what the DBA thinks is reasonable", whereas you apparently think it should mean "what the DBA thinks is reasonable, but only if the core developers and in particular Stephen Frost also think it's reasonable". Your proposed policy is essentially that functions should have built-in superuser checks if having access to that function is sufficient to escalate your account to full superuser privileges. But: 1. There's no consensus on any such policy. 2. If there were such a policy it would favor, not oppose, changing pg_ls_dir(), because you can't escalate to superuser given access to pg_ls_dir(). Yet you are also opposed to changing pg_ls_dir() for reasons that boil down to a personal preference on your part for people not using it to build monitoring scripts. 3. Such a policy can only be enforced to the extent that we can accurately predict which functions can be used to escalate to superuser, which is not necessarily obvious in every case. Under your proposed policy, if a given function turns out to be more dangerous than we'd previously thought, we'd have to stick the superuser check back in for the next release. And if it turns out to be less dangerous than we thought, we'd take the check out. That would be silly. 4. Such a policy is useless from a security perspective because you can't actually prevent superusers from delegating access to those functions. You can force them to use wrapper functions but that doesn't eo ipso improve security. It might make security better or worse depending on how well the functions are written, and it seems extremely optimistic to suppose that everyone who writes a security definer wrapper function will actually do anything more than expose the underlying function as-is (and maybe forget to schema-qualify something). 5. If you're worried about people granting access to functions that allow escalation to the superuser account, what you really ought to do is put some effort into documenting which functions have such hazards and for what general reasons. That would have a much better chance of preventing people from delegating access to dangerous functions inadvertently than the current method, which relies on people knowing (without documentation) that you've attempted to leave hard-coded superuser() checks in some functions but not others for reasons that sometimes but not always include privilege escalation, correctly distinguishing which such cases involve privilege escalation as opposed to other arbitrary criteria, and deciding neither to create secdef wrappers for anything that has a built-in check for reasons of privilege isolation nor to give up on privilege isolation and hand out superuser to people who need pg_ls_dir(). While such a clever chain of deductive reasoning cannot be ruled out, it would be astonishing if it happened very often. I'd be willing to agree to write documentation along the lines suggested in (5) as a condition of removing the remaining superuser checks if you'd be willing to review it and suggest a place to put it. But I have a feeling compromise may not be possible here. To me, the hand-wringing about the evils of pg_ls_dir() on this thread contrasts rather starkly with the complete lack of discussion about whether the patch removing superuser checks from pgstattuple was opening up any security vulnerabilities. And given that the aforesaid patch lets a user who has EXECUTE privileges on pgstattuple run that function even on relations for which they have no other privileges, such as say pg_authid, it hardly seems self-evident that there are no leaks there. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > Your proposed policy is essentially that functions should have > built-in superuser checks if having access to that function is > sufficient to escalate your account to full superuser privileges. Yes, I do. > 1. There's no consensus on any such policy. Evidently. I will say that it was brought up on a thread with quite a bit of general discussion and resulted in a committed patch. If you want my 2c on it, there was at least a consensus on it at that time, even if one no longer exists. I certainly don't recall anyone clamouring at the time that we should re-evaluate how my assessment was done or the choices made regarding the functions which you now, 2 years later, wish to change. > 2. If there were such a policy it would favor, not oppose, changing > pg_ls_dir(), because you can't escalate to superuser given access to > pg_ls_dir(). Yet you are also opposed to changing pg_ls_dir() for > reasons that boil down to a personal preference on your part for > people not using it to build monitoring scripts. If your argument was specifically regarding pg_ls_dir() and pg_ls_dir() only then I would have misgivings about it because it's a piss-poor solution to the problem that you've actually presented and I am concerned that such a "design" will lead to later implication that it's the "right" thing to do and that we shouldn't try to do better. In other words, the exact argument we always tend to have when we're talking about new features and their design- is it the right design, etc. > 3. Such a policy can only be enforced to the extent that we can > accurately predict which functions can be used to escalate to > superuser, which is not necessarily obvious in every case. Under your > proposed policy, if a given function turns out to be more dangerous > than we'd previously thought, we'd have to stick the superuser check > back in for the next release. And if it turns out to be less > dangerous than we thought, we'd take the check out. That would be > silly. If the proposed function was able to be used to escalate a user to superuser status then I'd think we'd want to do a security release to fix whatever that hole is, since it's almost certainly not the intended use of the function and the lack of proper checking is a security risk. If we stop doing that, where do we draw the line? Is it ok for pg_termiante_backend() to be used to gain superuser-level access through some kind of race condition? What about pg_start_backup()? Which functions are "OK" to be allowed to make users superuser, and which are not, when we don't have superuser() checks in any of them? Further, how is the user supposed to know? Are we going to start sticking labels on functions which say "WARNING: This can be used to become a superuser!" I certainly hope not. > 4. Such a policy is useless from a security perspective because you > can't actually prevent superusers from delegating access to those > functions. You can force them to use wrapper functions but that > doesn't eo ipso improve security. It might make security better or > worse depending on how well the functions are written, and it seems > extremely optimistic to suppose that everyone who writes a security > definer wrapper function will actually do anything more than expose > the underlying function as-is (and maybe forget to schema-qualify > something). This is not about preventing superusers from delegating access to whichever users they wish to, where it makes sense to do so. > 5. If you're worried about people granting access to functions that > allow escalation to the superuser account, what you really ought to do > is put some effort into documenting which functions have such hazards > and for what general reasons. I did, by making the ones that I don't believe can be used to gain superuser level access GRANT'able to non-superusers. The ones which I didn't feel comfortable with changing in that way were left with the superuser() checks in them. Maybe I got that wrong, but that was certainly the idea, as discussed on the thread which I linked you to. > I'd be willing to agree to write documentation along the lines > suggested in (5) as a condition of removing the remaining superuser > checks if you'd be willing to review it and suggest a place to put it. I'm really not anxious to have a bunch of such warnings throughout the docs. Nor am I thrilled about the idea of having to argue about what can and what can't- would you say pg_read_file() is superuser-equiv? I would, because in a great many cases, for all practical purposes, it is, because a ton of users run with md5 and the auth info could be pulled from pg_authid directly with pg_read_file(). The lack of any way to contain the access that such a function would provide would also mean that users would end up making a similar choice- GRANT access to this function that an attacker could use to gain superuser() rights, or not? That's not that far different from the choice of giving a user superuser rights, and if the DBA has already said that they won't run their monitoring solution with superuser rights, why would they say yes to running it with access to a function that'll let the monitoring user become a superuser? > But I have a feeling compromise may not be possible here. To me, the > hand-wringing about the evils of pg_ls_dir() on this thread contrasts > rather starkly with the complete lack of discussion about whether the > patch removing superuser checks from pgstattuple was opening up any > security vulnerabilities. And given that the aforesaid patch lets a > user who has EXECUTE privileges on pgstattuple run that function even > on relations for which they have no other privileges, such as say > pg_authid, it hardly seems self-evident that there are no leaks there. Perhaps you might review what is returned by pgstattuple. I don't disagree that perhaps we should add a permissions check to those functions because it makes sense to, but getting to superuser based on the knowledge of how big a table is, how many tuples are in it, and similar information, strikes me as not quite the same as letting users use pg_file_write(), which, despite all of your complaints regarding my stance on pg_ls_dir(), is one of the functions you'd like to change. Were we to require some kind of per-table permission check on pgstattuple, I'd prefer that it be something different from SELECT, considering you can't actually get any of the table's DATA using pgstattuple(), requiring SELECT rights on the table would end up increasing a user's access to that table, possibly beyond what the DBA wishes, understandably. Frankly, I get quite tired of the argument essentially being made here that because pg_ls_dir() wouldn't grant someone superuser rights, that we should remove superuser checks from everything. As long as you are presenting it like that, I'm going to be quite dead-set against any of it. Thanks! Stephen
On Wed, Jan 25, 2017 at 10:31 PM, Stephen Frost <sfrost@snowman.net> wrote: > Frankly, I get quite tired of the argument essentially being made here > that because pg_ls_dir() wouldn't grant someone superuser rights, that > we should remove superuser checks from everything. As long as you are > presenting it like that, I'm going to be quite dead-set against any of > it. I'm not presenting it like that, and I think I've been quite clear about that. I'm making two separate arguments which you keep conflating. The first is that restricting the ability to GRANT access to a function, even a function that allows escalation to superuser privileges, doesn't improve security, because the superuser can still grant those privileges with more work. That argument favors removing all superuser checks as pointless. The second is that if we were to accept as official policy the idea that functions should have built-in superuser() checks when and only when they allow escalation to superuser, then those checks should be removed from those functions which do not allow such an escalation. That argument favors removing at least some of the superuser checks as inconsistent with policy. If there's a policy -- even one that I don't like -- it should be enforced consistently. I audited the core backend for hard-coded superuser() checks that made it categorically impossible to call a certain SQL function. I looked only for functions where the check was precisely if (!superuser()) ereport(ERROR, ...), so things that were contingent on rolreplication or anything other than superuser-ness are not included in this analysis. I found a total of five such functions, of which I can think of possible escalation-to-superuser risk for two. I did not examine contrib although that might be worth doing at some point. 1. pg_ls_dir. I cannot see how this can ever be used to get superuser privileges. 2. pg_stat_file. I cannot see how this can ever be used to get superuser privileges. 3. pg_read_binary_file. This could potentially let you get superuser or other privileges. Most obviously, if you read and parse the contents of pg_authid, you might be able to find passwords and then break into things. There might be other methods as well. 4. pg_read_file. Essentially the same risks, although reading data files will be a little harder with this function because any NUL byte is going to make the read error out. But given time and determination you can probably still ascertain the contents of every byte in the database, so the risks are about the same. 5. pg_import_system_collations. There doesn't seem to be any obvious way to use this to get superuser privileges, but there might be some subtle risk I'm missing. Attached is a patch that removes the hard-coded superuser checks from all five of these functions, based on the first argument above. Currently, I count three votes in favor of this approach and one opposed. If anyone else wants to weigh in, please do. It would be helpful if anyone weighing in can be clear about whether (a) they are in favor of the patch as proposed, or (b) they are not in favor of the patch as proposed but could support a narrower patch that removed the checks only from functions with no known escalate-to-superuser risks, or (c) they oppose all change. It would also be helpful if the reasons why each person takes the position that they do could be mentioned. Thanks, -- 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
Attachment
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Wed, Jan 25, 2017 at 10:31 PM, Stephen Frost <sfrost@snowman.net> wrote: > > Frankly, I get quite tired of the argument essentially being made here > > that because pg_ls_dir() wouldn't grant someone superuser rights, that > > we should remove superuser checks from everything. As long as you are > > presenting it like that, I'm going to be quite dead-set against any of > > it. > > I'm not presenting it like that, and I think I've been quite clear > about that. I'm making two separate arguments which you keep > conflating. The only function you mentioned in the last email was pg_ls_dir(), this one looks better in that you're at least calling out the other changes you wish to make explicitly with reasoning behind them. > The first is that restricting the ability to GRANT access > to a function, even a function that allows escalation to superuser > privileges, doesn't improve security, because the superuser can still > grant those privileges with more work. Having various inconsistent and unclear ways to grant access to a privileged user is making security worse and that is very much part of my concern. I don't agree, regardless of how many times you claim it, that because the superuser could still grant superuser to someone (or could grant the individual privileges with more work) keeps things status-quo regarding security, or somehow that not making the changes you are proposing reduces existing security. I've certainly learned and come to accept that security, particularly extremely privileged security, is much better when it's kept simple and having multiple ways for someone to become a superuser, even if we admit that there are and document the different ways, certainly isn't simpler and doesn't make me feel like we're improving security. > The second is that if we were to > accept as official policy the idea that functions should have built-in > superuser() checks when and only when they allow escalation to > superuser, then those checks should be removed from those functions > which do not allow such an escalation. That argument favors removing > at least some of the superuser checks as inconsistent with policy. If > there's a policy -- even one that I don't like -- it should be > enforced consistently. This I tend to agree with and I do believe there are may be additional superuser() checks which could be removed. In my initial work, I focused on just the back-end, with some subsequent work on a particular contrib module which I was asked to look at. There are likely further contrib modules which could also be changed, as long as those changes are reviewed and done carefully, with consideration that we don't want the admin to end up granting away superuser rights when they really only intended to grant some specific subset of privileges. > I audited the core backend for hard-coded superuser() checks that made > it categorically impossible to call a certain SQL function. I looked > only for functions where the check was precisely if (!superuser()) > ereport(ERROR, ...), so things that were contingent on rolreplication > or anything other than superuser-ness are not included in this > analysis. I found a total of five such functions, of which I can > think of possible escalation-to-superuser risk for two. I did not > examine contrib although that might be worth doing at some point. > 1. pg_ls_dir. I cannot see how this can ever be used to get superuser > privileges. > 2. pg_stat_file. I cannot see how this can ever be used to get > superuser privileges. I appreciate your efforts, but, for my part, I still don't agree with removing the checks from functions which are, essentially, providing filesystem-level access to non-superusers. The right way to support this would be to work through the issues that were already brought up when Adam and I proposed similar capabilities on this very list, complete with patches, three years ago: https://www.postgresql.org/message-id/CAKRt6CRCQ1jmh3o8gkBBHxWqBJz4StnYUQjO0W8Kauru_SfPKA%40mail.gmail.com As it relates specifically to *monitoring* tools, I continue to be the opinion that we should provide better ways for those tools to get at the information they need. > 3. pg_read_binary_file. This could potentially let you get superuser > or other privileges. Most obviously, if you read and parse the > contents of pg_authid, you might be able to find passwords and then > break into things. There might be other methods as well. > > 4. pg_read_file. Essentially the same risks, although reading data > files will be a little harder with this function because any NUL byte > is going to make the read error out. But given time and determination > you can probably still ascertain the contents of every byte in the > database, so the risks are about the same. Yes, these are clearly functions which can, in most if not all cases, be used to get superuser-level access, and I don't believe we do anyone a service by making it less obvious that it's possible to become a superuser using them. > 5. pg_import_system_collations. There doesn't seem to be any obvious > way to use this to get superuser privileges, but there might be some > subtle risk I'm missing. I haven't looked particularly carefully at it myself. Why aren't you including the other functions mentioned? Not including them, even if they're in contrib, would still end up with things in an inconssitent state, and it hardly seems like it'd be much effort to go through the rest of them with equal care. > Attached is a patch that removes the hard-coded superuser checks from > all five of these functions, based on the first argument above. Perhaps unsuprisingly, but you've still not convinced me, so I don't agree with this change. > Currently, I count three votes in favor of this approach and one > opposed. If anyone else wants to weigh in, please do. It would be > helpful if anyone weighing in can be clear about whether (a) they are > in favor of the patch as proposed, or (b) they are not in favor of the > patch as proposed but could support a narrower patch that removed the > checks only from functions with no known escalate-to-superuser risks, > or (c) they oppose all change. It would also be helpful if the > reasons why each person takes the position that they do could be > mentioned. I agree that it'd be nice if others would weigh in on this. Thanks! Stephen
Stephen Frost wrote: > I agree that it'd be nice if others would weigh in on this. I support your position. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jan 26, 2017 at 10:36 PM, Stephen Frost <sfrost@snowman.net> wrote: > > Perhaps unsuprisingly, but you've still not convinced me, so I don't > agree with this change. > >> Currently, I count three votes in favor of this approach and one >> opposed. If anyone else wants to weigh in, please do. It would be >> helpful if anyone weighing in can be clear about whether (a) they are >> in favor of the patch as proposed, or (b) they are not in favor of the >> patch as proposed but could support a narrower patch that removed the >> checks only from functions with no known escalate-to-superuser risks, >> or (c) they oppose all change. It would also be helpful if the >> reasons why each person takes the position that they do could be >> mentioned. > > I agree that it'd be nice if others would weigh in on this. As a general point I'm entirely in favour of removing any superuser checks and replacing them either with standard GRANT ACL config, or where appropriate, some other type of permission that we can grant to roles as needed. Probably the most common complaint I get from users regarding the management & monitoring tools I work on is that they have to use superuser accounts to get the full benefits, unlike other DBMSs where you can create a role with just the required privileges (or indeed, other DBMSs that ship with such roles pre-defined for convenience). -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 26 January 2017 at 22:36, Stephen Frost <sfrost@snowman.net> wrote: >> Currently, I count three votes in favor of this approach and one >> opposed. If anyone else wants to weigh in, please do. It would be >> helpful if anyone weighing in can be clear about whether (a) they are >> in favor of the patch as proposed, or (b) they are not in favor of the >> patch as proposed but could support a narrower patch that removed the >> checks only from functions with no known escalate-to-superuser risks, >> or (c) they oppose all change. It would also be helpful if the >> reasons why each person takes the position that they do could be >> mentioned. > > I agree that it'd be nice if others would weigh in on this. I oppose the patch as currently presented. In general, I support the viewpoint that we reduce the number of superuser checks. I also recognize that its unlikely that this can be reduced to zero without a clear way forwards. What I suggest we do is this 1. Take the discussion onto the appropriate private forum, which isn't here, IMHO. 2. Try to agree policy first that matches what other security folk will allow. Not much point releasing PostgreSQL and then having other people block parts of it so it matches their view of security. We should seek to resolve that inherent conflict. 3. Make a list of all functions that would cause security problems. One by one, precisely. If we did remove all superuser checks we would need this list documented to advise people of the risks, so it must exist before any commit can be made, assuming we believe in documentation. Notice that I am after risk documentation, not just "By default, use of this function is restricted to superusers" because that just leads to people exposing themselves unknowingly when they follow the next part which seems like official advice, yet is potentially unsafe: "access can be given to other users via GRANT". 4. Later, work towards a patch. We have some weeks to get this right. I'm willing to spend time on workshopping this in Brussels, with any attendees. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jan 27, 2017 at 1:02 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 26 January 2017 at 22:36, Stephen Frost <sfrost@snowman.net> wrote: > >>> Currently, I count three votes in favor of this approach and one >>> opposed. If anyone else wants to weigh in, please do. It would be >>> helpful if anyone weighing in can be clear about whether (a) they are >>> in favor of the patch as proposed, or (b) they are not in favor of the >>> patch as proposed but could support a narrower patch that removed the >>> checks only from functions with no known escalate-to-superuser risks, >>> or (c) they oppose all change. It would also be helpful if the >>> reasons why each person takes the position that they do could be >>> mentioned. >> >> I agree that it'd be nice if others would weigh in on this. > > I oppose the patch as currently presented. > > In general, I support the viewpoint that we reduce the number of > superuser checks. I also recognize that its unlikely that this can be > reduced to zero without a clear way forwards. > > What I suggest we do is this > > 1. Take the discussion onto the appropriate private forum, which isn't > here, IMHO. > > 2. Try to agree policy first that matches what other security folk > will allow. Not much point releasing PostgreSQL and then having other > people block parts of it so it matches their view of security. We > should seek to resolve that inherent conflict. > > 3. Make a list of all functions that would cause security problems. > One by one, precisely. If we did remove all superuser checks we would > need this list documented to advise people of the risks, so it must > exist before any commit can be made, assuming we believe in > documentation. Notice that I am after risk documentation, not just "By > default, use of this function is restricted to superusers" because > that just leads to people exposing themselves unknowingly when they > follow the next part which seems like official advice, yet is > potentially unsafe: "access can be given to other users via GRANT". > > 4. Later, work towards a patch. We have some weeks to get this right. > > I'm willing to spend time on workshopping this in Brussels, with any attendees. I already added it to the agenda items. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Simon Riggs <simon@2ndquadrant.com> writes:
> [ good general plan ]
> 3. Make a list of all functions that would cause security problems.
> One by one, precisely. If we did remove all superuser checks we would
> need this list documented to advise people of the risks, so it must
> exist before any commit can be made, assuming we believe in
> documentation. Notice that I am after risk documentation,
Yeah, I think documentation is the crux of the issue. If there is some
non-obvious reason why letting somebody use pg_ls_dir() is more of a
security hazard than it appears on the surface, the answer is to document
that so DBAs can decide for themselves whether to take the risk.
Count me +1 for removing hard-wired superuser checks, but carefully
and with an overall plan.
regards, tom lane
On 27 January 2017 at 12:56, Dave Page <dpage@pgadmin.org> wrote: > Probably the most common complaint I get from users > regarding the management & monitoring tools I work on is that they > have to use superuser accounts to get the full benefits, unlike other > DBMSs where you can create a role with just the required privileges > (or indeed, other DBMSs that ship with such roles pre-defined for > convenience). This is still just the Adminpack argument. This has been going on for about a decade? Longer. If the monitoring tool requires superuser then that is a problem, so it would be helpful if it didn't do that, please. Not much use having a cool tool if it don't work with the server. The management and monitoring tool could be more specific about what it actually needs, rather than simply requesting generic read and write against the filesystem. Then we can put those specific things into the server and we can all be happy. Again, a detailed list would help here. Does the latest version of pgadmin provide access to log files? I can't see much that really needs Adminpack anymore, though I've not done a thorough analysis at all. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jan 27, 2017 at 1:18 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 27 January 2017 at 12:56, Dave Page <dpage@pgadmin.org> wrote: > >> Probably the most common complaint I get from users >> regarding the management & monitoring tools I work on is that they >> have to use superuser accounts to get the full benefits, unlike other >> DBMSs where you can create a role with just the required privileges >> (or indeed, other DBMSs that ship with such roles pre-defined for >> convenience). > > This is still just the Adminpack argument. This has been going on for > about a decade? Longer. Not entirely. Some simple examples: - Controlling recovery. This is fixed in 9.6+, but prior to that, granting execute privs on pg_xlog_replay_pause() and/or pg_xlog_replay_resume() would be accepted but wouldn't work. - Access to certain GUCs. For example, it could be argued that "SHOW log_directory" is quite reasonable for a monitoring tool to run, for the purposes of auditing/alerting admins to any changes made by a rogue superuser. - ALTER SYSTEM - clearly there is a use case for allow certain users to configure the database server, but not necessarily have the full rights of a superuser that would allow them access to all the data (yeah, I know that separation is far more complex than that alone, but I hope you get the point). > If the monitoring tool requires superuser then that is a problem, so > it would be helpful if it didn't do that, please. Not much use having > a cool tool if it don't work with the server. Sure, that's what I want - to provide the management and monitoring capabilities without requiring superuser. Limiting the capability of the tools is not an option when you talk to users - however for some of them, having to use full superuser accounts is a problem as well (especially for those who are used to other DBMSs that do offer more fine-grained permissions). > The management and monitoring tool could be more specific about what > it actually needs, rather than simply requesting generic read and > write against the filesystem. Then we can put those specific things > into the server and we can all be happy. Again, a detailed list would > help here. Agreed - I do need to do that, and it's on my (extremely long) list. I'm just chiming in on this thread as requested! > Does the latest version of pgadmin provide access to log files? I > can't see much that really needs Adminpack anymore, though I've not > done a thorough analysis at all. Not yet. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Jan 26, 2017 at 5:36 PM, Stephen Frost <sfrost@snowman.net> wrote: >> The first is that restricting the ability to GRANT access >> to a function, even a function that allows escalation to superuser >> privileges, doesn't improve security, because the superuser can still >> grant those privileges with more work. > > Having various inconsistent and unclear ways to grant access to a > privileged user is making security worse and that is very much part of > my concern. I don't agree, regardless of how many times you claim it, > that because the superuser could still grant superuser to someone (or > could grant the individual privileges with more work) keeps things > status-quo regarding security, or somehow that not making the changes > you are proposing reduces existing security. I think it certainly increases security, especially when the functions don't themselves confer superuser access, because then people won't use superuser access when something less will do. I agree that pg_read_file() will often allow an escalation to superuser, but not always, and not necessarily easily. If somebody breaks into the account I'm using for monitoring my system, I'd way, way rather have them get pg_read_file() than superuser. And for pg_ls_dir(), about a hundred times moreso. > Why aren't you including the other functions mentioned? Not including > them, even if they're in contrib, would still end up with things in an > inconssitent state, and it hardly seems like it'd be much effort to go > through the rest of them with equal care. The contrib stuff is harder to change because of backward-compatibility with previous extensions. I could go through and analyze it if we had some consensus on the basic principle here, but we don't seem to agree on anything. Your current policy proposal, as I understand it, is that things should have builtin superuser privileges if (a) there is any chance they can be used to escalate to superuser or (b) they involve any access to the filesystem, even access that can't be used to escalate to superuser. Even if I were to agree to (a), (b) looks like a random wart to justify the status quo, so I'm not real hopeful about further analysis finding any common ground. But that having been said, the broad picture here is that there are two contrib modules which have hard-coded superuser checks at the top of SQL-callable functions: - contrib/adminpack has some very dangerous functions (pg_file_write, pg_file_rename, pg_file_unlink) that have a hard-coded superuser check. It also has an apparently-not-that-dangerous function (pg_logdir_ls) with such a check. - contrib/pageinspect has lots of superuser checks, basically because they have known input-validation weaknesses. See 3e1338475ffc2eac25de60a9de9ce689b763aced for the rationale in detail. The purist in me is inclined to think that the best answer here would be to fix the functions so that they're safe, but that might not be terribly easy to do. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> - contrib/pageinspect has lots of superuser checks, basically because
> they have known input-validation weaknesses. See
> 3e1338475ffc2eac25de60a9de9ce689b763aced for the rationale in detail.
FWIW, I think that's a bit of an oversimplification. There are two
components to contrib/pageinspect: get_raw_page() and a pile of page
interpretation functions. The input-validation issue appies primarily
to the interpretation functions.
In principle, if we could make the interpretation functions completely
bulletproof, we could remove all security checks for them. However,
they're largely useless without get_raw_page(), so it's not apparent
what's the point of doing a lot of work (and taking the risk of missing
something) unless we first get rid of get_raw_page()'s superuser check.
And that seems fraught with questions. Maybe it'd be all right to
allow it for tables you have full read access to (can select all columns,
no RLS) but I'm not convinced. For example, full read access doesn't
allow you to see values that were in the table in the past, before you
were granted read access ... but get_raw_page() might.
regards, tom lane
On Fri, Jan 27, 2017 at 8:18 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > This is still just the Adminpack argument. This has been going on for > about a decade? Longer. Right. > If the monitoring tool requires superuser then that is a problem, so > it would be helpful if it didn't do that, please. Not much use having > a cool tool if it don't work with the server. The argument keeps going on because users aren't willing to give up some of the monitoring capabilities that currently require functionality which is arbitrarily restricted to superusers, and not everyone here is willing to recognize those monitoring needs as legitimate. I believe we should take it as a given that any check somebody's bothered building into a popular monitoring tool like check_postgres.pl is probably a good check written by a smart developer, and the question we should ask ourselves here is "what can we do in the core project to let those checks run without needing superuser privileges?". Instead, we say things like... > The management and monitoring tool could be more specific about what > it actually needs, rather than simply requesting generic read and > write against the filesystem. ...which basically means we think the current checks are written is a way that is wrong, and should be changed to use some not-yet-invented alternative mechanism. But that's position is a bit arrogant and a bit counterfactual. It's counterfactual because the functions in question NOT give generic read and write access against the filesystem; they are limited to the data directory and the log directory. IOW, significant efforts have already been made to impose reasonable security precautions. More could be done, but particularly with regards to basically-harmless things like pg_ls_dir() and pg_stat_file(), there's no real security benefit to doing so. (I agree that there could be some benefit for pg_read/write_file, but the question in my mind is how much complexity it's worth adding for a corner case; I'd be fine with a new mechanism if it's relatively simple.) And it's arrogant because it supposes that we know better than the tool authors what the right way to do everything is. If Greg Sabino Mullane has had a check that uses pg_ls_dir() in check_postgres.pl for ten years, it's probably a great way of checking the thing that he's using it to check, because Greg Sabino Mullane is a smart, experienced, long-time community member. Any argument to the contrary -- especially one offered only in the context of trying to make that check work without requiring superuser privileges -- seems to me to be Monday-morning-quarterbacking (apologies for the Americanism; I don't know what the equivalent British expression would be). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Jan 27, 2017 at 9:35 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> - contrib/pageinspect has lots of superuser checks, basically because >> they have known input-validation weaknesses. See >> 3e1338475ffc2eac25de60a9de9ce689b763aced for the rationale in detail. > > FWIW, I think that's a bit of an oversimplification. There are two > components to contrib/pageinspect: get_raw_page() and a pile of page > interpretation functions. The input-validation issue appies primarily > to the interpretation functions. > > In principle, if we could make the interpretation functions completely > bulletproof, we could remove all security checks for them. However, > they're largely useless without get_raw_page(), so it's not apparent > what's the point of doing a lot of work (and taking the risk of missing > something) unless we first get rid of get_raw_page()'s superuser check. > And that seems fraught with questions. Maybe it'd be all right to > allow it for tables you have full read access to (can select all columns, > no RLS) but I'm not convinced. For example, full read access doesn't > allow you to see values that were in the table in the past, before you > were granted read access ... but get_raw_page() might. The problem is if the interpretation functions aren't completely bulletproof, they might do things like crash the server if you use them to read a corrupt page. That is not any more appealing if you happen to be running as superuser() than otherwise. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> The problem is if the interpretation functions aren't completely
> bulletproof, they might do things like crash the server if you use
> them to read a corrupt page. That is not any more appealing if you
> happen to be running as superuser() than otherwise.
I'm not aware that they're likely to crash the server, and if they
are, so would any regular access to the page in question. The
things we were worried about were more along the lines of unexpected
information disclosure.
This is not to say that I'm against making those functions more
bulletproof. I'm just saying that I find little point in reducing
their superuser checks if we can't get rid of the one in get_raw_page.
regards, tom lane
On Fri, Jan 27, 2017 at 9:42 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> The problem is if the interpretation functions aren't completely >> bulletproof, they might do things like crash the server if you use >> them to read a corrupt page. That is not any more appealing if you >> happen to be running as superuser() than otherwise. > > I'm not aware that they're likely to crash the server, and if they > are, so would any regular access to the page in question. The > things we were worried about were more along the lines of unexpected > information disclosure. > > This is not to say that I'm against making those functions more > bulletproof. I'm just saying that I find little point in reducing > their superuser checks if we can't get rid of the one in get_raw_page. OK, fair enough. get_raw_page() is clearly not something that we really want everybody to have access to by default, but if it were up to me, I'd change the permissions check inside the function to do a check for select privileges on the relation, and then I'd change the SQL script to revoke access from everybody but the superuser. That way, by default, only superusers could use the function, but if they wanted to grant access to others, they could do so. However, even with an access grant, people could only read raw pages from relations from which they can read data in general. Actually, I think that's Stephen should have done something similar for pgstattuple in commit fd321a1dfd64d30bf1652ea6b39b654304f68ae4, and I think we should go back and fix it that way before that code lands in a released branch. As I mentioned upthread, a user could run that even on sensitive tables like pg_authid. That doesn't directly reveal any data, but it does let you take a lock on a table which you otherwise couldn't lock, and it reveals at least some information about the contents of the table. If you ran it just before and after an insert or update, you might be able to infer the length of the new tuple. That's probably not enough to give you a whole lotta help guessing what password that tuple contains ... but it's more than no information. More fundamentally, I think it's entirely reasonable for the superuser to be able to decide who can and can't run the pgstattuple functions, but I strongly suspect that very few superusers would want to give users rights to run those functions on tables to which those users otherwise have no access. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > OK, fair enough. get_raw_page() is clearly not something that we > really want everybody to have access to by default, but if it were up > to me, I'd change the permissions check inside the function to do a > check for select privileges on the relation, and then I'd change the > SQL script to revoke access from everybody but the superuser. The way to do properly do this would not be to have some conflation of the right to execute get_raw_page() combined with a typically granted right like 'SELECT'. Instead, it would be to have an explicit RAW_PAGE or similar permission which could be GRANT'd to a user for a particular relation, and then we could change the superuser() check into a check against that right, and call it done. The difficulty with the approach you're advocating is that a great many users will likely have SELECT rights on a great many relations, but that doesn't mean I really want them to have RAW_PAGE rights on all of them. Of course, what this is really missing is an actual use-case where it makes sense to grant out get_raw_page() to anyone. Without that, I've got a really hard time agreeing that it makes sense to remove the existing superuser check or to spend any effort here. > Actually, I think that's Stephen should have done something similar > for pgstattuple in commit fd321a1dfd64d30bf1652ea6b39b654304f68ae4, > and I think we should go back and fix it that way before that code > lands in a released branch. As I mentioned upthread, a user could run > that even on sensitive tables like pg_authid. That doesn't directly > reveal any data, but it does let you take a lock on a table which you > otherwise couldn't lock, and it reveals at least some information > about the contents of the table. If you ran it just before and after > an insert or update, you might be able to infer the length of the new > tuple. That's probably not enough to give you a whole lotta help > guessing what password that tuple contains ... but it's more than no > information. More fundamentally, I think it's entirely reasonable for > the superuser to be able to decide who can and can't run the > pgstattuple functions, but I strongly suspect that very few superusers > would want to give users rights to run those functions on tables to > which those users otherwise have no access. Requiring pgstattuple() to check if a user has any rights on a table means that an existing non-superuser monitoring tool that's calling pgstattuple() for summary information would be required to have SELECT rights across, most likely, all tables, even though the monitoring user's got no need for that level of access. Now, we could argue that having access to just one column would be enough for that user, but that's still more access than the monitoring user needs, and there's still the issue of new tables (and default privileges don't currently support column-level privileges anyway). A new capability or way to grant "lock access" to all tables might be a way to approach this, but it would have to be controllable on a per-lock-mode basis, or a way to say "this user is allowed to lock and extract high-level counts from every table in the system", though the latter is essentially what GRANT'ing EXECUTE on the pgstattuple() functions today is. Perhaps the docs should be clearer as to what that access means, but I don't see a need to include a warning with those functions saying that they could be used to escalate privileges to a superuser. Perhaps a more generalized approach would be to structure a generic function+table GRANT system, in addition to our existing GRANT-RIGHT+table system, that allowed for per-function-per-table-level grants. My thinking is something along the lines of: GRANT pgstattuple() ON TABLE q; GRANT pgstattuple() ON ALL TABLES IN SCHEMA x; GRANT pgstattuple() ON ALL TABLES IN DATABASE db; Which would specifically tie together the two. Of course, we'd need similar support in ALTER DEFAULT PRIVILEGES to allow the capability to be maintained. What is unfortunate with any of this is that we don't, currently, have any way to specify DEFAULT PRIVILEGES on pg_catalog, meaning that new tables which are added to pg_catalog down the road won't get these privileges. For a monitoring tool that's trying to check $something (bloat, for example) across all tables, that's a bit unfortunate. Generally speaking, there's only a few catalog tables that tend to have bloat issues and those are well-defined and don't change, so perhaps this issue could be ignored, but a more complete solution would find a way to address that case also. Thanks! Stephen
* Tom Lane (tgl@sss.pgh.pa.us) wrote: > Simon Riggs <simon@2ndquadrant.com> writes: > > [ good general plan ] > > > 3. Make a list of all functions that would cause security problems. > > One by one, precisely. If we did remove all superuser checks we would > > need this list documented to advise people of the risks, so it must > > exist before any commit can be made, assuming we believe in > > documentation. Notice that I am after risk documentation, > > Yeah, I think documentation is the crux of the issue. If there is some > non-obvious reason why letting somebody use pg_ls_dir() is more of a > security hazard than it appears on the surface, the answer is to document > that so DBAs can decide for themselves whether to take the risk. > > Count me +1 for removing hard-wired superuser checks, but carefully > and with an overall plan. To be clear, I'm also in agreement with (and have been making efforts to, for quite a few years, with some progress) removing the hard-wired superuser checks. I've tried to do so carefully and with a guiding principle that the existing superuser-only capabilities should be split up in a way which makes sense for particular use-cases and allows the administrator to grant only precisely what they wish to for the use-case in question. The individual-level grants for things like pg_start/stop_backup(), along with some others, were really already at the "right" level for the use-case in question, which meant that what was needed was a way to revoke EXECUTE from public initially and to maintain what the administrator wished the rights to be, which is what was done. I do like the idea which Dave mentioned up-thread of having default roles which start out with the necessary privileges, so that the user doesn't have to grant EXECUTE on pg_start_backup(), and then also grant EXECUTE on pg_stop_backup(). We've gotten to the point where that might actually be possible, now that we have a couple of existing default roles, and a namespace in which to create more, if we choose to. That a monitoring tool needs to be able to list the contents of the pg_wal PG directory is great, but if that's what it needs, then that's what the admin should be able to provide it with, and not have to provide it with the ability to list every directory on the system, or even every other directory in PG's data directory. Privileges should be granted out, as much as possible, to match what the requirement is, and tossing everything out the window because we happen to have an existing function like pg_ls_dir() that just needs that pesky superuser() check removed isn't a good answer to the use-case in question. I'm all for generalizing things and providing blanket access when that's what the use-case calls for, to be clear, but that's not where this discussion has (had?) been going. Thanks! Stephen
On Fri, Jan 27, 2017 at 11:34 AM, Stephen Frost <sfrost@snowman.net> wrote: > * Robert Haas (robertmhaas@gmail.com) wrote: >> OK, fair enough. get_raw_page() is clearly not something that we >> really want everybody to have access to by default, but if it were up >> to me, I'd change the permissions check inside the function to do a >> check for select privileges on the relation, and then I'd change the >> SQL script to revoke access from everybody but the superuser. > > The way to do properly do this would not be to have some conflation of > the right to execute get_raw_page() combined with a typically granted > right like 'SELECT'. Instead, it would be to have an explicit RAW_PAGE > or similar permission which could be GRANT'd to a user for a particular > relation, and then we could change the superuser() check into a check > against that right, and call it done. That's moving the goalposts a very large distance. >> Actually, I think that's Stephen should have done something similar >> for pgstattuple in commit fd321a1dfd64d30bf1652ea6b39b654304f68ae4, ... > > Requiring pgstattuple() to check if a user has any rights on a table > means that an existing non-superuser monitoring tool that's calling > pgstattuple() for summary information would be required to have SELECT > rights across, most likely, all tables, even though the monitoring > user's got no need for that level of access. Now, we could argue that > having access to just one column would be enough for that user, but > that's still more access than the monitoring user needs, and there's > still the issue of new tables (and default privileges don't currently > support column-level privileges anyway). ...whereas here you don't want to move the goalposts at all. I can't understand this. When I want the superuser to have the option to hand out pg_ls_dir() access for all directories pg_ls_dir() can access, you complain that's too broad. Yet your own previous commit, which allows pgstattuple() access to be granted, makes no provision for any granularity of access control at all. And I think there is an excellent argument - which I have made - that pgstattuple() is more likely to expose sensitive information than pg_ls_dir(). Even if we someday have the capability for people to grant pg_ls_dir() access on a directory-by-directory basis, I am sure there will still be a way for them to grant access on all the directories to which pg_ls_dir() can access today; after all, that's just two directories, and their subdirectories. At most somebody would have to make two GRANTs. Removing the hard-coded superuser() checks allows that use case now, and doesn't prohibit you from implementing the other thing later when and if and when we reach agreement on it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 27 January 2017 at 14:09, Dave Page <dpage@pgadmin.org> wrote: > On Fri, Jan 27, 2017 at 1:18 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > >> If the monitoring tool requires superuser then that is a problem, so >> it would be helpful if it didn't do that, please. Not much use having >> a cool tool if it don't work with the server. > > Sure, that's what I want - to provide the management and monitoring > capabilities without requiring superuser. Limiting the capability of > the tools is not an option when you talk to users - however for some > of them, having to use full superuser accounts is a problem as well > (especially for those who are used to other DBMSs that do offer more > fine-grained permissions). > >> The management and monitoring tool could be more specific about what >> it actually needs, rather than simply requesting generic read and >> write against the filesystem. Then we can put those specific things >> into the server and we can all be happy. Again, a detailed list would >> help here. > > Agreed - I do need to do that, and it's on my (extremely long) list. > I'm just chiming in on this thread as requested! So I think it would be useful to have two modes in tools, one where they know they have superuser and one where they know we don't have it. At least we'll know we can't do certain things rather than just have them fail. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Fri, Jan 27, 2017 at 11:34 AM, Stephen Frost <sfrost@snowman.net> wrote: > > * Robert Haas (robertmhaas@gmail.com) wrote: > >> OK, fair enough. get_raw_page() is clearly not something that we > >> really want everybody to have access to by default, but if it were up > >> to me, I'd change the permissions check inside the function to do a > >> check for select privileges on the relation, and then I'd change the > >> SQL script to revoke access from everybody but the superuser. > > > > The way to do properly do this would not be to have some conflation of > > the right to execute get_raw_page() combined with a typically granted > > right like 'SELECT'. Instead, it would be to have an explicit RAW_PAGE > > or similar permission which could be GRANT'd to a user for a particular > > relation, and then we could change the superuser() check into a check > > against that right, and call it done. > > That's moving the goalposts a very large distance. > > >> Actually, I think that's Stephen should have done something similar > >> for pgstattuple in commit fd321a1dfd64d30bf1652ea6b39b654304f68ae4, ... > > > > Requiring pgstattuple() to check if a user has any rights on a table > > means that an existing non-superuser monitoring tool that's calling > > pgstattuple() for summary information would be required to have SELECT > > rights across, most likely, all tables, even though the monitoring > > user's got no need for that level of access. Now, we could argue that > > having access to just one column would be enough for that user, but > > that's still more access than the monitoring user needs, and there's > > still the issue of new tables (and default privileges don't currently > > support column-level privileges anyway). > > ...whereas here you don't want to move the goalposts at all. I can't > understand this. When I want the superuser to have the option to hand > out pg_ls_dir() access for all directories pg_ls_dir() can access, you > complain that's too broad. Yet your own previous commit, which allows > pgstattuple() access to be granted, makes no provision for any > granularity of access control at all. And I think there is an > excellent argument - which I have made - that pgstattuple() is more > likely to expose sensitive information than pg_ls_dir(). You're completely ignoring the use-cases for which these are being done. I've outlined the precise use-case for pgstattuple()'s usage across the entire database for which the admin has granted the EXECUTE access in. I've not yet seen a use-case for access to pg_ls_dir() across all directories pg_ls_dir() can access. My recollection is that you brought up pg_wal, but that's hardly every directory, and some hypothetical tool which could intelligently figure out what orphaned files exist. For the former, I would recommend a function for exactly that (or perhaps something better which provides more than just a count of files), for the latter, that's something that I would be very worried that someone trying to implement outside of core would get wrong or which could be version-dependent. We've already got some code in core to find files left over from a crash and deal with them and perhaps that could be expanded to deal with other cases. > Even if we someday have the capability for people to grant pg_ls_dir() > access on a directory-by-directory basis, I am sure there will still > be a way for them to grant access on all the directories to which > pg_ls_dir() can access today; after all, that's just two directories, > and their subdirectories. At most somebody would have to make two > GRANTs. Removing the hard-coded superuser() checks allows that use > case now, and doesn't prohibit you from implementing the other thing > later when and if and when we reach agreement on it. With an appropriate use-case for it, I wouldn't be against it. I linked to both use-cases and a provided patch for finer-grained access to pg_ls_dir() and friends three years ago, which got shot down. I'm not against it if the community wishes to reconsider that decision and support having filesystem-like access where there's an appropriate use-case for it, and with fine-grained access control provided to meet that use-case. Further, as it relates to goal-posts, if your goal is to let an admin grant out the ability to see how many files are in pg_wal, you're spending a great deal more effort than that would require. I wouldn't object (and would actually appreciate) a function which is able to do exactly that, and I'd go work with Greg S-M right away to make sure that check_postgres.pl knows about that function and uses it in PG10 and above. I do object to someone coming along and saying "let's rip out all the superuser() checks" and it would seem that I'm not alone. Thanks! Stephen
Greetings, * Simon Riggs (simon@2ndquadrant.com) wrote: > On 27 January 2017 at 14:09, Dave Page <dpage@pgadmin.org> wrote: > > On Fri, Jan 27, 2017 at 1:18 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > > > >> If the monitoring tool requires superuser then that is a problem, so > >> it would be helpful if it didn't do that, please. Not much use having > >> a cool tool if it don't work with the server. > > > > Sure, that's what I want - to provide the management and monitoring > > capabilities without requiring superuser. Limiting the capability of > > the tools is not an option when you talk to users - however for some > > of them, having to use full superuser accounts is a problem as well > > (especially for those who are used to other DBMSs that do offer more > > fine-grained permissions). Right, I'm all about providing fine-grained permissions and granting those out to monitoring users, but we need to have an understanding of what the monitoring really needs (and doesn't need...) to be able to ensure that the fine-grained permission system which is built matches those needs and allows the admin to grant out exactly the rights needed. > >> The management and monitoring tool could be more specific about what > >> it actually needs, rather than simply requesting generic read and > >> write against the filesystem. Then we can put those specific things > >> into the server and we can all be happy. Again, a detailed list would > >> help here. > > > > Agreed - I do need to do that, and it's on my (extremely long) list. > > I'm just chiming in on this thread as requested! That would certainly be really nice to have. I have some ideas, and I've been meaning to try and work towards them, but knowing what other monitoring systems do would be great. > So I think it would be useful to have two modes in tools, one where > they know they have superuser and one where they know we don't have > it. At least we'll know we can't do certain things rather than just > have them fail. Having such a flag in monitoring tools where it makes sense sounds reasonable to me, though there isn't really anything different for the backend to do to support this (I don't think..?). Thanks! Stephen
> On 27 Jan 2017, at 17:39, Stephen Frost <sfrost@snowman.net> wrote: > > Greetings, > > * Simon Riggs (simon@2ndquadrant.com) wrote: >>> On 27 January 2017 at 14:09, Dave Page <dpage@pgadmin.org> wrote: >>>> On Fri, Jan 27, 2017 at 1:18 PM, Simon Riggs <simon@2ndquadrant.com> wrote: >>>> >>>> If the monitoring tool requires superuser then that is a problem, so >>>> it would be helpful if it didn't do that, please. Not much use having >>>> a cool tool if it don't work with the server. >>> >>> Sure, that's what I want - to provide the management and monitoring >>> capabilities without requiring superuser. Limiting the capability of >>> the tools is not an option when you talk to users - however for some >>> of them, having to use full superuser accounts is a problem as well >>> (especially for those who are used to other DBMSs that do offer more >>> fine-grained permissions). > > Right, I'm all about providing fine-grained permissions and granting > those out to monitoring users, but we need to have an understanding of > what the monitoring really needs (and doesn't need...) to be able to > ensure that the fine-grained permission system which is built matches > those needs and allows the admin to grant out exactly the rights needed. > >>>> The management and monitoring tool could be more specific about what >>>> it actually needs, rather than simply requesting generic read and >>>> write against the filesystem. Then we can put those specific things >>>> into the server and we can all be happy. Again, a detailed list would >>>> help here. >>> >>> Agreed - I do need to do that, and it's on my (extremely long) list. >>> I'm just chiming in on this thread as requested! > > That would certainly be really nice to have. I have some ideas, and > I've been meaning to try and work towards them, but knowing what other > monitoring systems do would be great. > >> So I think it would be useful to have two modes in tools, one where >> they know they have superuser and one where they know we don't have >> it. At least we'll know we can't do certain things rather than just >> have them fail. > > Having such a flag in monitoring tools where it makes sense sounds > reasonable to me, though there isn't really anything different for the > backend to do to support this (I don't think..?). No, that's exactly what we don't want, because then users cannot do anything that we can currently grant them permissionsfor - it's the all-or-nothing approach. What we currently do is allow users to try every thing, then let the backend complain if it wants, and relay the access deniedmessage to the user.
On Fri, Jan 27, 2017 at 12:32 PM, Stephen Frost <sfrost@snowman.net> wrote: > You're completely ignoring the use-cases for which these are being done. > > I've outlined the precise use-case for pgstattuple()'s usage across the > entire database for which the admin has granted the EXECUTE access in. > I've not yet seen a use-case for access to pg_ls_dir() across all > directories pg_ls_dir() can access. My recollection is that you brought > up pg_wal, but that's hardly every directory, and some hypothetical tool > which could intelligently figure out what orphaned files exist. For the > former, I would recommend a function for exactly that (or perhaps > something better which provides more than just a count of files), for > the latter, that's something that I would be very worried that someone > trying to implement outside of core would get wrong or which could be > version-dependent. We've already got some code in core to find files > left over from a crash and deal with them and perhaps that could be > expanded to deal with other cases. I agree that those things could be done other ways, but I don't think that's a valid argument against what I'm proposing. Sure, for any given use case, you could come up with a way that the use case could be satisfied without access to pg_ls_dir(). No question about it. What I don't understand is why we should particularly want to go to that trouble. pg_ls_dir() has been serving authors of monitoring tools well for many years, it presents minimal security risks, and it could be useful for other purposes. A new thing won't be immediately adopted, has no real advantage on the security front because pg_ls_dir() isn't actually dangerous, and is by design single-purpose. I am completely flummoxed by the idea that giving out pg_ls_dir() access to all directories it can access -- namely the data and log directories -- is some kind of massive security problem, whereas giving out superuser access for the same purpose apparently is no problem at all. And that is CLEARLY what is happening. The documentation for check_postgres.pl recommends it! > With an appropriate use-case for it, I wouldn't be against it. I linked > to both use-cases and a provided patch for finer-grained access to > pg_ls_dir() and friends three years ago, which got shot down. I'm not > against it if the community wishes to reconsider that decision and > support having filesystem-like access where there's an appropriate > use-case for it, and with fine-grained access control provided to meet > that use-case. I hope you're not saying that you plan to hold this patch hostage until I agree to something you want to do, because that would be uncool. At any rate, I don't even think that patch really got shot down; the major complaint about that patch was that it arrived out of nowhere, fully formed, with no prior discussion of the design, and several people - including me - were concerned that you might commit it before consensus had been reached, as you had recently done with another large patch that I was in the middle of reviewing. Aside from the "please don't commit this because it isn't agreed" objection, there were comments like (1) DIRALIAS is not a word, but all other SQL objects have names which are words or phrases and (2) hey, maybe we could use a couple of GUCs for this instead of inventing a whole new SQL object type and (3) what about adding some syntax for COPY that uses these directory alias things, none of which qualify as trying to stop the project dead in its tracks. The only person who was dead set against the whole concept was Tom, and several people including me expressed at least some doubt about whether the problems with the idea were so intractable as he suggested. For the record, I'm not as keen on the GUC idea as I was when we had this previous discussion. This is really a privilege and probably deserves to be handled as part of the privileges system rather than crammed into a GUC mechanism which isn't intended for that purpose. I am also somewhat more in favor of the idea overall than I was back then; as we work toward giving users as little privilege as possible, it's certainly useful to be able to let a monitoring user read the logs but not the data directory, or the toplevel files (i.e. config files) in the data directory but not files in subdirectories. But I really can't see a lick of need for that kind of fine-grained control for pg_ls_dir() or pg_stat_files(), which are six kinds of harmless, and I'm not sure that pg_read_file() permissions really need to be as granular as allowing each directory on the filesystem to have individual permissions. There are a couple of broad categories that might need to be distinguished: log file, config file, data file, file I want to COPY, but there might be ways to handle those cases that are simpler than a full-blown new SQL object type. > Further, as it relates to goal-posts, if your goal is to let an admin > grant out the ability to see how many files are in pg_wal, you're > spending a great deal more effort than that would require. I wouldn't > object (and would actually appreciate) a function which is able to do > exactly that, and I'd go work with Greg S-M right away to make sure that > check_postgres.pl knows about that function and uses it in PG10 and > above. Well, that is true, or would be if that were really what my goal was, but I didn't know that when I wrote the original email. Now that you've done all of the work to allow privileges on default objects to survive pg_dump, I really see no benefit to having hard-coded superuser() checks. I assumed that the fact that we'd left a few in was because they'd been overlooked or that nobody had gotten around to removing them yet, and I'm pretty gobsmacked by the way this discussion has gone. I was kind of expecting a response along the lines of "oh, yeah, thanks for taking care of that". -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Jan 26, 2017 at 08:50:27AM -0500, Robert Haas wrote: > On Wed, Jan 25, 2017 at 10:31 PM, Stephen Frost <sfrost@snowman.net> wrote: > > Frankly, I get quite tired of the argument essentially being made > > here that because pg_ls_dir() wouldn't grant someone superuser > > rights, that we should remove superuser checks from everything. > > As long as you are presenting it like that, I'm going to be quite > > dead-set against any of it. > 1. pg_ls_dir. I cannot see how this can ever be used to get > superuser privileges. With pilot error, all things are possible. A file name under $PGDATA could be the superuser password. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Sun, Jan 29, 2017 at 5:39 PM, David Fetter <david@fetter.org> wrote: > On Thu, Jan 26, 2017 at 08:50:27AM -0500, Robert Haas wrote: >> On Wed, Jan 25, 2017 at 10:31 PM, Stephen Frost <sfrost@snowman.net> wrote: >> > Frankly, I get quite tired of the argument essentially being made >> > here that because pg_ls_dir() wouldn't grant someone superuser >> > rights, that we should remove superuser checks from everything. >> > As long as you are presenting it like that, I'm going to be quite >> > dead-set against any of it. >> 1. pg_ls_dir. I cannot see how this can ever be used to get >> superuser privileges. > > With pilot error, all things are possible. A file name under $PGDATA > could be the superuser password. Uh, true. The default value of application_name could be the superuser password, too, but we still allow access to it by unprivileged users. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sun, Jan 29, 2017 at 05:52:51PM -0500, Robert Haas wrote: > On Sun, Jan 29, 2017 at 5:39 PM, David Fetter <david@fetter.org> wrote: > > On Thu, Jan 26, 2017 at 08:50:27AM -0500, Robert Haas wrote: > >> On Wed, Jan 25, 2017 at 10:31 PM, Stephen Frost <sfrost@snowman.net> wrote: > >> > Frankly, I get quite tired of the argument essentially being made > >> > here that because pg_ls_dir() wouldn't grant someone superuser > >> > rights, that we should remove superuser checks from everything. > >> > As long as you are presenting it like that, I'm going to be quite > >> > dead-set against any of it. > >> 1. pg_ls_dir. I cannot see how this can ever be used to get > >> superuser privileges. > > > > With pilot error, all things are possible. A file name under $PGDATA > > could be the superuser password. > > Uh, true. The default value of application_name could be the > superuser password, too, but we still allow access to it by > unprivileged users. Of course. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate