pg_amcheck option to install extension
Hi, Peter Geoghegan suggested that I have the cross version upgrade checker run pg_amcheck on the upgraded module. This seemed to me like a good idea, so I tried it, only to find that it refuses to run unless the amcheck extension is installed. That's fair enough, but it also seems to me like we should have an option to have pg_amcheck install the extension is it's not present, by running something like 'create extension if not exists amcheck'. Maybe in such a case there could also be an option to drop the extension when pg_amcheck's work is done - I haven't thought through all the implications. Given pg_amcheck is a new piece of work I'm not sure if we can sneak this in under the wire for release 14. I will certainly undertake to review anything expeditiously. I can work around this issue in the buildfarm, but it seems like something other users are likely to want. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: pg_amcheck option to install extension
> On Apr 16, 2021, at 11:06 AM, Andrew Dunstan wrote: > > > Hi, > > Peter Geoghegan suggested that I have the cross version upgrade checker > run pg_amcheck on the upgraded module. This seemed to me like a good > idea, so I tried it, only to find that it refuses to run unless the > amcheck extension is installed. That's fair enough, but it also seems to > me like we should have an option to have pg_amcheck install the > extension is it's not present, by running something like 'create > extension if not exists amcheck'. Maybe in such a case there could also > be an option to drop the extension when pg_amcheck's work is done - I > haven't thought through all the implications. > > Given pg_amcheck is a new piece of work I'm not sure if we can sneak > this in under the wire for release 14. I will certainly undertake to > review anything expeditiously. I can work around this issue in the > buildfarm, but it seems like something other users are likely to want. We cannot quite use a "create extension if not exists amcheck" command, as we clear the search path and so must specify the schema to use. Should we instead avoid clearing the search path for this? What are the security implications of using the first schema of the search path? When called as `pg_amcheck --install-missing`, the implementation in the attached patch runs per database being checked a "create extension if not exists amcheck with schema public". If called as `pg_amcheck --install-missing=foo`, it instead runs "create extension if not exists amcheck with schema foo` having escaped "foo" appropriately for the given database. There is no option to use different schemas for different databases. Nor is there any option to use the search path. If somebody needs that, I think they can manage installing amcheck themselves. Does this meet your needs for v14? I'd like to get this nailed down quickly, as it is unclear to me that we should even be doing this so late in the development cycle. I'd also like your impressions on whether we're likely to move contrib/amcheck into core anytime soon. If so, is it worth adding an option that we'll soon need to deprecate? v1-0001-Adding-install-missing-option-to-pg_amcheck.patch Description: Binary data — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pg_amcheck option to install extension
On 4/17/21 3:43 PM, Mark Dilger wrote: > >> On Apr 16, 2021, at 11:06 AM, Andrew Dunstan wrote: >> >> >> Hi, >> >> Peter Geoghegan suggested that I have the cross version upgrade checker >> run pg_amcheck on the upgraded module. This seemed to me like a good >> idea, so I tried it, only to find that it refuses to run unless the >> amcheck extension is installed. That's fair enough, but it also seems to >> me like we should have an option to have pg_amcheck install the >> extension is it's not present, by running something like 'create >> extension if not exists amcheck'. Maybe in such a case there could also >> be an option to drop the extension when pg_amcheck's work is done - I >> haven't thought through all the implications. >> >> Given pg_amcheck is a new piece of work I'm not sure if we can sneak >> this in under the wire for release 14. I will certainly undertake to >> review anything expeditiously. I can work around this issue in the >> buildfarm, but it seems like something other users are likely to want. > We cannot quite use a "create extension if not exists amcheck" command, as we > clear the search path and so must specify the schema to use. Should we > instead avoid clearing the search path for this? What are the security > implications of using the first schema of the search path? > > When called as `pg_amcheck --install-missing`, the implementation in the > attached patch runs per database being checked a "create extension if not > exists amcheck with schema public". If called as `pg_amcheck > --install-missing=foo`, it instead runs "create extension if not exists > amcheck with schema foo` having escaped "foo" appropriately for the given > database. There is no option to use different schemas for different > databases. Nor is there any option to use the search path. If somebody > needs that, I think they can manage installing amcheck themselves. how about specifying pg_catalog as the schema instead of public? > > Does this meet your needs for v14? I'd like to get this nailed down quickly, > as it is unclear to me that we should even be doing this so late in the > development cycle. I'm ok with or without - I'll just have the buildfarm client pull a list of databases and install the extension in all of them. > > I'd also like your impressions on whether we're likely to move > contrib/amcheck into core anytime soon. If so, is it worth adding an option > that we'll soon need to deprecate? I think if it stays as an extension it will stay in contrib. But it sure feels very odd to have a core bin program that relies on a contrib extension. It seems one or the other is misplaced. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: pg_amcheck option to install extension
> On Apr 18, 2021, at 6:19 AM, Andrew Dunstan wrote: > > how about specifying pg_catalog as the schema instead of public? Done. v2-0001-Adding-install-missing-option-to-pg_amcheck.patch Description: Binary data — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pg_amcheck option to install extension
On 2021-Apr-18, Andrew Dunstan wrote: > On 4/17/21 3:43 PM, Mark Dilger wrote: > > I'd also like your impressions on whether we're likely to move > > contrib/amcheck into core anytime soon. If so, is it worth adding > > an option that we'll soon need to deprecate? > > I think if it stays as an extension it will stay in contrib. But it sure > feels very odd to have a core bin program that relies on a contrib > extension. It seems one or the other is misplaced. I've proposed in the past that we should have a way to provide extensions other than contrib -- specifically src/extensions/ -- and then have those extensions installed together with the rest of core. Then it would be perfectly legitimate to have src/bin/pg_amcheck that depending that extension. I agree that the current situation is not great. -- Álvaro Herrera39°49'30"S 73°17'W "Thou shalt not follow the NULL pointer, for chaos and madness await thee at its end." (2nd Commandment for C programmers)
Re: pg_amcheck option to install extension
On 4/18/21 7:32 PM, Alvaro Herrera wrote: > On 2021-Apr-18, Andrew Dunstan wrote: > >> On 4/17/21 3:43 PM, Mark Dilger wrote: >>> I'd also like your impressions on whether we're likely to move >>> contrib/amcheck into core anytime soon. If so, is it worth adding >>> an option that we'll soon need to deprecate? >> I think if it stays as an extension it will stay in contrib. But it sure >> feels very odd to have a core bin program that relies on a contrib >> extension. It seems one or the other is misplaced. > I've proposed in the past that we should have a way to provide > extensions other than contrib -- specifically src/extensions/ -- and > then have those extensions installed together with the rest of core. > Then it would be perfectly legitimate to have src/bin/pg_amcheck that > depending that extension. I agree that the current situation is not > great. > OK, so let's fix it. If amcheck is going to stay in contrib then ISTM pg_amcheck should move there. I can organize that if there's agreement. Or else let's move amcheck as Alvaro suggests. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: pg_amcheck option to install extension
> On Apr 19, 2021, at 9:32 AM, Andrew Dunstan wrote: > > > On 4/18/21 7:32 PM, Alvaro Herrera wrote: >> On 2021-Apr-18, Andrew Dunstan wrote: >> >>> On 4/17/21 3:43 PM, Mark Dilger wrote: I'd also like your impressions on whether we're likely to move contrib/amcheck into core anytime soon. If so, is it worth adding an option that we'll soon need to deprecate? >>> I think if it stays as an extension it will stay in contrib. But it sure >>> feels very odd to have a core bin program that relies on a contrib >>> extension. It seems one or the other is misplaced. >> I've proposed in the past that we should have a way to provide >> extensions other than contrib -- specifically src/extensions/ -- and >> then have those extensions installed together with the rest of core. >> Then it would be perfectly legitimate to have src/bin/pg_amcheck that >> depending that extension. I agree that the current situation is not >> great. >> > > > OK, so let's fix it. If amcheck is going to stay in contrib then ISTM > pg_amcheck should move there. I can organize that if there's agreement. > Or else let's move amcheck as Alvaro suggests. Ah, no. I wrote pg_amcheck in contrib originally, and moved it to src/bin as requested during the v14 development cycle. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pg_amcheck option to install extension
On Mon, Apr 19, 2021 at 12:37 PM Mark Dilger wrote: > > OK, so let's fix it. If amcheck is going to stay in contrib then ISTM > > pg_amcheck should move there. I can organize that if there's agreement. > > Or else let's move amcheck as Alvaro suggests. > > Ah, no. I wrote pg_amcheck in contrib originally, and moved it to src/bin as > requested during the v14 development cycle. Yeah, I am not that excited about moving this again. I realize it was never committed anywhere else, but it was moved at least one during development. And I don't see that moving it to contrib really fixes anything anyway here, except perhaps conceptually. Maybe inventing src/extensions is the right idea, but there's no real need to do that at this point in the release cycle, and it doesn't actually fix anything either. The only thing that's really needed here is to either (a) teach the test script to install amcheck as a separate step or (b) teach pg_amcheck to install amcheck in a user-specified schema. If we do that, AIUI, this issue is fixed regardless of whether we move any source code around, and if we don't do that, AIUI, this issue is not fixed no matter how much source code we move. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_amcheck option to install extension
Andrew Dunstan writes: > OK, so let's fix it. If amcheck is going to stay in contrib then ISTM > pg_amcheck should move there. I can organize that if there's agreement. > Or else let's move amcheck as Alvaro suggests. FWIW, I think that putting them both in contrib makes the most sense from a structural standpoint. Either way, though, you'll still need the proposed option to let the executable issue a CREATE EXTENSION to get the shlib loaded. Unless somebody is proposing that the extension be installed-by-default like plpgsql, and that I am unequivocally not for. regards, tom lane
Re: pg_amcheck option to install extension
> On Apr 19, 2021, at 9:53 AM, Tom Lane wrote: > > Andrew Dunstan writes: >> OK, so let's fix it. If amcheck is going to stay in contrib then ISTM >> pg_amcheck should move there. I can organize that if there's agreement. >> Or else let's move amcheck as Alvaro suggests. > > FWIW, I think that putting them both in contrib makes the most > sense from a structural standpoint. That was my original thought also, largely from a package management perspective. Just as an example, postgresql-client and postgresql-contrib are separate rpms. There isn't much point to having pg_amcheck installed as part of the postgresql-client package while having amcheck in the postgresql-contrib package which might not be installed. A counter argument is that amcheck is server side, and pg_amcheck is client side. Having pg_amcheck installed on a system makes sense if you are connecting to a server on a different system. > On Mar 11, 2021, at 12:12 AM, Peter Eisentraut > wrote: > > I want to register, if we are going to add this, it ought to be in src/bin/. > If we think it's a useful tool, it should be there with all the other useful > tools. > > I realize there is a dependency on a module in contrib, and it's probably now > not the time to re-debate reorganizing contrib. But if we ever get to that, > this program should be the prime example why the current organization is > problematic, and we should be prepared to make the necessary moves then. This was the request that motivated the move to src/bin. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pg_amcheck option to install extension
On 4/19/21 1:25 PM, Mark Dilger wrote: > >> On Apr 19, 2021, at 9:53 AM, Tom Lane wrote: >> >> Andrew Dunstan writes: >>> OK, so let's fix it. If amcheck is going to stay in contrib then ISTM >>> pg_amcheck should move there. I can organize that if there's agreement. >>> Or else let's move amcheck as Alvaro suggests. >> FWIW, I think that putting them both in contrib makes the most >> sense from a structural standpoint. > That was my original thought also, largely from a package management > perspective. Just as an example, postgresql-client and postgresql-contrib > are separate rpms. There isn't much point to having pg_amcheck installed as > part of the postgresql-client package while having amcheck in the > postgresql-contrib package which might not be installed. > > A counter argument is that amcheck is server side, and pg_amcheck is client > side. Having pg_amcheck installed on a system makes sense if you are > connecting to a server on a different system. There are at least two other client side programs in contrib. So this argument doesn't quite hold water from a consistency POV. > >> On Mar 11, 2021, at 12:12 AM, Peter Eisentraut >> wrote: >> >> I want to register, if we are going to add this, it ought to be in src/bin/. >> If we think it's a useful tool, it should be there with all the other >> useful tools. >> >> I realize there is a dependency on a module in contrib, and it's probably >> now not the time to re-debate reorganizing contrib. But if we ever get to >> that, this program should be the prime example why the current organization >> is problematic, and we should be prepared to make the necessary moves then. > This was the request that motivated the move to src/bin. > I missed that, so I guess maybe I can't complain too loudly. But if I'd seen it I would have disagreed. :-) cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: pg_amcheck option to install extension
On Mon, Apr 19, 2021 at 12:53:29PM -0400, Tom Lane wrote: > FWIW, I think that putting them both in contrib makes the most > sense from a structural standpoint. > > Either way, though, you'll still need the proposed option to > let the executable issue a CREATE EXTENSION to get the shlib > loaded. Unless somebody is proposing that the extension be > installed-by-default like plpgsql, and that I am unequivocally > not for. Agreed. Something like src/extensions/ would be a tempting option, but I don't think that it is a good idea to introduce a new piece of infrastructure at this stage, so moving both to contrib/ would be the best balance with the current pieces at hand. -- Michael signature.asc Description: PGP signature
Re: pg_amcheck option to install extension
> On Apr 19, 2021, at 6:41 PM, Michael Paquier wrote: > > On Mon, Apr 19, 2021 at 12:53:29PM -0400, Tom Lane wrote: >> FWIW, I think that putting them both in contrib makes the most >> sense from a structural standpoint. >> >> Either way, though, you'll still need the proposed option to >> let the executable issue a CREATE EXTENSION to get the shlib >> loaded. Unless somebody is proposing that the extension be >> installed-by-default like plpgsql, and that I am unequivocally >> not for. > > Agreed. Something like src/extensions/ would be a tempting option, > but I don't think that it is a good idea to introduce a new piece of > infrastructure at this stage, so moving both to contrib/ would be the > best balance with the current pieces at hand. There is another issue to consider. Installing pg_amcheck in no way opens up an avenue of attack that I can see. It is just a client application with no special privileges. But installing amcheck arguably opens a line of attack; not one as significant as installing pageinspect, but of the same sort. Amcheck allows privileged database users to potentially get information from the tables that would otherwise be invisible even to them according to mvcc rules. (Is this already the case via some other functionality? Maybe this security problem already exists?) If the privileged database user has file system access, then this is not at all concerning, since they can already just open the files in a tool of their choice, but I don't see any reason why installations should require that privileged database users also be privileged to access the file system. If you are not buying my argument here, perhaps a reference to how encryption functions are evaluated might help you see my point of view. You don't ask, "can the attacker recover the plain text from the encrypted text", but rather, "can the attacker tell the difference between encrypted plain text and encrypted random noise." That's because it is incredibly hard to reason about what an attacker might be able to learn. Even though learning about old data using amcheck would be hard, you can't say that an attacker would never be able to recover information about deleted rows. As such, security conscious installations are within reason to refuse to install it. Since amcheck (and to a much larger extent, pageinspect) open potential data leakage issues, it makes sense for some security conscious sites to refuse to install it. pg_amcheck on the other hand could be installed everywhere. I understand why it might *feel* like pg_amcheck and amcheck have to both be installed to work, but I don't think that point of view makes much sense in reality. The computer running the client and the computer running the server are frequently not the same computer. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pg_amcheck option to install extension
On Mon, Apr 19, 2021 at 07:15:23PM -0700, Mark Dilger wrote: > There is another issue to consider. Installing pg_amcheck in no way > opens up an avenue of attack that I can see. It is just a client > application with no special privileges. But installing amcheck > arguably opens a line of attack; not one as significant as > installing pageinspect, but of the same sort. Amcheck allows > privileged database users to potentially get information from the > tables that would otherwise be invisible even to them according to > mvcc rules. (Is this already the case via some other functionality? > Maybe this security problem already exists?) If the privileged > database user has file system access, then this is not at all > concerning, since they can already just open the files in a tool of > their choice, but I don't see any reason why installations should > require that privileged database users also be privileged to access > the file system. By default, any functions deployed with amcheck have their execution rights revoked from public, meaning that only a superuser can run them with a default installation. A non-superuser could execute them only once GRANT'd access to them. -- Michael signature.asc Description: PGP signature
Re: pg_amcheck option to install extension
> On Apr 19, 2021, at 8:06 PM, Michael Paquier wrote: > > On Mon, Apr 19, 2021 at 07:15:23PM -0700, Mark Dilger wrote: >> There is another issue to consider. Installing pg_amcheck in no way >> opens up an avenue of attack that I can see. It is just a client >> application with no special privileges. But installing amcheck >> arguably opens a line of attack; not one as significant as >> installing pageinspect, but of the same sort. Amcheck allows >> privileged database users to potentially get information from the >> tables that would otherwise be invisible even to them according to >> mvcc rules. (Is this already the case via some other functionality? >> Maybe this security problem already exists?) If the privileged >> database user has file system access, then this is not at all >> concerning, since they can already just open the files in a tool of >> their choice, but I don't see any reason why installations should >> require that privileged database users also be privileged to access >> the file system. > > By default, any functions deployed with amcheck have their execution > rights revoked from public, meaning that only a superuser can run them > with a default installation. A non-superuser could execute them only > once GRANT'd access to them. Correct. So having amcheck installed on the system provides the database superuser with a privilege escalation attack vector. I am assuming here the database superuser is not a privileged system user, and can only log in remotely, has no direct access to the file system, etc. Alice has a database with sensitive data. She hires Bob to be her new database admin, with superuser privilege, but doesn't want Bob to see the sensitive data, so she deletes it first. The data is dead but still on disk. Bob discovers a bug in postgres that will corrupt dead rows that some index is still pointing at. This attack requires sufficient privilege to trigger the bug, but presumably he has that much privilege, because he is a database superuser. Let's call this attack C(x) where "C" means the corruption inducing function, and "x" is the indexed key for which dead rows will be corrupted. Bob runs "CREATE EXTENSION amcheck", and then successively runs C(x) followed by amcheck for each interesting value of x. Bob learns which of these values were in the system before Alice deleted them. This is a classic privilege escalation attack. Bob has one privilege, and uses it to get another. Alice might foresee this behavior from Bob and choose not to install contrib/amcheck. This is paranoid on her part, but does not cross the line into insanity. The postgres community has every reason to keep amcheck in contrib so that users such as Alice can make this decision. No similar argument has been put forward for why pg_amcheck should be kept in contrib. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pg_amcheck option to install extension
On Mon, Apr 19, 2021 at 08:39:06PM -0700, Mark Dilger wrote: > This is a classic privilege escalation attack. Bob has one > privilege, and uses it to get another. Bob is a superuser, so it has all the privileges of the world for this instance. In what is that different from BASE_BACKUP or just COPY FROM PROGRAM? I am not following your argument here. -- Michael signature.asc Description: PGP signature
Re: pg_amcheck option to install extension
> On Apr 19, 2021, at 9:22 PM, Michael Paquier wrote: > > On Mon, Apr 19, 2021 at 08:39:06PM -0700, Mark Dilger wrote: >> This is a classic privilege escalation attack. Bob has one >> privilege, and uses it to get another. > > Bob is a superuser, so it has all the privileges of the world for this > instance. In what is that different from BASE_BACKUP or just COPY > FROM PROGRAM? I think you are conflating the concept of an operating system adminstrator with the concept of the database superuser/owner. If the operating system user that postgres is running as cannot execute any binaries, then "copy from program" is not a way for a database admistrator to escape the jail. If Bob does not have ssh access to the system, he cannot run pg_basebackup. > I am not following your argument here. The argument is that the operating system user that postgres is running as, perhaps user "postgres", can read the files in the $PGDATA directory, but Bob can only see the MVCC view of the data, not the raw data. Installing contrib/amcheck allows Bob to get a peak behind the curtain. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pg_amcheck option to install extension
On Mon, Apr 19, 2021 at 10:31:18PM -0700, Mark Dilger wrote: > I think you are conflating the concept of an operating system > adminstrator with the concept of the database superuser/owner. If > the operating system user that postgres is running as cannot execute > any binaries, then "copy from program" is not a way for a database > admistrator to escape the jail. If Bob does not have ssh access to > the system, he cannot run pg_basebackup. You don't need much to be able to take a base backup once you have a connection to the backend as long as your have the permissions to do so. In this case that would be just the replication permissions. > The argument is that the operating system user that postgres is > running as, perhaps user "postgres", can read the files in the > $PGDATA directory, but Bob can only see the MVCC view of the data, > not the raw data. Installing contrib/amcheck allows Bob to get a > peak behind the curtain. In my world, a superuser is considered as an entity holding the same rights as the OS user running the PostgreSQL instance, so that's wider than the definition you are thinking of here. There are many fancy things one can do in this case, just to name a few that give access to any files of the data directory or even other paths: - pg_read_file(), and take the equivalent of a base backup with a RECURSIVE CTE. - BASE_BACKUP, doable from a simple psql session with a replication connection. - Untrusted languages. So I don't understand your argument with amcheck here because any of its features *requires* superuser rights unless granted. Coming back to your example, Alice actually gave up the control of her database to Bob the moment she gave him superuser rights. If she really wanted to protect her privacy, she'd better think about a more restricted set of ACLs for Bob before letting him manage her data, even if she considers herself "safe" after she deleted it, but she's really not safe by any means. I still stand with the point of upthread to put all that in contrib/ for now, without discarding that this could be moved somewhere else in the future. -- Michael signature.asc Description: PGP signature
Re: pg_amcheck option to install extension
On Mon, Apr 19, 2021 at 2:55 PM Andrew Dunstan wrote: > There are at least two other client side programs in contrib. So this > argument doesn't quite hold water from a consistency POV. I thought that at first, too. But then I realized that those programs are oid2name and vacuumlo. And oid2name, at least, seems like something we ought to just consider removing. It's unclear why this is something that really deserves a command-line utility rather than just some additional psql options or something. Does anyone really use it? vacuumlo isn't that impressive either, since it makes the very tenuous assumption that an oid column is intended to reference a large object, and the documentation doesn't even acknowledge what a shaky idea that actually is. But I suspect it has much better chances of being useful in practice than oid2name. In fact, I've heard of people using it and, I think, finding it useful, so we probably don't want to just nuke it. But the point is, as things stand today, almost everything in contrib is an extension, not a binary. And we might want to view the exceptions as loose ends to be cleaned up, rather than a pattern to emulate. It's a judgement call, though. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_amcheck option to install extension
On Tue, Apr 20, 2021 at 2:47 PM Robert Haas wrote: > > On Mon, Apr 19, 2021 at 2:55 PM Andrew Dunstan wrote: > > There are at least two other client side programs in contrib. So this > > argument doesn't quite hold water from a consistency POV. > > I thought that at first, too. But then I realized that those programs > are oid2name and vacuumlo. And oid2name, at least, seems like > something we ought to just consider removing. It's unclear why this is > something that really deserves a command-line utility rather than just > some additional psql options or something. Does anyone really use it? Yeah, this seems like it could relatively simply just be a SQL query in psql. > vacuumlo isn't that impressive either, since it makes the very tenuous > assumption that an oid column is intended to reference a large object, > and the documentation doesn't even acknowledge what a shaky idea that > actually is. But I suspect it has much better chances of being useful > in practice than oid2name. In fact, I've heard of people using it and, > I think, finding it useful, so we probably don't want to just nuke it. Yes, I've definitely run into using vacuumlo many times. > But the point is, as things stand today, almost everything in contrib > is an extension, not a binary. And we might want to view the > exceptions as loose ends to be cleaned up, rather than a pattern to > emulate. I could certainly sign up for moving vacuumlo to bin/ and replacing oid2name with something in psql for example. (But yes, I realize this rapidly turns into another instance of the bikeshedding about the future of contrib..) -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Re: pg_amcheck option to install extension
On Tue, Apr 20, 2021 at 1:31 AM Mark Dilger wrote: > I think you are conflating the concept of an operating system adminstrator > with the concept of the database superuser/owner. You should conflate those things, because there's no meaningful privilege boundary between them: http://rhaas.blogspot.com/2020/12/cve-2019-9193.html If reading the whole thing is too much, scroll down to the part in fixed-width font and behold me trivially compromising the OS account using plperlu. I actually think this is a design error on our part. A lot of people, apparently including you, feel that there should be a privilege boundary between the PostgreSQL superuser and the OS user, or want such a boundary to exist. It would be quite useful if there were a boundary there, because it's entirely reasonable to want to have a user who is allowed to do everything with the database except escape into the OS account, and I can't think of any reason why we couldn't set things up so that this is possible. We'd have to bar some things that the superuser can currently do, like directly modify system tables and use COPY TO/FROM PROGRAM, but there's a lot of things we could allow too, like reading all the data and creating and deleting accounts and setting their permissions arbitrarily, except maybe for any special super-DUPER users who are allowed to do things that escape the sandbox. Now it would take a fair amount of work to make that distinction in a rigorous way and figure out exactly what the design ought to be, and I'm not volunteering. But I bet a lot of people would like it. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_amcheck option to install extension
On 2021-Apr-20, Michael Paquier wrote: > Agreed. Something like src/extensions/ would be a tempting option, > but I don't think that it is a good idea to introduce a new piece of > infrastructure at this stage, so moving both to contrib/ would be the > best balance with the current pieces at hand. Actually I think the best balance would be to leave things where they are, and move amcheck to src/extensions/ once the next devel cycle opens. That way, we avoid the (pretty much pointless) laborious task of moving pg_amcheck to contrib only to move it back on the next cycle. What I'm afraid of, if we move pg_amcheck to contrib, is that during the next cycle people will say that they are both perfectly fine in contrib/ and so we don't need to move anything anywhere. And next time someone wants to create a new extension that would be perfectly fine in core, they will not want to have that one be the one that creates src/extensions/, because then that in itself is a contentious point that can get the whole patch rejected. In a sense, what I'm doing is support the idea that "incremental development" applies to procedure too. -- Álvaro Herrera39°49'30"S 73°17'W
Re: pg_amcheck option to install extension
On 4/20/21 8:47 AM, Robert Haas wrote: > On Mon, Apr 19, 2021 at 2:55 PM Andrew Dunstan wrote: >> There are at least two other client side programs in contrib. So this >> argument doesn't quite hold water from a consistency POV. > I thought that at first, too. But then I realized that those programs > are oid2name and vacuumlo. And oid2name, at least, seems like > something we ought to just consider removing. It's unclear why this is > something that really deserves a command-line utility rather than just > some additional psql options or something. Does anyone really use it? > > vacuumlo isn't that impressive either, since it makes the very tenuous > assumption that an oid column is intended to reference a large object, > and the documentation doesn't even acknowledge what a shaky idea that > actually is. But I suspect it has much better chances of being useful > in practice than oid2name. In fact, I've heard of people using it and, > I think, finding it useful, so we probably don't want to just nuke it. > > But the point is, as things stand today, almost everything in contrib > is an extension, not a binary. And we might want to view the > exceptions as loose ends to be cleaned up, rather than a pattern to > emulate. > > It's a judgement call, though. > Yeah. I'll go along with Alvaro and say let's let sleeping dogs lie at this stage of the dev process, and pick the discussion up after we branch. I will just note one thing: the binaries in contrib have one important function that hasn't been mentioned, namely to test using pgxs to build binaries. That doesn't have to live in contrib, but we should have something that does that somewhere in the build process, so if we remmove oid2name and vacuumlo from contrib we need to look into that. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: pg_amcheck option to install extension
Alvaro Herrera writes: > Actually I think the best balance would be to leave things where they > are, and move amcheck to src/extensions/ once the next devel cycle > opens. That way, we avoid the (pretty much pointless) laborious task of > moving pg_amcheck to contrib only to move it back on the next cycle. > What I'm afraid of, if we move pg_amcheck to contrib, is that during the > next cycle people will say that they are both perfectly fine in contrib/ > and so we don't need to move anything anywhere. Indeed. But I'm down on this idea of inventing src/extensions, because then there will constantly be questions about whether FOO belongs in contrib/ or src/extensions/. Unless we just move everything there, and then the question becomes why bother. Sure, "contrib" is kind of a legacy name, but PG is full of legacy names. regards, tom lane
Re: pg_amcheck option to install extension
> On Apr 20, 2021, at 5:54 AM, Robert Haas wrote: > > On Tue, Apr 20, 2021 at 1:31 AM Mark Dilger > wrote: >> I think you are conflating the concept of an operating system adminstrator >> with the concept of the database superuser/owner. > > You should conflate those things, because there's no meaningful > privilege boundary between them: This discussion started in response to the idea that pg_amcheck needs to be moved into contrib, presumably because that's where amcheck lives. I am arguing against the move. The actual use case I have in mind is "Postgres as a service", where a company (Alice) rents space in the cloud and runs postgres databases which can be rented out to a tenant (Bob) who is the owner of his database, but not privileged on the underlying system in any way. Bob has enough privileges to run CREATE EXTENSION, but is limited to the extensions that Alice has made available. Alice evaluates packages and chooses not to install most of them, including amcheck. Or perhaps Alice chooses not to install any contrib modules. Either way, the location of amcheck in contrib is useful to Alice because it makes her choice not to install it very simple. Bob, however, is connecting to databases provided by Alice, and is not trying to limit himself. He is happy to have the pg_amcheck client installed. If Alice's databases don't allow him to run amcheck, pg_amcheck is not useful relative to those databases, but perhaps Bob is also renting database space from Charlie and Charlie's databases have amcheck installed. Now, the question is, "In which postgres package does Bob think pg_amcheck should reside?" It would be strange to say that Bob needs to install the postgresql-contrib rpm in order to get the pg_amcheck client. That rpm is mostly a bunch of modules, and may even have a package dependency on postgresql-server. Bob doesn't want either of those. He just wants the clients. The discussion about using amcheck as a privilege escalation attack was mostly to give some background for why Alice might not want to install amcheck. I think it got a bit out of hand, in no small part because I was being imprecise about Bob's exact privilege levels. I was being imprecise about that part because my argument wasn't "here's how to leverage amcheck to exploit postgres", but rather, "here's what Alice might rationally be concerned about." To run CREATE EXTENSION does not actually require superuser privileges. It depends on the package. At the moment, you can't load amcheck without superuser privileges, but you can load some others, such as intarray: bob=> create extension amcheck; 2021-04-20 07:40:46.758 PDT [80340] ERROR: permission denied to create extension "amcheck" 2021-04-20 07:40:46.758 PDT [80340] HINT: Must be superuser to create this extension. 2021-04-20 07:40:46.758 PDT [80340] STATEMENT: create extension amcheck; ERROR: permission denied to create extension "amcheck" HINT: Must be superuser to create this extension. bob=> create extension intarray; CREATE EXTENSION bob=> Alice might prefer to avoid installing amcheck altogether, not wanting to have to evaluate on each upgrade whether the privileges necessary to load amcheck have changed. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pg_amcheck option to install extension
On 4/20/21 11:09 AM, Tom Lane wrote: > Alvaro Herrera writes: >> Actually I think the best balance would be to leave things where they >> are, and move amcheck to src/extensions/ once the next devel cycle >> opens. That way, we avoid the (pretty much pointless) laborious task of >> moving pg_amcheck to contrib only to move it back on the next cycle. >> What I'm afraid of, if we move pg_amcheck to contrib, is that during the >> next cycle people will say that they are both perfectly fine in contrib/ >> and so we don't need to move anything anywhere. > Indeed. But I'm down on this idea of inventing src/extensions, > because then there will constantly be questions about whether FOO > belongs in contrib/ or src/extensions/. Unless we just move > everything there, and then the question becomes why bother. Sure, > "contrib" is kind of a legacy name, but PG is full of legacy names. > > I think the distinction I would draw is between things we would expect to be present in every Postgres installation (e.g. pg_stat_statements, auto_explain, postgres_fdw, hstore) and things we don't for one reason or another (e.g. pgcrypto, adminpack) cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: pg_amcheck option to install extension
Andrew Dunstan writes: > On 4/20/21 11:09 AM, Tom Lane wrote: >> Indeed. But I'm down on this idea of inventing src/extensions, >> because then there will constantly be questions about whether FOO >> belongs in contrib/ or src/extensions/. > I think the distinction I would draw is between things we would expect > to be present in every Postgres installation (e.g. pg_stat_statements, > auto_explain, postgres_fdw, hstore) and things we don't for one reason > or another (e.g. pgcrypto, adminpack) I dunno, that division appears quite arbitrary and endlessly bikesheddable. It's something I'd prefer not to spend time arguing about, but the only way we won't have such arguments is if we don't make the distinction in the first place. regards, tom lane
Re: pg_amcheck option to install extension
On Tue, Apr 20, 2021 at 12:05 PM Tom Lane wrote: > > I think the distinction I would draw is between things we would expect > > to be present in every Postgres installation (e.g. pg_stat_statements, > > auto_explain, postgres_fdw, hstore) and things we don't for one reason > > or another (e.g. pgcrypto, adminpack) > > I dunno, that division appears quite arbitrary and endlessly > bikesheddable. +1. I wouldn't expect those things to be present in every installation, for sure. I don't know that I've *ever* seen a customer use hstore. If I have, it wasn't often. There's no way we'll ever get consensus on which stuff people use, because it's different depending on what customers you work with. The stuff I feel bad about is stuff like 'isn' and 'earthdistance' and 'intarray', which are basically useless toys with low code quality. You'd hate for people to confuse that with stuff like 'dblink' or 'pgcrypto' which might actually be useful. But there's a big, broad fuzzy area in the middle where everyone is going to have different opinions. And even things like 'isn' and 'earthdistance' and 'intarray' may well have defenders, either because somebody thinks it's valuable as a coding example, or because somebody really did use it in anger and had success. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_amcheck option to install extension
On 4/18/21 5:58 PM, Mark Dilger wrote: > >> On Apr 18, 2021, at 6:19 AM, Andrew Dunstan wrote: >> >> how about specifying pg_catalog as the schema instead of public? > Done. > Pushed with slight changes. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Privilege boundary between sysadmin and database superuser [Was: Re: pg_amcheck option to install extension]
> On Apr 20, 2021, at 5:54 AM, Robert Haas wrote: > > On Tue, Apr 20, 2021 at 1:31 AM Mark Dilger > wrote: >> I think you are conflating the concept of an operating system adminstrator >> with the concept of the database superuser/owner. > > You should conflate those things, because there's no meaningful > privilege boundary between them: I understand why you say so, but I think the situation is more nuanced than that. > http://rhaas.blogspot.com/2020/12/cve-2019-9193.html > > If reading the whole thing is too much, scroll down to the part in > fixed-width font and behold me trivially compromising the OS account > using plperlu. I think the question here is whether PostgreSQL is inherently insecure, meaning that it cannot function unless installed in a way that would allow the database superuser Bob to compromise the OS administered by Alice. Magnus seems to object even to this formulation in his blog post, https://blog.hagander.net/when-a-vulnerability-is-not-a-vulnerability-244/, saying "a common setup is to only allow the postgres OS user itself to act as superuser, in which case there is no escalation at all." He seems to view Bob taking over the OS account as nothing more than Alice taking over her own account, since nobody but Alice should ever be able to log in as Bob. At a minimum, I think that means that Alice must trust PostgreSQL to contain zero exploits. If database user Charlie can escalate his privileges to the level of Bob, then Alice has a big problem. Assuming Alice is an average prudent system administrator, she doesn't really want to trust that PostgreSQL is completely exploit free. She just wants to quarantine it enough that she can sleep at night. I think we have made the situation for Alice a bit difficult. She needs to make sure that whichever user the backend runs as does not have permission to access anything beyond the PGDATA directory and a handful of postgres binaries, otherwise Bob, and perhaps Charlie, can access them. She can do this most easily with containers, or at least it seems so to me. The only binaries that should be executable from within the container are "postgres", "locale", and whichever hardened archive command, recovery command, and restore command Alice wants to allow. The only shell that should be executable from within the container should be minimal, maybe something custom written by Alice that only works to recognize the very limited set of commands Alice wants to allow and then forks/execs those commands without allowing any further shell magic. "Copy to program" and "copy from program" internally call popen, which calls the shell, and if Alice's custom shell doesn't offer to pipe anything to the target program, Bob can't really do anything that way. "locale -a" doesn't seem particularly vulnerable to being fed garbage, and in any event, Alice's custom shell doesn't have to implement the pipe stream logic in that direction. She could make it unidirectional from `locale -a` back to postgres. The archive, recovery, and restore commands are internally invoked using system() which calls those commands indirectly using Alice's shell. Once again, she could write the shell to not pipe anything in either direction, which pretty well prevents Bob from doing anything malicious with them. Reading and writing postgresql data files seems a much trickier problem. The "copy to file" and "copy from file" implementations don't go through the shell, and Alice can't deny the database reading or writing the data directory, so there doesn't seem to be any quarantine trick that will work. Bob can copy arbitrary malicious content to or from that directory. I don't see how this gets Bob any closer to compromising the OS account, though. All Bob is doing is messing up his own database. Even if corrupting these files convinces the postgres backend to attempt to write somewhere else in the system, the container should be sufficient to prevent it from actually succeeding outside its own data directory. The issue of the pg_read_file() sql function, and similar functions, would seem to fall into the same category as "copy to file" and "copy from file". Bob can read and write his own data directory, but not anything else, assuming Alice set up the container properly. > I actually think this is a design error on our part. A lot of people, > apparently including you, feel that there should be a privilege > boundary between the PostgreSQL superuser and the OS user, or want > such a boundary to exist. I'm arguing that the boundary does currently (almost) exist, but is violated by default, easy to further violate without realizing you are doing so, inconvenient and hard to maintain in practice, requires segregating the database superuser from whichever adminstrator(s) execute other tools, requires being paranoid when running such tools against the database because any content found therein could have
Re: Privilege boundary between sysadmin and database superuser [Was: Re: pg_amcheck option to install extension]
Mark Dilger writes: >> On Apr 20, 2021, at 5:54 AM, Robert Haas wrote: >> On Tue, Apr 20, 2021 at 1:31 AM Mark Dilger >> wrote: >>> I think you are conflating the concept of an operating system adminstrator >>> with the concept of the database superuser/owner. >> You should conflate those things, because there's no meaningful >> privilege boundary between them: > I understand why you say so, but I think the situation is more nuanced than > that. Maybe I too am confused, but I understand "operating system administrator" to mean "somebody who has root, or some elevated OS privilege level, on the box running Postgres". That is 100% distinct from the operating system user that runs Postgres, which should generally be a bog-standard OS user. (In fact, we try to prevent you from running Postgres as root.) What there is not a meaningful privilege boundary between is that standard OS user and a within-the-database superuser. Since we allow superusers to trigger file reads and writes, and indeed execute programs, from within the DB, a superuser can surely reach anything the OS user can do. The rest of your analysis seems a bit off-point to me, which is what makes me think that one of us is confused. If Alice is storing her data in a Postgres database, she had better trust both the Postgres superuser and the box's administrators ... otherwise, she should go get her own box and her own Postgres installation. regards, tom lane
Re: Privilege boundary between sysadmin and database superuser [Was: Re: pg_amcheck option to install extension]
> On Apr 20, 2021, at 3:19 PM, Tom Lane wrote: > > Mark Dilger writes: >>> On Apr 20, 2021, at 5:54 AM, Robert Haas wrote: >>> On Tue, Apr 20, 2021 at 1:31 AM Mark Dilger >>> wrote: I think you are conflating the concept of an operating system adminstrator with the concept of the database superuser/owner. > >>> You should conflate those things, because there's no meaningful >>> privilege boundary between them: > >> I understand why you say so, but I think the situation is more nuanced than >> that. > > Maybe I too am confused, but I understand "operating system administrator" > to mean "somebody who has root, or some elevated OS privilege level, on > the box running Postgres". That is 100% distinct from the operating > system user that runs Postgres, which should generally be a bog-standard > OS user. (In fact, we try to prevent you from running Postgres as root.) > > What there is not a meaningful privilege boundary between is that standard > OS user and a within-the-database superuser. Since we allow superusers to > trigger file reads and writes, and indeed execute programs, from within > the DB, a superuser can surely reach anything the OS user can do. Right. This is the part that Alice might want to restrict, and we don't have an easy way for her to do so. > The rest of your analysis seems a bit off-point to me, which is what > makes me think that one of us is confused. If Alice is storing her > data in a Postgres database, she had better trust both the Postgres > superuser and the box's administrators ... otherwise, she should go > get her own box and her own Postgres installation. It is the other way around. Alice is the operating system administrator who doesn't trust Bob. She wants Bob to be able to do any database thing he wants within the PostgreSQL environment, but doesn't want that to leak out as an ability to run arbitrary stuff on the system, not even if it's just stuff running as bog-standard user "postgres". In my view, Alice can accomplish this goal using a very tightly designed container, but it is far from easy to do and to get right. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Privilege boundary between sysadmin and database superuser [Was: Re: pg_amcheck option to install extension]
Greetings, * Mark Dilger (mark.dil...@enterprisedb.com) wrote: > > On Apr 20, 2021, at 3:19 PM, Tom Lane wrote: > > The rest of your analysis seems a bit off-point to me, which is what > > makes me think that one of us is confused. If Alice is storing her > > data in a Postgres database, she had better trust both the Postgres > > superuser and the box's administrators ... otherwise, she should go > > get her own box and her own Postgres installation. > > It is the other way around. Alice is the operating system administrator who > doesn't trust Bob. She wants Bob to be able to do any database thing he > wants within the PostgreSQL environment, but doesn't want that to leak out as > an ability to run arbitrary stuff on the system, not even if it's just stuff > running as bog-standard user "postgres". In my view, Alice can accomplish > this goal using a very tightly designed container, but it is far from easy to > do and to get right. Then Bob doesn't get to be a superuser. There's certainly some capabilities that aren't able to be GRANT'd out today and which are reserved for superusers, but there's been ongoing work to improve on that situation (pg_read_all_data being one of the recent improvements in this area, in fact...). Certainly, if others are interested in that then it'd be great to have more folks working to improve the situation. We do need to make it clear when a given capability isn't intended to allow a user who has that capability to be able to become a superuser and when the capability itself means that they would be able to. The predefined role 'pg_execute_server_program' grants out the capability to execute programs on the server, which both allows a user to become a superuser if they wished, and goes against your stated requirement that Bob isn't allowed to do that, so that predefined role shouldn't be GRANT'd to Bob. The question is: what do you wish Bob could do, as a non-superuser, that Bob isn't able to do today? Assuming that there's a set of capabilities there that both wouldn't allow Bob to become a superuser (which implies that Bob can't do things like execute arbitrary programs or read/write arbitrary files on the server) and which would allow Bob to perform the actions you'd like Bob to be able to do, it's mostly a matter of programming to make it happen... Thanks, Stephen signature.asc Description: PGP signature
