[PATCH] rtld: Add glibc.rtld.enable_secure tunable.

[PATCH] rtld: Add glibc.rtld.enable_secure tunable.

[Joe Simmons-Talbott]

Add a tunable for setting __libc_enable_secure to 1.  Does not set
__libc_enable_secure to 0 if the tunable is set to 0.  Ignores any
tunables following glib.rtld.enable_secure.  One use-case for this
addition is to enable testing code paths that depend on
__libc_eanble_secure being set without the need to use setxid binaries.
---
NOTE: I'm not certain I've picked the appropriate place to handle
glibc.rtld.enable_secure.  I tried to make it happen as early as
possible to minimize and places where __libc_enable_secure might be
checked before the tunable initialization takes place.

 NEWS                             |   4 ++
 csu/libc-start.c                 |   4 ++
 elf/Makefile                     |   2 +
 elf/dl-tunables.c                |   8 ++-
 elf/dl-tunables.h                |  11 +++
 elf/dl-tunables.list             |   6 ++
 elf/tst-rtld-list-tunables.exp   |   1 +
 elf/tst-tunables-enable_secure.c | 115 +++++++++++++++++++++++++++++++
 8 files changed, 150 insertions(+), 1 deletion(-)
 create mode 100644 elf/tst-tunables-enable_secure.c

diff --git a/NEWS b/NEWS
index 8c1c149f91..b04fb15064 100644
--- a/NEWS
+++ b/NEWS
@@ -43,6 +43,10 @@ Major new features:
   on thread stack created by pthread_create or memory allocated by
   malloc).
 
+* A new tunable, glibc.rtld.enable_secure, used to set __libc_enable_secure
+  to 1.  Setting the tunable to 0 does *NOT* set __libc_enable_secure to 0.
+  Once the tunable is encountered all following tunables are ignored.
+
 Deprecated and removed features, and other changes affecting compatibility:
 
 * The ldconfig program now skips file names containing ';' or ending in
diff --git a/csu/libc-start.c b/csu/libc-start.c
index c3bb6d09bc..2f546a3677 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -266,6 +266,10 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
 
   __tunables_init (__environ);
 
+  int32_t tes = TUNABLE_GET_FULL (glibc, rtld, enable_secure, int32_t, NULL);
+  if (tes == 1)
+    __libc_enable_secure = 1;
+
   ARCH_INIT_CPU_FEATURES ();
 
   /* Do static pie self relocation after tunables and cpu features
diff --git a/elf/Makefile b/elf/Makefile
index afec7be084..6c38a72b0d 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -285,6 +285,7 @@ tests-static-internal := \
   tst-tls1-static \
   tst-tls1-static-non-pie \
   tst-tunables \
+  tst-tunables-enable_secure \
   # tests-static-internal
 
 CRT-tst-tls1-static-non-pie := $(csu-objpfx)crt1.o
@@ -2672,6 +2673,7 @@ $(objpfx)tst-glibc-hwcaps-mask.out: \
 $(objpfx)tst-glibc-hwcaps-cache.out: $(objpfx)tst-glibc-hwcaps
 
 tst-tunables-ARGS = -- $(host-test-program-cmd)
+tst-tunables-enable_secure-ARGS = -- $(host-test-program-cmd)
 
 $(objpfx)list-tunables.out: tst-rtld-list-tunables.sh $(objpfx)ld.so
 	$(SHELL) $< $(objpfx)ld.so '$(test-wrapper-env)' \
diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 644d21d1b0..d6c55cfaf2 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -250,7 +250,13 @@ parse_tunables (char *valstring)
     }
 
   for (int i = 0; i < ntunables; i++)
-    tunable_initialize (tunables[i].t, tunables[i].value);
+    {
+      tunable_initialize (tunables[i].t, tunables[i].value);
+
+      /* Ignore all tunables after encountering "glibc.rtld.enable_secure" */
+      if (tunable_strcmp(tunables[i].t->name, "glibc.rtld.enable_secure") == 0)
+	break;
+    }
 }
 
 /* Initialize the tunables list from the environment.  For now we only use the
diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
index 0df4dde24e..2d4fc84d74 100644
--- a/elf/dl-tunables.h
+++ b/elf/dl-tunables.h
@@ -164,4 +164,15 @@ tunable_is_name (const char *orig, const char *envname)
     return false;
 }
 
+/* Compare two strings. */
+static __always_inline int
+tunable_strcmp (const char *left, const char *right)
+{
+  for (;*left != '\0' && *right != '\0'; right++, left++)
+    if (*left != *right)
+      break;
+
+  return *left - *right;
+}
+
 #endif
diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
index 1b23fc9473..cb1b35854e 100644
--- a/elf/dl-tunables.list
+++ b/elf/dl-tunables.list
@@ -136,6 +136,12 @@ glibc {
       minval: 0
       default: 512
     }
+    enable_secure {
+      type: INT_32
+      minval: 0
+      maxval: 1
+      default: 0
+    }
   }
 
   mem {
diff --git a/elf/tst-rtld-list-tunables.exp b/elf/tst-rtld-list-tunables.exp
index 2233ea9c7c..db0e1c86e9 100644
--- a/elf/tst-rtld-list-tunables.exp
+++ b/elf/tst-rtld-list-tunables.exp
@@ -12,5 +12,6 @@ glibc.malloc.tcache_unsorted_limit: 0x0 (min: 0x0, max: 0x[f]+)
 glibc.malloc.top_pad: 0x20000 (min: 0x0, max: 0x[f]+)
 glibc.malloc.trim_threshold: 0x0 (min: 0x0, max: 0x[f]+)
 glibc.rtld.dynamic_sort: 2 (min: 1, max: 2)
+glibc.rtld.enable_secure: 0 (min: 0, max: 1)
 glibc.rtld.nns: 0x4 (min: 0x1, max: 0x10)
 glibc.rtld.optional_static_tls: 0x200 (min: 0x0, max: 0x[f]+)
diff --git a/elf/tst-tunables-enable_secure.c b/elf/tst-tunables-enable_secure.c
new file mode 100644
index 0000000000..e749ec9f17
--- /dev/null
+++ b/elf/tst-tunables-enable_secure.c
@@ -0,0 +1,115 @@
+/* Check GLIBC_TUNABLES parsing for enable_secure.
+   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>
+#include <sys/auxv.h>
+#include <unistd.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;
+  int32_t expected_enable_secure;
+} tests[] =
+{
+  /* Expected tunable format.  */
+  {
+    "glibc.malloc.check=2:glibc.rtld.enable_secure=1",
+    2,
+    1,
+  },
+  /* Tunables encountered after enable_secure should be ignored. */
+  {
+    "glibc.rtld.enable_secure=1:glibc.malloc.check=2",
+    0,
+    1,
+  },
+};
+
+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_enable_secure,
+		TUNABLE_GET_FULL (glibc, rtld, enable_secure, int32_t, NULL));
+  if (tests[i].expected_enable_secure == 1)
+    {
+      TEST_COMPARE (1, __libc_enable_secure);
+    }
+  return 0;
+}
+
+static int
+do_test (int argc, char *argv[])
+{
+  /* We must have either:
+     - One or four 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-enable_secure",
+		                        0, sc_allow_stderr);
+      support_capture_subprocess_free (&result);
+    }
+
+  return 0;
+}
+
+#define TEST_FUNCTION_ARGV do_test
+#include <support/test-driver.c>
-- 
2.41.0

Re: [PATCH] rtld: Add glibc.rtld.enable_secure tunable.

[Szabolcs Nagy]

The 12/05/2023 10:35, Joe Simmons-Talbott wrote:
> Add a tunable for setting __libc_enable_secure to 1.  Does not set
> __libc_enable_secure to 0 if the tunable is set to 0.  Ignores any
> tunables following glib.rtld.enable_secure.  One use-case for this

why do you want to ignore later tunables?

> addition is to enable testing code paths that depend on
> __libc_eanble_secure being set without the need to use setxid binaries.
> ---
> NOTE: I'm not certain I've picked the appropriate place to handle
> glibc.rtld.enable_secure.  I tried to make it happen as early as
> possible to minimize and places where __libc_enable_secure might be
> checked before the tunable initialization takes place.
> 
>  NEWS                             |   4 ++
>  csu/libc-start.c                 |   4 ++

your code only seem to affect static linking.
(apart from the 'ignore later tunables' behaviour)

e.g. i'd expect some change in sysdeps/unix/sysv/linux/dl-sysdep.c

>  elf/Makefile                     |   2 +
>  elf/dl-tunables.c                |   8 ++-
>  elf/dl-tunables.h                |  11 +++
>  elf/dl-tunables.list             |   6 ++
>  elf/tst-rtld-list-tunables.exp   |   1 +
>  elf/tst-tunables-enable_secure.c | 115 +++++++++++++++++++++++++++++++
>  8 files changed, 150 insertions(+), 1 deletion(-)
>  create mode 100644 elf/tst-tunables-enable_secure.c

Re: [PATCH] rtld: Add glibc.rtld.enable_secure tunable.

[Joe Simmons-Talbott]

On Tue, Dec 05, 2023 at 03:51:19PM +0000, Szabolcs Nagy wrote:
> The 12/05/2023 10:35, Joe Simmons-Talbott wrote:
> > Add a tunable for setting __libc_enable_secure to 1.  Does not set
> > __libc_enable_secure to 0 if the tunable is set to 0.  Ignores any
> > tunables following glib.rtld.enable_secure.  One use-case for this
> 
> why do you want to ignore later tunables?

Tunables are currently ignored in __tunables_init when __libc_enable_secure
is set.  Therefore once we set __libc_enable_secure during tunable
processing we should not set any more tunables.

> 
> > addition is to enable testing code paths that depend on
> > __libc_eanble_secure being set without the need to use setxid binaries.
> > ---
> > NOTE: I'm not certain I've picked the appropriate place to handle
> > glibc.rtld.enable_secure.  I tried to make it happen as early as
> > possible to minimize and places where __libc_enable_secure might be
> > checked before the tunable initialization takes place.
> > 
> >  NEWS                             |   4 ++
> >  csu/libc-start.c                 |   4 ++
> 
> your code only seem to affect static linking.
> (apart from the 'ignore later tunables' behaviour)
> 
> e.g. i'd expect some change in sysdeps/unix/sysv/linux/dl-sysdep.c
> 

Thanks for catching that.  I've sent an updated v2 patch[1].

Thanks,
Joe

[1] https://inbox.sourceware.org/libc-alpha/20231205174527.1689844-1-josimmon@redhat.com/

> >  elf/Makefile                     |   2 +
> >  elf/dl-tunables.c                |   8 ++-
> >  elf/dl-tunables.h                |  11 +++
> >  elf/dl-tunables.list             |   6 ++
> >  elf/tst-rtld-list-tunables.exp   |   1 +
> >  elf/tst-tunables-enable_secure.c | 115 +++++++++++++++++++++++++++++++
> >  8 files changed, 150 insertions(+), 1 deletion(-)
> >  create mode 100644 elf/tst-tunables-enable_secure.c
> 

Re: [PATCH] rtld: Add glibc.rtld.enable_secure tunable.

[Szabolcs Nagy]

The 12/05/2023 12:54, Joe Simmons-Talbott wrote:
> On Tue, Dec 05, 2023 at 03:51:19PM +0000, Szabolcs Nagy wrote:
> > The 12/05/2023 10:35, Joe Simmons-Talbott wrote:
> > > Add a tunable for setting __libc_enable_secure to 1.  Does not set
> > > __libc_enable_secure to 0 if the tunable is set to 0.  Ignores any
> > > tunables following glib.rtld.enable_secure.  One use-case for this
> > 
> > why do you want to ignore later tunables?
> 
> Tunables are currently ignored in __tunables_init when __libc_enable_secure
> is set.  Therefore once we set __libc_enable_secure during tunable
> processing we should not set any more tunables.

i disagree.

imo foo:bar and bar:foo tunables should have the same effect.

if you want to eliminate some tunables, then process them in
two passes. but i'm not convinced that is useful at all in this
case: you introduce new code paths even though this is for
testing secure execution, any new code path is going against
that goal. just don't pass other tunables if you use this one
for testing, that's much more reliable.

Re: [PATCH] rtld: Add glibc.rtld.enable_secure tunable.

[Joe Simmons-Talbott]

On Wed, Dec 06, 2023 at 10:03:09AM +0000, Szabolcs Nagy wrote:
> The 12/05/2023 12:54, Joe Simmons-Talbott wrote:
> > On Tue, Dec 05, 2023 at 03:51:19PM +0000, Szabolcs Nagy wrote:
> > > The 12/05/2023 10:35, Joe Simmons-Talbott wrote:
> > > > Add a tunable for setting __libc_enable_secure to 1.  Does not set
> > > > __libc_enable_secure to 0 if the tunable is set to 0.  Ignores any
> > > > tunables following glib.rtld.enable_secure.  One use-case for this
> > > 
> > > why do you want to ignore later tunables?
> > 
> > Tunables are currently ignored in __tunables_init when __libc_enable_secure
> > is set.  Therefore once we set __libc_enable_secure during tunable
> > processing we should not set any more tunables.
> 
> i disagree.
> 
> imo foo:bar and bar:foo tunables should have the same effect.
> 
> if you want to eliminate some tunables, then process them in
> two passes. but i'm not convinced that is useful at all in this
> case: you introduce new code paths even though this is for
> testing secure execution, any new code path is going against
> that goal. just don't pass other tunables if you use this one
> for testing, that's much more reliable.
> 

I've updated the patch (v3) to not set any tunables if enable_secure is
set.

Thanks,
Joe