[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220817152905.GC7074@brightrain.aerifal.cx>
Date: Wed, 17 Aug 2022 11:29:05 -0400
From: Rich Felker <dalias@...c.org>
To: Elvis Pranskevichus <elvis@...edb.com>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH] ldso/dynlink: Protect LD_ env vars from getting
clobbered by apps
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.
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
