Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
> On Nov 10, 2017, at 3:58 PM, Stephen Frost wrote: > > Michael, Tom, > > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> On Fri, Nov 10, 2017 at 10:00 AM, Tom Lane wrote: >>> Stephen Frost writes: I'm guessing no, which essentially means that *we* consider access to lo_import/lo_export to be equivilant to superuser and therefore we're not going to implement anything to try and prevent the user who has access to those functions from becoming superuser. If we aren't willing to do that, then how can we really say that there's some difference between access to these functions and being a superuser? >>> >>> We seem to be talking past each other. Yes, if a user has malicious >>> intentions, it's possibly to parlay lo_export into obtaining a superuser >>> login (I'm less sure that that's necessarily true for lo_import). >>> That does NOT make it "equivalent", except perhaps in the view of someone >>> who is only considering blocking malevolent actors. It does not mean that >>> there's no value in preventing a task that needs to run lo_export from >>> being able to accidentally destroy any data in the database. There's a >>> range of situations where you are concerned about accidents and errors, >>> not malicious intent; but your argument ignores those use-cases. >> >> That will not sound much as a surprise as I spawned the original >> thread, but like Robert I understand that getting rid of all superuser >> checks is a goal that we are trying to reach to allow admins to have >> more flexibility in handling permissions to a subset of objects. >> Forcing an admin to give full superuser rights to one user willing to >> work only on LOs import and export is a wrong concept. > > The problem I have with this is that 'full superuser rights' actually > are being granted by this. You're able to read any file and write any > file on the filesystem that the PG OS user can. How is that not 'full > superuser rights'? The concerns about a mistake being made are just as > serious as they are with someone who has superuser rights as someone > could pretty easily end up overwriting the control file or various other > extremely important bits of the system. This isn't just about what > 'blackhats' can do with this. > > As I mentioned up-thread, if we want to make it so that non-superusers > can use lo_import/lo_export, then we should do that in a way that > allows the administrator to specify exactly the rights they really want > to give, based on a use-case for them. There's no use-case for allowing > someone to use lo_export() to overwrite pg_control. The use-case would > be to allow them to export to a specific directory (or perhaps a set of > directories), or to read from same. The same is true of COPY. Further, > I wonder what would happen if we allow this and then someone comes along > and re-proposes the 'CREATE DIRECTORY' concept that I had once proposed > (ideally with things cleaned up and tightened up to avoid there being > issues using it) to address the actual use-case for these functions to > be available to a non-superuser. We wouldn't be able to change these > functions to start checking the 'directory' rights or we would break > existing non-superuser usage of them. I imagine we could create > additional functions which check the 'directory' privileges, but that's > hardly ideal, imv. > > I'm disappointed to apparently be in the minority on this, but that > seems to be the case unless someone else wishes to comment. While I > appreciate the discussion, I'm certainly no more convinced today than I > was when I first saw this go in that allowing these functions to be > granted to non-superusers does anything but effectively make them into > superusers who aren't explicitly identified as superusers. Just to help understand your concern, and not to propose actual features, would it help if there were (1) a function, perhaps pg_catalog.pg_exploiters(), which would return all roles that have been granted exploitable privileges, such that it would be easier to identify all superusers, including those whose superuserishness derives from a known export, and (2) a syntax change for GRANT that would require an extra token, so that you'd have to write something like GRANT EXPLOITABLE lo_export TO trusted_user_foo so that you couldn't unknowingly grant a dangerous privilege. Or is there more to it than that? mark -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
Stephen Frost writes: > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> Forcing an admin to give full superuser rights to one user willing to >> work only on LOs import and export is a wrong concept. > The problem I have with this is that 'full superuser rights' actually > are being granted by this. You're able to read any file and write any > file on the filesystem that the PG OS user can. How is that not 'full > superuser rights'? It doesn't cause, say, "DELETE FROM pg_proc;" to succeed. You're still not getting the point that this is for people who want self-imposed restrictions. Sure, you can't give out lo_export privilege to someone you would not trust with superuser. But somebody who needs lo_export, and is trustworthy enough to have it, may still wish to do the inside-the-database part of their work with less than full superuser, just as a safety measure. It's the *exact same* reason why cautious people use "sudo" rather than just running as root all the time. > As I mentioned up-thread, if we want to make it so that non-superusers > can use lo_import/lo_export, then we should do that in a way that > allows the administrator to specify exactly the rights they really want > to give, based on a use-case for them. Our current answer for that is wrapper functions. This patch does not make that answer any less applicable than before. > I wonder what would happen if we allow this and then someone comes along > and re-proposes the 'CREATE DIRECTORY' concept that I had once proposed > (ideally with things cleaned up and tightened up to avoid there being > issues using it) to address the actual use-case for these functions to > be available to a non-superuser. We wouldn't be able to change these > functions to start checking the 'directory' rights or we would break > existing non-superuser usage of them. This is a straw man argument --- if we tighten up the function to check this as-yet-nonexistent feature, how is that not breaking existing use-cases anyway? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
On Sat, Nov 11, 2017 at 12:50 AM, Robert Haas wrote: > On Fri, Nov 10, 2017 at 10:34 AM, Tom Lane wrote: >> I think as far as that goes, we can just change to "Therefore, by default >> their use is restricted ...". Then I suggest adding a para >> after that, with wording along the lines of >> >> It is possible to GRANT use of server-side lo_import and lo_export to >> non-superusers, but careful consideration of the security implications >> is required. A malicious user of such privileges could easily parlay >> them into becoming superuser (for example by rewriting server >> configuration files), or could attack the rest of the server's file >> system without bothering to obtain database superuser privileges as >> such. Access to roles having such privilege must therefore be guarded >> just as carefully as access to superuser roles. Nonetheless, if use >> of server-side lo_import or lo_export is needed for some routine task, >> it's safer to use a role of this sort than full superuser privilege, >> as that helps to reduce the risk of damage from accidental errors. > > +1. That seems like great language to me. +1. Not convinced that mentioning wrappers is worth the complication. Experienced admins likely already know such matters. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
Michael, Tom, * Michael Paquier (michael.paqu...@gmail.com) wrote: > On Fri, Nov 10, 2017 at 10:00 AM, Tom Lane wrote: > > Stephen Frost writes: > >> I'm guessing no, which essentially means that *we* consider access to > >> lo_import/lo_export to be equivilant to superuser and therefore we're > >> not going to implement anything to try and prevent the user who has > >> access to those functions from becoming superuser. If we aren't willing > >> to do that, then how can we really say that there's some difference > >> between access to these functions and being a superuser? > > > > We seem to be talking past each other. Yes, if a user has malicious > > intentions, it's possibly to parlay lo_export into obtaining a superuser > > login (I'm less sure that that's necessarily true for lo_import). > > That does NOT make it "equivalent", except perhaps in the view of someone > > who is only considering blocking malevolent actors. It does not mean that > > there's no value in preventing a task that needs to run lo_export from > > being able to accidentally destroy any data in the database. There's a > > range of situations where you are concerned about accidents and errors, > > not malicious intent; but your argument ignores those use-cases. > > That will not sound much as a surprise as I spawned the original > thread, but like Robert I understand that getting rid of all superuser > checks is a goal that we are trying to reach to allow admins to have > more flexibility in handling permissions to a subset of objects. > Forcing an admin to give full superuser rights to one user willing to > work only on LOs import and export is a wrong concept. The problem I have with this is that 'full superuser rights' actually are being granted by this. You're able to read any file and write any file on the filesystem that the PG OS user can. How is that not 'full superuser rights'? The concerns about a mistake being made are just as serious as they are with someone who has superuser rights as someone could pretty easily end up overwriting the control file or various other extremely important bits of the system. This isn't just about what 'blackhats' can do with this. As I mentioned up-thread, if we want to make it so that non-superusers can use lo_import/lo_export, then we should do that in a way that allows the administrator to specify exactly the rights they really want to give, based on a use-case for them. There's no use-case for allowing someone to use lo_export() to overwrite pg_control. The use-case would be to allow them to export to a specific directory (or perhaps a set of directories), or to read from same. The same is true of COPY. Further, I wonder what would happen if we allow this and then someone comes along and re-proposes the 'CREATE DIRECTORY' concept that I had once proposed (ideally with things cleaned up and tightened up to avoid there being issues using it) to address the actual use-case for these functions to be available to a non-superuser. We wouldn't be able to change these functions to start checking the 'directory' rights or we would break existing non-superuser usage of them. I imagine we could create additional functions which check the 'directory' privileges, but that's hardly ideal, imv. I'm disappointed to apparently be in the minority on this, but that seems to be the case unless someone else wishes to comment. While I appreciate the discussion, I'm certainly no more convinced today than I was when I first saw this go in that allowing these functions to be granted to non-superusers does anything but effectively make them into superusers who aren't explicitly identified as superusers. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
On Fri, Nov 10, 2017 at 10:34 AM, Tom Lane wrote: > I think as far as that goes, we can just change to "Therefore, by default > their use is restricted ...". Then I suggest adding a para > after that, with wording along the lines of > > It is possible to GRANT use of server-side lo_import and lo_export to > non-superusers, but careful consideration of the security implications > is required. A malicious user of such privileges could easily parlay > them into becoming superuser (for example by rewriting server > configuration files), or could attack the rest of the server's file > system without bothering to obtain database superuser privileges as > such. Access to roles having such privilege must therefore be guarded > just as carefully as access to superuser roles. Nonetheless, if use > of server-side lo_import or lo_export is needed for some routine task, > it's safer to use a role of this sort than full superuser privilege, > as that helps to reduce the risk of damage from accidental errors. +1. That seems like great language to me. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
Michael Paquier writes: > That will not sound much as a surprise as I spawned the original > thread, but like Robert I understand that getting rid of all superuser > checks is a goal that we are trying to reach to allow admins to have > more flexibility in handling permissions to a subset of objects. > Forcing an admin to give full superuser rights to one user willing to > work only on LOs import and export is a wrong concept. Right. I think the question may boil down to how we document this. The current para reads The server-side lo_import and lo_export functions behave considerably differently from their client-side analogs. These two functions read and write files in the server's file system, using the permissions of the database's owning user. Therefore, their use is restricted to superusers. In contrast, the client-side import and export functions read and write files in the client's file system, using the permissions of the client program. The client-side functions do not require superuser privilege. I think as far as that goes, we can just change to "Therefore, by default their use is restricted ...". Then I suggest adding a para after that, with wording along the lines of It is possible to GRANT use of server-side lo_import and lo_export to non-superusers, but careful consideration of the security implications is required. A malicious user of such privileges could easily parlay them into becoming superuser (for example by rewriting server configuration files), or could attack the rest of the server's file system without bothering to obtain database superuser privileges as such. Access to roles having such privilege must therefore be guarded just as carefully as access to superuser roles. Nonetheless, if use of server-side lo_import or lo_export is needed for some routine task, it's safer to use a role of this sort than full superuser privilege, as that helps to reduce the risk of damage from accidental errors. We could expand that by mentioning the possibility of wrapper functions, but it seems long enough already. Comments? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
On Fri, Nov 10, 2017 at 10:00 AM, Tom Lane wrote: > Stephen Frost writes: >> I'm guessing no, which essentially means that *we* consider access to >> lo_import/lo_export to be equivilant to superuser and therefore we're >> not going to implement anything to try and prevent the user who has >> access to those functions from becoming superuser. If we aren't willing >> to do that, then how can we really say that there's some difference >> between access to these functions and being a superuser? > > We seem to be talking past each other. Yes, if a user has malicious > intentions, it's possibly to parlay lo_export into obtaining a superuser > login (I'm less sure that that's necessarily true for lo_import). > That does NOT make it "equivalent", except perhaps in the view of someone > who is only considering blocking malevolent actors. It does not mean that > there's no value in preventing a task that needs to run lo_export from > being able to accidentally destroy any data in the database. There's a > range of situations where you are concerned about accidents and errors, > not malicious intent; but your argument ignores those use-cases. That will not sound much as a surprise as I spawned the original thread, but like Robert I understand that getting rid of all superuser checks is a goal that we are trying to reach to allow admins to have more flexibility in handling permissions to a subset of objects. Forcing an admin to give full superuser rights to one user willing to work only on LOs import and export is a wrong concept. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
Stephen Frost writes: > I'm guessing no, which essentially means that *we* consider access to > lo_import/lo_export to be equivilant to superuser and therefore we're > not going to implement anything to try and prevent the user who has > access to those functions from becoming superuser. If we aren't willing > to do that, then how can we really say that there's some difference > between access to these functions and being a superuser? We seem to be talking past each other. Yes, if a user has malicious intentions, it's possibly to parlay lo_export into obtaining a superuser login (I'm less sure that that's necessarily true for lo_import). That does NOT make it "equivalent", except perhaps in the view of someone who is only considering blocking malevolent actors. It does not mean that there's no value in preventing a task that needs to run lo_export from being able to accidentally destroy any data in the database. There's a range of situations where you are concerned about accidents and errors, not malicious intent; but your argument ignores those use-cases. To put it more plainly: your argument is much like saying that a person who knows a sudo password might as well do everything they ever do as root. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
Robert, * Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Nov 9, 2017 at 2:56 PM, Stephen Frost wrote: > > Further, I agree entirely that we > > shouldn't be deciding that certain capabilities are never allowed to be > > given to a user- but that's why superuser *exists* and why it's possible > > to give superuser access to multiple users. The question here is if > > it's actually sensible for us to make certain actions be grantable to > > non-superusers which allow that role to become, or to essentially be, a > > superuser. What that does, just as it does in the table case, from my > > viewpoint at least, is make it *look* to admins like they're grant'ing > > something less than superuser when, really, they're actually giving the > > user superuser-level access. That violates the POLA because the admin > > didn't write 'ALTER USER joe WITH SUPERUSER', they just wrote 'GRANT > > EXECUTE ON lo_import() TO joe;'. > > This is exactly the kind of thing that I'm talking about. Forcing an > administrator to hand out full superuser access instead of being able > to give just lo_import() makes life difficult for users for no good > reason. The existence of a theoretically-exploitable security > vulnerability is not tantamount to really having access, especially on > a system with a secured logging facility. This is where I disagree. The administrator *is* giving the user superuser-level access, they're just doing it in a different way from explicitly saying 'ALTER USER .. WITH SUPERUSER' and that's exactly the issue that I have. This isn't a theoretical exploit- the user with lo_import/lo_export access is able to read and write any file on the filesystem which the PG OS has access to, including things like pg_hba.conf in most configurations, the file backing pg_authid, the OS user's .bashrc, the Kerberos principal, the CA public key, the SSL keys, tables owned by other users that the in-database role doesn't have access to, et al. Further, will we consider these *actual*, non-theoretical, methods to gain superuser access, or to bypass the database authorization system, to be security issues or holes to plug? I'm guessing no, which essentially means that *we* consider access to lo_import/lo_export to be equivilant to superuser and therefore we're not going to implement anything to try and prevent the user who has access to those functions from becoming superuser. If we aren't willing to do that, then how can we really say that there's some difference between access to these functions and being a superuser? Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
Michael Paquier writes: > On Fri, Nov 10, 2017 at 7:05 AM, Tom Lane wrote: >> I did miss the need to fix the docs, and am happy to put in some strong >> wording about the security hazards of these functions while fixing the >> docs. But I do not think that leaving them with hardwired superuser >> checks is an improvement over being able to control them with GRANT. > Sorry about that. lobj.sgml indeed mentions superusers. Do you need a patch? No, I can write it. But I'm going to wait to see where this debate settles before expending effort on the docs. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
On Fri, Nov 10, 2017 at 7:05 AM, Tom Lane wrote: > I did miss the need to fix the docs, and am happy to put in some strong > wording about the security hazards of these functions while fixing the > docs. But I do not think that leaving them with hardwired superuser > checks is an improvement over being able to control them with GRANT. Sorry about that. lobj.sgml indeed mentions superusers. Do you need a patch? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
Robert Haas writes: > On Thu, Nov 9, 2017 at 2:56 PM, Stephen Frost wrote: >> Further, I agree entirely that we >> shouldn't be deciding that certain capabilities are never allowed to be >> given to a user- but that's why superuser *exists* and why it's possible >> to give superuser access to multiple users. The question here is if >> it's actually sensible for us to make certain actions be grantable to >> non-superusers which allow that role to become, or to essentially be, a >> superuser. What that does, just as it does in the table case, from my >> viewpoint at least, is make it *look* to admins like they're grant'ing >> something less than superuser when, really, they're actually giving the >> user superuser-level access. That violates the POLA because the admin >> didn't write 'ALTER USER joe WITH SUPERUSER', they just wrote 'GRANT >> EXECUTE ON lo_import() TO joe;'. > This is exactly the kind of thing that I'm talking about. Forcing an > administrator to hand out full superuser access instead of being able > to give just lo_import() makes life difficult for users for no good > reason. The existence of a theoretically-exploitable security > vulnerability is not tantamount to really having access, especially on > a system with a secured logging facility. Exactly. I think that Stephen's argument depends on what a black-hat privilege recipient could theoretically do, but fails to consider what's useful for white-hat users. One of the standard rules for careful admins is to do as little as possible as root/superuser. If you have a situation where it's necessary to use, say, lo_import as part of a routine data import task, then the only alternative previously was to do that task as superuser. That is not an improvement, by any stretch of the imagination, over granting lo_import privileges to some otherwise-vanilla role that can run the data import task. We've previously discussed workarounds such as creating SECURITY DEFINER wrapper functions, but I don't think that approach does much to change the terms of the debate: it'd still be better if the wrapper function itself didn't need full superuser. I did miss the need to fix the docs, and am happy to put in some strong wording about the security hazards of these functions while fixing the docs. But I do not think that leaving them with hardwired superuser checks is an improvement over being able to control them with GRANT. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
On Thu, Nov 9, 2017 at 2:56 PM, Stephen Frost wrote: > I agree that it may not be obvious which cases lead to a relatively easy > way to obtain superuser and which don't, and that's actually why I'd > much rather it be something that we're considering and looking out for > because, frankly, we're in a much better position generally to realize > those cases than our users are. I disagree. It's flattering to imagine that PostgreSQL developers, as a class, are smarter than PostgreSQL users, but it doesn't match my observations. > Further, I agree entirely that we > shouldn't be deciding that certain capabilities are never allowed to be > given to a user- but that's why superuser *exists* and why it's possible > to give superuser access to multiple users. The question here is if > it's actually sensible for us to make certain actions be grantable to > non-superusers which allow that role to become, or to essentially be, a > superuser. What that does, just as it does in the table case, from my > viewpoint at least, is make it *look* to admins like they're grant'ing > something less than superuser when, really, they're actually giving the > user superuser-level access. That violates the POLA because the admin > didn't write 'ALTER USER joe WITH SUPERUSER', they just wrote 'GRANT > EXECUTE ON lo_import() TO joe;'. This is exactly the kind of thing that I'm talking about. Forcing an administrator to hand out full superuser access instead of being able to give just lo_import() makes life difficult for users for no good reason. The existence of a theoretically-exploitable security vulnerability is not tantamount to really having access, especially on a system with a secured logging facility. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
Robert, * Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Nov 9, 2017 at 1:52 PM, Stephen Frost wrote: > > This is not unlike the discussions we've had in the past around allowing > > non-owners of a table to modify properties of a table, where the > > argument has been successfully and clearly made that if you make the > > ability to change the table a GRANT'able right, then that can be used to > > become the role which owns the table without much difficulty, and so > > there isn't really a point in making that right independently > > GRANT'able. We have some of that explicitly GRANT'able today due to > > requirements of the spec, but that's outside of our control. > > I don't think it's quite the same thing. I wouldn't go out of my way > to invent grantable table rights that just let you usurp the table > owner's permissions, but I still think it's better to have a uniform > rule that functions we ship don't contain hard-coded superuser checks. Just to be clear, we should certainly be thinking about more than just functions but also about things like publications and subscriptions and other bits of the system. I haven't done a detailed analysis quite yet, but I'm reasonably confident that a number of things in this last release cycle have resulted in quite a few additional explicit superuser checks, which I do think is an issue to be addressed. > One problem is that which functions allow an escalation to superuser > that is easy enough or reliable enough may not actually be a very > bright line in all cases. But more than that, I think it's a > legitimate decision to grant to a user a right that COULD lead to a > superuser escalation and trust them not to use that way, or prevent > them from using it that way by some means not known to the database > system (e.g. route all queries through pgbouncer and send them to a > teletype; if a breach is detected, go to the teletype room, read the > paper contained therein, and decide who to fire/imprison/execute at > gunpoint). It shouldn't be our job to decide that granting a certain > right is NEVER ok. I agree that it may not be obvious which cases lead to a relatively easy way to obtain superuser and which don't, and that's actually why I'd much rather it be something that we're considering and looking out for because, frankly, we're in a much better position generally to realize those cases than our users are. Further, I agree entirely that we shouldn't be deciding that certain capabilities are never allowed to be given to a user- but that's why superuser *exists* and why it's possible to give superuser access to multiple users. The question here is if it's actually sensible for us to make certain actions be grantable to non-superusers which allow that role to become, or to essentially be, a superuser. What that does, just as it does in the table case, from my viewpoint at least, is make it *look* to admins like they're grant'ing something less than superuser when, really, they're actually giving the user superuser-level access. That violates the POLA because the admin didn't write 'ALTER USER joe WITH SUPERUSER', they just wrote 'GRANT EXECUTE ON lo_import() TO joe;'. We can certainly argue about if the admin should have just known that lo_import()/lo_export() or similar functions were equivilant to grant'ing a user superuser access, but it's not entirely clear to me that it's actually advantageous to go out of our way to provide multiple ways for superuser-level access to be grant'ed out to users. Let's provide one very clear way to do that and strive to avoid building in others, either intentionally or unintentionally. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
On Thu, Nov 9, 2017 at 1:52 PM, Stephen Frost wrote: >> I disagree that that is, or should be, a guiding principle. > > It was what I was using as the basis of the work which I did in this > area and, at least at that time, there seemed to be little issue with > that. Well, there actually kind of was. It came up here: http://postgr.es/m/ca+tgmoy6ne5n4jc5awxser2g2gdgr4omf7edcexamvpf_jq...@mail.gmail.com I mis-remembered who was on which side of the debate, though, hence the comment about employers. But never mind about that, since I was wrong. Sorry for not checking that more carefully before. > This is not unlike the discussions we've had in the past around allowing > non-owners of a table to modify properties of a table, where the > argument has been successfully and clearly made that if you make the > ability to change the table a GRANT'able right, then that can be used to > become the role which owns the table without much difficulty, and so > there isn't really a point in making that right independently > GRANT'able. We have some of that explicitly GRANT'able today due to > requirements of the spec, but that's outside of our control. I don't think it's quite the same thing. I wouldn't go out of my way to invent grantable table rights that just let you usurp the table owner's permissions, but I still think it's better to have a uniform rule that functions we ship don't contain hard-coded superuser checks. One problem is that which functions allow an escalation to superuser that is easy enough or reliable enough may not actually be a very bright line in all cases. But more than that, I think it's a legitimate decision to grant to a user a right that COULD lead to a superuser escalation and trust them not to use that way, or prevent them from using it that way by some means not known to the database system (e.g. route all queries through pgbouncer and send them to a teletype; if a breach is detected, go to the teletype room, read the paper contained therein, and decide who to fire/imprison/execute at gunpoint). It shouldn't be our job to decide that granting a certain right is NEVER ok. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
Robert, * Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Nov 9, 2017 at 1:16 PM, Stephen Frost wrote: > > While we have been working to reduce the number of superuser() checks in > > the backend in favor of having the ability to GRANT explicit rights, one > > of the guideing principles has always been that capabilities which can > > be used to gain superuser rights with little effort should remain > > restricted to the superuser, which is why the lo_import/lo_export hadn't > > been under consideration for superuser-check removal in the analysis I > > provided previously. > > I disagree that that is, or should be, a guiding principle. It was what I was using as the basis of the work which I did in this area and, at least at that time, there seemed to be little issue with that. > I'm not > sure that anyone other than you and your fellow employees at Crunchy > has ever agreed that this is some kind of principle. You make it > sound like there's a consensus about this, but I think there isn't. It's unclear to me why you're bringing up employers in this discussion, particularly since Tom is the one who just moved things in the direction that you're evidently advocating for. Clearly there isn't consensus if you and others disagree. Things certainly can change over time as well, but if we're going to make a change here then we should make it willfully, plainly, and consistently. > I think our guiding principle should be to get rid of ALL of the > hard-coded superuser checks and let people GRANT what they want. If > they grant a permission that results in somebody escalating to > superuser, they get to keep both pieces. Such risks might be worth > documenting, but we shouldn't obstruct people from doing it. I would certainly like to see the many additional hard-coded superuser checks introduced into PG10 removed and replaced with more fine-grained GRANT-based privileges (either as additional GRANT rights or perhaps through the default roles system). What I dislike about allowing GRANT's of a privilege which allows someone to be superuser is that it makes it *look* like you're only GRANT'ing some subset of reasonable rights when, in reality, you're actually GRANT'ing a great deal more. This is not unlike the discussions we've had in the past around allowing non-owners of a table to modify properties of a table, where the argument has been successfully and clearly made that if you make the ability to change the table a GRANT'able right, then that can be used to become the role which owns the table without much difficulty, and so there isn't really a point in making that right independently GRANT'able. We have some of that explicitly GRANT'able today due to requirements of the spec, but that's outside of our control. > In the same way, Linux will not prevent you from making a binary > setuid regardless of what the binary does. If you make a binary > setuid root that lets someone hack root, that's your fault, not the > operating system. This is actually one of the things that SELinux is intended to address, so I don't agree that it's quite this cut-and-dry. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
On Thu, Nov 9, 2017 at 1:16 PM, Stephen Frost wrote: > While we have been working to reduce the number of superuser() checks in > the backend in favor of having the ability to GRANT explicit rights, one > of the guideing principles has always been that capabilities which can > be used to gain superuser rights with little effort should remain > restricted to the superuser, which is why the lo_import/lo_export hadn't > been under consideration for superuser-check removal in the analysis I > provided previously. I disagree that that is, or should be, a guiding principle. I'm not sure that anyone other than you and your fellow employees at Crunchy has ever agreed that this is some kind of principle. You make it sound like there's a consensus about this, but I think there isn't. I think our guiding principle should be to get rid of ALL of the hard-coded superuser checks and let people GRANT what they want. If they grant a permission that results in somebody escalating to superuser, they get to keep both pieces. Such risks might be worth documenting, but we shouldn't obstruct people from doing it. In the same way, Linux will not prevent you from making a binary setuid regardless of what the binary does. If you make a binary setuid root that lets someone hack root, that's your fault, not the operating system. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
Tom, Michael, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Michael Paquier writes: > > On Thu, Nov 9, 2017 at 6:05 AM, Tom Lane wrote: > >> Another idea would be to invent a new external flag bit "INV_WRITE_ONLY", > >> so that people who wanted true write-only could get it, without breaking > >> backwards-compatible behavior. But I'm inclined to wait for some field > >> demand to show up before adding even that little bit of complication. > > > Demand that may never show up, and the current behavior on HEAD does > > not receive any complains either. I am keeping the patch simple for > > now. That's less aspirin needed for everybody. > > Looks good to me, pushed. While we have been working to reduce the number of superuser() checks in the backend in favor of having the ability to GRANT explicit rights, one of the guideing principles has always been that capabilities which can be used to gain superuser rights with little effort should remain restricted to the superuser, which is why the lo_import/lo_export hadn't been under consideration for superuser-check removal in the analysis I provided previously. As such, I'm not particularly thrilled to see those checks simply just removed. If we are going to say that it's acceptable to allow non-superusers arbitrary access to files on the filesystem as the OS user then we would also be removing similar checks from adminpack, something I've also been against quite clearly in past discussions. This also didn't update the documentation regarding these functions which clearly states that superuser is required. If we are going to allow non-superusers access to these functions, we certainly need to remove the claim stating that we don't do that and further we must make it abundantly clear that these functions are extremely dangerous and could be trivially used to make someone who has access to them into a superuser. I continue to feel that this is something worthy of serious consideration and discussion, and not something to be done because we happen to be modifying the code in this area. I'm tempted to suggest we should have another role attribute or some other additional check when it comes to functions like these. The right way to allow users to be able to pull in data off the filesystem, imv, would be by providing a way to define specific locations on the filesystem which users can be granted access to import data from, as I once wrote a patch to do. That's certainly quite tricky to get correct, but that's much better than allowing non-superusers arbitrary access, which is what this does and what users may start using if we continue to remove these restrictions without providing a better option. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
Michael Paquier writes: > On Thu, Nov 9, 2017 at 6:05 AM, Tom Lane wrote: >> Another idea would be to invent a new external flag bit "INV_WRITE_ONLY", >> so that people who wanted true write-only could get it, without breaking >> backwards-compatible behavior. But I'm inclined to wait for some field >> demand to show up before adding even that little bit of complication. > Demand that may never show up, and the current behavior on HEAD does > not receive any complains either. I am keeping the patch simple for > now. That's less aspirin needed for everybody. Looks good to me, pushed. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
On Thu, Nov 9, 2017 at 6:05 AM, Tom Lane wrote: > Michael Paquier writes: >> On Tue, Sep 26, 2017 at 11:42 AM, Vaishnavi Prabakaran >> wrote: >>> I moved the cf entry to "ready for committer", and though my vote is for >>> keeping the existing API behavior with write implying read, I let the >>> committer decide whether the following behavior change is Ok or not. >>> "Reading from a large-object descriptor opened with INV_WRITE is NOT >>> possible" > >> Thanks for the review! > > After chewing on this some more, I'm inclined to agree that we should > not change the behavior of the read/write flags. There's been no > field requests for a true-write-only mode, so I think we're much more > likely to get complaints from users whose code we broke than plaudits > from people who think the change is helpful. Thanks for the input. Then that's two people favoring no changes. > I believe it would be easy enough to adjust the patch so that we can > still have the refactoring benefits; we'd just need the bit that > translates from external to internal flags to go more like > > if (flags & INV_WRITE) > descflags |= IFS_WRLOCK | IFS_RDLOCK; > if (flags & INV_READ) > descflags |= IFS_RDLOCK; > > (Preferably with a comment about why it's like this.) Sure, I have updated the patch with this comment: + /* +* Historically, no difference is made between (INV_WRITE) and +* (INV_WRITE | INV_READ), the caller being allowed to read the large +* object descriptor in either case. +*/ Of course please feel free to reword if you find something better-suited. > Another idea would be to invent a new external flag bit "INV_WRITE_ONLY", > so that people who wanted true write-only could get it, without breaking > backwards-compatible behavior. But I'm inclined to wait for some field > demand to show up before adding even that little bit of complication. Demand that may never show up, and the current behavior on HEAD does not receive any complains either. I am keeping the patch simple for now. That's less aspirin needed for everybody. -- Michael 0003-Move-ACL-checks-for-large-objects-when-opening-them.patch Description: Binary data 0001-Remove-ALLOW_DANGEROUS_LO_FUNCTIONS-for-LO-related-s.patch Description: Binary data 0002-Replace-superuser-checks-of-large-object-import-expo.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
Michael Paquier writes: > On Tue, Sep 26, 2017 at 11:42 AM, Vaishnavi Prabakaran > wrote: >> I moved the cf entry to "ready for committer", and though my vote is for >> keeping the existing API behavior with write implying read, I let the >> committer decide whether the following behavior change is Ok or not. >> "Reading from a large-object descriptor opened with INV_WRITE is NOT >> possible" > Thanks for the review! After chewing on this some more, I'm inclined to agree that we should not change the behavior of the read/write flags. There's been no field requests for a true-write-only mode, so I think we're much more likely to get complaints from users whose code we broke than plaudits from people who think the change is helpful. I believe it would be easy enough to adjust the patch so that we can still have the refactoring benefits; we'd just need the bit that translates from external to internal flags to go more like if (flags & INV_WRITE) descflags |= IFS_WRLOCK | IFS_RDLOCK; if (flags & INV_READ) descflags |= IFS_RDLOCK; (Preferably with a comment about why it's like this.) Another idea would be to invent a new external flag bit "INV_WRITE_ONLY", so that people who wanted true write-only could get it, without breaking backwards-compatible behavior. But I'm inclined to wait for some field demand to show up before adding even that little bit of complication. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
On Tue, Sep 26, 2017 at 11:42 AM, Vaishnavi Prabakaran wrote: > I moved the cf entry to "ready for committer", and though my vote is for > keeping the existing API behavior with write implying read, I let the > committer decide whether the following behavior change is Ok or not. > "Reading from a large-object descriptor opened with INV_WRITE is NOT > possible" Thanks for the review! -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
On Tue, Sep 26, 2017 at 11:45 AM, Michael Paquier wrote: > On Tue, Sep 26, 2017 at 9:04 AM, Vaishnavi Prabakaran > wrote: > > Yes, I did realize on further reading the patch and what led to the > > confusion is that in the 3rd patch , updated documentation(copied below) > > still says that reading from a descriptor opened with INV_WRITE is > possible. > > I think we need some correction here to reflect the modified code > behavior. > > > > + or other transactions. Reading from a descriptor opened with > > + INV_WRITE or INV_READ | > > + INV_WRITE returns data that reflects all writes of > > + other committed transactions as well as writes of the current > > + transaction. > > Indeed, you are right. There is an error here. This should read as > "INV_READ | INV_WRITE" only. Using "INV_WRITE" implies that reads > cannot happen. > > Thanks for correcting. I moved the cf entry to "ready for committer", and though my vote is for keeping the existing API behavior with write implying read, I let the committer decide whether the following behavior change is Ok or not. "Reading from a large-object descriptor opened with INV_WRITE is NOT possible" Thanks & Regards, Vaishnavi Fujitsu Australia.
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
On Tue, Sep 26, 2017 at 9:04 AM, Vaishnavi Prabakaran wrote: > Yes, I did realize on further reading the patch and what led to the > confusion is that in the 3rd patch , updated documentation(copied below) > still says that reading from a descriptor opened with INV_WRITE is possible. > I think we need some correction here to reflect the modified code behavior. > > + or other transactions. Reading from a descriptor opened with > + INV_WRITE or INV_READ | > + INV_WRITE returns data that reflects all writes of > + other committed transactions as well as writes of the current > + transaction. Indeed, you are right. There is an error here. This should read as "INV_READ | INV_WRITE" only. Using "INV_WRITE" implies that reads cannot happen. -- Michael 0001-Remove-ALLOW_DANGEROUS_LO_FUNCTIONS-for-LO-related-s.patch Description: Binary data 0002-Replace-superuser-checks-of-large-object-import-expo.patch Description: Binary data 0003-Move-ACL-checks-for-large-objects-when-opening-them.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
Hi,
On Tue, Sep 19, 2017 at 5:12 PM, Michael Paquier
wrote:
>
> >>@@ -163,22 +150,16 @@ lo_read(int fd, char *buf, int len)
> >>
> >> + if ((lobj->flags & IFS_RDLOCK) == 0)
> >>+ ereport(ERROR,
> >>+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> >>+ errmsg("large object descriptor %d was not opened for reading",
> >>+ fd)));
> >
> > Do we ever reach this error? Because per my understanding
>
> This error can be reached, and it is part of the regression tests. One
> query which passed previously is now failing:
> +SELECT loread(lo_open(1001, x'2'::int), 32); -- fail, wrong mode
> +ERROR: large object descriptor 0 was not opened for reading
Yes, I did realize on further reading the patch and what led to the
confusion is that in the 3rd patch , updated documentation(copied below)
still says that reading from a descriptor opened with INV_WRITE is
possible. I think we need some correction here to reflect the modified code
behavior.
+ or other transactions. Reading from a descriptor opened with
+ INV_WRITE or INV_READ |
+ INV_WRITE returns data that reflects all writes of
+ other committed transactions as well as writes of the current
+ transaction.
Thanks & Regards,
Vaishnavi,
Fujitsu Australia.
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
On Tue, Sep 19, 2017 at 1:24 PM, Vaishnavi Prabakaran
wrote:
> On Mon, Aug 14, 2017, Michael Paquier wrote:
>>Attached is a set of 3 patches:
>
> I tried to review the patch and firstly patch applies cleanly without any
> noise.
Thanks for the review, Vaishnavi.
>>- 0001 removes ALLOW_DANGEROUS_LO_FUNCTIONS
>
> I think it is better to remove "ALLOW_DANGEROUS_LO_FUNCTIONS" from
> pg_config_manual.h as well.
Indeed. Fixed.
>>@@ -163,22 +150,16 @@ lo_read(int fd, char *buf, int len)
>>
>> + if ((lobj->flags & IFS_RDLOCK) == 0)
>>+ ereport(ERROR,
>>+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>>+ errmsg("large object descriptor %d was not opened for reading",
>>+ fd)));
>
> Do we ever reach this error? Because per my understanding
This error can be reached, and it is part of the regression tests. One
query which passed previously is now failing:
+SELECT loread(lo_open(1001, x'2'::int), 32); -- fail, wrong mode
+ERROR: large object descriptor 0 was not opened for reading
> I think documented behavior is more appropriate and post ACL
> checks for select and update permissions in inv_open(), IFS_RDLOCK flag
> should be set regardless of mode specified.
OK, so that's one vote for keeping the existing API behavior with
write implying read. Thanks for the input. At least I can see no
complains about patches 1 and 2 :)
--
Michael
0001-Remove-ALLOW_DANGEROUS_LO_FUNCTIONS-for-LO-related-s.patch
Description: Binary data
0002-Replace-superuser-checks-of-large-object-import-expo.patch
Description: Binary data
0003-Move-ACL-checks-for-large-objects-when-opening-them.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
Hi,
On Mon, Aug 14, 2017, Michael Paquier wrote:
>Attached is a set of 3 patches:
I tried to review the patch and firstly patch applies cleanly without any
noise.
>- 0001 removes ALLOW_DANGEROUS_LO_FUNCTIONS
I think it is better to remove "ALLOW_DANGEROUS_LO_FUNCTIONS" from
pg_config_manual.h as well.
>- 0002 replaces the superuser checks with GRANT permissions
>- 0003 refactors the LO code to improve the ACL handling. Note that
>this patch introduces a more debatable change: now there is no
>distinction between INV_WRITE and INV_WRITE|INV_READ, as when
>INV_WRITE is used INV_READ is implied. The patch as proposed does a
>complete split of both permissions to give a system more natural and
>more flexible. The current behavior is documented.
>@@ -163,22 +150,16 @@ lo_read(int fd, char *buf, int len)
>
> + if ((lobj->flags & IFS_RDLOCK) == 0)
>+ ereport(ERROR,
>+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>+ errmsg("large object descriptor %d was not opened for reading",
>+ fd)));
Do we ever reach this error? Because per my understanding
1) if the application didn't call lo_open before lo_read, then file
descriptor validation in the beginning of this function should capture it.
2) if the application called lo_open only with write mode should be able to
read as well per documentation.
Ok, in the inv_open(), IFS_RDLOCK is set only when explicit READ mode is
requested. So, it causes lo_read failure when large-object is opened in
WRITE mode? I think documented behavior is more appropriate and post ACL
checks for select and update permissions in inv_open(), IFS_RDLOCK flag
should be set regardless of mode specified.
Thanks & Regards,
Vaishnavi,
Fujitsu Australia.
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
On Tue, Aug 15, 2017 at 10:35 PM, Robert Haas wrote: > +1 for 0001 and 0002 in general, but I can't help noticing that they > lead to a noticeable worsening of the error messages in the regression > tests. As the access restriction gets handled by GRANT in this patch, that's a price to pay. The verbosity of this message could be kept by introducing a default role dedicated to LOs. Personally, I am of the opinion that a default role in this case is not justified for only those functions. Other opinions are welcome. I have noticed that 0002 should update lobj.sgml as well, something I missed. Now the docs say "Therefore, their use is restricted to superusers." for lo_import and lo_export, but I think that "by default" should be appended. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
On Sun, Aug 13, 2017 at 11:20 PM, Michael Paquier wrote: > Recent commit 8d98819 has added a missing permissoin check to lo_put() > to make sure that the write permissions of the object are properly set > before writing to a large object. When studying the problem, we bumped > into the fact that it is possible to actually simplify those ACL > checks and replace them by checks when opening the large object. This > makes all the checks now in be-fsstubs.c happen in inv_api.c, which is > where all the large object handling happens at a lower level. This > way, it is also possible to remove the extra logic in place to have > the permission checks happen only once. > > At the same time, we have bumped into two things: > - ALLOW_DANGEROUS_LO_FUNCTIONS has been introduced in 1999, so it > could be time to let it go. I have known of no place where this is > actively used. > - lo_import and lo_export on the backend have superuser checks. We > could remove them and replace them with proper REVOKE permissions to > allow administrators to give access to those functions. > > Attached is a set of 3 patches: > - 0001 removes ALLOW_DANGEROUS_LO_FUNCTIONS > - 0002 replaces the superuser checks with GRANT permissions +1 for 0001 and 0002 in general, but I can't help noticing that they lead to a noticeable worsening of the error messages in the regression tests. -- 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
[HACKERS] Simplify ACL handling for large objects and removal of superuser() checks
Hi all, Recent commit 8d98819 has added a missing permissoin check to lo_put() to make sure that the write permissions of the object are properly set before writing to a large object. When studying the problem, we bumped into the fact that it is possible to actually simplify those ACL checks and replace them by checks when opening the large object. This makes all the checks now in be-fsstubs.c happen in inv_api.c, which is where all the large object handling happens at a lower level. This way, it is also possible to remove the extra logic in place to have the permission checks happen only once. At the same time, we have bumped into two things: - ALLOW_DANGEROUS_LO_FUNCTIONS has been introduced in 1999, so it could be time to let it go. I have known of no place where this is actively used. - lo_import and lo_export on the backend have superuser checks. We could remove them and replace them with proper REVOKE permissions to allow administrators to give access to those functions. Attached is a set of 3 patches: - 0001 removes ALLOW_DANGEROUS_LO_FUNCTIONS - 0002 replaces the superuser checks with GRANT permissions - 0003 refactors the LO code to improve the ACL handling. Note that this patch introduces a more debatable change: now there is no distinction between INV_WRITE and INV_WRITE|INV_READ, as when INV_WRITE is used INV_READ is implied. The patch as proposed does a complete split of both permissions to give a system more natural and more flexible. The current behavior is documented. Please note as well that it is possible to implement a patch that keeps compatibility as well, but I would welcome a debate on the matter. This patch owns also a good deal to Tom. I am parking them to the next commit fest for PG11. Thanks, -- Michael From 2a2b2beddc5b01ffe6de7e442e20e00b4e518859 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Mon, 14 Aug 2017 12:01:58 +0900 Subject: [PATCH 3/3] Move ACL checks for large objects when opening them Up to now, ACL checks for large objects happened at the the level of the SQL-callable functions, which led to CVE-2017-7548 because of a missing check. It is actually possible to simplify those checks when opening a large object, which simplifies the code checking them so as there is no need to track if a check has already been done or not and this removes any risk of missing again permission checks if a function related to large objects is added in the future. A more debatable change that this patch introduces is that opening a large object for write is equivalent to read-write, which is a behavior documented, into a complete split of the checks. This gives the user more flexibility at the risk of breaking some applications, and makes the new behavior more natural. Patch by Tom Lane and Michael Paquier. --- doc/src/sgml/lobj.sgml | 25 +++ src/backend/catalog/objectaddress.c| 2 +- src/backend/libpq/be-fsstubs.c | 88 +--- src/backend/storage/large_object/inv_api.c | 103 ++--- src/backend/utils/misc/guc.c | 2 +- src/include/libpq/be-fsstubs.h | 5 -- src/include/storage/large_object.h | 13 ++-- src/test/regress/expected/privileges.out | 8 +-- src/test/regress/sql/privileges.sql| 2 +- 9 files changed, 121 insertions(+), 127 deletions(-) diff --git a/doc/src/sgml/lobj.sgml b/doc/src/sgml/lobj.sgml index 7757e1e441..2025545c75 100644 --- a/doc/src/sgml/lobj.sgml +++ b/doc/src/sgml/lobj.sgml @@ -276,20 +276,17 @@ int lo_open(PGconn *conn, Oid lobjId, int mode); - The server currently does not distinguish between modes - INV_WRITE and INV_READ | - INV_WRITE: you are allowed to read from the descriptor - in either case. However there is a significant difference between - these modes and INV_READ alone: with INV_READ - you cannot write on the descriptor, and the data read from it will - reflect the contents of the large object at the time of the transaction - snapshot that was active when lo_open was executed, - regardless of later writes by this or other transactions. Reading - from a descriptor opened with INV_WRITE returns - data that reflects all writes of other committed transactions as well - as writes of the current transaction. This is similar to the behavior - of REPEATABLE READ versus READ COMMITTED transaction - modes for ordinary SQL SELECT commands. + With only INV_READ you cannot write on the descriptor, + and the data read from it will reflect the contents of the large object + at the time of the transaction snapshot that was active when + lo_open was executed, regardless of later writes by this + or other transactions. Reading from a descriptor opened with + INV_WRITE or INV_READ | + INV_WRITE returns data that reflects all writes of + other committed transactions as well as writes of the current + trans
