From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from insect.birch.relay.mailchannels.net (insect.birch.relay.mailchannels.net [23.83.209.93]) by sourceware.org (Postfix) with ESMTPS id 9B11038582B0 for ; Mon, 20 Nov 2023 23:09:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9B11038582B0 Authentication-Results: sourceware.org; dmarc=fail (p=none dis=none) header.from=sourceware.org Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=sourceware.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 9B11038582B0 Authentication-Results: server2.sourceware.org; arc=fail smtp.remote-ip=23.83.209.93 ARC-Seal: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1700521782; cv=fail; b=FTvE3WCc3tET8dncUDSWnrgOzS5W97LX4O3UlWXzyzYCdwc69TIwnT1dkdrITPRj41QTkOxMPokQgjSru+FBdBLzeNWT5N0nb2aAf8du5eip6mkysvZgpnjcImZJqhEouIYuSuu+0JRSLvt6bwZvjE474GRa0sqtmHSRjbRl3/A= ARC-Message-Signature: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1700521782; c=relaxed/simple; bh=VyMdUX0PfjAhBM5tVBQD9jSJuqTN8MGWJ4FQ8mQieUk=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=uUzvdRAiRUw9vDfAy/5+gdpOp0c8nojWiUSHp9nXj6gKt5C3hbvSYwppD5FP4pM4pH72PloFnD320YHHCoQTYCKAZ/iNPl33h/RON+F++xSuT/5iMFKzr7Pfj6IPBcZjmp4sfv+UOXOQ6VGBtowvxRaH1NQ+lA9izD+d5YSmva4= ARC-Authentication-Results: i=2; server2.sourceware.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id 8383E8176E; Mon, 20 Nov 2023 23:09:39 +0000 (UTC) Received: from pdx1-sub0-mail-a272.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 389AD81338; Mon, 20 Nov 2023 23:09:39 +0000 (UTC) ARC-Seal: i=1; s=arc-2022; d=mailchannels.net; t=1700521779; a=rsa-sha256; cv=none; b=fx7n8o/E7IH4+6OMPKGQcQiXY/zls4lNlDczQjkdxdI67IHqwTt0vW403eHZ+3OIGAjCsY HBPYwkJYRe32GCeJqmT7YGxZR24QuizNuMVHPYdI5v41/qi+vd6L5Pg8mdfnXs/JKFfcy4 39x/6d2dpajLyYLJoZvu6C7PLARxUPa0xu0zybxSx+rdE0gWsQ/NR7wuLsOEpw33w909oI XNO5DX0ICkXvRb3L8k4GMi2Gtyj4FwtdYYCTWRB8hYRi7L6N8g5vzRtfnyjkT3tmZj/tfQ qA8KAtBAyZRbOGccs7GQNW0YzM/GCBE7q/hcbu3PCZ/KcYQMp1KVUWPmQo+1xw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=mailchannels.net; s=arc-2022; t=1700521779; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=FMywrN1zqhPYZDejnj2+8++vgCl0GXO13LanhHufeqU=; b=8uh44q5IDbXIgR4zmmSBRpcEPTB95J3YNIPhWUekc1YV0jviOXkg1ZInFibIPPMcQMiQj3 SESMgQFE9BzqgYeboxeWJPGINs5lINa17Y2360Z6dn237dRmG9gJhaMxlxV86nAz0b+sPd 0pxf/PeqIGgmbmklC5pQ5pBYCkgc5OT+uyq3yngvob7zZkxybFlecVBuIiRDca8CaTRCji 0QJmHkPNheJ3lI52k67j223lUa0zJqAOMRBeDLn8TOeaAe7aWrhDkFMvcb6TwCGdY5OPSE Ai96LuRqpnYb+xBqQEApenvRUs+qu4GUnRy3wGWRpi5sRaYVhkLtQf3YDvbDlg== ARC-Authentication-Results: i=1; rspamd-55bcb54c45-ksbwd; auth=pass smtp.auth=dreamhost smtp.mailfrom=siddhesh@sourceware.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Absorbed-Obese: 203330450527128c_1700521779379_143322220 X-MC-Loop-Signature: 1700521779379:4015481088 X-MC-Ingress-Time: 1700521779378 Received: from pdx1-sub0-mail-a272.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.104.134.218 (trex/6.9.2); Mon, 20 Nov 2023 23:09:39 +0000 Received: from [192.168.2.12] (bras-vprn-toroon4834w-lp130-02-142-113-138-136.dsl.bell.ca [142.113.138.136]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a272.dreamhost.com (Postfix) with ESMTPSA id 4SZ39t6QcCz87; Mon, 20 Nov 2023 15:09:38 -0800 (PST) Message-ID: <4f6da9ac-880e-40da-970a-1cf3dfe0baf6@sourceware.org> Date: Mon, 20 Nov 2023 18:09:37 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 19/19] elf: Refactor process_envvars Content-Language: en-US To: Adhemerval Zanella , libc-alpha@sourceware.org References: <20231106202552.3404059-1-adhemerval.zanella@linaro.org> <20231106202552.3404059-20-adhemerval.zanella@linaro.org> From: Siddhesh Poyarekar In-Reply-To: <20231106202552.3404059-20-adhemerval.zanella@linaro.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-1172.2 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_SOFTFAIL,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2023-11-06 15:25, Adhemerval Zanella wrote: > It splits between process_envvars_secure and process_envvars_default, > with the former used to process arguments for __libc_enable_secure. > It does not have any semantic change, just simplify the code so there > is no need to handle __libc_enable_secure on each len switch. > > Checked on x86_64-linux-gnu and aarch64-linux-gnu. > --- > elf/rtld.c | 132 ++++++++++++++++++++++++++++++++++------------------- > 1 file changed, 84 insertions(+), 48 deletions(-) > > diff --git a/elf/rtld.c b/elf/rtld.c > index cfba30eba0..95dcd32185 100644 > --- a/elf/rtld.c > +++ b/elf/rtld.c > @@ -2527,7 +2527,67 @@ a filename can be specified using the LD_DEBUG_OUTPUT environment variable.\n"); > } > > static void > -process_envvars (struct dl_main_state *state) > +process_envvars_secure (struct dl_main_state *state) > +{ > + char **runp = _environ; > + char *envline; > + > + while ((envline = _dl_next_ld_env_entry (&runp)) != NULL) > + { > + size_t len = 0; > + > + while (envline[len] != '\0' && envline[len] != '=') > + ++len; > + > + if (envline[len] != '=') > + /* This is a "LD_" variable at the end of the string without > + a '=' character. Ignore it since otherwise we will access > + invalid memory below. */ > + continue; > + > + switch (len) > + { > + case 5: > + /* For __libc_enable_secure mode, audit pathnames containing slashes > + are ignored. Also, shared audit objects are only loaded only from > + the standard search directories and only if they have set-user-ID > + mode bit enabled. */ > + if (memcmp (envline, "AUDIT", 5) == 0) > + audit_list_add_string (&state->audit_list, &envline[6]); > + break; > + > + case 7: > + /* For __libc_enable_secure mode, preload pathnames containing slashes > + are ignored. Also, shared objects are only preloaded from the > + standard search directories and only if they have set-user-ID mode > + bit enabled. */ > + if (memcmp (envline, "PRELOAD", 7) == 0) > + state->preloadlist = &envline[8]; > + break; > + } > + } We need to talk about whether we should get rid of these too in the context of setuid programs, but that's a separate discussion. LGTM. Reviewed-by: Siddhesh Poyarekar > + > + /* Extra security for SUID binaries. Remove all dangerous environment > + variables. */ > + const char *nextp = UNSECURE_ENVVARS; > + do > + { > + unsetenv (nextp); > + nextp = strchr (nextp, '\0') + 1; > + } > + while (*nextp != '\0'); > + > + if (GLRO(dl_debug_mask) != 0 > + || GLRO(dl_verbose) != 0 > + || GLRO(dl_lazy) != 1 > + || GLRO(dl_bind_not) != 0 > + || state->mode != rtld_mode_normal > + || state->version_info) > + _exit (5); > +} > + > +static void > +process_envvars_default (struct dl_main_state *state) > { > char **runp = _environ; > char *envline; I wondered if we should add an assert here on __libc_enable_secure ==0, but there is only one caller, so maybe we can get away without it. > @@ -2550,15 +2610,13 @@ process_envvars (struct dl_main_state *state) > { > case 4: > /* Warning level, verbose or not. */ > - if (!__libc_enable_secure > - && memcmp (envline, "WARN", 4) == 0) > + if (memcmp (envline, "WARN", 4) == 0) > GLRO(dl_verbose) = envline[5] != '\0'; > break; > > case 5: > /* Debugging of the dynamic linker? */ > - if (!__libc_enable_secure > - && memcmp (envline, "DEBUG", 5) == 0) > + if (memcmp (envline, "DEBUG", 5) == 0) > { > process_dl_debug (state, &envline[6]); > break; > @@ -2573,8 +2631,7 @@ process_envvars (struct dl_main_state *state) > > case 7: > /* Print information about versions. */ > - if (!__libc_enable_secure > - && memcmp (envline, "VERBOSE", 7) == 0) > + if (memcmp (envline, "VERBOSE", 7) == 0) > { > state->version_info = envline[8] != '\0'; > break; > @@ -2591,43 +2648,37 @@ process_envvars (struct dl_main_state *state) > } > > /* Which shared object shall be profiled. */ > - if (!__libc_enable_secure > - && memcmp (envline, "PROFILE", 7) == 0 && envline[8] != '\0') > + if (memcmp (envline, "PROFILE", 7) == 0 && envline[8] != '\0') > GLRO(dl_profile) = &envline[8]; > break; > > case 8: > /* Do we bind early? */ > - if (!__libc_enable_secure > - && memcmp (envline, "BIND_NOW", 8) == 0) > + if (memcmp (envline, "BIND_NOW", 8) == 0) > { > GLRO(dl_lazy) = envline[9] == '\0'; > break; > } > - if (! __libc_enable_secure > - && memcmp (envline, "BIND_NOT", 8) == 0) > + if (memcmp (envline, "BIND_NOT", 8) == 0) > GLRO(dl_bind_not) = envline[9] != '\0'; > break; > > case 9: > /* Test whether we want to see the content of the auxiliary > array passed up from the kernel. */ > - if (!__libc_enable_secure > - && memcmp (envline, "SHOW_AUXV", 9) == 0) > + if (memcmp (envline, "SHOW_AUXV", 9) == 0) > _dl_show_auxv (); > break; > > case 11: > /* Path where the binary is found. */ > - if (!__libc_enable_secure > - && memcmp (envline, "ORIGIN_PATH", 11) == 0) > + if (memcmp (envline, "ORIGIN_PATH", 11) == 0) > GLRO(dl_origin_path) = &envline[12]; > break; > > case 12: > /* The library search path. */ > - if (!__libc_enable_secure > - && memcmp (envline, "LIBRARY_PATH", 12) == 0) > + if (memcmp (envline, "LIBRARY_PATH", 12) == 0) > { > state->library_path = &envline[13]; > state->library_path_source = "LD_LIBRARY_PATH"; > @@ -2635,30 +2686,26 @@ process_envvars (struct dl_main_state *state) > } > > /* Where to place the profiling data file. */ > - if (!__libc_enable_secure > - && memcmp (envline, "DEBUG_OUTPUT", 12) == 0) > + if (memcmp (envline, "DEBUG_OUTPUT", 12) == 0) > { > debug_output = &envline[13]; > break; > } > > - if (!__libc_enable_secure > - && memcmp (envline, "DYNAMIC_WEAK", 12) == 0) > + if (memcmp (envline, "DYNAMIC_WEAK", 12) == 0) > GLRO(dl_dynamic_weak) = 1; > break; > > case 14: > /* Where to place the profiling data file. */ > - if (!__libc_enable_secure > - && memcmp (envline, "PROFILE_OUTPUT", 14) == 0 > + if (memcmp (envline, "PROFILE_OUTPUT", 14) == 0 > && envline[15] != '\0') > GLRO(dl_profile_output) = &envline[15]; > break; > > case 20: > /* The mode of the dynamic linker can be set. */ > - if (!__libc_enable_secure > - && memcmp (envline, "TRACE_LOADED_OBJECTS", 20) == 0) > + if (memcmp (envline, "TRACE_LOADED_OBJECTS", 20) == 0) > { > state->mode = rtld_mode_trace; > state->mode_trace_program > @@ -2668,30 +2715,10 @@ process_envvars (struct dl_main_state *state) > } > } > > - /* Extra security for SUID binaries. Remove all dangerous environment > - variables. */ > - if (__glibc_unlikely (__libc_enable_secure)) > - { > - const char *nextp = UNSECURE_ENVVARS; > - do > - { > - unsetenv (nextp); > - nextp = strchr (nextp, '\0') + 1; > - } > - while (*nextp != '\0'); > - > - if (GLRO(dl_debug_mask) != 0 > - || GLRO(dl_verbose) != 0 > - || GLRO(dl_lazy) != 1 > - || GLRO(dl_bind_not) != 0 > - || state->mode != rtld_mode_normal > - || state->version_info) > - _exit (5); > - } > /* If we have to run the dynamic linker in debugging mode and the > LD_DEBUG_OUTPUT environment variable is given, we write the debug > messages to this file. */ > - else if (GLRO(dl_debug_mask) != 0 && debug_output != NULL) > + if (GLRO(dl_debug_mask) != 0 && debug_output != NULL) > { > const int flags = O_WRONLY | O_APPEND | O_CREAT | O_NOFOLLOW; > size_t name_len = strlen (debug_output); > @@ -2710,6 +2737,15 @@ process_envvars (struct dl_main_state *state) > } > } > > +static void > +process_envvars (struct dl_main_state *state) > +{ > + if (__glibc_unlikely (__libc_enable_secure)) > + process_envvars_secure (state); > + else > + process_envvars_default (state); > +} > + > #if HP_TIMING_INLINE > static void > print_statistics_item (const char *title, hp_timing_t time,