RFC: Additional Directory for Extensions

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 this path 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 the person 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 whatever the 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 whatever the 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 just those 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 the extension directory, where it would be loaded in preference to the core libpgtypes library, if they couldn’t exploit the 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.

This would also allow the lookup of extension libraries prefixed by the directory field from the control file, which would enable much tidier extension installation: The control file, SQL scripts, and DSOs could all be in a single directory for an extension.

Thoughts?

Best,

David

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 whatever the 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 just those 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 the extension directory, where it would be loaded in preference to the core libpgtypes library, if they couldn’t exploit the 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 would enable much tidier extension installation: The control file, SQL scripts, and DSOs could all be in a single directory for 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 have to 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 would enable much tidier extension installation: The control file, SQL scripts, and DSOs could all be in a single directory for 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 was hoping to find a path where it knows it’s opening a DSO for the purpose of an extension, so we could limit the lookup there. 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 SHAREDIR to 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 because we need to really think it through?

I’ve added some docs based on your GUC description; updated patch attached.

Best,

David

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 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]https://justatheory.com/2024/02/extension-ecosystem-summit/#extension-ecosystem-mini-summit, so I’m sure I could get some collaborators to make improvements or look at a different approach.

Best,

David

[1]: https://justatheory.com/2024/02/extension-ecosystem-summit/#extension-ecosystem-mini-summit

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 postmaster what directories to read files from. It feels a little outside the scope of a regular user to even be aware of the 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 this patch 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 and any other SHAREDIR files an extension might include. But also, I think it should support all the pg_config installation targets 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 postmaster what directories to read files from. It feels a little outside the scope of a regular user to even be aware of the 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 and any other SHAREDIR files an extension might include. But also, I think it should support all the pg_config installation targets 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 puts items 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]https://justatheory.com/2024/06/trunk-poc/ and its POC[2] around them.

Best,

David

[1]: https://justatheory.com/2024/06/trunk-poc/
[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 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.

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 (PGXS puts items 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.

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)