mailing list of musl libc
 help / color / mirror / code / Atom feed
From: Rich Felker <dalias@libc.org>
To: Elvis Pranskevichus <elvis@edgedb.com>
Cc: musl@lists.openwall.com
Subject: Re: [musl] [PATCH] ldso/dynlink: Protect LD_ env vars from getting clobbered by apps
Date: Wed, 17 Aug 2022 11:29:05 -0400	[thread overview]
Message-ID: <20220817152905.GC7074@brightrain.aerifal.cx> (raw)
In-Reply-To: <3818608.tdWV9SEqCh@vulcan.edgedb.net>

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.

  parent reply	other threads:[~2022-08-17 15:29 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-17  5:45 Elvis Pranskevichus
2022-08-17  6:23 ` [musl] [RESEND PATCH] " Elvis Pranskevichus
2022-08-17 15:29 ` Rich Felker [this message]
2022-08-17 16:26   ` [musl] [PATCH] " Elvis Pranskevichus
2022-08-17 16:10 ` James Y Knight
2022-08-17 19:28   ` Rich Felker
2022-08-17 21:30     ` James Y Knight

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220817152905.GC7074@brightrain.aerifal.cx \
    --to=dalias@libc.org \
    --cc=elvis@edgedb.com \
    --cc=musl@lists.openwall.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).