[musl] [PATCH] ldso/dynlink: Protect LD_ env vars from getting clobbered by apps

[musl] [PATCH] ldso/dynlink: Protect LD_ env vars from getting clobbered by apps

[Elvis Pranskevichus]

There is no guarantee that the environment block will remain intact.
For example, PostgreSQL clobbers argv/environ area to implement its
"setproctitle" emulation on non-BSD [1], and there is a popular Python
library inspired by it [2].  As a result, setting `LD_LIBRARY_PATH`
or `LD_PRELOAD` has no effect on Postgres subprocesses when linking
against musl.

Protect against this by making a copies instead of storing the
original pointers directly.

(please CC me, I'm not subscribed to the list)

---
 ldso/dynlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index cc677952..703342b8 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -1756,8 +1756,8 @@ void __dls3(size_t *sp, size_t *auxv)

 	/* Only trust user/env if kernel says we're not suid/sgid */
 	if (!libc.secure) {
-		env_path = getenv("LD_LIBRARY_PATH");
-		env_preload = getenv("LD_PRELOAD");
+		env_path = strdup(getenv("LD_LIBRARY_PATH"));
+		env_preload = strdup(getenv("LD_PRELOAD"));
 	}

 	/* Activate error handler function */

[musl] [RESEND PATCH] ldso/dynlink: Protect LD_ env vars from getting clobbered by apps

[Elvis Pranskevichus]

(sorry, the previous patch is obviously wrong, this is a better
version)

There is no guarantee that the environment block will remain intact.
For example, PostgreSQL clobbers argv/environ area to implement its
"setproctitle" emulation on non-BSD [1], and there is a popular Python
library inspired by it [2].  As a result, setting `LD_LIBRARY_PATH`
or `LD_PRELOAD` has no effect on Postgres subprocesses when linking
against musl.

Protect against this by making a copies instead of storing the original
pointers directly.
---
 ldso/dynlink.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index cc677952..2b816ce0 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -1757,7 +1757,23 @@ void __dls3(size_t *sp, size_t *auxv)
 	/* Only trust user/env if kernel says we're not suid/sgid */
 	if (!libc.secure) {
 		env_path = getenv("LD_LIBRARY_PATH");
+		if (env_path != NULL) {
+			/* Prevent value from getting clobbered by the application */
+			env_path = strdup(env_path);
+			if (!env_path) {
+				dprintf(2, "%s: out of memory\n", argv[0]);
+				_exit(127);
+			}
+		}
 		env_preload = getenv("LD_PRELOAD");
+		if (env_preload != NULL) {
+			/* Prevent value from getting clobbered by the application */
+			env_preload = strdup(env_preload);
+			if (!env_preload) {
+				dprintf(2, "%s: out of memory\n", argv[0]);
+				_exit(127);
+			}
+		}
 	}
 
 	/* Activate error handler function */
-- 
2.35.1

Re: [musl] [PATCH] ldso/dynlink: Protect LD_ env vars from getting clobbered by apps

[Rich Felker]

On Tue, Aug 16, 2022 at 10:45:45PM -0700, Elvis Pranskevichus wrote:
> There is no guarantee that the environment block will remain intact.
> For example, PostgreSQL clobbers argv/environ area to implement its
> "setproctitle" emulation on non-BSD [1], and there is a popular Python
> library inspired by it [2].  As a result, setting `LD_LIBRARY_PATH`
> or `LD_PRELOAD` has no effect on Postgres subprocesses when linking
> against musl.

This is explicitly not allowed and is UB. This memory is not available
for the application to clobber, and code attempting to do that needs
to be patched out. Aside from the general principle, POSIX is very
clear in the specification of environ:

    "Any application that directly modifies the pointers to which the
    environ variable points has undefined behavior."

and overwriting the whole ELF entry vector in order to put text
messages over top of it necessarily involved modifying that memory.
Moreover, it also necessarily involves clobbering the aux vector and
other implementation details the application has no entitlement to
modify (at least if the environment is small or empty), even if it
were allowed to modify the environment this way.

Applications that *really* want setproctitle type functionality can
presumably do something like re-exec themselves with a suitably large
argv[0] to give them safe space to overwrite with their preferred
message, rather than UB trying to relocate the environment (and auxv?
how? they can't tell libc they moved it) to some other location.

> Protect against this by making a copies instead of storing the
> original pointers directly.
> 
> (please CC me, I'm not subscribed to the list)
> 
> ---
>  ldso/dynlink.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ldso/dynlink.c b/ldso/dynlink.c
> index cc677952..703342b8 100644
> --- a/ldso/dynlink.c
> +++ b/ldso/dynlink.c
> @@ -1756,8 +1756,8 @@ void __dls3(size_t *sp, size_t *auxv)
> 
>  	/* Only trust user/env if kernel says we're not suid/sgid */
>  	if (!libc.secure) {
> -		env_path = getenv("LD_LIBRARY_PATH");
> -		env_preload = getenv("LD_PRELOAD");
> +		env_path = strdup(getenv("LD_LIBRARY_PATH"));
> +		env_preload = strdup(getenv("LD_PRELOAD"));
>  	}
> 
>  	/* Activate error handler function */

This assumes strdup succeeds (not a valid assumption) and moreover
takes place before it's okay to call malloc. The latter could be fixed
by moving the duplication to a point after loading has finished but
before the application runs, but then it would also need to use
[__libc_]malloc directly rather than strdup, which calls malloc,
because application code should not be called before the application
entry point and malloc may be application code (doing so will actually
crash on some archs).

Aside from that, it imposes a penalty (bringing up memory for malloc)
on all programs for the sake of supporting UB.

Re: [musl] [PATCH] ldso/dynlink: Protect LD_ env vars from getting clobbered by apps

[Elvis Pranskevichus]

On Wednesday, August 17, 2022 8:29:05 AM PDT Rich Felker wrote:
> On Tue, Aug 16, 2022 at 10:45:45PM -0700, Elvis Pranskevichus wrote:
> > There is no guarantee that the environment block will remain intact.
> > For example, PostgreSQL clobbers argv/environ area to implement its
> > "setproctitle" emulation on non-BSD [1], and there is a popular
> > Python library inspired by it [2].  As a result, setting
> > `LD_LIBRARY_PATH` or `LD_PRELOAD` has no effect on Postgres
> > subprocesses when linking against musl.
> 
> This is explicitly not allowed and is UB. This memory is not available
> for the application to clobber, and code attempting to do that needs
> to be patched out. Aside from the general principle, POSIX is very
> clear in the specification of environ:
> 
>     "Any application that directly modifies the pointers to which the
>     environ variable points has undefined behavior."

I understand that what Postgres et al are doing is a nasty hack.  My 
thinking was that it is a question of compatibility with glibc's 
behavior, which seems to tolerate these shenanigans (at least with 
respect to `LD_*` variables.  Good point regarding the allocator bringup 
overhead, I haven't thought of that.

Thanks,

                                Elvis

Re: [musl] [PATCH] ldso/dynlink: Protect LD_ env vars from getting clobbered by apps

[James Y Knight]

[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]

Sidenote: Linux does support a less awful way to change the kernel's view
of argv these days, using prctl(PR_SET_MM, PR_SET_MM_ARG_START (or _END),
addr, 0, 0). Sadly, it only allows root (CAP_SYS_RESOURCE) to use it. I'm
not sure why, perhaps that restriction could be relaxed for future
kernels...

See
https://github.com/systemd/systemd/blob/87305b0fbfc0e40a948cf0a683bcf9d47b8a41a3/src/basic/process-util.c#L256
for an example of use (including ugly workaround for the API being silly
and setting START/END with separate syscalls, but requiring START <= END at
all times)

On Wed, Aug 17, 2022 at 6:05 AM Elvis Pranskevichus <elvis@edgedb.com>
wrote:

> There is no guarantee that the environment block will remain intact.
> For example, PostgreSQL clobbers argv/environ area to implement its
> "setproctitle" emulation on non-BSD [1], and there is a popular Python
> library inspired by it [2].  As a result, setting `LD_LIBRARY_PATH`
> or `LD_PRELOAD` has no effect on Postgres subprocesses when linking
> against musl.
>
> Protect against this by making a copies instead of storing the
> original pointers directly.
>
> (please CC me, I'm not subscribed to the list)
>
> ---
>  ldso/dynlink.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ldso/dynlink.c b/ldso/dynlink.c
> index cc677952..703342b8 100644
> --- a/ldso/dynlink.c
> +++ b/ldso/dynlink.c
> @@ -1756,8 +1756,8 @@ void __dls3(size_t *sp, size_t *auxv)
>
>         /* Only trust user/env if kernel says we're not suid/sgid */
>         if (!libc.secure) {
> -               env_path = getenv("LD_LIBRARY_PATH");
> -               env_preload = getenv("LD_PRELOAD");
> +               env_path = strdup(getenv("LD_LIBRARY_PATH"));
> +               env_preload = strdup(getenv("LD_PRELOAD"));
>         }
>
>         /* Activate error handler function */
>
>
>
>
>

[-- Attachment #2: Type: text/html, Size: 2551 bytes --]

Re: [musl] [PATCH] ldso/dynlink: Protect LD_ env vars from getting clobbered by apps

[Rich Felker]

On Wed, Aug 17, 2022 at 12:10:48PM -0400, James Y Knight wrote:
> Sidenote: Linux does support a less awful way to change the kernel's view
> of argv these days, using prctl(PR_SET_MM, PR_SET_MM_ARG_START (or _END),
> addr, 0, 0). Sadly, it only allows root (CAP_SYS_RESOURCE) to use it. I'm
> not sure why, perhaps that restriction could be relaxed for future
> kernels...
> 
> See
> https://github.com/systemd/systemd/blob/87305b0fbfc0e40a948cf0a683bcf9d47b8a41a3/src/basic/process-util.c#L256
> for an example of use (including ugly workaround for the API being silly
> and setting START/END with separate syscalls, but requiring START <= END at
> all times)

Yes, unfortunately (at least last I checked) it's also only available
if CRIU support was enabled in the kernel since that's the only thing
its creators envisioned it being used for... *sigh*

> 
> On Wed, Aug 17, 2022 at 6:05 AM Elvis Pranskevichus <elvis@edgedb.com>
> wrote:
> 
> > There is no guarantee that the environment block will remain intact.
> > For example, PostgreSQL clobbers argv/environ area to implement its
> > "setproctitle" emulation on non-BSD [1], and there is a popular Python
> > library inspired by it [2].  As a result, setting `LD_LIBRARY_PATH`
> > or `LD_PRELOAD` has no effect on Postgres subprocesses when linking
> > against musl.
> >
> > Protect against this by making a copies instead of storing the
> > original pointers directly.
> >
> > (please CC me, I'm not subscribed to the list)
> >
> > ---
> >  ldso/dynlink.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/ldso/dynlink.c b/ldso/dynlink.c
> > index cc677952..703342b8 100644
> > --- a/ldso/dynlink.c
> > +++ b/ldso/dynlink.c
> > @@ -1756,8 +1756,8 @@ void __dls3(size_t *sp, size_t *auxv)
> >
> >         /* Only trust user/env if kernel says we're not suid/sgid */
> >         if (!libc.secure) {
> > -               env_path = getenv("LD_LIBRARY_PATH");
> > -               env_preload = getenv("LD_PRELOAD");
> > +               env_path = strdup(getenv("LD_LIBRARY_PATH"));
> > +               env_preload = strdup(getenv("LD_PRELOAD"));
> >         }
> >
> >         /* Activate error handler function */
> >
> >
> >
> >
> >

Re: [musl] [PATCH] ldso/dynlink: Protect LD_ env vars from getting clobbered by apps

[James Y Knight]

[-- Attachment #1: Type: text/plain, Size: 2805 bytes --]

That issue was already fixed in 3.10. Currently, only PR_SET_MM_MAP and
PR_SET_MM_MAP_SIZE operations require CRIU support enabled, the others do
not.

If anyone sufficiently motivated would like to propose a patch to the
kernel to allow {ARG,ENV}_{START_END} operations by non-root users, that'd
be pretty awesome. Then maybe in another couple decades, programs can stop
using these awful hacks.

On Wed, Aug 17, 2022 at 3:28 PM Rich Felker <dalias@libc.org> wrote:

> On Wed, Aug 17, 2022 at 12:10:48PM -0400, James Y Knight wrote:
> > Sidenote: Linux does support a less awful way to change the kernel's view
> > of argv these days, using prctl(PR_SET_MM, PR_SET_MM_ARG_START (or _END),
> > addr, 0, 0). Sadly, it only allows root (CAP_SYS_RESOURCE) to use it. I'm
> > not sure why, perhaps that restriction could be relaxed for future
> > kernels...
> >
> > See
> >
> https://github.com/systemd/systemd/blob/87305b0fbfc0e40a948cf0a683bcf9d47b8a41a3/src/basic/process-util.c#L256
> > for an example of use (including ugly workaround for the API being silly
> > and setting START/END with separate syscalls, but requiring START <= END
> at
> > all times)
>
> Yes, unfortunately (at least last I checked) it's also only available
> if CRIU support was enabled in the kernel since that's the only thing
> its creators envisioned it being used for... *sigh*
>
> >
> > On Wed, Aug 17, 2022 at 6:05 AM Elvis Pranskevichus <elvis@edgedb.com>
> > wrote:
> >
> > > There is no guarantee that the environment block will remain intact.
> > > For example, PostgreSQL clobbers argv/environ area to implement its
> > > "setproctitle" emulation on non-BSD [1], and there is a popular Python
> > > library inspired by it [2].  As a result, setting `LD_LIBRARY_PATH`
> > > or `LD_PRELOAD` has no effect on Postgres subprocesses when linking
> > > against musl.
> > >
> > > Protect against this by making a copies instead of storing the
> > > original pointers directly.
> > >
> > > (please CC me, I'm not subscribed to the list)
> > >
> > > ---
> > >  ldso/dynlink.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/ldso/dynlink.c b/ldso/dynlink.c
> > > index cc677952..703342b8 100644
> > > --- a/ldso/dynlink.c
> > > +++ b/ldso/dynlink.c
> > > @@ -1756,8 +1756,8 @@ void __dls3(size_t *sp, size_t *auxv)
> > >
> > >         /* Only trust user/env if kernel says we're not suid/sgid */
> > >         if (!libc.secure) {
> > > -               env_path = getenv("LD_LIBRARY_PATH");
> > > -               env_preload = getenv("LD_PRELOAD");
> > > +               env_path = strdup(getenv("LD_LIBRARY_PATH"));
> > > +               env_preload = strdup(getenv("LD_PRELOAD"));
> > >         }
> > >
> > >         /* Activate error handler function */
> > >
> > >
> > >
> > >
> > >
>

[-- Attachment #2: Type: text/html, Size: 3892 bytes --]