Thread: [MASSMAIL]RFC: Additional Directory for Extensions
Hackers, In the Security lessons from liblzma thread[1], walther broached the subject of an extension directory path[1]: > Also a configurable directoy to look up extensions, possibly even to be > changed at run-time like [2]. The patch says this: > >> This directory is prepended to paths when loading extensions (control and SQL files), and to the '$libdir' directive whenloading modules that back functions. The location is made configurable to allow build-time testing of extensions thatdo not have been installed to their proper location yet. > > This seems like a great thing to have. This might also be relevant in > light of recent discussions in the ecosystem around extension management. That quotation comes from this Debian patch[2] maintained by Christoph Berg. I’d like to formally propose integrating thispatch into the core. And not only because it’s overhead for package maintainers like Christoph, but because a numberof use cases have emerged since we originally discussed something like this back in 2013[3]: Docker Immutability ------------------- Docker images must be immutable. In order for users of a Docker image to install extensions that persist, they must createa persistent volume, map it to SHAREDIR/extensions, and copy over all the core extensions (or muck with symlink magic[4]).This makes upgrades trickier, because the core extensions are mixed in with third party extensions. By supporting a second directory pretended to the list of directories to search, as the Debian patch does, users of Dockerimages can keep extensions they install separate from core extensions, in a directory mounted to a persistent volumewith none of the core extensions. Along with tweaking dynamic_library_path to support additional directories for sharedobject libraries, which can also be mounted to a separate path, we can have a persistent and clean separation of immutablecore extensions and extensions installed at runtime. Postgres.app ------------ The Postgres.app project also supports installing extensions. However, because they must go into the SHAREDIR/extensions,once a user installs one the package has been modified and the Apple bundle signature will be broken.The OS will no longer be able to validate that the app is legit. If the core supported an additional extension (and PKGLIBDIR), it would allow an immutable PostgreSQL base package and stillallow extensions to be installed into directories outside the app bundle, and thus preserve bundle signing on macOS(and presumably other systems --- is this the nix issue, too?) RFC --- I know there was some objection to changes like this in the past, but the support I’m seeing in the liblzma thread for makingpkglibdir configurable me optimistic that this might be the right time to support additional configuration for theextension directory, as well, starting with the Debian patch, perhaps. Thoughts? I would be happy to submit a clean version of the Debian patch[2]. Best, David [1] https://www.postgresql.org/message-id/99c41b46-616e-49d0-9ffd-a29432cec818%40technowledgy.de [2] https://salsa.debian.org/postgresql/postgresql/-/blob/17/debian/patches/extension_destdir?ref_type=heads [3] https://www.postgresql.org/message-id/flat/51AE0845.8010600@ocharles.org.uk [4] https://speakerdeck.com/ongres/postgres-extensions-in-kubernetes?slide=14
On 2024-Apr-02, David E. Wheeler wrote: > That quotation comes from this Debian patch[2] maintained by Christoph > Berg. I’d like to formally propose integrating this patch into the > core. And not only because it’s overhead for package maintainers like > Christoph, but because a number of use cases have emerged since we > originally discussed something like this back in 2013[3]: I support the idea of there being a second location from where to load shared libraries ... but I don't like the idea of making it runtime-configurable. If we want to continue to tighten up what superuser can do, then one of the things that has to go away is the ability to load shared libraries from arbitrary locations (dynamic_library_path). I think we should instead look at making those locations hardcoded at compile time. The packager can then decide where those things go, and the superuser no longer has the ability to load arbitrary code from arbitrary locations. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ Al principio era UNIX, y UNIX habló y dijo: "Hello world\n". No dijo "Hello New Jersey\n", ni "Hello USA\n".
Alvaro Herrera: > I support the idea of there being a second location from where to load > shared libraries ... but I don't like the idea of making it > runtime-configurable. If we want to continue to tighten up what > superuser can do, then one of the things that has to go away is the > ability to load shared libraries from arbitrary locations > (dynamic_library_path). I think we should instead look at making those > locations hardcoded at compile time. The packager can then decide where > those things go, and the superuser no longer has the ability to load > arbitrary code from arbitrary locations. The use-case for runtime configuration of this seems to be build-time testing of extensions against an already installed server. For this purpose it should be enough to be able to set this directory at startup - it doesn't need to be changed while the server is actually running. Then you could spin up a temporary postgres instance with the extension directory pointing a the build directory and test. Would startup-configurable be better than runtime-configurable regarding your concerns? I can also imagine that it would be very helpful in a container setup to be able to set an environment variable with this path instead of having to recompile all of postgres to change it. Best, Wolfgang
> On 3 Apr 2024, at 09:13, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2024-Apr-02, David E. Wheeler wrote: > >> That quotation comes from this Debian patch[2] maintained by Christoph >> Berg. I’d like to formally propose integrating this patch into the >> core. And not only because it’s overhead for package maintainers like >> Christoph, but because a number of use cases have emerged since we >> originally discussed something like this back in 2013[3]: > > I support the idea of there being a second location from where to load > shared libraries Agreed, the case made upthread that installing an extension breaks the app signing seems like a compelling reason to do this. The implementation of this need to make sure the directory is properly set up however to avoid similar problems that CVE 2019-10211 showed. -- Daniel Gustafsson
On Apr 3, 2024, at 3:57 AM, walther@technowledgy.de wrote: > I can also imagine that it would be very helpful in a container setup to be able to set an environment variable with thispath instead of having to recompile all of postgres to change it. Yes, I like the suggestion to make it require a restart, which lets the sysadmin control it and not limited to whatever theperson who compiled it thought would make sense. Best, David
On Apr 3, 2024, at 8:54 AM, David E. Wheeler <david@justatheory.com> wrote: > Yes, I like the suggestion to make it require a restart, which lets the sysadmin control it and not limited to whateverthe person who compiled it thought would make sense. Or SIGHUP? D
On Apr 3, 2024, at 8:54 AM, David E. Wheeler <david@justatheory.com> wrote: > Yes, I like the suggestion to make it require a restart, which lets the sysadmin control it and not limited to whateverthe person who compiled it thought would make sense. Here’s a revision of the Debian patch that requires a server start. However, in studying the patch, it appears that the `extension_directory` is searched for *all* shared libraries, not justthose being loaded for an extension. Am I reading the `expand_dynamic_library_name()` function right? If so, this seems like a good way for a bad actor to muck with things, by putting an exploited libpgtypes library into theextension directory, where it would be loaded in preference to the core libpgtypes library, if they couldn’t exploit theoriginal. I’m thinking it would be better to have the dynamic library lookup for extension libraries (and LOAD libraries?) separate,so that the `extension_directory` would not be used for core libraries. This would also allow the lookup of extension libraries prefixed by the directory field from the control file, which wouldenable much tidier extension installation: The control file, SQL scripts, and DSOs could all be in a single directoryfor an extension. Thoughts? Best, David
Attachment
Re: David E. Wheeler > > Yes, I like the suggestion to make it require a restart, which lets the sysadmin control it and not limited to whateverthe person who compiled it thought would make sense. > > Here’s a revision of the Debian patch that requires a server start. Thanks for bringing this up, I should have submitted this years ago. (The patch is originally from September 2020.) I designed the patch to require a superuser to set it, so it doesn't matter very much by which mechanism it gets updated. There should be little reason to vary it at run-time, so I'd be fine with requiring a restart, but otoh, why restrict the superuser from reloading it if they know what they are doing? > However, in studying the patch, it appears that the `extension_directory` is searched for *all* shared libraries, not justthose being loaded for an extension. Am I reading the `expand_dynamic_library_name()` function right? > > If so, this seems like a good way for a bad actor to muck with things, by putting an exploited libpgtypes library intothe extension directory, where it would be loaded in preference to the core libpgtypes library, if they couldn’t exploitthe original. > > I’m thinking it would be better to have the dynamic library lookup for extension libraries (and LOAD libraries?) separate,so that the `extension_directory` would not be used for core libraries. I'm not sure the concept of "core libraries" exists. PG happens to dlopen things at run time, and it doesn't know/care if they were installed by users or by the original PG server. Also, an exploited libpgtypes library is not worse than any other untrusted "user" library, so you really don't want to allow users to provide their own .so files, no matter by what mechanism. > This would also allow the lookup of extension libraries prefixed by the directory field from the control file, which wouldenable much tidier extension installation: The control file, SQL scripts, and DSOs could all be in a single directoryfor an extension. Nice idea, but that would mean installing .so files into PGSHAREDIR. Perhaps the whole extension stuff would have to move to PKGLIBDIR instead. Fwiw, I wrote this patch to solve the problem of testing extensions at build-time where the build process does not have write access to PGSHAREDIR. It solves that problem quite well, almost all PG extensions have build-time test coverage now (where there was basically 0 before). Security is not a concern at this point as everything is running as the same user, and the test cluster will be wiped right after the test. I figured marking the setting as "super user" only was enough security at that point, but I would recommend another audit before using it together with "trusted" extensions and other things in production. Christoph
> Fwiw, I wrote this patch to solve the problem of testing extensions at > build-time where the build process does not have write access to > PGSHAREDIR. It solves that problem quite well, almost all PG > extensions have build-time test coverage now (where there was > basically 0 before). Also, it's called extension "destdir" because it behaves like DESTDIR in Makefiles: It prepends the given path to the path that PG is trying to open when set. So it doesn't allow arbitrary new locations as of now, just /home/build/foo-1/debian/foo/usr/share/postgresql/17/extension in addition to /usr/share/postgresql/17/extension. (That is what the Debian package build process needs, so that restriction/design choice made sense.) > Security is not a concern at this point as everything is running as > the same user, and the test cluster will be wiped right after the > test. I figured marking the setting as "super user" only was enough > security at that point, but I would recommend another audit before > using it together with "trusted" extensions and other things in > production. That's also included in the current GUC description: This directory is prepended to paths when loading extensions (control and SQL files), and to the '$libdir' directive when loading modules that back functions. The location is made configurable to allow build-time testing of extensions that do not have been installed to their proper location yet. Perhaps I should have included a more verbose "NOT FOR PRODUCTION" there. As for compatibility, the patch has been part of the PG 9.5..17 now for several years, and I'm very happy with extra test coverage it provides, especially on the Debian architectures that don't have "autopkgtest" runners yet. Christoph
On Apr 3, 2024, at 12:46 PM, Christoph Berg <myon@debian.org> wrote: > Thanks for bringing this up, I should have submitted this years ago. > (The patch is originally from September 2020.) That’s okay, it’s still 2020 in some ways. 😂 > I designed the patch to require a superuser to set it, so it doesn't > matter very much by which mechanism it gets updated. There should be > little reason to vary it at run-time, so I'd be fine with requiring a > restart, but otoh, why restrict the superuser from reloading it if > they know what they are doing? I think that’s fair. I’ll keep it requiring a restart now, on the theory it would be easier to loosen it later than haveto tighten it later. > I'm not sure the concept of "core libraries" exists. PG happens to > dlopen things at run time, and it doesn't know/care if they were > installed by users or by the original PG server. Also, an exploited > libpgtypes library is not worse than any other untrusted "user" > library, so you really don't want to allow users to provide their own > .so files, no matter by what mechanism. Yes, I guess my concern is whether it could be used to “shadow” core libraries. Maybe it’s no different, really. >> This would also allow the lookup of extension libraries prefixed by the directory field from the control file, which wouldenable much tidier extension installation: The control file, SQL scripts, and DSOs could all be in a single directoryfor an extension. > > Nice idea, but that would mean installing .so files into PGSHAREDIR. > Perhaps the whole extension stuff would have to move to PKGLIBDIR > instead. Yes, I was just poking around the code, and realized that, when extension functions are created they may or may not not use`MODULE_PATHNAME`, but in any event, there is nothing different about loading an extension DSO than any other DSO. I washoping to find a path where it knows it’s opening a DSO for the purpose of an extension, so we could limit the lookupthere. But that does not (currently) exist. Maybe we could add an `$extensiondir` variable to complement `$libdir`? Or is PGKLIBDIR is the way to go? I’m not familiar with it. It looks like extension JIT files are put there already. > Fwiw, I wrote this patch to solve the problem of testing extensions at > build-time where the build process does not have write access to > PGSHAREDIR. It solves that problem quite well, almost all PG > extensions have build-time test coverage now (where there was > basically 0 before). Yeah, good additional use case. On Apr 3, 2024, at 1:03 PM, Christoph Berg <myon@debian.org> wrote: > Also, it's called extension "destdir" because it behaves like DESTDIR > in Makefiles: It prepends the given path to the path that PG is trying > to open when set. So it doesn't allow arbitrary new locations as of > now, just /home/build/foo-1/debian/foo/usr/share/postgresql/17/extension > in addition to /usr/share/postgresql/17/extension. (That is what the > Debian package build process needs, so that restriction/design choice > made sense. Right, this makes perfect sense, in that you don’t have to copy all the extension files from the destdir to the SHAREDIRto test them, which I imagine could be a PITA. > That's also included in the current GUC description: > > This directory is prepended to paths when loading extensions > (control and SQL files), and to the '$libdir' directive when > loading modules that back functions. The location is made > configurable to allow build-time testing of extensions that do not > have been installed to their proper location yet. > > Perhaps I should have included a more verbose "NOT FOR PRODUCTION" > there. The use cases I described upthread are very much production use cases. Do you think it’s not for production just becausewe need to really think it through? I’ve added some docs based on your GUC description; updated patch attached. Best, David
Attachment
On Apr 4, 2024, at 1:20 PM, David E. Wheeler <david@justatheory.com> wrote: > I’ve added some docs based on your GUC description; updated patch attached. Here’s a rebase. I realize this probably isn’t going to happen for 17, given the freeze, but I would very much welcome feedback and pointersto address concerns about providing a second directory for extensions and DSOs. Quite a few people have talked aboutthe need for this in the Extension Mini Summits[1], so I’m sure I could get some collaborators to make improvementsor look at a different approach. Best, David [1] https://justatheory.com/2024/02/extension-ecosystem-summit/#extension-ecosystem-mini-summit
Attachment
On Thu, Apr 11, 2024 at 01:52:26PM -0400, David E. Wheeler wrote: > I realize this probably isn´t going to happen for 17, given the freeze, > but I would very much welcome feedback and pointers to address concerns > about providing a second directory for extensions and DSOs. Quite a few > people have talked about the need for this in the Extension Mini > Summits[1], so I´m sure I could get some collaborators to make > improvements or look at a different approach. At first glance, the general idea seems reasonable to me. I'm wondering whether there is a requirement for this directory to be prepended or if it could be appended to the end. That way, the existing ones would take priority, which might be desirable from a security standpoint. -- nathan
On Wed, Apr 3, 2024 at 3:13 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > I support the idea of there being a second location from where to load > shared libraries ... but I don't like the idea of making it > runtime-configurable. If we want to continue to tighten up what > superuser can do, then one of the things that has to go away is the > ability to load shared libraries from arbitrary locations > (dynamic_library_path). I think we should instead look at making those > locations hardcoded at compile time. The packager can then decide where > those things go, and the superuser no longer has the ability to load > arbitrary code from arbitrary locations. Is "tighten up what the superuser can do" on our list of objectives? Personally, I think we should be focusing mostly, and maybe entirely, on letting non-superusers do more things, with appropriate security controls. The superuser can ultimately do anything, since they can cause shell commands to be run and load arbitrary code into the backend and write code in untrusted procedural languages and mutilate the system catalogs and lots of other terrible things. Now, I think there are environments where people have used things like containers to try to lock down the superuser, and while I'm not sure that can ever be particularly water-tight, if it were the case that this patch would make it a whole lot easier for a superuser to bypass the kinds of controls that people are imposing today, that might be an argument against this patch. But ... off-hand, I'm not seeing such an exposure. On the patch itself, I find the documentation for this to be fairly hard to understand. I think it could benefit from an example. I'm confused about whether this is intended to let me search for extensions in /my/temp/root/usr/lib/postgresql/... by setting extension_directory=/my/temp/dir, or whether it's intended me to search both /usr/lib/postgresql as I normally would and also /some/other/place. If the latter, I wonder why we don't handle shared libraries by setting dynamic_library_path and then just have an analogue of that for control files. -- Robert Haas EDB: http://www.enterprisedb.com
On Jun 24, 2024, at 1:53 PM, Robert Haas <robertmhaas@gmail.com> wrote: > Is "tighten up what the superuser can do" on our list of objectives? > Personally, I think we should be focusing mostly, and maybe entirely, > on letting non-superusers do more things, with appropriate security > controls. The superuser can ultimately do anything, since they can > cause shell commands to be run and load arbitrary code into the > backend and write code in untrusted procedural languages and mutilate > the system catalogs and lots of other terrible things. I guess the question then is what security controls are appropriate for this feature, which after all tells the postmasterwhat directories to read files from. It feels a little outside the scope of a regular user to even be aware ofthe file system undergirding the service. But perhaps there’s a non-superuser role for whom it is appropriate? > Now, I think there are environments where people have used things like > containers to try to lock down the superuser, and while I'm not sure > that can ever be particularly water-tight, if it were the case that > this patch would make it a whole lot easier for a superuser to bypass > the kinds of controls that people are imposing today, that might be an > argument against this patch. But ... off-hand, I'm not seeing such an > exposure. Yeah I’m not even sure I follow. Containers are immutable, other than mutable mounted volumes --- which is one use case thispatch is attempting to enable. > On the patch itself, I find the documentation for this to be fairly > hard to understand. I think it could benefit from an example. I'm > confused about whether this is intended to let me search for > extensions in /my/temp/root/usr/lib/postgresql/... by setting > extension_directory=/my/temp/dir, or whether it's intended me to > search both /usr/lib/postgresql as I normally would and also > /some/other/place. I sketched them quickly, so agree they can be better. Reading the code, I now see that it appears to be the former case.I’d like to advocate for the latter. > If the latter, I wonder why we don't handle shared > libraries by setting dynamic_library_path and then just have an > analogue of that for control files. The challenge is that it applies not just to shared object libraries and control files, but also extension SQL files andany other SHAREDIR files an extension might include. But also, I think it should support all the pg_config installationtargets that extensions might use, including: BINDIR DOCDIR HTMLDIR PKGINCLUDEDIR LOCALEDIR MANDIR I can imagine an extension wanting or needing to use any and all of these. Best, David
On Mon, Jun 24, 2024 at 3:37 PM David E. Wheeler <david@justatheory.com> wrote: > I guess the question then is what security controls are appropriate for this feature, which after all tells the postmasterwhat directories to read files from. It feels a little outside the scope of a regular user to even be aware ofthe file system undergirding the service. But perhaps there’s a non-superuser role for whom it is appropriate? As long as the GUC is superuser-only, I'm not sure what else there is to do here. The only question is whether there's some reason to disallow this even from the superuser, but I'm not quite seeing such a reason. > > On the patch itself, I find the documentation for this to be fairly > > hard to understand. I think it could benefit from an example. I'm > > confused about whether this is intended to let me search for > > extensions in /my/temp/root/usr/lib/postgresql/... by setting > > extension_directory=/my/temp/dir, or whether it's intended me to > > search both /usr/lib/postgresql as I normally would and also > > /some/other/place. > > I sketched them quickly, so agree they can be better. Reading the code, I now see that it appears to be the former case.I’d like to advocate for the latter. Sounds good. > > If the latter, I wonder why we don't handle shared > > libraries by setting dynamic_library_path and then just have an > > analogue of that for control files. > > The challenge is that it applies not just to shared object libraries and control files, but also extension SQL files andany other SHAREDIR files an extension might include. But also, I think it should support all the pg_config installationtargets that extensions might use, including: > > BINDIR > DOCDIR > HTMLDIR > PKGINCLUDEDIR > LOCALEDIR > MANDIR > > I can imagine an extension wanting or needing to use any and all of these. Are these really all relevant to backend code? -- Robert Haas EDB: http://www.enterprisedb.com
On Jun 24, 2024, at 4:28 PM, Robert Haas <robertmhaas@gmail.com> wrote: > As long as the GUC is superuser-only, I'm not sure what else there is > to do here. The only question is whether there's some reason to > disallow this even from the superuser, but I'm not quite seeing such a > reason. I can switch it back from requiring a restart to allowing a superuser to set it. >> I sketched them quickly, so agree they can be better. Reading the code, I now see that it appears to be the former case.I’d like to advocate for the latter. > > Sounds good. Yeah, though then I have a harder time deciding how it should work. pg_config’s paths are absolute. With your first example,we just use them exactly as they are, but prefix them with the destination directory. So if it’s set to `/my/temp/root/`,then files go into /my/temp/root/$(pg_conifg --sharedir) /my/temp/root/$(pg_conifg --pkglibdir) /my/temp/root/$(pg_conifg --bindir) # etc. Which is exactly how RPM and Apt packages are built, but seems like an odd configuration for general use. >> BINDIR >> DOCDIR >> HTMLDIR >> PKGINCLUDEDIR >> LOCALEDIR >> MANDIR >> >> I can imagine an extension wanting or needing to use any and all of these. > > Are these really all relevant to backend code? Oh I think so. Especially BINDIR; lots of extensions ship with binary applications. And most ship with docs, too (PGXS putsitems listed in DOCS into DOCDIR). Some might also produce man pages (for their binaries), HTML docs, and other stuff.Maybe an FTE extension would include locale files? I find it pretty easy to imagine use cases for all of them. So much so that I wrote an extension binary distribution RFC[1]and its POC[2] around them. Best, David [1]: https://github.com/orgs/pgxn/discussions/2 [1]: https://justatheory.com/2024/06/trunk-poc/
On Thu, 11 Apr 2024 at 19:52, David E. Wheeler <david@justatheory.com> wrote: > I realize this probably isn’t going to happen for 17, given the freeze, but I would very much welcome feedback and pointersto address concerns about providing a second directory for extensions and DSOs. Quite a few people have talked aboutthe need for this in the Extension Mini Summits[1], so I’m sure I could get some collaborators to make improvementsor look at a different approach. Overall +1 for the idea. We're running into this same limitation (only a single place to put extension files) at Microsoft at the moment. + and to the '$libdir' directive when loading modules + that back functions. I feel like this is a bit strange. Either its impact is too wide, or it's not wide enough depending on your intent. If you want to only change $libdir during CREATE EXTENSION (or ALTER EXTENSION UPDATE), then why not just change it there. And really you'd only want to change it when creating an extension from which the control file is coming from extension_destdir. However, I can also see a case for really always changing $libdir. Because some extensions in shared_preload_libraries, might want to trigger loading other libraries that they ship with dynamically. And these libraries are probably also in extension_destdir.
On Mon, 24 Jun 2024 at 18:11, Nathan Bossart <nathandbossart@gmail.com> wrote: > At first glance, the general idea seems reasonable to me. I'm wondering > whether there is a requirement for this directory to be prepended or if it > could be appended to the end. That way, the existing ones would take > priority, which might be desirable from a security standpoint. Citus does ship with some override library for pgoutput to make logical replication/CDC work correctly with sharded tables. Right now using this override library requires changing dynamic_library_path. It would be nice if that wasn't necessary. But this is obviously a small thing. And I definitely agree that there's a security angle to this as well, but honestly that seems rather small too. If an attacker can put shared libraries into the extension_destdir, I'm pretty sure you've lost already, no matter if extension_destdir is prepended or appended to the existing $libdir.
On Jun 24, 2024, at 17:17, Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > If you want to only change $libdir during CREATE EXTENSION (or ALTER > EXTENSION UPDATE), then why not just change it there. And really you'd > only want to change it when creating an extension from which the > control file is coming from extension_destdir. IIUC, the postmaster needs to load an extension on first use in every session unless it’s in shared_preload_libraries. > However, I can also see a case for really always changing $libdir. > Because some extensions in shared_preload_libraries, might want to > trigger loading other libraries that they ship with dynamically. And > these libraries are probably also in extension_destdir. Right, it can be more than just the DSOs for the extension itself. Best, David
On Mon, 24 Jun 2024 at 22:42, David E. Wheeler <david@justatheory.com> wrote: > >> BINDIR > >> DOCDIR > >> HTMLDIR > >> PKGINCLUDEDIR > >> LOCALEDIR > >> MANDIR > >> > >> I can imagine an extension wanting or needing to use any and all of these. > > > > Are these really all relevant to backend code? > > Oh I think so. Especially BINDIR; lots of extensions ship with binary applications. And most ship with docs, too (PGXSputs items listed in DOCS into DOCDIR). Some might also produce man pages (for their binaries), HTML docs, and otherstuff. Maybe an FTE extension would include locale files? > > I find it pretty easy to imagine use cases for all of them. So much so that I wrote an extension binary distribution RFC[1]and its POC[2] around them. Definitely agreed on BINDIR needing to be supported. And while lots of extensions ship with docs, I expect this feature to mostly be used in production environments to make deploying extensions easier. And I'm not sure that many people care about deploying docs to production (honestly lots of people would probably want to strip them). Still, for the sake of completeness it might make sense to support this whole list in extension_destdir. (assuming it's easy to do)
Re: Nathan Bossart > At first glance, the general idea seems reasonable to me. I'm wondering > whether there is a requirement for this directory to be prepended or if it > could be appended to the end. That way, the existing ones would take > priority, which might be desirable from a security standpoint. My use case for this is to test things at compile time (where I can't write to /usr/share/postgresql/). If installed things would take priority over the things that I'm trying to test, I'd be disappointed. Christoph
On 2024-Jun-24, Robert Haas wrote: > Is "tighten up what the superuser can do" on our list of objectives? > Personally, I think we should be focusing mostly, and maybe entirely, > on letting non-superusers do more things, with appropriate security > controls. The superuser can ultimately do anything, since they can > cause shell commands to be run and load arbitrary code into the > backend and write code in untrusted procedural languages and mutilate > the system catalogs and lots of other terrible things. I don't agree that we should focus _solely_ on allowing non-superusers to do more things. Sure, it's a good thing to do -- but we shouldn't completely close the option of securing superuser itself. I think it's not completely impossible to have a future where superuser is just so within the database, i.e. that it can't escape to the operating system. I'm sure that would be useful in many environments. On this list, many people frequently make the argument that it is impossible to secure, but I'm not convinced. They can mutilate the system catalogs: yes, they can TRUNCATE pg_type. So what? They've just destroyed their own ability to do anything else. The real issue here is that they can edit pg_proc to cause SQL function calls to call arbitrary code. But what if we limit functions so that the C code that they can call is located in specific places that are known to only contain secure code? This is easy: make sure the OS-installation only contains safe code in $libdir. I hear you say: ah, but they can modify dynamic_library_path, which is a GUC, to load code from anywhere -- especially /tmp, where the newest bitcoin-mining library was just written. This is true. I suggest, to solve this problem, that we should make dynamic_library_path no longer a GUC. It should be a setting that comes from a different origin, one that even superuser cannot write to. Only the OS-installation can modify that file; that way, superuser cannot load arbitrary code that way. This is where the new GUC setting being proposed in this thread rubs me the wrong way: it's adding yet another avenue for this to be exploited. I would like this new directory not to be a GUC either, just like dynamic_library_path. I hear you argue: ah, but they can use COPY to write a new file to $libdir. Yes, they can, and I think that's foolish. We could have another non-GUC setting which takes a list of directories where COPY can write files into. Problem solved. Do people really need the ability to write files on arbitrary locations? Untrusted extensions: well, just don't have those in the OS-installation and you'll be fine. I'm okay with saying that a superuser-restricted system is incompatible with plpython. archive_command and so on: we could disable these too. Nathan did some work to implement those using dynamic libraries, so it shouldn't be too much of a loss; anything that is done with a shell script can also be done with a small library. Those libraries can be made safe. If there are other ways to invoke shell commands from GUCs, let's add the ability to use libraries for those too. What other exploits do we know about? How can we close them? Now, I'm not saying that this is an easy journey. But if we don't start, we're not going to get there. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Doing what he did amounts to sticking his fingers under the hood of the implementation; if he gets his fingers burnt, it's his problem." (Tom Lane)
On Tue, Jun 25, 2024 at 6:12 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Now, I'm not saying that this is an easy journey. But if we don't > start, we're not going to get there. I actually kind of agree with you. I think I've said something similar in a previous email to the list somewhere. But I don't agree that this patch should be burdened with taking the first step. We seem to often find reasons why patches that packagers for prominent distributions are carrying shouldn't be put into core, and I think that's a bad habit. They're not going to stop applying those packages because we refuse to put suitable functionality in core; we're just creating a situation where lots of people are running slightly patched versions of PostgreSQL instead of straight-up PostgreSQL. That's not improving anything. If we want to work on making the sorts of changes that you're proposing, let's do it on a separate thread. It's not going to be meaningfully harder to move in that direction after some patch like this than it is today. -- Robert Haas EDB: http://www.enterprisedb.com
On Jun 24, 2024, at 5:32 PM, Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > Still, for the sake of completeness it might make sense to support > this whole list in extension_destdir. (assuming it's easy to do) It should be with the current patch, which just uses a prefix to paths in `pg_config`. So if SHAREDIR is set to /usr/share/postgresql/16and extension_destdir is set to /mount/ext, then Postgres will look for files in /mount/ext/usr/share/postgresql/16.The same rule applies (or should apply) for all other pg_config directory configs andwhere the postmaster looks for specific files. And PGXS already supports installing files in these locations, thanks toits DESTDIR param. (I don’t know how it works on Windows, though.) That said, this is very much a pattern designed for RPM and Debian package management patterns, and not for actually installingand managing extensions. And maybe that’s fine for now, as it can still be used to address the immutability problemsdescried in the original post in this thread. Ultimately, I’d like to figure out a way to more tidily organize installed extension files, but I think that, too, mightbe a separate thread. Best, David
On Jun 25, 2024, at 10:43 AM, Robert Haas <robertmhaas@gmail.com> wrote: > If we want to work on making the sorts of changes that > you're proposing, let's do it on a separate thread. It's not going to > be meaningfully harder to move in that direction after some patch like > this than it is today. I appreciate this separation of concerns, Robert. In other news, here’s an updated patch that expands the documentation to record that the destination directory is a prefix,and full paths should be used under it. Also take the opportunity to document the PGXS DESTDIR variable as the thingto use to install files under the destination directory. It still requires a server restart; I can change it back to superuser-only if that’s the consensus. For those who prefer a GitHub patch review experience, see this PR: https://github.com/theory/postgres/pull/3/files Best, David
Attachment
On Wed, 26 Jun 2024 at 00:32, David E. Wheeler <david@justatheory.com> wrote:
> In other news, here’s an updated patch that expands the documentation to record that the destination directory is a
prefix,and full paths should be used under it. Also take the opportunity to document the PGXS DESTDIR variable as the
thingto use to install files under the destination directory.
Docs are much clearer now thanks.
full = substitute_libpath_macro(name);
+ /*
+ * If extension_destdir is set, try to find the file there first
+ */
+ if (*extension_destdir != '\0')
+ {
+ full2 = psprintf("%s%s", extension_destdir, full);
+ if (pg_file_exists(full2))
+ {
+ pfree(full);
+ return full2;
+ }
+ pfree(full2);
+ }
I think this should be done differently. For two reasons:
1. I don't think extension_destdir should be searched when $libdir is
not part of the name.
2. find_in_dynamic_libpath currently doesn't use extension_destdir at
all, so if there is no slash in the filename we do not search
extension_destdir.
I feel like changing the substitute_libpath_macro function a bit (or
adding a new similar function) is probably the best way to achieve
that.
We should also check somewhere (probably GUC check hook) that
extension_destdir is an absolute path.
> It still requires a server restart;
When reading the code I see no reason why this cannot be PGC_SIGHUP.
Even though it's probably not needed to change on a running server, I
think it's better to allow that. Even just so people can disable it if
necessary for some reason without restarting the process.
> I can change it back to superuser-only if that’s the consensus.
It still is GUC_SUPERUSER_ONLY, right?
> For those who prefer a GitHub patch review experience, see this PR:
>
> https://github.com/theory/postgres/pull/3/files
Sidenote: The "D" link for each patch on cfbot[1] now gives a similar
link for all commitfest entries[2].
[1]: http://cfbot.cputube.org/
[2]: https://github.com/postgresql-cfbot/postgresql/compare/cf/4913~1...cf/4913
On Tue, 25 Jun 2024 at 19:33, David E. Wheeler <david@justatheory.com> wrote: > > On Jun 24, 2024, at 5:32 PM, Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > > > Still, for the sake of completeness it might make sense to support > > this whole list in extension_destdir. (assuming it's easy to do) > > It should be with the current patch, which just uses a prefix to paths in `pg_config`. Ah alright, I think it confused me because I never saw bindir being used. But as it turns out the current backend code never uses bindir. So that makes sense. I guess to actually use the binaries from the extension_destdir/$BINDIR the operator needs to set PATH accordingly, or the extension needs to be changed to support extension_destdir. It might be nice to add a helper function to find binaries in BINDIR, now that the resolution logic is more complex. Even if postgres itself doesn't use it. That would make it easier for extensions to be modified to support extension_distdir. Something like find_bindir_executable(char *name)