[v2,03/19] elf: Ignore GLIBC_TUNABLES for setuid/setgid binaries
Checks
Context |
Check |
Description |
redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Testing passed
|
Commit Message
The tunable privilege levels were a retrofit to try and keep the malloc
tunable environment variables' behavior unchanged across security
boundaries. However, CVE-2023-4911 shows how tricky can be
tunable parsing in a security-sensitive environment.
Not only parsing, but the malloc tunable essentially changes some
semantics on setuid/setgid processes. Although it is not a direct
security issue, allowing users to change setuid/setgid semantics is not
a good security practice, and requires extra code and analysis to check
if each tunable is safe to use on all security boundaries.
It also means that security opt-in features, like aarch64 MTE, would
need to be explicit enabled by an administrator with a wrapper script
or with a possible future system-wide tunable setting.
Co-authored-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
elf/Makefile | 5 +-
elf/dl-tunable-types.h | 10 --
elf/dl-tunables.c | 127 ++++-----------
elf/dl-tunables.list | 9 --
elf/tst-env-setuid-tunables.c | 29 +++-
elf/tst-tunables.c | 244 +++++++++++++++++++++++++++++
manual/README.tunables | 9 --
scripts/gen-tunables.awk | 18 +--
sysdeps/x86_64/64/dl-tunables.list | 1 -
9 files changed, 299 insertions(+), 153 deletions(-)
create mode 100644 elf/tst-tunables.c
Comments
On 2023-10-17 09:05, Adhemerval Zanella wrote:
> The tunable privilege levels were a retrofit to try and keep the malloc
> tunable environment variables' behavior unchanged across security
> boundaries. However, CVE-2023-4911 shows how tricky can be
> tunable parsing in a security-sensitive environment.
>
> Not only parsing, but the malloc tunable essentially changes some
> semantics on setuid/setgid processes. Although it is not a direct
> security issue, allowing users to change setuid/setgid semantics is not
> a good security practice, and requires extra code and analysis to check
> if each tunable is safe to use on all security boundaries.
>
> It also means that security opt-in features, like aarch64 MTE, would
> need to be explicit enabled by an administrator with a wrapper script
> or with a possible future system-wide tunable setting.
>
> Co-authored-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
I'll leave this for someone else to review since I wrote some parts of this.
Thanks,
Sid
> ---
> elf/Makefile | 5 +-
> elf/dl-tunable-types.h | 10 --
> elf/dl-tunables.c | 127 ++++-----------
> elf/dl-tunables.list | 9 --
> elf/tst-env-setuid-tunables.c | 29 +++-
> elf/tst-tunables.c | 244 +++++++++++++++++++++++++++++
> manual/README.tunables | 9 --
> scripts/gen-tunables.awk | 18 +--
> sysdeps/x86_64/64/dl-tunables.list | 1 -
> 9 files changed, 299 insertions(+), 153 deletions(-)
> create mode 100644 elf/tst-tunables.c
>
> diff --git a/elf/Makefile b/elf/Makefile
> index 9176cbf1e3..c824f7dab7 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -263,7 +263,6 @@ tests-static-normal := \
> tst-dl-iter-static \
> tst-dst-static \
> tst-env-setuid \
> - tst-env-setuid-tunables \
> tst-getauxval-static \
> tst-linkall-static \
> tst-single_threaded-pthread-static \
> @@ -276,10 +275,12 @@ tests-static-normal := \
> tests-static-internal := \
> tst-dl-printf-static \
> tst-dl_find_object-static \
> + tst-env-setuid-tunables \
> tst-ptrguard1-static \
> tst-stackguard1-static \
> tst-tls1-static \
> tst-tls1-static-non-pie \
> + tst-tunables \
> # tests-static-internal
>
> CRT-tst-tls1-static-non-pie := $(csu-objpfx)crt1.o
> @@ -2696,6 +2697,8 @@ $(objpfx)tst-glibc-hwcaps-mask.out: \
> # tst-glibc-hwcaps-cache.
> $(objpfx)tst-glibc-hwcaps-cache.out: $(objpfx)tst-glibc-hwcaps
>
> +tst-tunables-ARGS = -- $(host-test-program-cmd)
> +
> $(objpfx)list-tunables.out: tst-rtld-list-tunables.sh $(objpfx)ld.so
> $(SHELL) $< $(objpfx)ld.so '$(test-wrapper-env)' \
> '$(run_program_env)' > $(objpfx)/tst-rtld-list-tunables.out
> diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h
> index c88332657e..62d6d9e629 100644
> --- a/elf/dl-tunable-types.h
> +++ b/elf/dl-tunable-types.h
> @@ -64,16 +64,6 @@ struct _tunable
> tunable_val_t val; /* The value. */
> bool initialized; /* Flag to indicate that the tunable is
> initialized. */
> - tunable_seclevel_t security_level; /* Specify the security level for the
> - tunable with respect to AT_SECURE
> - programs. See description of
> - tunable_seclevel_t to see a
> - description of the values.
> -
> - Note that even if the tunable is
> - read, it may not get used by the
> - target module if the value is
> - considered unsafe. */
> /* Compatibility elements. */
> const char env_alias[TUNABLE_ALIAS_MAX]; /* The compatibility environment
> variable name. */
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 24252af22c..a83bd2b8bc 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -154,50 +154,51 @@ __tunable_set_val (tunable_id_t id, tunable_val_t *valp, tunable_num_t *minp,
> do_tunable_update_val (cur, valp, minp, maxp);
> }
>
> -/* Parse the tunable string TUNESTR and adjust it to drop any tunables that may
> - be unsafe for AT_SECURE processes so that it can be used as the new
> - environment variable value for GLIBC_TUNABLES. VALSTRING is the original
> - environment variable string which we use to make NULL terminated values so
> - that we don't have to allocate memory again for it. */
> +/* Parse the tunable string VALSTRING. VALSTRING is a duplicated values,
> + where delimiters ':' are replaced with '\0', so string tunables are null
> + terminated. */
> static void
> -parse_tunables (char *tunestr, char *valstring)
> +parse_tunables (char *valstring)
> {
> - if (tunestr == NULL || *tunestr == '\0')
> + if (valstring == NULL || *valstring == '\0')
> return;
>
> - char *p = tunestr;
> - size_t off = 0;
> + char *p = valstring;
> + bool done = false;
>
> - while (true)
> + while (!done)
> {
> char *name = p;
> - size_t len = 0;
>
> /* First, find where the name ends. */
> - while (p[len] != '=' && p[len] != ':' && p[len] != '\0')
> - len++;
> + while (*p != '=' && *p != ':' && *p != '\0')
> + p++;
>
> /* If we reach the end of the string before getting a valid name-value
> pair, bail out. */
> - if (p[len] == '\0')
> + if (*p == '\0')
> break;
>
> /* We did not find a valid name-value pair before encountering the
> colon. */
> - if (p[len]== ':')
> + if (*p == ':')
> {
> - p += len + 1;
> + p++;
> continue;
> }
>
> - p += len + 1;
> + /* Skip the ':' or '='. */
> + p++;
>
> - /* Take the value from the valstring since we need to NULL terminate it. */
> - char *value = &valstring[p - tunestr];
> - len = 0;
> + const char *value = p;
>
> - while (p[len] != ':' && p[len] != '\0')
> - len++;
> + while (*p != ':' && *p != '\0')
> + p++;
> +
> + if (*p == '\0')
> + done = true;
> + else
> + *p++ = '\0';
>
> /* Add the tunable if it exists. */
> for (size_t i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
> @@ -206,50 +207,11 @@ parse_tunables (char *tunestr, char *valstring)
>
> if (tunable_is_name (cur->name, name))
> {
> - /* If we are in a secure context (AT_SECURE) then ignore the
> - tunable unless it is explicitly marked as secure. Tunable
> - values take precedence over their envvar aliases. We write
> - the tunables that are not SXID_ERASE back to TUNESTR, thus
> - dropping all SXID_ERASE tunables and any invalid or
> - unrecognized tunables. */
> - if (__libc_enable_secure)
> - {
> - if (cur->security_level != TUNABLE_SECLEVEL_SXID_ERASE)
> - {
> - if (off > 0)
> - tunestr[off++] = ':';
> -
> - const char *n = cur->name;
> -
> - while (*n != '\0')
> - tunestr[off++] = *n++;
> -
> - tunestr[off++] = '=';
> -
> - for (size_t j = 0; j < len; j++)
> - tunestr[off++] = value[j];
> - }
> -
> - if (cur->security_level != TUNABLE_SECLEVEL_NONE)
> - break;
> - }
> -
> - value[len] = '\0';
> tunable_initialize (cur, value);
> break;
> }
> }
> -
> - /* We reached the end while processing the tunable string. */
> - if (p[len] == '\0')
> - break;
> -
> - p += len + 1;
> }
> -
> - /* Terminate tunestr before we leave. */
> - if (__libc_enable_secure)
> - tunestr[off] = '\0';
> }
>
> /* Initialize the tunables list from the environment. For now we only use the
> @@ -263,16 +225,16 @@ __tunables_init (char **envp)
> size_t len = 0;
> char **prev_envp = envp;
>
> + /* Ignore tunables for AT_SECURE programs. */
> + if (__libc_enable_secure)
> + return;
> +
> while ((envp = get_next_env (envp, &envname, &len, &envval,
> &prev_envp)) != NULL)
> {
> if (tunable_is_name ("GLIBC_TUNABLES", envname))
> {
> - char *new_env = tunables_strdup (envname);
> - if (new_env != NULL)
> - parse_tunables (new_env + len + 1, envval);
> - /* Put in the updated envval. */
> - *prev_envp = new_env;
> + parse_tunables (tunables_strdup (envval));
> continue;
> }
>
> @@ -290,39 +252,6 @@ __tunables_init (char **envp)
> /* We have a match. Initialize and move on to the next line. */
> if (tunable_is_name (name, envname))
> {
> - /* For AT_SECURE binaries, we need to check the security settings of
> - the tunable and decide whether we read the value and also whether
> - we erase the value so that child processes don't inherit them in
> - the environment. */
> - if (__libc_enable_secure)
> - {
> - if (cur->security_level == TUNABLE_SECLEVEL_SXID_ERASE)
> - {
> - /* Erase the environment variable. */
> - char **ep = prev_envp;
> -
> - while (*ep != NULL)
> - {
> - if (tunable_is_name (name, *ep))
> - {
> - char **dp = ep;
> -
> - do
> - dp[0] = dp[1];
> - while (*dp++);
> - }
> - else
> - ++ep;
> - }
> - /* Reset the iterator so that we read the environment again
> - from the point we erased. */
> - envp = prev_envp;
> - }
> -
> - if (cur->security_level != TUNABLE_SECLEVEL_NONE)
> - continue;
> - }
> -
> tunable_initialize (cur, envval);
> break;
> }
> diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
> index 695ba7192e..471895af7b 100644
> --- a/elf/dl-tunables.list
> +++ b/elf/dl-tunables.list
> @@ -21,14 +21,6 @@
> # minval: Optional minimum acceptable value
> # maxval: Optional maximum acceptable value
> # env_alias: An alias environment variable
> -# security_level: Specify security level of the tunable for AT_SECURE binaries.
> -# Valid values are:
> -#
> -# SXID_ERASE: (default) Do not read and do not pass on to
> -# child processes.
> -# SXID_IGNORE: Do not read, but retain for non-AT_SECURE
> -# subprocesses.
> -# NONE: Read all the time.
>
> glibc {
> malloc {
> @@ -158,7 +150,6 @@ glibc {
> type: INT_32
> minval: 0
> maxval: 255
> - security_level: SXID_IGNORE
> }
> }
>
> diff --git a/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c
> index 2603007b7b..ca02dbba58 100644
> --- a/elf/tst-env-setuid-tunables.c
> +++ b/elf/tst-env-setuid-tunables.c
> @@ -15,14 +15,10 @@
> License along with the GNU C Library; if not, see
> <https://www.gnu.org/licenses/>. */
>
> -/* Verify that tunables correctly filter out unsafe tunables like
> - glibc.malloc.check and glibc.malloc.mmap_threshold but also retain
> - glibc.malloc.mmap_threshold in an unprivileged child. */
> -
> -#define _LIBC 1
> -#include "config.h"
> -#undef _LIBC
> +/* Verify that GLIBC_TUNABLES is kept unchanged but no tunable is actually
> + enabled for AT_SECURE processes. */
>
> +#include <dl-tunables.h>
> #include <errno.h>
> #include <fcntl.h>
> #include <stdlib.h>
> @@ -40,7 +36,7 @@
> #include <support/test-driver.h>
> #include <support/capture_subprocess.h>
>
> -const char *teststrings[] =
> +static const char *teststrings[] =
> {
> "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> "glibc.malloc.check=2:glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> @@ -74,6 +70,23 @@ test_child (int off)
> ret = 0;
> fflush (stdout);
>
> + /* Also check if the set tunables are effectively unchanged. */
> + int32_t check = TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL);
> + size_t mmap_threshold = TUNABLE_GET_FULL (glibc, malloc, mmap_threshold,
> + size_t, NULL);
> + int32_t perturb = TUNABLE_GET_FULL (glibc, malloc, perturb, int32_t, NULL);
> +
> + printf (" [%d] glibc.malloc.check=%d\n", off, check);
> + fflush (stdout);
> + printf (" [%d] glibc.malloc.mmap_threshold=%zu\n", off, mmap_threshold);
> + fflush (stdout);
> + printf (" [%d] glibc.malloc.perturb=%d\n", off, perturb);
> + fflush (stdout);
> +
> + ret |= check != 0;
> + ret |= mmap_threshold != 0;
> + ret |= perturb != 0;
> +
> return ret;
> }
>
> diff --git a/elf/tst-tunables.c b/elf/tst-tunables.c
> new file mode 100644
> index 0000000000..57cf532fc4
> --- /dev/null
> +++ b/elf/tst-tunables.c
> @@ -0,0 +1,244 @@
> +/* Check GLIBC_TUNABLES parsing.
> + Copyright (C) 2023 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#include <array_length.h>
> +#include <dl-tunables.h>
> +#include <getopt.h>
> +#include <intprops.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <support/capture_subprocess.h>
> +#include <support/check.h>
> +
> +static int restart;
> +#define CMDLINE_OPTIONS \
> + { "restart", no_argument, &restart, 1 },
> +
> +static const struct test_t
> +{
> + const char *env;
> + int32_t expected_malloc_check;
> + size_t expected_mmap_threshold;
> + int32_t expected_perturb;
> +} tests[] =
> +{
> + /* Expected tunable format. */
> + {
> + "glibc.malloc.check=2",
> + 2,
> + 0,
> + 0,
> + },
> + {
> + "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> + 2,
> + 4096,
> + 0,
> + },
> + /* Empty tunable are ignored. */
> + {
> + "glibc.malloc.check=2::glibc.malloc.mmap_threshold=4096",
> + 2,
> + 4096,
> + 0,
> + },
> + /* As well empty values. */
> + {
> + "glibc.malloc.check=:glibc.malloc.mmap_threshold=4096",
> + 0,
> + 4096,
> + 0,
> + },
> + /* Tunable are processed from left to right, so last one is the one set. */
> + {
> + "glibc.malloc.check=1:glibc.malloc.check=2",
> + 2,
> + 0,
> + 0,
> + },
> + {
> + "glibc.malloc.check=2:glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> + 2,
> + 4096,
> + 0,
> + },
> + {
> + "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2",
> + 2,
> + 4096,
> + 0,
> + },
> + /* 0x800 is larger than tunable maxval (0xff), so the tunable is unchanged. */
> + {
> + "glibc.malloc.perturb=0x800",
> + 0,
> + 0,
> + 0,
> + },
> + {
> + "glibc.malloc.perturb=0x55",
> + 0,
> + 0,
> + 0x55,
> + },
> + /* Out of range values are just ignored. */
> + {
> + "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
> + 0,
> + 4096,
> + 0,
> + },
> + /* Invalid keys are ignored. */
> + {
> + ":glibc.malloc.garbage=2:glibc.malloc.check=1",
> + 1,
> + 0,
> + 0,
> + },
> + {
> + "glibc.malloc.perturb=0x800:not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> + 0,
> + 4096,
> + 0,
> + },
> + {
> + "glibc.not_valid.check=2:glibc.malloc.mmap_threshold=4096",
> + 0,
> + 4096,
> + 0,
> + },
> + {
> + "not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> + 0,
> + 4096,
> + 0,
> + },
> + /* Invalid subkeys are ignored. */
> + {
> + "glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096:glibc.malloc.check=2",
> + 2,
> + 0,
> + 0,
> + },
> + {
> + "glibc.malloc.check=4:glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096",
> + 0,
> + 0,
> + 0,
> + },
> + {
> + "not_valid.malloc.check=2",
> + 0,
> + 0,
> + 0,
> + },
> + {
> + "glibc.not_valid.check=2",
> + 0,
> + 0,
> + 0,
> + },
> + /* An ill-formatted tunable in the for key=key=value will considere the
> + value as 'key=value' (which can not be parsed as an integer). */
> + {
> + "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096",
> + 0,
> + 0,
> + 0,
> + },
> + /* The ill-formatted tunable is also skipped. */
> + {
> + "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2",
> + 2,
> + 0,
> + 0,
> + },
> + /* For an integer tunable, parse will stop on non number character. */
> + {
> + "glibc.malloc.check=2=2",
> + 2,
> + 0,
> + 0,
> + },
> + {
> + "glibc.malloc.check=2=2:glibc.malloc.mmap_threshold=4096",
> + 2,
> + 4096,
> + 0,
> + }
> +};
> +
> +static int
> +handle_restart (int i)
> +{
> + TEST_COMPARE (tests[i].expected_malloc_check,
> + TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL));
> + TEST_COMPARE (tests[i].expected_mmap_threshold,
> + TUNABLE_GET_FULL (glibc, malloc, mmap_threshold, size_t, NULL));
> + TEST_COMPARE (tests[i].expected_perturb,
> + TUNABLE_GET_FULL (glibc, malloc, perturb, int32_t, NULL));
> + return 0;
> +}
> +
> +static int
> +do_test (int argc, char *argv[])
> +{
> + /* We must have either:
> + - One our fource parameters left if called initially:
> + + path to ld.so optional
> + + "--library-path" optional
> + + the library path optional
> + + the application name
> + + the test to check */
> +
> + TEST_VERIFY_EXIT (argc == 2 || argc == 5);
> +
> + if (restart)
> + return handle_restart (atoi (argv[1]));
> +
> + char nteststr[INT_BUFSIZE_BOUND (int)];
> +
> + char *spargv[10];
> + {
> + int i = 0;
> + for (; i < argc - 1; i++)
> + spargv[i] = argv[i + 1];
> + spargv[i++] = (char *) "--direct";
> + spargv[i++] = (char *) "--restart";
> + spargv[i++] = nteststr;
> + spargv[i] = NULL;
> + }
> +
> + for (int i = 0; i < array_length (tests); i++)
> + {
> + snprintf (nteststr, sizeof nteststr, "%d", i);
> +
> + printf ("[%d] Spawned test for %s\n", i, tests[i].env);
> + setenv ("GLIBC_TUNABLES", tests[i].env, 1);
> + struct support_capture_subprocess result
> + = support_capture_subprogram (spargv[0], spargv);
> + support_capture_subprocess_check (&result, "tst-tunables", 0,
> + sc_allow_stderr);
> + support_capture_subprocess_free (&result);
> + }
> +
> + return 0;
> +}
> +
> +#define TEST_FUNCTION_ARGV do_test
> +#include <support/test-driver.c>
> diff --git a/manual/README.tunables b/manual/README.tunables
> index 605ddd78cd..72ae00dc02 100644
> --- a/manual/README.tunables
> +++ b/manual/README.tunables
> @@ -59,15 +59,6 @@ The list of allowed attributes are:
>
> - env_alias: An alias environment variable
>
> -- security_level: Specify security level of the tunable for AT_SECURE
> - binaries. Valid values are:
> -
> - SXID_ERASE: (default) Do not read and do not pass on to
> - child processes.
> - SXID_IGNORE: Do not read, but retain for non-AT_SECURE
> - child processes.
> - NONE: Read all the time.
> -
> 2. Use TUNABLE_GET/TUNABLE_SET/TUNABLE_SET_WITH_BOUNDS to get and set tunables.
>
> 3. OPTIONAL: If tunables in a namespace are being used multiple times within a
> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
> index d6de100df0..1e9d6b534e 100644
> --- a/scripts/gen-tunables.awk
> +++ b/scripts/gen-tunables.awk
> @@ -61,9 +61,6 @@ $1 == "}" {
> if (!env_alias[top_ns,ns,tunable]) {
> env_alias[top_ns,ns,tunable] = "{0}"
> }
> - if (!security_level[top_ns,ns,tunable]) {
> - security_level[top_ns,ns,tunable] = "SXID_ERASE"
> - }
> len = length(top_ns"."ns"."tunable)
> if (len > max_name_len)
> max_name_len = len
> @@ -118,17 +115,6 @@ $1 == "}" {
> if (len > max_alias_len)
> max_alias_len = len
> }
> - else if (attr == "security_level") {
> - if (val == "SXID_ERASE" || val == "SXID_IGNORE" || val == "NONE") {
> - security_level[top_ns,ns,tunable] = val
> - }
> - else {
> - printf("Line %d: Invalid value (%s) for security_level: %s, ", NR, val,
> - $0)
> - print("Allowed values are 'SXID_ERASE', 'SXID_IGNORE', or 'NONE'")
> - exit 1
> - }
> - }
> else if (attr == "default") {
> if (types[top_ns,ns,tunable] == "STRING") {
> default_val[top_ns,ns,tunable] = sprintf(".strval = \"%s\"", val);
> @@ -177,9 +163,9 @@ END {
> n = indices[2];
> m = indices[3];
> printf (" {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
> - printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, false, TUNABLE_SECLEVEL_%s, %s},\n",
> + printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, false, %s},\n",
> types[t,n,m], minvals[t,n,m], maxvals[t,n,m],
> - default_val[t,n,m], security_level[t,n,m], env_alias[t,n,m]);
> + default_val[t,n,m], env_alias[t,n,m]);
> }
> print "};"
> print "#endif"
> diff --git a/sysdeps/x86_64/64/dl-tunables.list b/sysdeps/x86_64/64/dl-tunables.list
> index 0aab52e662..54a216a461 100644
> --- a/sysdeps/x86_64/64/dl-tunables.list
> +++ b/sysdeps/x86_64/64/dl-tunables.list
> @@ -23,7 +23,6 @@ glibc {
> minval: 0
> maxval: 1
> env_alias: LD_PREFER_MAP_32BIT_EXEC
> - security_level: SXID_IGNORE
> }
> }
> }
On 2023-10-18 09:04, Siddhesh Poyarekar wrote:
> On 2023-10-17 09:05, Adhemerval Zanella wrote:
>> The tunable privilege levels were a retrofit to try and keep the malloc
>> tunable environment variables' behavior unchanged across security
>> boundaries. However, CVE-2023-4911 shows how tricky can be
>> tunable parsing in a security-sensitive environment.
>>
>> Not only parsing, but the malloc tunable essentially changes some
>> semantics on setuid/setgid processes. Although it is not a direct
>> security issue, allowing users to change setuid/setgid semantics is not
>> a good security practice, and requires extra code and analysis to check
>> if each tunable is safe to use on all security boundaries.
>>
>> It also means that security opt-in features, like aarch64 MTE, would
>> need to be explicit enabled by an administrator with a wrapper script
>> or with a possible future system-wide tunable setting.
>>
>> Co-authored-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
>
> I'll leave this for someone else to review since I wrote some parts of
> this.
Carlos, can you please review this?
Thanks,
Sid
>
> Thanks,
> Sid
>
>> ---
>> elf/Makefile | 5 +-
>> elf/dl-tunable-types.h | 10 --
>> elf/dl-tunables.c | 127 ++++-----------
>> elf/dl-tunables.list | 9 --
>> elf/tst-env-setuid-tunables.c | 29 +++-
>> elf/tst-tunables.c | 244 +++++++++++++++++++++++++++++
>> manual/README.tunables | 9 --
>> scripts/gen-tunables.awk | 18 +--
>> sysdeps/x86_64/64/dl-tunables.list | 1 -
>> 9 files changed, 299 insertions(+), 153 deletions(-)
>> create mode 100644 elf/tst-tunables.c
>>
>> diff --git a/elf/Makefile b/elf/Makefile
>> index 9176cbf1e3..c824f7dab7 100644
>> --- a/elf/Makefile
>> +++ b/elf/Makefile
>> @@ -263,7 +263,6 @@ tests-static-normal := \
>> tst-dl-iter-static \
>> tst-dst-static \
>> tst-env-setuid \
>> - tst-env-setuid-tunables \
>> tst-getauxval-static \
>> tst-linkall-static \
>> tst-single_threaded-pthread-static \
>> @@ -276,10 +275,12 @@ tests-static-normal := \
>> tests-static-internal := \
>> tst-dl-printf-static \
>> tst-dl_find_object-static \
>> + tst-env-setuid-tunables \
>> tst-ptrguard1-static \
>> tst-stackguard1-static \
>> tst-tls1-static \
>> tst-tls1-static-non-pie \
>> + tst-tunables \
>> # tests-static-internal
>> CRT-tst-tls1-static-non-pie := $(csu-objpfx)crt1.o
>> @@ -2696,6 +2697,8 @@ $(objpfx)tst-glibc-hwcaps-mask.out: \
>> # tst-glibc-hwcaps-cache.
>> $(objpfx)tst-glibc-hwcaps-cache.out: $(objpfx)tst-glibc-hwcaps
>> +tst-tunables-ARGS = -- $(host-test-program-cmd)
>> +
>> $(objpfx)list-tunables.out: tst-rtld-list-tunables.sh $(objpfx)ld.so
>> $(SHELL) $< $(objpfx)ld.so '$(test-wrapper-env)' \
>> '$(run_program_env)' > $(objpfx)/tst-rtld-list-tunables.out
>> diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h
>> index c88332657e..62d6d9e629 100644
>> --- a/elf/dl-tunable-types.h
>> +++ b/elf/dl-tunable-types.h
>> @@ -64,16 +64,6 @@ struct _tunable
>> tunable_val_t val; /* The value. */
>> bool initialized; /* Flag to indicate that the tunable is
>> initialized. */
>> - tunable_seclevel_t security_level; /* Specify the security level
>> for the
>> - tunable with respect to AT_SECURE
>> - programs. See description of
>> - tunable_seclevel_t to see a
>> - description of the values.
>> -
>> - Note that even if the tunable is
>> - read, it may not get used by the
>> - target module if the value is
>> - considered unsafe. */
>> /* Compatibility elements. */
>> const char env_alias[TUNABLE_ALIAS_MAX]; /* The compatibility
>> environment
>> variable name. */
>> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
>> index 24252af22c..a83bd2b8bc 100644
>> --- a/elf/dl-tunables.c
>> +++ b/elf/dl-tunables.c
>> @@ -154,50 +154,51 @@ __tunable_set_val (tunable_id_t id,
>> tunable_val_t *valp, tunable_num_t *minp,
>> do_tunable_update_val (cur, valp, minp, maxp);
>> }
>> -/* Parse the tunable string TUNESTR and adjust it to drop any
>> tunables that may
>> - be unsafe for AT_SECURE processes so that it can be used as the new
>> - environment variable value for GLIBC_TUNABLES. VALSTRING is the
>> original
>> - environment variable string which we use to make NULL terminated
>> values so
>> - that we don't have to allocate memory again for it. */
>> +/* Parse the tunable string VALSTRING. VALSTRING is a duplicated
>> values,
>> + where delimiters ':' are replaced with '\0', so string tunables
>> are null
>> + terminated. */
>> static void
>> -parse_tunables (char *tunestr, char *valstring)
>> +parse_tunables (char *valstring)
>> {
>> - if (tunestr == NULL || *tunestr == '\0')
>> + if (valstring == NULL || *valstring == '\0')
>> return;
>> - char *p = tunestr;
>> - size_t off = 0;
>> + char *p = valstring;
>> + bool done = false;
>> - while (true)
>> + while (!done)
>> {
>> char *name = p;
>> - size_t len = 0;
>> /* First, find where the name ends. */
>> - while (p[len] != '=' && p[len] != ':' && p[len] != '\0')
>> - len++;
>> + while (*p != '=' && *p != ':' && *p != '\0')
>> + p++;
>> /* If we reach the end of the string before getting a valid
>> name-value
>> pair, bail out. */
>> - if (p[len] == '\0')
>> + if (*p == '\0')
>> break;
>> /* We did not find a valid name-value pair before encountering
>> the
>> colon. */
>> - if (p[len]== ':')
>> + if (*p == ':')
>> {
>> - p += len + 1;
>> + p++;
>> continue;
>> }
>> - p += len + 1;
>> + /* Skip the ':' or '='. */
>> + p++;
>> - /* Take the value from the valstring since we need to NULL
>> terminate it. */
>> - char *value = &valstring[p - tunestr];
>> - len = 0;
>> + const char *value = p;
>> - while (p[len] != ':' && p[len] != '\0')
>> - len++;
>> + while (*p != ':' && *p != '\0')
>> + p++;
>> +
>> + if (*p == '\0')
>> + done = true;
>> + else
>> + *p++ = '\0';
>> /* Add the tunable if it exists. */
>> for (size_t i = 0; i < sizeof (tunable_list) / sizeof
>> (tunable_t); i++)
>> @@ -206,50 +207,11 @@ parse_tunables (char *tunestr, char *valstring)
>> if (tunable_is_name (cur->name, name))
>> {
>> - /* If we are in a secure context (AT_SECURE) then ignore the
>> - tunable unless it is explicitly marked as secure. Tunable
>> - values take precedence over their envvar aliases. We write
>> - the tunables that are not SXID_ERASE back to TUNESTR, thus
>> - dropping all SXID_ERASE tunables and any invalid or
>> - unrecognized tunables. */
>> - if (__libc_enable_secure)
>> - {
>> - if (cur->security_level != TUNABLE_SECLEVEL_SXID_ERASE)
>> - {
>> - if (off > 0)
>> - tunestr[off++] = ':';
>> -
>> - const char *n = cur->name;
>> -
>> - while (*n != '\0')
>> - tunestr[off++] = *n++;
>> -
>> - tunestr[off++] = '=';
>> -
>> - for (size_t j = 0; j < len; j++)
>> - tunestr[off++] = value[j];
>> - }
>> -
>> - if (cur->security_level != TUNABLE_SECLEVEL_NONE)
>> - break;
>> - }
>> -
>> - value[len] = '\0';
>> tunable_initialize (cur, value);
>> break;
>> }
>> }
>> -
>> - /* We reached the end while processing the tunable string. */
>> - if (p[len] == '\0')
>> - break;
>> -
>> - p += len + 1;
>> }
>> -
>> - /* Terminate tunestr before we leave. */
>> - if (__libc_enable_secure)
>> - tunestr[off] = '\0';
>> }
>> /* Initialize the tunables list from the environment. For now we
>> only use the
>> @@ -263,16 +225,16 @@ __tunables_init (char **envp)
>> size_t len = 0;
>> char **prev_envp = envp;
>> + /* Ignore tunables for AT_SECURE programs. */
>> + if (__libc_enable_secure)
>> + return;
>> +
>> while ((envp = get_next_env (envp, &envname, &len, &envval,
>> &prev_envp)) != NULL)
>> {
>> if (tunable_is_name ("GLIBC_TUNABLES", envname))
>> {
>> - char *new_env = tunables_strdup (envname);
>> - if (new_env != NULL)
>> - parse_tunables (new_env + len + 1, envval);
>> - /* Put in the updated envval. */
>> - *prev_envp = new_env;
>> + parse_tunables (tunables_strdup (envval));
>> continue;
>> }
>> @@ -290,39 +252,6 @@ __tunables_init (char **envp)
>> /* We have a match. Initialize and move on to the next line. */
>> if (tunable_is_name (name, envname))
>> {
>> - /* For AT_SECURE binaries, we need to check the security
>> settings of
>> - the tunable and decide whether we read the value and also
>> whether
>> - we erase the value so that child processes don't inherit
>> them in
>> - the environment. */
>> - if (__libc_enable_secure)
>> - {
>> - if (cur->security_level == TUNABLE_SECLEVEL_SXID_ERASE)
>> - {
>> - /* Erase the environment variable. */
>> - char **ep = prev_envp;
>> -
>> - while (*ep != NULL)
>> - {
>> - if (tunable_is_name (name, *ep))
>> - {
>> - char **dp = ep;
>> -
>> - do
>> - dp[0] = dp[1];
>> - while (*dp++);
>> - }
>> - else
>> - ++ep;
>> - }
>> - /* Reset the iterator so that we read the environment
>> again
>> - from the point we erased. */
>> - envp = prev_envp;
>> - }
>> -
>> - if (cur->security_level != TUNABLE_SECLEVEL_NONE)
>> - continue;
>> - }
>> -
>> tunable_initialize (cur, envval);
>> break;
>> }
>> diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
>> index 695ba7192e..471895af7b 100644
>> --- a/elf/dl-tunables.list
>> +++ b/elf/dl-tunables.list
>> @@ -21,14 +21,6 @@
>> # minval: Optional minimum acceptable value
>> # maxval: Optional maximum acceptable value
>> # env_alias: An alias environment variable
>> -# security_level: Specify security level of the tunable for AT_SECURE
>> binaries.
>> -# Valid values are:
>> -#
>> -# SXID_ERASE: (default) Do not read and do not pass on to
>> -# child processes.
>> -# SXID_IGNORE: Do not read, but retain for non-AT_SECURE
>> -# subprocesses.
>> -# NONE: Read all the time.
>> glibc {
>> malloc {
>> @@ -158,7 +150,6 @@ glibc {
>> type: INT_32
>> minval: 0
>> maxval: 255
>> - security_level: SXID_IGNORE
>> }
>> }
>> diff --git a/elf/tst-env-setuid-tunables.c
>> b/elf/tst-env-setuid-tunables.c
>> index 2603007b7b..ca02dbba58 100644
>> --- a/elf/tst-env-setuid-tunables.c
>> +++ b/elf/tst-env-setuid-tunables.c
>> @@ -15,14 +15,10 @@
>> License along with the GNU C Library; if not, see
>> <https://www.gnu.org/licenses/>. */
>> -/* Verify that tunables correctly filter out unsafe tunables like
>> - glibc.malloc.check and glibc.malloc.mmap_threshold but also retain
>> - glibc.malloc.mmap_threshold in an unprivileged child. */
>> -
>> -#define _LIBC 1
>> -#include "config.h"
>> -#undef _LIBC
>> +/* Verify that GLIBC_TUNABLES is kept unchanged but no tunable is
>> actually
>> + enabled for AT_SECURE processes. */
>> +#include <dl-tunables.h>
>> #include <errno.h>
>> #include <fcntl.h>
>> #include <stdlib.h>
>> @@ -40,7 +36,7 @@
>> #include <support/test-driver.h>
>> #include <support/capture_subprocess.h>
>> -const char *teststrings[] =
>> +static const char *teststrings[] =
>> {
>> "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>>
>> "glibc.malloc.check=2:glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>> @@ -74,6 +70,23 @@ test_child (int off)
>> ret = 0;
>> fflush (stdout);
>> + /* Also check if the set tunables are effectively unchanged. */
>> + int32_t check = TUNABLE_GET_FULL (glibc, malloc, check, int32_t,
>> NULL);
>> + size_t mmap_threshold = TUNABLE_GET_FULL (glibc, malloc,
>> mmap_threshold,
>> + size_t, NULL);
>> + int32_t perturb = TUNABLE_GET_FULL (glibc, malloc, perturb,
>> int32_t, NULL);
>> +
>> + printf (" [%d] glibc.malloc.check=%d\n", off, check);
>> + fflush (stdout);
>> + printf (" [%d] glibc.malloc.mmap_threshold=%zu\n", off,
>> mmap_threshold);
>> + fflush (stdout);
>> + printf (" [%d] glibc.malloc.perturb=%d\n", off, perturb);
>> + fflush (stdout);
>> +
>> + ret |= check != 0;
>> + ret |= mmap_threshold != 0;
>> + ret |= perturb != 0;
>> +
>> return ret;
>> }
>> diff --git a/elf/tst-tunables.c b/elf/tst-tunables.c
>> new file mode 100644
>> index 0000000000..57cf532fc4
>> --- /dev/null
>> +++ b/elf/tst-tunables.c
>> @@ -0,0 +1,244 @@
>> +/* Check GLIBC_TUNABLES parsing.
>> + Copyright (C) 2023 Free Software Foundation, Inc.
>> + This file is part of the GNU C Library.
>> +
>> + The GNU C Library is free software; you can redistribute it and/or
>> + modify it under the terms of the GNU Lesser General Public
>> + License as published by the Free Software Foundation; either
>> + version 2.1 of the License, or (at your option) any later version.
>> +
>> + The GNU C Library is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + Lesser General Public License for more details.
>> +
>> + You should have received a copy of the GNU Lesser General Public
>> + License along with the GNU C Library; if not, see
>> + <https://www.gnu.org/licenses/>. */
>> +
>> +#include <array_length.h>
>> +#include <dl-tunables.h>
>> +#include <getopt.h>
>> +#include <intprops.h>
>> +#include <stdint.h>
>> +#include <stdlib.h>
>> +#include <support/capture_subprocess.h>
>> +#include <support/check.h>
>> +
>> +static int restart;
>> +#define CMDLINE_OPTIONS \
>> + { "restart", no_argument, &restart, 1 },
>> +
>> +static const struct test_t
>> +{
>> + const char *env;
>> + int32_t expected_malloc_check;
>> + size_t expected_mmap_threshold;
>> + int32_t expected_perturb;
>> +} tests[] =
>> +{
>> + /* Expected tunable format. */
>> + {
>> + "glibc.malloc.check=2",
>> + 2,
>> + 0,
>> + 0,
>> + },
>> + {
>> + "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>> + 2,
>> + 4096,
>> + 0,
>> + },
>> + /* Empty tunable are ignored. */
>> + {
>> + "glibc.malloc.check=2::glibc.malloc.mmap_threshold=4096",
>> + 2,
>> + 4096,
>> + 0,
>> + },
>> + /* As well empty values. */
>> + {
>> + "glibc.malloc.check=:glibc.malloc.mmap_threshold=4096",
>> + 0,
>> + 4096,
>> + 0,
>> + },
>> + /* Tunable are processed from left to right, so last one is the one
>> set. */
>> + {
>> + "glibc.malloc.check=1:glibc.malloc.check=2",
>> + 2,
>> + 0,
>> + 0,
>> + },
>> + {
>> +
>> "glibc.malloc.check=2:glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>> + 2,
>> + 4096,
>> + 0,
>> + },
>> + {
>> +
>> "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2",
>> + 2,
>> + 4096,
>> + 0,
>> + },
>> + /* 0x800 is larger than tunable maxval (0xff), so the tunable is
>> unchanged. */
>> + {
>> + "glibc.malloc.perturb=0x800",
>> + 0,
>> + 0,
>> + 0,
>> + },
>> + {
>> + "glibc.malloc.perturb=0x55",
>> + 0,
>> + 0,
>> + 0x55,
>> + },
>> + /* Out of range values are just ignored. */
>> + {
>> + "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
>> + 0,
>> + 4096,
>> + 0,
>> + },
>> + /* Invalid keys are ignored. */
>> + {
>> + ":glibc.malloc.garbage=2:glibc.malloc.check=1",
>> + 1,
>> + 0,
>> + 0,
>> + },
>> + {
>> +
>> "glibc.malloc.perturb=0x800:not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>> + 0,
>> + 4096,
>> + 0,
>> + },
>> + {
>> + "glibc.not_valid.check=2:glibc.malloc.mmap_threshold=4096",
>> + 0,
>> + 4096,
>> + 0,
>> + },
>> + {
>> + "not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>> + 0,
>> + 4096,
>> + 0,
>> + },
>> + /* Invalid subkeys are ignored. */
>> + {
>> +
>> "glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096:glibc.malloc.check=2",
>> + 2,
>> + 0,
>> + 0,
>> + },
>> + {
>> +
>> "glibc.malloc.check=4:glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096",
>> + 0,
>> + 0,
>> + 0,
>> + },
>> + {
>> + "not_valid.malloc.check=2",
>> + 0,
>> + 0,
>> + 0,
>> + },
>> + {
>> + "glibc.not_valid.check=2",
>> + 0,
>> + 0,
>> + 0,
>> + },
>> + /* An ill-formatted tunable in the for key=key=value will considere
>> the
>> + value as 'key=value' (which can not be parsed as an integer). */
>> + {
>> + "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096",
>> + 0,
>> + 0,
>> + 0,
>> + },
>> + /* The ill-formatted tunable is also skipped. */
>> + {
>> +
>> "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2",
>> + 2,
>> + 0,
>> + 0,
>> + },
>> + /* For an integer tunable, parse will stop on non number
>> character. */
>> + {
>> + "glibc.malloc.check=2=2",
>> + 2,
>> + 0,
>> + 0,
>> + },
>> + {
>> + "glibc.malloc.check=2=2:glibc.malloc.mmap_threshold=4096",
>> + 2,
>> + 4096,
>> + 0,
>> + }
>> +};
>> +
>> +static int
>> +handle_restart (int i)
>> +{
>> + TEST_COMPARE (tests[i].expected_malloc_check,
>> + TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL));
>> + TEST_COMPARE (tests[i].expected_mmap_threshold,
>> + TUNABLE_GET_FULL (glibc, malloc, mmap_threshold, size_t, NULL));
>> + TEST_COMPARE (tests[i].expected_perturb,
>> + TUNABLE_GET_FULL (glibc, malloc, perturb, int32_t, NULL));
>> + return 0;
>> +}
>> +
>> +static int
>> +do_test (int argc, char *argv[])
>> +{
>> + /* We must have either:
>> + - One our fource parameters left if called initially:
>> + + path to ld.so optional
>> + + "--library-path" optional
>> + + the library path optional
>> + + the application name
>> + + the test to check */
>> +
>> + TEST_VERIFY_EXIT (argc == 2 || argc == 5);
>> +
>> + if (restart)
>> + return handle_restart (atoi (argv[1]));
>> +
>> + char nteststr[INT_BUFSIZE_BOUND (int)];
>> +
>> + char *spargv[10];
>> + {
>> + int i = 0;
>> + for (; i < argc - 1; i++)
>> + spargv[i] = argv[i + 1];
>> + spargv[i++] = (char *) "--direct";
>> + spargv[i++] = (char *) "--restart";
>> + spargv[i++] = nteststr;
>> + spargv[i] = NULL;
>> + }
>> +
>> + for (int i = 0; i < array_length (tests); i++)
>> + {
>> + snprintf (nteststr, sizeof nteststr, "%d", i);
>> +
>> + printf ("[%d] Spawned test for %s\n", i, tests[i].env);
>> + setenv ("GLIBC_TUNABLES", tests[i].env, 1);
>> + struct support_capture_subprocess result
>> + = support_capture_subprogram (spargv[0], spargv);
>> + support_capture_subprocess_check (&result, "tst-tunables", 0,
>> + sc_allow_stderr);
>> + support_capture_subprocess_free (&result);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +#define TEST_FUNCTION_ARGV do_test
>> +#include <support/test-driver.c>
>> diff --git a/manual/README.tunables b/manual/README.tunables
>> index 605ddd78cd..72ae00dc02 100644
>> --- a/manual/README.tunables
>> +++ b/manual/README.tunables
>> @@ -59,15 +59,6 @@ The list of allowed attributes are:
>> - env_alias: An alias environment variable
>> -- security_level: Specify security level of the tunable for AT_SECURE
>> - binaries. Valid values are:
>> -
>> - SXID_ERASE: (default) Do not read and do not pass on to
>> - child processes.
>> - SXID_IGNORE: Do not read, but retain for non-AT_SECURE
>> - child processes.
>> - NONE: Read all the time.
>> -
>> 2. Use TUNABLE_GET/TUNABLE_SET/TUNABLE_SET_WITH_BOUNDS to get and
>> set tunables.
>> 3. OPTIONAL: If tunables in a namespace are being used multiple
>> times within a
>> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
>> index d6de100df0..1e9d6b534e 100644
>> --- a/scripts/gen-tunables.awk
>> +++ b/scripts/gen-tunables.awk
>> @@ -61,9 +61,6 @@ $1 == "}" {
>> if (!env_alias[top_ns,ns,tunable]) {
>> env_alias[top_ns,ns,tunable] = "{0}"
>> }
>> - if (!security_level[top_ns,ns,tunable]) {
>> - security_level[top_ns,ns,tunable] = "SXID_ERASE"
>> - }
>> len = length(top_ns"."ns"."tunable)
>> if (len > max_name_len)
>> max_name_len = len
>> @@ -118,17 +115,6 @@ $1 == "}" {
>> if (len > max_alias_len)
>> max_alias_len = len
>> }
>> - else if (attr == "security_level") {
>> - if (val == "SXID_ERASE" || val == "SXID_IGNORE" || val == "NONE") {
>> - security_level[top_ns,ns,tunable] = val
>> - }
>> - else {
>> - printf("Line %d: Invalid value (%s) for security_level: %s, ",
>> NR, val,
>> - $0)
>> - print("Allowed values are 'SXID_ERASE', 'SXID_IGNORE', or 'NONE'")
>> - exit 1
>> - }
>> - }
>> else if (attr == "default") {
>> if (types[top_ns,ns,tunable] == "STRING") {
>> default_val[top_ns,ns,tunable] = sprintf(".strval = \"%s\"",
>> val);
>> @@ -177,9 +163,9 @@ END {
>> n = indices[2];
>> m = indices[3];
>> printf (" {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
>> - printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, false,
>> TUNABLE_SECLEVEL_%s, %s},\n",
>> + printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, false, %s},\n",
>> types[t,n,m], minvals[t,n,m], maxvals[t,n,m],
>> - default_val[t,n,m], security_level[t,n,m], env_alias[t,n,m]);
>> + default_val[t,n,m], env_alias[t,n,m]);
>> }
>> print "};"
>> print "#endif"
>> diff --git a/sysdeps/x86_64/64/dl-tunables.list
>> b/sysdeps/x86_64/64/dl-tunables.list
>> index 0aab52e662..54a216a461 100644
>> --- a/sysdeps/x86_64/64/dl-tunables.list
>> +++ b/sysdeps/x86_64/64/dl-tunables.list
>> @@ -23,7 +23,6 @@ glibc {
>> minval: 0
>> maxval: 1
>> env_alias: LD_PREFER_MAP_32BIT_EXEC
>> - security_level: SXID_IGNORE
>> }
>> }
>> }
>
One semi-related question.
Two spelling nits.
One comment nit.
Some suggestions for improving the test cases.
Ok with those addressed.
Reviewed-by: DJ Delorie <dj@redhat.com>
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> diff --git a/elf/Makefile b/elf/Makefile
> index 9176cbf1e3..c824f7dab7 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -263,7 +263,6 @@ tests-static-normal := \
> tst-dl-iter-static \
> tst-dst-static \
> tst-env-setuid \
> - tst-env-setuid-tunables \
> tst-getauxval-static \
> tst-linkall-static \
> tst-single_threaded-pthread-static \
> @@ -276,10 +275,12 @@ tests-static-normal := \
> tests-static-internal := \
> tst-dl-printf-static \
> tst-dl_find_object-static \
> + tst-env-setuid-tunables \
> tst-ptrguard1-static \
> tst-stackguard1-static \
> tst-tls1-static \
> tst-tls1-static-non-pie \
> + tst-tunables \
> # tests-static-internal
Ok.
> CRT-tst-tls1-static-non-pie := $(csu-objpfx)crt1.o
> @@ -2696,6 +2697,8 @@ $(objpfx)tst-glibc-hwcaps-mask.out: \
> # tst-glibc-hwcaps-cache.
> $(objpfx)tst-glibc-hwcaps-cache.out: $(objpfx)tst-glibc-hwcaps
>
> +tst-tunables-ARGS = -- $(host-test-program-cmd)
> +
Ok.
> diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h
> index c88332657e..62d6d9e629 100644
> --- a/elf/dl-tunable-types.h
> +++ b/elf/dl-tunable-types.h
> @@ -64,16 +64,6 @@ struct _tunable
> tunable_val_t val; /* The value. */
> bool initialized; /* Flag to indicate that the tunable is
> initialized. */
> - tunable_seclevel_t security_level; /* Specify the security level for the
> - tunable with respect to AT_SECURE
> - programs. See description of
> - tunable_seclevel_t to see a
> - description of the values.
> -
> - Note that even if the tunable is
> - read, it may not get used by the
> - target module if the value is
> - considered unsafe. */
> /* Compatibility elements. */
> const char env_alias[TUNABLE_ALIAS_MAX]; /* The compatibility environment
> variable name. */
Ok. Should there be a related patch to remove "security_level" entries
from dl-tunables.list? I see support for parsing it in gen-tunables is
removed below, but leaving in the entry may create a false sense of
security.
I see a patch from the 3rd that does this, but it doesn't seem committed
yet.
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 24252af22c..a83bd2b8bc 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -154,50 +154,51 @@ __tunable_set_val (tunable_id_t id, tunable_val_t *valp, tunable_num_t *minp,
> do_tunable_update_val (cur, valp, minp, maxp);
> }
>
> -/* Parse the tunable string TUNESTR and adjust it to drop any tunables that may
> - be unsafe for AT_SECURE processes so that it can be used as the new
> - environment variable value for GLIBC_TUNABLES. VALSTRING is the original
> - environment variable string which we use to make NULL terminated values so
> - that we don't have to allocate memory again for it. */
> +/* Parse the tunable string VALSTRING. VALSTRING is a duplicated values,
Spelling: "a values" should be "a value" ?
> + where delimiters ':' are replaced with '\0', so string tunables are null
> + terminated. */
> static void
> -parse_tunables (char *tunestr, char *valstring)
> +parse_tunables (char *valstring)
> {
> - if (tunestr == NULL || *tunestr == '\0')
> + if (valstring == NULL || *valstring == '\0')
> return;
Ok.
> - char *p = tunestr;
> - size_t off = 0;
> + char *p = valstring;
> + bool done = false;
>
> - while (true)
> + while (!done)
> {
Ok.
> char *name = p;
> - size_t len = 0;
>
> /* First, find where the name ends. */
> - while (p[len] != '=' && p[len] != ':' && p[len] != '\0')
> - len++;
> + while (*p != '=' && *p != ':' && *p != '\0')
> + p++;
Ok, p points to the end of the tunable name
> /* If we reach the end of the string before getting a valid name-value
> pair, bail out. */
> - if (p[len] == '\0')
> + if (*p == '\0')
> break;
Missing value, ok.
> /* We did not find a valid name-value pair before encountering the
> colon. */
> - if (p[len]== ':')
> + if (*p == ':')
> {
> - p += len + 1;
> + p++;
> continue;
> }
Missing value, ok.
> - p += len + 1;
> + /* Skip the ':' or '='. */
> + p++;
Nit: This can't skip the ':' because we know there isn't one (above).
p now points to the start of the value. Ok.
> - /* Take the value from the valstring since we need to NULL terminate it. */
> - char *value = &valstring[p - tunestr];
> - len = 0;
> + const char *value = p;
"value" points to value, "valstring" still points to name. Ok.
> - while (p[len] != ':' && p[len] != '\0')
> - len++;
> + while (*p != ':' && *p != '\0')
> + p++;
> +
> + if (*p == '\0')
> + done = true;
> + else
> + *p++ = '\0';
Ok.
> @@ -206,50 +207,11 @@ parse_tunables (char *tunestr, char *valstring)
>
> if (tunable_is_name (cur->name, name))
> {
> - /* If we are in a secure context (AT_SECURE) then ignore the
> - tunable unless it is explicitly marked as secure. Tunable
> - values take precedence over their envvar aliases. We write
> - the tunables that are not SXID_ERASE back to TUNESTR, thus
> - dropping all SXID_ERASE tunables and any invalid or
> - unrecognized tunables. */
> - if (__libc_enable_secure)
> - {
> - if (cur->security_level != TUNABLE_SECLEVEL_SXID_ERASE)
> - {
> - if (off > 0)
> - tunestr[off++] = ':';
> -
> - const char *n = cur->name;
> -
> - while (*n != '\0')
> - tunestr[off++] = *n++;
> -
> - tunestr[off++] = '=';
> -
> - for (size_t j = 0; j < len; j++)
> - tunestr[off++] = value[j];
> - }
> -
> - if (cur->security_level != TUNABLE_SECLEVEL_NONE)
> - break;
> - }
> -
> - value[len] = '\0';
> tunable_initialize (cur, value);
> break;
> }
> }
Ok.
> -
> - /* We reached the end while processing the tunable string. */
> - if (p[len] == '\0')
> - break;
> -
> - p += len + 1;
> }
Ok. The "done" replaces this.
> -
> - /* Terminate tunestr before we leave. */
> - if (__libc_enable_secure)
> - tunestr[off] = '\0';
> }
Ok.
> @@ -263,16 +225,16 @@ __tunables_init (char **envp)
> size_t len = 0;
> char **prev_envp = envp;
>
> + /* Ignore tunables for AT_SECURE programs. */
> + if (__libc_enable_secure)
> + return;
> +
Ok.
> while ((envp = get_next_env (envp, &envname, &len, &envval,
> &prev_envp)) != NULL)
> {
> if (tunable_is_name ("GLIBC_TUNABLES", envname))
> {
> - char *new_env = tunables_strdup (envname);
> - if (new_env != NULL)
> - parse_tunables (new_env + len + 1, envval);
> - /* Put in the updated envval. */
> - *prev_envp = new_env;
> + parse_tunables (tunables_strdup (envval));
> continue;
> }
We don't free these, but that's OK.
> @@ -290,39 +252,6 @@ __tunables_init (char **envp)
> /* We have a match. Initialize and move on to the next line. */
> if (tunable_is_name (name, envname))
> {
> - /* For AT_SECURE binaries, we need to check the security settings of
> - the tunable and decide whether we read the value and also whether
> - we erase the value so that child processes don't inherit them in
> - the environment. */
> - if (__libc_enable_secure)
> - {
> - if (cur->security_level == TUNABLE_SECLEVEL_SXID_ERASE)
> - {
> - /* Erase the environment variable. */
> - char **ep = prev_envp;
> -
> - while (*ep != NULL)
> - {
> - if (tunable_is_name (name, *ep))
> - {
> - char **dp = ep;
> -
> - do
> - dp[0] = dp[1];
> - while (*dp++);
> - }
> - else
> - ++ep;
> - }
> - /* Reset the iterator so that we read the environment again
> - from the point we erased. */
> - envp = prev_envp;
> - }
> -
> - if (cur->security_level != TUNABLE_SECLEVEL_NONE)
> - continue;
> - }
> -
> tunable_initialize (cur, envval);
> break;
> }
Ok.
> diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
> index 695ba7192e..471895af7b 100644
> --- a/elf/dl-tunables.list
> +++ b/elf/dl-tunables.list
> @@ -21,14 +21,6 @@
> # minval: Optional minimum acceptable value
> # maxval: Optional maximum acceptable value
> # env_alias: An alias environment variable
> -# security_level: Specify security level of the tunable for AT_SECURE binaries.
> -# Valid values are:
> -#
> -# SXID_ERASE: (default) Do not read and do not pass on to
> -# child processes.
> -# SXID_IGNORE: Do not read, but retain for non-AT_SECURE
> -# subprocesses.
> -# NONE: Read all the time.
Ok.
> @@ -158,7 +150,6 @@ glibc {
> type: INT_32
> minval: 0
> maxval: 255
> - security_level: SXID_IGNORE
> }
> }
I assume this is based on some other patch which removes the rest, so ok.
> diff --git a/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c
> index 2603007b7b..ca02dbba58 100644
> --- a/elf/tst-env-setuid-tunables.c
> +++ b/elf/tst-env-setuid-tunables.c
> @@ -15,14 +15,10 @@
> License along with the GNU C Library; if not, see
> <https://www.gnu.org/licenses/>. */
>
> -/* Verify that tunables correctly filter out unsafe tunables like
> - glibc.malloc.check and glibc.malloc.mmap_threshold but also retain
> - glibc.malloc.mmap_threshold in an unprivileged child. */
> -
> -#define _LIBC 1
> -#include "config.h"
> -#undef _LIBC
> +/* Verify that GLIBC_TUNABLES is kept unchanged but no tunable is actually
> + enabled for AT_SECURE processes. */
Ok.
> +#include <dl-tunables.h>
Ok.
> @@ -40,7 +36,7 @@
> #include <support/test-driver.h>
> #include <support/capture_subprocess.h>
>
> -const char *teststrings[] =
> +static const char *teststrings[] =
Ok.
> @@ -74,6 +70,23 @@ test_child (int off)
> ret = 0;
> fflush (stdout);
>
> + /* Also check if the set tunables are effectively unchanged. */
> + int32_t check = TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL);
> + size_t mmap_threshold = TUNABLE_GET_FULL (glibc, malloc, mmap_threshold,
> + size_t, NULL);
> + int32_t perturb = TUNABLE_GET_FULL (glibc, malloc, perturb, int32_t, NULL);
> +
> + printf (" [%d] glibc.malloc.check=%d\n", off, check);
> + fflush (stdout);
> + printf (" [%d] glibc.malloc.mmap_threshold=%zu\n", off, mmap_threshold);
> + fflush (stdout);
> + printf (" [%d] glibc.malloc.perturb=%d\n", off, perturb);
> + fflush (stdout);
> +
> + ret |= check != 0;
> + ret |= mmap_threshold != 0;
> + ret |= perturb != 0;
> +
Ok.
> diff --git a/elf/tst-tunables.c b/elf/tst-tunables.c
> new file mode 100644
> index 0000000000..57cf532fc4
> --- /dev/null
> +++ b/elf/tst-tunables.c
> @@ -0,0 +1,244 @@
> +/* Check GLIBC_TUNABLES parsing.
> + Copyright (C) 2023 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#include <array_length.h>
> +#include <dl-tunables.h>
> +#include <getopt.h>
> +#include <intprops.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <support/capture_subprocess.h>
> +#include <support/check.h>
Ok.
> +static int restart;
> +#define CMDLINE_OPTIONS \
> + { "restart", no_argument, &restart, 1 },
Ok.
> +static const struct test_t
> +{
> + const char *env;
> + int32_t expected_malloc_check;
> + size_t expected_mmap_threshold;
> + int32_t expected_perturb;
> +} tests[] =
> +{
> + /* Expected tunable format. */
> + {
> + "glibc.malloc.check=2",
> + 2,
> + 0,
> + 0,
> + },
> + {
> + "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> + 2,
> + 4096,
> + 0,
> + },
> + /* Empty tunable are ignored. */
> + {
> + "glibc.malloc.check=2::glibc.malloc.mmap_threshold=4096",
> + 2,
> + 4096,
> + 0,
> + },
> + /* As well empty values. */
> + {
> + "glibc.malloc.check=:glibc.malloc.mmap_threshold=4096",
> + 0,
> + 4096,
> + 0,
> + },
> + /* Tunable are processed from left to right, so last one is the one set. */
> + {
> + "glibc.malloc.check=1:glibc.malloc.check=2",
> + 2,
> + 0,
> + 0,
> + },
> + {
> + "glibc.malloc.check=2:glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
Suggest different values for check, so you know which is used.
> + 2,
> + 4096,
> + 0,
> + },
> + {
> + "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2",
Likewise.
> + 2,
> + 4096,
> + 0,
> + },
> + /* 0x800 is larger than tunable maxval (0xff), so the tunable is unchanged. */
> + {
> + "glibc.malloc.perturb=0x800",
> + 0,
> + 0,
> + 0,
> + },
> + {
> + "glibc.malloc.perturb=0x55",
> + 0,
> + 0,
> + 0x55,
> + },
> + /* Out of range values are just ignored. */
> + {
> + "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
> + 0,
> + 4096,
> + 0,
> + },
> + /* Invalid keys are ignored. */
> + {
> + ":glibc.malloc.garbage=2:glibc.malloc.check=1",
> + 1,
> + 0,
> + 0,
> + },
> + {
> + "glibc.malloc.perturb=0x800:not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> + 0,
> + 4096,
> + 0,
> + },
> + {
> + "glibc.not_valid.check=2:glibc.malloc.mmap_threshold=4096",
> + 0,
> + 4096,
> + 0,
> + },
> + {
> + "not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> + 0,
> + 4096,
> + 0,
> + },
> + /* Invalid subkeys are ignored. */
> + {
> + "glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096:glibc.malloc.check=2",
> + 2,
> + 0,
> + 0,
> + },
> + {
> + "glibc.malloc.check=4:glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096",
> + 0,
> + 0,
> + 0,
> + },
> + {
> + "not_valid.malloc.check=2",
> + 0,
> + 0,
> + 0,
> + },
> + {
> + "glibc.not_valid.check=2",
> + 0,
> + 0,
> + 0,
> + },
> + /* An ill-formatted tunable in the for key=key=value will considere the
> + value as 'key=value' (which can not be parsed as an integer). */
> + {
> + "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096",
> + 0,
> + 0,
> + 0,
> + },
> + /* The ill-formatted tunable is also skipped. */
> + {
> + "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2",
> + 2,
> + 0,
> + 0,
> + },
> + /* For an integer tunable, parse will stop on non number character. */
> + {
> + "glibc.malloc.check=2=2",
> + 2,
Better to put different numbers here, so you know which is parsed, but ok.
> + 0,
> + 0,
> + },
> + {
> + "glibc.malloc.check=2=2:glibc.malloc.mmap_threshold=4096",
> + 2,
> + 4096,
> + 0,
> + }
> +};
Ok.
> +static int
> +handle_restart (int i)
> +{
> + TEST_COMPARE (tests[i].expected_malloc_check,
> + TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL));
> + TEST_COMPARE (tests[i].expected_mmap_threshold,
> + TUNABLE_GET_FULL (glibc, malloc, mmap_threshold, size_t, NULL));
> + TEST_COMPARE (tests[i].expected_perturb,
> + TUNABLE_GET_FULL (glibc, malloc, perturb, int32_t, NULL));
> + return 0;
> +}
Ok.
> +static int
> +do_test (int argc, char *argv[])
> +{
> + /* We must have either:
> + - One our fource parameters left if called initially:
Spelling?
> + + path to ld.so optional
> + + "--library-path" optional
> + + the library path optional
> + + the application name
> + + the test to check */
> +
> + TEST_VERIFY_EXIT (argc == 2 || argc == 5);
Ok.
> + if (restart)
> + return handle_restart (atoi (argv[1]));
Set by --restart, ok.
> + char nteststr[INT_BUFSIZE_BOUND (int)];
Ok.
> + char *spargv[10];
> + {
> + int i = 0;
> + for (; i < argc - 1; i++)
> + spargv[i] = argv[i + 1];
> + spargv[i++] = (char *) "--direct";
> + spargv[i++] = (char *) "--restart";
> + spargv[i++] = nteststr;
> + spargv[i] = NULL;
> + }
No overflow prevention, but this is internal, so ok.
> + for (int i = 0; i < array_length (tests); i++)
> + {
> + snprintf (nteststr, sizeof nteststr, "%d", i);
> +
> + printf ("[%d] Spawned test for %s\n", i, tests[i].env);
> + setenv ("GLIBC_TUNABLES", tests[i].env, 1);
> + struct support_capture_subprocess result
> + = support_capture_subprogram (spargv[0], spargv);
> + support_capture_subprocess_check (&result, "tst-tunables", 0,
> + sc_allow_stderr);
> + support_capture_subprocess_free (&result);
> + }
> +
> + return 0;
> +}
Ok.
> +#define TEST_FUNCTION_ARGV do_test
> +#include <support/test-driver.c>
Ok.
> diff --git a/manual/README.tunables b/manual/README.tunables
> index 605ddd78cd..72ae00dc02 100644
> --- a/manual/README.tunables
> +++ b/manual/README.tunables
> @@ -59,15 +59,6 @@ The list of allowed attributes are:
>
> - env_alias: An alias environment variable
>
> -- security_level: Specify security level of the tunable for AT_SECURE
> - binaries. Valid values are:
> -
> - SXID_ERASE: (default) Do not read and do not pass on to
> - child processes.
> - SXID_IGNORE: Do not read, but retain for non-AT_SECURE
> - child processes.
> - NONE: Read all the time.
> -
Ok.
> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
> index d6de100df0..1e9d6b534e 100644
> --- a/scripts/gen-tunables.awk
> +++ b/scripts/gen-tunables.awk
> @@ -61,9 +61,6 @@ $1 == "}" {
> if (!env_alias[top_ns,ns,tunable]) {
> env_alias[top_ns,ns,tunable] = "{0}"
> }
> - if (!security_level[top_ns,ns,tunable]) {
> - security_level[top_ns,ns,tunable] = "SXID_ERASE"
> - }
Ok.
> @@ -118,17 +115,6 @@ $1 == "}" {
> if (len > max_alias_len)
> max_alias_len = len
> }
> - else if (attr == "security_level") {
> - if (val == "SXID_ERASE" || val == "SXID_IGNORE" || val == "NONE") {
> - security_level[top_ns,ns,tunable] = val
> - }
> - else {
> - printf("Line %d: Invalid value (%s) for security_level: %s, ", NR, val,
> - $0)
> - print("Allowed values are 'SXID_ERASE', 'SXID_IGNORE', or 'NONE'")
> - exit 1
> - }
> - }
Ok.
> @@ -177,9 +163,9 @@ END {
> n = indices[2];
> m = indices[3];
> printf (" {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
> - printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, false, TUNABLE_SECLEVEL_%s, %s},\n",
> + printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, false, %s},\n",
> types[t,n,m], minvals[t,n,m], maxvals[t,n,m],
> - default_val[t,n,m], security_level[t,n,m], env_alias[t,n,m]);
> + default_val[t,n,m], env_alias[t,n,m]);
> }
Ok.
> diff --git a/sysdeps/x86_64/64/dl-tunables.list b/sysdeps/x86_64/64/dl-tunables.list
> index 0aab52e662..54a216a461 100644
> --- a/sysdeps/x86_64/64/dl-tunables.list
> +++ b/sysdeps/x86_64/64/dl-tunables.list
> @@ -23,7 +23,6 @@ glibc {
> minval: 0
> maxval: 1
> env_alias: LD_PREFER_MAP_32BIT_EXEC
> - security_level: SXID_IGNORE
> }
> }
> }
Ok.
On 27/10/23 23:14, DJ Delorie wrote:
>
> One semi-related question.
> Two spelling nits.
> One comment nit.
> Some suggestions for improving the test cases.
>
> Ok with those addressed.
Thanks!
>
> Reviewed-by: DJ Delorie <dj@redhat.com>
>
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>> diff --git a/elf/Makefile b/elf/Makefile
>> index 9176cbf1e3..c824f7dab7 100644
>> --- a/elf/Makefile
>> +++ b/elf/Makefile
>> @@ -263,7 +263,6 @@ tests-static-normal := \
>> tst-dl-iter-static \
>> tst-dst-static \
>> tst-env-setuid \
>> - tst-env-setuid-tunables \
>> tst-getauxval-static \
>> tst-linkall-static \
>> tst-single_threaded-pthread-static \
>> @@ -276,10 +275,12 @@ tests-static-normal := \
>> tests-static-internal := \
>> tst-dl-printf-static \
>> tst-dl_find_object-static \
>> + tst-env-setuid-tunables \
>> tst-ptrguard1-static \
>> tst-stackguard1-static \
>> tst-tls1-static \
>> tst-tls1-static-non-pie \
>> + tst-tunables \
>> # tests-static-internal
>
> Ok.
>
>> CRT-tst-tls1-static-non-pie := $(csu-objpfx)crt1.o
>> @@ -2696,6 +2697,8 @@ $(objpfx)tst-glibc-hwcaps-mask.out: \
>> # tst-glibc-hwcaps-cache.
>> $(objpfx)tst-glibc-hwcaps-cache.out: $(objpfx)tst-glibc-hwcaps
>>
>> +tst-tunables-ARGS = -- $(host-test-program-cmd)
>> +
>
> Ok.
>
>> diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h
>> index c88332657e..62d6d9e629 100644
>> --- a/elf/dl-tunable-types.h
>> +++ b/elf/dl-tunable-types.h
>> @@ -64,16 +64,6 @@ struct _tunable
>> tunable_val_t val; /* The value. */
>> bool initialized; /* Flag to indicate that the tunable is
>> initialized. */
>> - tunable_seclevel_t security_level; /* Specify the security level for the
>> - tunable with respect to AT_SECURE
>> - programs. See description of
>> - tunable_seclevel_t to see a
>> - description of the values.
>> -
>> - Note that even if the tunable is
>> - read, it may not get used by the
>> - target module if the value is
>> - considered unsafe. */
>> /* Compatibility elements. */
>> const char env_alias[TUNABLE_ALIAS_MAX]; /* The compatibility environment
>> variable name. */
>
> Ok. Should there be a related patch to remove "security_level" entries
> from dl-tunables.list? I see support for parsing it in gen-tunables is
> removed below, but leaving in the entry may create a false sense of
> security.
>
> I see a patch from the 3rd that does this, but it doesn't seem committed
> yet.
It was an onverlook from my patch, I send a newer version with
security_level removed from all tunables list files.
>
>> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
>> index 24252af22c..a83bd2b8bc 100644
>> --- a/elf/dl-tunables.c
>> +++ b/elf/dl-tunables.c
>> @@ -154,50 +154,51 @@ __tunable_set_val (tunable_id_t id, tunable_val_t *valp, tunable_num_t *minp,
>> do_tunable_update_val (cur, valp, minp, maxp);
>> }
>>
>> -/* Parse the tunable string TUNESTR and adjust it to drop any tunables that may
>> - be unsafe for AT_SECURE processes so that it can be used as the new
>> - environment variable value for GLIBC_TUNABLES. VALSTRING is the original
>> - environment variable string which we use to make NULL terminated values so
>> - that we don't have to allocate memory again for it. */
>> +/* Parse the tunable string VALSTRING. VALSTRING is a duplicated values,
>
> Spelling: "a values" should be "a value" ?
Ack.
>
>> + where delimiters ':' are replaced with '\0', so string tunables are null
>> + terminated. */
>
>
>
>> static void
>> -parse_tunables (char *tunestr, char *valstring)
>> +parse_tunables (char *valstring)
>> {
>> - if (tunestr == NULL || *tunestr == '\0')
>> + if (valstring == NULL || *valstring == '\0')
>> return;
>
> Ok.
>
>> - char *p = tunestr;
>> - size_t off = 0;
>> + char *p = valstring;
>> + bool done = false;
>>
>> - while (true)
>> + while (!done)
>> {
>
> Ok.
>
>> char *name = p;
>> - size_t len = 0;
>>
>> /* First, find where the name ends. */
>> - while (p[len] != '=' && p[len] != ':' && p[len] != '\0')
>> - len++;
>> + while (*p != '=' && *p != ':' && *p != '\0')
>> + p++;
>
> Ok, p points to the end of the tunable name
>
>> /* If we reach the end of the string before getting a valid name-value
>> pair, bail out. */
>> - if (p[len] == '\0')
>> + if (*p == '\0')
>> break;
>
> Missing value, ok.
>
>> /* We did not find a valid name-value pair before encountering the
>> colon. */
>> - if (p[len]== ':')
>> + if (*p == ':')
>> {
>> - p += len + 1;
>> + p++;
>> continue;
>> }
>
> Missing value, ok.
>
>> - p += len + 1;
>> + /* Skip the ':' or '='. */
>> + p++;
>
> Nit: This can't skip the ':' because we know there isn't one (above).
>
> p now points to the start of the value. Ok.
Ack, I will fix the comment.
>
>> - /* Take the value from the valstring since we need to NULL terminate it. */
>> - char *value = &valstring[p - tunestr];
>> - len = 0;
>> + const char *value = p;
>
> "value" points to value, "valstring" still points to name. Ok.
>
>> - while (p[len] != ':' && p[len] != '\0')
>> - len++;
>> + while (*p != ':' && *p != '\0')
>> + p++;
>> +
>> + if (*p == '\0')
>> + done = true;
>> + else
>> + *p++ = '\0';
>
> Ok.
>
>> @@ -206,50 +207,11 @@ parse_tunables (char *tunestr, char *valstring)
>>
>> if (tunable_is_name (cur->name, name))
>> {
>> - /* If we are in a secure context (AT_SECURE) then ignore the
>> - tunable unless it is explicitly marked as secure. Tunable
>> - values take precedence over their envvar aliases. We write
>> - the tunables that are not SXID_ERASE back to TUNESTR, thus
>> - dropping all SXID_ERASE tunables and any invalid or
>> - unrecognized tunables. */
>> - if (__libc_enable_secure)
>> - {
>> - if (cur->security_level != TUNABLE_SECLEVEL_SXID_ERASE)
>> - {
>> - if (off > 0)
>> - tunestr[off++] = ':';
>> -
>> - const char *n = cur->name;
>> -
>> - while (*n != '\0')
>> - tunestr[off++] = *n++;
>> -
>> - tunestr[off++] = '=';
>> -
>> - for (size_t j = 0; j < len; j++)
>> - tunestr[off++] = value[j];
>> - }
>> -
>> - if (cur->security_level != TUNABLE_SECLEVEL_NONE)
>> - break;
>> - }
>> -
>> - value[len] = '\0';
>> tunable_initialize (cur, value);
>> break;
>> }
>> }
>
> Ok.
>
>> -
>> - /* We reached the end while processing the tunable string. */
>> - if (p[len] == '\0')
>> - break;
>> -
>> - p += len + 1;
>> }
>
> Ok. The "done" replaces this.
>
>> -
>> - /* Terminate tunestr before we leave. */
>> - if (__libc_enable_secure)
>> - tunestr[off] = '\0';
>> }
>
> Ok.
>
>> @@ -263,16 +225,16 @@ __tunables_init (char **envp)
>> size_t len = 0;
>> char **prev_envp = envp;
>>
>> + /* Ignore tunables for AT_SECURE programs. */
>> + if (__libc_enable_secure)
>> + return;
>> +
>
> Ok.
>
>> while ((envp = get_next_env (envp, &envname, &len, &envval,
>> &prev_envp)) != NULL)
>> {
>> if (tunable_is_name ("GLIBC_TUNABLES", envname))
>> {
>> - char *new_env = tunables_strdup (envname);
>> - if (new_env != NULL)
>> - parse_tunables (new_env + len + 1, envval);
>> - /* Put in the updated envval. */
>> - *prev_envp = new_env;
>> + parse_tunables (tunables_strdup (envval));
>> continue;
>> }
>
> We don't free these, but that's OK.
>
>> @@ -290,39 +252,6 @@ __tunables_init (char **envp)
>> /* We have a match. Initialize and move on to the next line. */
>> if (tunable_is_name (name, envname))
>> {
>> - /* For AT_SECURE binaries, we need to check the security settings of
>> - the tunable and decide whether we read the value and also whether
>> - we erase the value so that child processes don't inherit them in
>> - the environment. */
>> - if (__libc_enable_secure)
>> - {
>> - if (cur->security_level == TUNABLE_SECLEVEL_SXID_ERASE)
>> - {
>> - /* Erase the environment variable. */
>> - char **ep = prev_envp;
>> -
>> - while (*ep != NULL)
>> - {
>> - if (tunable_is_name (name, *ep))
>> - {
>> - char **dp = ep;
>> -
>> - do
>> - dp[0] = dp[1];
>> - while (*dp++);
>> - }
>> - else
>> - ++ep;
>> - }
>> - /* Reset the iterator so that we read the environment again
>> - from the point we erased. */
>> - envp = prev_envp;
>> - }
>> -
>> - if (cur->security_level != TUNABLE_SECLEVEL_NONE)
>> - continue;
>> - }
>> -
>> tunable_initialize (cur, envval);
>> break;
>> }
>
> Ok.
>
>> diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
>> index 695ba7192e..471895af7b 100644
>> --- a/elf/dl-tunables.list
>> +++ b/elf/dl-tunables.list
>> @@ -21,14 +21,6 @@
>> # minval: Optional minimum acceptable value
>> # maxval: Optional maximum acceptable value
>> # env_alias: An alias environment variable
>> -# security_level: Specify security level of the tunable for AT_SECURE binaries.
>> -# Valid values are:
>> -#
>> -# SXID_ERASE: (default) Do not read and do not pass on to
>> -# child processes.
>> -# SXID_IGNORE: Do not read, but retain for non-AT_SECURE
>> -# subprocesses.
>> -# NONE: Read all the time.
>
> Ok.
>
>> @@ -158,7 +150,6 @@ glibc {
>> type: INT_32
>> minval: 0
>> maxval: 255
>> - security_level: SXID_IGNORE
>> }
>> }
>
> I assume this is based on some other patch which removes the rest, so ok.
>
>> diff --git a/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c
>> index 2603007b7b..ca02dbba58 100644
>> --- a/elf/tst-env-setuid-tunables.c
>> +++ b/elf/tst-env-setuid-tunables.c
>> @@ -15,14 +15,10 @@
>> License along with the GNU C Library; if not, see
>> <https://www.gnu.org/licenses/>. */
>>
>> -/* Verify that tunables correctly filter out unsafe tunables like
>> - glibc.malloc.check and glibc.malloc.mmap_threshold but also retain
>> - glibc.malloc.mmap_threshold in an unprivileged child. */
>> -
>> -#define _LIBC 1
>> -#include "config.h"
>> -#undef _LIBC
>> +/* Verify that GLIBC_TUNABLES is kept unchanged but no tunable is actually
>> + enabled for AT_SECURE processes. */
>
> Ok.
>
>> +#include <dl-tunables.h>
>
> Ok.
>
>> @@ -40,7 +36,7 @@
>> #include <support/test-driver.h>
>> #include <support/capture_subprocess.h>
>>
>> -const char *teststrings[] =
>> +static const char *teststrings[] =
>
> Ok.
>
>> @@ -74,6 +70,23 @@ test_child (int off)
>> ret = 0;
>> fflush (stdout);
>>
>> + /* Also check if the set tunables are effectively unchanged. */
>> + int32_t check = TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL);
>> + size_t mmap_threshold = TUNABLE_GET_FULL (glibc, malloc, mmap_threshold,
>> + size_t, NULL);
>> + int32_t perturb = TUNABLE_GET_FULL (glibc, malloc, perturb, int32_t, NULL);
>> +
>> + printf (" [%d] glibc.malloc.check=%d\n", off, check);
>> + fflush (stdout);
>> + printf (" [%d] glibc.malloc.mmap_threshold=%zu\n", off, mmap_threshold);
>> + fflush (stdout);
>> + printf (" [%d] glibc.malloc.perturb=%d\n", off, perturb);
>> + fflush (stdout);
>> +
>> + ret |= check != 0;
>> + ret |= mmap_threshold != 0;
>> + ret |= perturb != 0;
>> +
>
> Ok.
>
>> diff --git a/elf/tst-tunables.c b/elf/tst-tunables.c
>> new file mode 100644
>> index 0000000000..57cf532fc4
>> --- /dev/null
>> +++ b/elf/tst-tunables.c
>> @@ -0,0 +1,244 @@
>> +/* Check GLIBC_TUNABLES parsing.
>> + Copyright (C) 2023 Free Software Foundation, Inc.
>> + This file is part of the GNU C Library.
>> +
>> + The GNU C Library is free software; you can redistribute it and/or
>> + modify it under the terms of the GNU Lesser General Public
>> + License as published by the Free Software Foundation; either
>> + version 2.1 of the License, or (at your option) any later version.
>> +
>> + The GNU C Library is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + Lesser General Public License for more details.
>> +
>> + You should have received a copy of the GNU Lesser General Public
>> + License along with the GNU C Library; if not, see
>> + <https://www.gnu.org/licenses/>. */
>> +
>> +#include <array_length.h>
>> +#include <dl-tunables.h>
>> +#include <getopt.h>
>> +#include <intprops.h>
>> +#include <stdint.h>
>> +#include <stdlib.h>
>> +#include <support/capture_subprocess.h>
>> +#include <support/check.h>
>
> Ok.
>
>> +static int restart;
>> +#define CMDLINE_OPTIONS \
>> + { "restart", no_argument, &restart, 1 },
>
> Ok.
>
>> +static const struct test_t
>> +{
>> + const char *env;
>> + int32_t expected_malloc_check;
>> + size_t expected_mmap_threshold;
>> + int32_t expected_perturb;
>> +} tests[] =
>> +{
>> + /* Expected tunable format. */
>> + {
>> + "glibc.malloc.check=2",
>> + 2,
>> + 0,
>> + 0,
>> + },
>> + {
>> + "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>> + 2,
>> + 4096,
>> + 0,
>> + },
>> + /* Empty tunable are ignored. */
>> + {
>> + "glibc.malloc.check=2::glibc.malloc.mmap_threshold=4096",
>> + 2,
>> + 4096,
>> + 0,
>> + },
>> + /* As well empty values. */
>> + {
>> + "glibc.malloc.check=:glibc.malloc.mmap_threshold=4096",
>> + 0,
>> + 4096,
>> + 0,
>> + },
>> + /* Tunable are processed from left to right, so last one is the one set. */
>> + {
>> + "glibc.malloc.check=1:glibc.malloc.check=2",
>> + 2,
>> + 0,
>> + 0,
>> + },
>> + {
>> + "glibc.malloc.check=2:glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>
> Suggest different values for check, so you know which is used.
Ack.
>
>> + 2,
>> + 4096,
>> + 0,
>> + },
>> + {
>> + "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2",
>
> Likewise.
Ack.
>
>> + 2,
>> + 4096,
>> + 0,
>> + },
>> + /* 0x800 is larger than tunable maxval (0xff), so the tunable is unchanged. */
>> + {
>> + "glibc.malloc.perturb=0x800",
>> + 0,
>> + 0,
>> + 0,
>> + },
>> + {
>> + "glibc.malloc.perturb=0x55",
>> + 0,
>> + 0,
>> + 0x55,
>> + },
>> + /* Out of range values are just ignored. */
>> + {
>> + "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
>> + 0,
>> + 4096,
>> + 0,
>> + },
>> + /* Invalid keys are ignored. */
>> + {
>> + ":glibc.malloc.garbage=2:glibc.malloc.check=1",
>> + 1,
>> + 0,
>> + 0,
>> + },
>> + {
>> + "glibc.malloc.perturb=0x800:not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>> + 0,
>> + 4096,
>> + 0,
>> + },
>> + {
>> + "glibc.not_valid.check=2:glibc.malloc.mmap_threshold=4096",
>> + 0,
>> + 4096,
>> + 0,
>> + },
>> + {
>> + "not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>> + 0,
>> + 4096,
>> + 0,
>> + },
>> + /* Invalid subkeys are ignored. */
>> + {
>> + "glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096:glibc.malloc.check=2",
>> + 2,
>> + 0,
>> + 0,
>> + },
>> + {
>> + "glibc.malloc.check=4:glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096",
>> + 0,
>> + 0,
>> + 0,
>> + },
>> + {
>> + "not_valid.malloc.check=2",
>> + 0,
>> + 0,
>> + 0,
>> + },
>> + {
>> + "glibc.not_valid.check=2",
>> + 0,
>> + 0,
>> + 0,
>> + },
>> + /* An ill-formatted tunable in the for key=key=value will considere the
>> + value as 'key=value' (which can not be parsed as an integer). */
>> + {
>> + "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096",
>> + 0,
>> + 0,
>> + 0,
>> + },
>> + /* The ill-formatted tunable is also skipped. */
>> + {
>> + "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2",
>> + 2,
>> + 0,
>> + 0,
>> + },
>> + /* For an integer tunable, parse will stop on non number character. */
>> + {
>> + "glibc.malloc.check=2=2",
>> + 2,
>
> Better to put different numbers here, so you know which is parsed, but ok.
>
>> + 0,
>> + 0,
>> + },
>> + {
>> + "glibc.malloc.check=2=2:glibc.malloc.mmap_threshold=4096",
>> + 2,
>> + 4096,
>> + 0,
>> + }
>> +};
>
> Ok.
>
>> +static int
>> +handle_restart (int i)
>> +{
>> + TEST_COMPARE (tests[i].expected_malloc_check,
>> + TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL));
>> + TEST_COMPARE (tests[i].expected_mmap_threshold,
>> + TUNABLE_GET_FULL (glibc, malloc, mmap_threshold, size_t, NULL));
>> + TEST_COMPARE (tests[i].expected_perturb,
>> + TUNABLE_GET_FULL (glibc, malloc, perturb, int32_t, NULL));
>> + return 0;
>> +}
>
> Ok.
>
>> +static int
>> +do_test (int argc, char *argv[])
>> +{
>> + /* We must have either:
>> + - One our fource parameters left if called initially:
>
> Spelling?
>
>> + + path to ld.so optional
>> + + "--library-path" optional
>> + + the library path optional
>> + + the application name
>> + + the test to check */
>> +
>> + TEST_VERIFY_EXIT (argc == 2 || argc == 5);
>
> Ok.
>
>> + if (restart)
>> + return handle_restart (atoi (argv[1]));
>
> Set by --restart, ok.
>
>> + char nteststr[INT_BUFSIZE_BOUND (int)];
>
> Ok.
>
>> + char *spargv[10];
>> + {
>> + int i = 0;
>> + for (; i < argc - 1; i++)
>> + spargv[i] = argv[i + 1];
>> + spargv[i++] = (char *) "--direct";
>> + spargv[i++] = (char *) "--restart";
>> + spargv[i++] = nteststr;
>> + spargv[i] = NULL;
>> + }
>
> No overflow prevention, but this is internal, so ok.
>
>> + for (int i = 0; i < array_length (tests); i++)
>> + {
>> + snprintf (nteststr, sizeof nteststr, "%d", i);
>> +
>> + printf ("[%d] Spawned test for %s\n", i, tests[i].env);
>> + setenv ("GLIBC_TUNABLES", tests[i].env, 1);
>> + struct support_capture_subprocess result
>> + = support_capture_subprogram (spargv[0], spargv);
>> + support_capture_subprocess_check (&result, "tst-tunables", 0,
>> + sc_allow_stderr);
>> + support_capture_subprocess_free (&result);
>> + }
>> +
>> + return 0;
>> +}
>
> Ok.
>
>> +#define TEST_FUNCTION_ARGV do_test
>> +#include <support/test-driver.c>
>
> Ok.
>
>> diff --git a/manual/README.tunables b/manual/README.tunables
>> index 605ddd78cd..72ae00dc02 100644
>> --- a/manual/README.tunables
>> +++ b/manual/README.tunables
>> @@ -59,15 +59,6 @@ The list of allowed attributes are:
>>
>> - env_alias: An alias environment variable
>>
>> -- security_level: Specify security level of the tunable for AT_SECURE
>> - binaries. Valid values are:
>> -
>> - SXID_ERASE: (default) Do not read and do not pass on to
>> - child processes.
>> - SXID_IGNORE: Do not read, but retain for non-AT_SECURE
>> - child processes.
>> - NONE: Read all the time.
>> -
>
> Ok.
>
>> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
>> index d6de100df0..1e9d6b534e 100644
>> --- a/scripts/gen-tunables.awk
>> +++ b/scripts/gen-tunables.awk
>> @@ -61,9 +61,6 @@ $1 == "}" {
>> if (!env_alias[top_ns,ns,tunable]) {
>> env_alias[top_ns,ns,tunable] = "{0}"
>> }
>> - if (!security_level[top_ns,ns,tunable]) {
>> - security_level[top_ns,ns,tunable] = "SXID_ERASE"
>> - }
>
> Ok.
>
>> @@ -118,17 +115,6 @@ $1 == "}" {
>> if (len > max_alias_len)
>> max_alias_len = len
>> }
>> - else if (attr == "security_level") {
>> - if (val == "SXID_ERASE" || val == "SXID_IGNORE" || val == "NONE") {
>> - security_level[top_ns,ns,tunable] = val
>> - }
>> - else {
>> - printf("Line %d: Invalid value (%s) for security_level: %s, ", NR, val,
>> - $0)
>> - print("Allowed values are 'SXID_ERASE', 'SXID_IGNORE', or 'NONE'")
>> - exit 1
>> - }
>> - }
>
> Ok.
>
>> @@ -177,9 +163,9 @@ END {
>> n = indices[2];
>> m = indices[3];
>> printf (" {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
>> - printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, false, TUNABLE_SECLEVEL_%s, %s},\n",
>> + printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, false, %s},\n",
>> types[t,n,m], minvals[t,n,m], maxvals[t,n,m],
>> - default_val[t,n,m], security_level[t,n,m], env_alias[t,n,m]);
>> + default_val[t,n,m], env_alias[t,n,m]);
>> }
>
> Ok.
>
>> diff --git a/sysdeps/x86_64/64/dl-tunables.list b/sysdeps/x86_64/64/dl-tunables.list
>> index 0aab52e662..54a216a461 100644
>> --- a/sysdeps/x86_64/64/dl-tunables.list
>> +++ b/sysdeps/x86_64/64/dl-tunables.list
>> @@ -23,7 +23,6 @@ glibc {
>> minval: 0
>> maxval: 1
>> env_alias: LD_PREFER_MAP_32BIT_EXEC
>> - security_level: SXID_IGNORE
>> }
>> }
>> }
>
> Ok.
>
@@ -263,7 +263,6 @@ tests-static-normal := \
tst-dl-iter-static \
tst-dst-static \
tst-env-setuid \
- tst-env-setuid-tunables \
tst-getauxval-static \
tst-linkall-static \
tst-single_threaded-pthread-static \
@@ -276,10 +275,12 @@ tests-static-normal := \
tests-static-internal := \
tst-dl-printf-static \
tst-dl_find_object-static \
+ tst-env-setuid-tunables \
tst-ptrguard1-static \
tst-stackguard1-static \
tst-tls1-static \
tst-tls1-static-non-pie \
+ tst-tunables \
# tests-static-internal
CRT-tst-tls1-static-non-pie := $(csu-objpfx)crt1.o
@@ -2696,6 +2697,8 @@ $(objpfx)tst-glibc-hwcaps-mask.out: \
# tst-glibc-hwcaps-cache.
$(objpfx)tst-glibc-hwcaps-cache.out: $(objpfx)tst-glibc-hwcaps
+tst-tunables-ARGS = -- $(host-test-program-cmd)
+
$(objpfx)list-tunables.out: tst-rtld-list-tunables.sh $(objpfx)ld.so
$(SHELL) $< $(objpfx)ld.so '$(test-wrapper-env)' \
'$(run_program_env)' > $(objpfx)/tst-rtld-list-tunables.out
@@ -64,16 +64,6 @@ struct _tunable
tunable_val_t val; /* The value. */
bool initialized; /* Flag to indicate that the tunable is
initialized. */
- tunable_seclevel_t security_level; /* Specify the security level for the
- tunable with respect to AT_SECURE
- programs. See description of
- tunable_seclevel_t to see a
- description of the values.
-
- Note that even if the tunable is
- read, it may not get used by the
- target module if the value is
- considered unsafe. */
/* Compatibility elements. */
const char env_alias[TUNABLE_ALIAS_MAX]; /* The compatibility environment
variable name. */
@@ -154,50 +154,51 @@ __tunable_set_val (tunable_id_t id, tunable_val_t *valp, tunable_num_t *minp,
do_tunable_update_val (cur, valp, minp, maxp);
}
-/* Parse the tunable string TUNESTR and adjust it to drop any tunables that may
- be unsafe for AT_SECURE processes so that it can be used as the new
- environment variable value for GLIBC_TUNABLES. VALSTRING is the original
- environment variable string which we use to make NULL terminated values so
- that we don't have to allocate memory again for it. */
+/* Parse the tunable string VALSTRING. VALSTRING is a duplicated values,
+ where delimiters ':' are replaced with '\0', so string tunables are null
+ terminated. */
static void
-parse_tunables (char *tunestr, char *valstring)
+parse_tunables (char *valstring)
{
- if (tunestr == NULL || *tunestr == '\0')
+ if (valstring == NULL || *valstring == '\0')
return;
- char *p = tunestr;
- size_t off = 0;
+ char *p = valstring;
+ bool done = false;
- while (true)
+ while (!done)
{
char *name = p;
- size_t len = 0;
/* First, find where the name ends. */
- while (p[len] != '=' && p[len] != ':' && p[len] != '\0')
- len++;
+ while (*p != '=' && *p != ':' && *p != '\0')
+ p++;
/* If we reach the end of the string before getting a valid name-value
pair, bail out. */
- if (p[len] == '\0')
+ if (*p == '\0')
break;
/* We did not find a valid name-value pair before encountering the
colon. */
- if (p[len]== ':')
+ if (*p == ':')
{
- p += len + 1;
+ p++;
continue;
}
- p += len + 1;
+ /* Skip the ':' or '='. */
+ p++;
- /* Take the value from the valstring since we need to NULL terminate it. */
- char *value = &valstring[p - tunestr];
- len = 0;
+ const char *value = p;
- while (p[len] != ':' && p[len] != '\0')
- len++;
+ while (*p != ':' && *p != '\0')
+ p++;
+
+ if (*p == '\0')
+ done = true;
+ else
+ *p++ = '\0';
/* Add the tunable if it exists. */
for (size_t i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
@@ -206,50 +207,11 @@ parse_tunables (char *tunestr, char *valstring)
if (tunable_is_name (cur->name, name))
{
- /* If we are in a secure context (AT_SECURE) then ignore the
- tunable unless it is explicitly marked as secure. Tunable
- values take precedence over their envvar aliases. We write
- the tunables that are not SXID_ERASE back to TUNESTR, thus
- dropping all SXID_ERASE tunables and any invalid or
- unrecognized tunables. */
- if (__libc_enable_secure)
- {
- if (cur->security_level != TUNABLE_SECLEVEL_SXID_ERASE)
- {
- if (off > 0)
- tunestr[off++] = ':';
-
- const char *n = cur->name;
-
- while (*n != '\0')
- tunestr[off++] = *n++;
-
- tunestr[off++] = '=';
-
- for (size_t j = 0; j < len; j++)
- tunestr[off++] = value[j];
- }
-
- if (cur->security_level != TUNABLE_SECLEVEL_NONE)
- break;
- }
-
- value[len] = '\0';
tunable_initialize (cur, value);
break;
}
}
-
- /* We reached the end while processing the tunable string. */
- if (p[len] == '\0')
- break;
-
- p += len + 1;
}
-
- /* Terminate tunestr before we leave. */
- if (__libc_enable_secure)
- tunestr[off] = '\0';
}
/* Initialize the tunables list from the environment. For now we only use the
@@ -263,16 +225,16 @@ __tunables_init (char **envp)
size_t len = 0;
char **prev_envp = envp;
+ /* Ignore tunables for AT_SECURE programs. */
+ if (__libc_enable_secure)
+ return;
+
while ((envp = get_next_env (envp, &envname, &len, &envval,
&prev_envp)) != NULL)
{
if (tunable_is_name ("GLIBC_TUNABLES", envname))
{
- char *new_env = tunables_strdup (envname);
- if (new_env != NULL)
- parse_tunables (new_env + len + 1, envval);
- /* Put in the updated envval. */
- *prev_envp = new_env;
+ parse_tunables (tunables_strdup (envval));
continue;
}
@@ -290,39 +252,6 @@ __tunables_init (char **envp)
/* We have a match. Initialize and move on to the next line. */
if (tunable_is_name (name, envname))
{
- /* For AT_SECURE binaries, we need to check the security settings of
- the tunable and decide whether we read the value and also whether
- we erase the value so that child processes don't inherit them in
- the environment. */
- if (__libc_enable_secure)
- {
- if (cur->security_level == TUNABLE_SECLEVEL_SXID_ERASE)
- {
- /* Erase the environment variable. */
- char **ep = prev_envp;
-
- while (*ep != NULL)
- {
- if (tunable_is_name (name, *ep))
- {
- char **dp = ep;
-
- do
- dp[0] = dp[1];
- while (*dp++);
- }
- else
- ++ep;
- }
- /* Reset the iterator so that we read the environment again
- from the point we erased. */
- envp = prev_envp;
- }
-
- if (cur->security_level != TUNABLE_SECLEVEL_NONE)
- continue;
- }
-
tunable_initialize (cur, envval);
break;
}
@@ -21,14 +21,6 @@
# minval: Optional minimum acceptable value
# maxval: Optional maximum acceptable value
# env_alias: An alias environment variable
-# security_level: Specify security level of the tunable for AT_SECURE binaries.
-# Valid values are:
-#
-# SXID_ERASE: (default) Do not read and do not pass on to
-# child processes.
-# SXID_IGNORE: Do not read, but retain for non-AT_SECURE
-# subprocesses.
-# NONE: Read all the time.
glibc {
malloc {
@@ -158,7 +150,6 @@ glibc {
type: INT_32
minval: 0
maxval: 255
- security_level: SXID_IGNORE
}
}
@@ -15,14 +15,10 @@
License along with the GNU C Library; if not, see
<https://www.gnu.org/licenses/>. */
-/* Verify that tunables correctly filter out unsafe tunables like
- glibc.malloc.check and glibc.malloc.mmap_threshold but also retain
- glibc.malloc.mmap_threshold in an unprivileged child. */
-
-#define _LIBC 1
-#include "config.h"
-#undef _LIBC
+/* Verify that GLIBC_TUNABLES is kept unchanged but no tunable is actually
+ enabled for AT_SECURE processes. */
+#include <dl-tunables.h>
#include <errno.h>
#include <fcntl.h>
#include <stdlib.h>
@@ -40,7 +36,7 @@
#include <support/test-driver.h>
#include <support/capture_subprocess.h>
-const char *teststrings[] =
+static const char *teststrings[] =
{
"glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
"glibc.malloc.check=2:glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
@@ -74,6 +70,23 @@ test_child (int off)
ret = 0;
fflush (stdout);
+ /* Also check if the set tunables are effectively unchanged. */
+ int32_t check = TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL);
+ size_t mmap_threshold = TUNABLE_GET_FULL (glibc, malloc, mmap_threshold,
+ size_t, NULL);
+ int32_t perturb = TUNABLE_GET_FULL (glibc, malloc, perturb, int32_t, NULL);
+
+ printf (" [%d] glibc.malloc.check=%d\n", off, check);
+ fflush (stdout);
+ printf (" [%d] glibc.malloc.mmap_threshold=%zu\n", off, mmap_threshold);
+ fflush (stdout);
+ printf (" [%d] glibc.malloc.perturb=%d\n", off, perturb);
+ fflush (stdout);
+
+ ret |= check != 0;
+ ret |= mmap_threshold != 0;
+ ret |= perturb != 0;
+
return ret;
}
new file mode 100644
@@ -0,0 +1,244 @@
+/* Check GLIBC_TUNABLES parsing.
+ Copyright (C) 2023 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <array_length.h>
+#include <dl-tunables.h>
+#include <getopt.h>
+#include <intprops.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <support/capture_subprocess.h>
+#include <support/check.h>
+
+static int restart;
+#define CMDLINE_OPTIONS \
+ { "restart", no_argument, &restart, 1 },
+
+static const struct test_t
+{
+ const char *env;
+ int32_t expected_malloc_check;
+ size_t expected_mmap_threshold;
+ int32_t expected_perturb;
+} tests[] =
+{
+ /* Expected tunable format. */
+ {
+ "glibc.malloc.check=2",
+ 2,
+ 0,
+ 0,
+ },
+ {
+ "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
+ 2,
+ 4096,
+ 0,
+ },
+ /* Empty tunable are ignored. */
+ {
+ "glibc.malloc.check=2::glibc.malloc.mmap_threshold=4096",
+ 2,
+ 4096,
+ 0,
+ },
+ /* As well empty values. */
+ {
+ "glibc.malloc.check=:glibc.malloc.mmap_threshold=4096",
+ 0,
+ 4096,
+ 0,
+ },
+ /* Tunable are processed from left to right, so last one is the one set. */
+ {
+ "glibc.malloc.check=1:glibc.malloc.check=2",
+ 2,
+ 0,
+ 0,
+ },
+ {
+ "glibc.malloc.check=2:glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
+ 2,
+ 4096,
+ 0,
+ },
+ {
+ "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2",
+ 2,
+ 4096,
+ 0,
+ },
+ /* 0x800 is larger than tunable maxval (0xff), so the tunable is unchanged. */
+ {
+ "glibc.malloc.perturb=0x800",
+ 0,
+ 0,
+ 0,
+ },
+ {
+ "glibc.malloc.perturb=0x55",
+ 0,
+ 0,
+ 0x55,
+ },
+ /* Out of range values are just ignored. */
+ {
+ "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
+ 0,
+ 4096,
+ 0,
+ },
+ /* Invalid keys are ignored. */
+ {
+ ":glibc.malloc.garbage=2:glibc.malloc.check=1",
+ 1,
+ 0,
+ 0,
+ },
+ {
+ "glibc.malloc.perturb=0x800:not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
+ 0,
+ 4096,
+ 0,
+ },
+ {
+ "glibc.not_valid.check=2:glibc.malloc.mmap_threshold=4096",
+ 0,
+ 4096,
+ 0,
+ },
+ {
+ "not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
+ 0,
+ 4096,
+ 0,
+ },
+ /* Invalid subkeys are ignored. */
+ {
+ "glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096:glibc.malloc.check=2",
+ 2,
+ 0,
+ 0,
+ },
+ {
+ "glibc.malloc.check=4:glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096",
+ 0,
+ 0,
+ 0,
+ },
+ {
+ "not_valid.malloc.check=2",
+ 0,
+ 0,
+ 0,
+ },
+ {
+ "glibc.not_valid.check=2",
+ 0,
+ 0,
+ 0,
+ },
+ /* An ill-formatted tunable in the for key=key=value will considere the
+ value as 'key=value' (which can not be parsed as an integer). */
+ {
+ "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096",
+ 0,
+ 0,
+ 0,
+ },
+ /* The ill-formatted tunable is also skipped. */
+ {
+ "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2",
+ 2,
+ 0,
+ 0,
+ },
+ /* For an integer tunable, parse will stop on non number character. */
+ {
+ "glibc.malloc.check=2=2",
+ 2,
+ 0,
+ 0,
+ },
+ {
+ "glibc.malloc.check=2=2:glibc.malloc.mmap_threshold=4096",
+ 2,
+ 4096,
+ 0,
+ }
+};
+
+static int
+handle_restart (int i)
+{
+ TEST_COMPARE (tests[i].expected_malloc_check,
+ TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL));
+ TEST_COMPARE (tests[i].expected_mmap_threshold,
+ TUNABLE_GET_FULL (glibc, malloc, mmap_threshold, size_t, NULL));
+ TEST_COMPARE (tests[i].expected_perturb,
+ TUNABLE_GET_FULL (glibc, malloc, perturb, int32_t, NULL));
+ return 0;
+}
+
+static int
+do_test (int argc, char *argv[])
+{
+ /* We must have either:
+ - One our fource parameters left if called initially:
+ + path to ld.so optional
+ + "--library-path" optional
+ + the library path optional
+ + the application name
+ + the test to check */
+
+ TEST_VERIFY_EXIT (argc == 2 || argc == 5);
+
+ if (restart)
+ return handle_restart (atoi (argv[1]));
+
+ char nteststr[INT_BUFSIZE_BOUND (int)];
+
+ char *spargv[10];
+ {
+ int i = 0;
+ for (; i < argc - 1; i++)
+ spargv[i] = argv[i + 1];
+ spargv[i++] = (char *) "--direct";
+ spargv[i++] = (char *) "--restart";
+ spargv[i++] = nteststr;
+ spargv[i] = NULL;
+ }
+
+ for (int i = 0; i < array_length (tests); i++)
+ {
+ snprintf (nteststr, sizeof nteststr, "%d", i);
+
+ printf ("[%d] Spawned test for %s\n", i, tests[i].env);
+ setenv ("GLIBC_TUNABLES", tests[i].env, 1);
+ struct support_capture_subprocess result
+ = support_capture_subprogram (spargv[0], spargv);
+ support_capture_subprocess_check (&result, "tst-tunables", 0,
+ sc_allow_stderr);
+ support_capture_subprocess_free (&result);
+ }
+
+ return 0;
+}
+
+#define TEST_FUNCTION_ARGV do_test
+#include <support/test-driver.c>
@@ -59,15 +59,6 @@ The list of allowed attributes are:
- env_alias: An alias environment variable
-- security_level: Specify security level of the tunable for AT_SECURE
- binaries. Valid values are:
-
- SXID_ERASE: (default) Do not read and do not pass on to
- child processes.
- SXID_IGNORE: Do not read, but retain for non-AT_SECURE
- child processes.
- NONE: Read all the time.
-
2. Use TUNABLE_GET/TUNABLE_SET/TUNABLE_SET_WITH_BOUNDS to get and set tunables.
3. OPTIONAL: If tunables in a namespace are being used multiple times within a
@@ -61,9 +61,6 @@ $1 == "}" {
if (!env_alias[top_ns,ns,tunable]) {
env_alias[top_ns,ns,tunable] = "{0}"
}
- if (!security_level[top_ns,ns,tunable]) {
- security_level[top_ns,ns,tunable] = "SXID_ERASE"
- }
len = length(top_ns"."ns"."tunable)
if (len > max_name_len)
max_name_len = len
@@ -118,17 +115,6 @@ $1 == "}" {
if (len > max_alias_len)
max_alias_len = len
}
- else if (attr == "security_level") {
- if (val == "SXID_ERASE" || val == "SXID_IGNORE" || val == "NONE") {
- security_level[top_ns,ns,tunable] = val
- }
- else {
- printf("Line %d: Invalid value (%s) for security_level: %s, ", NR, val,
- $0)
- print("Allowed values are 'SXID_ERASE', 'SXID_IGNORE', or 'NONE'")
- exit 1
- }
- }
else if (attr == "default") {
if (types[top_ns,ns,tunable] == "STRING") {
default_val[top_ns,ns,tunable] = sprintf(".strval = \"%s\"", val);
@@ -177,9 +163,9 @@ END {
n = indices[2];
m = indices[3];
printf (" {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
- printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, false, TUNABLE_SECLEVEL_%s, %s},\n",
+ printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, false, %s},\n",
types[t,n,m], minvals[t,n,m], maxvals[t,n,m],
- default_val[t,n,m], security_level[t,n,m], env_alias[t,n,m]);
+ default_val[t,n,m], env_alias[t,n,m]);
}
print "};"
print "#endif"
@@ -23,7 +23,6 @@ glibc {
minval: 0
maxval: 1
env_alias: LD_PREFER_MAP_32BIT_EXEC
- security_level: SXID_IGNORE
}
}
}