From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_MSPIKE_H2,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 9117 invoked from network); 17 Aug 2022 15:29:21 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 17 Aug 2022 15:29:21 -0000 Received: (qmail 3505 invoked by uid 550); 17 Aug 2022 15:29:18 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 3466 invoked from network); 17 Aug 2022 15:29:17 -0000 Date: Wed, 17 Aug 2022 11:29:05 -0400 From: Rich Felker To: Elvis Pranskevichus Cc: musl@lists.openwall.com Message-ID: <20220817152905.GC7074@brightrain.aerifal.cx> References: <3818608.tdWV9SEqCh@vulcan.edgedb.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3818608.tdWV9SEqCh@vulcan.edgedb.net> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [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.