[PATCH 0/2] make all tunables SXID_ERASE

[PATCH 0/2] make all tunables SXID_ERASE

[Siddhesh Poyarekar]

The tunable privilege levels were a retrofit to try and keep the malloc
tunable envvar behaviour unchanged.  In retrospect, especially in the
light of CVE-2023-4911, I wonder if this feature of carrying allocator
tuning knobs (and more generally, tunables in general) has much value.

This patchset takes the first step by proposing to make all existing
tunables SXID_ERASE.  This will have the effect of deactivating tunables
parsing in the at_secure context for all current tunables, making it
trivial to drop GLIBC_TUNABLES form unsecvars.h in future, hopefully
soon.

Further, this paves the way for future cleanups to tunables parsing,
where we could simply skip over tunables parsing for
__libc_enable_secure instead of the complicated dance we're doing today.

Tested on x86_64.

Siddhesh Poyarekar (2):
  Make all malloc tunables SXID_ERASE
  aarch64: Make glibc.mem.tagging SXID_ERASE

 elf/dl-tunables.list          | 13 +++----------
 elf/tst-env-setuid-tunables.c | 25 ++-----------------------
 elf/tst-env-setuid.c          |  4 ++--
 sysdeps/generic/unsecvars.h   |  7 +++++++
 4 files changed, 14 insertions(+), 35 deletions(-)

-- 
2.41.0

[PATCH 1/2] Make all malloc tunables SXID_ERASE

[Siddhesh Poyarekar]

The malloc tunables were made SXID_IGNORE to mimic the environment
variables they aliased, in order to maintain compatibility.  This
allowed alteration of allocator behaviour across setuid boundaries,
where a setuid program may ignore the tunable but its non-setuid child
can read it and adjust allocator behaviour accordingly.

It's not clear how useful this misfeature is; most library behaviour
tuning is limited to the current process and does not bleed in scope
like this.  If behaviour change across privilege boundaries is
desirable, it should be done with a wrapper program around the
non-setuid child that sets these envvars, instead of using the setuid
process as the messenger.  In future, maybe systemwide tunables could
allow setting tunable values across privilege boundaries.

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 elf/dl-tunables.list          | 12 +++---------
 elf/tst-env-setuid-tunables.c | 25 ++-----------------------
 elf/tst-env-setuid.c          |  4 ++--
 sysdeps/generic/unsecvars.h   |  7 +++++++
 4 files changed, 14 insertions(+), 34 deletions(-)

diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
index 695ba7192e..42d8ffd06d 100644
--- a/elf/dl-tunables.list
+++ b/elf/dl-tunables.list
@@ -22,7 +22,9 @@
 # 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:
+# 		  Valid values are as follows. There must be a strong, well
+# 		  documented reason for a tunable to be marked SXID_IGNORE or
+# 		  SXID_NONE:
 #
 # 	     SXID_ERASE: (default) Do not read and do not pass on to
 # 	     child processes.
@@ -41,7 +43,6 @@ glibc {
     top_pad {
       type: SIZE_T
       env_alias: MALLOC_TOP_PAD_
-      security_level: SXID_IGNORE
       default: 131072
     }
     perturb {
@@ -49,35 +50,29 @@ glibc {
       minval: 0
       maxval: 0xff
       env_alias: MALLOC_PERTURB_
-      security_level: SXID_IGNORE
     }
     mmap_threshold {
       type: SIZE_T
       env_alias: MALLOC_MMAP_THRESHOLD_
-      security_level: SXID_IGNORE
     }
     trim_threshold {
       type: SIZE_T
       env_alias: MALLOC_TRIM_THRESHOLD_
-      security_level: SXID_IGNORE
     }
     mmap_max {
       type: INT_32
       env_alias: MALLOC_MMAP_MAX_
-      security_level: SXID_IGNORE
       minval: 0
     }
     arena_max {
       type: SIZE_T
       env_alias: MALLOC_ARENA_MAX
       minval: 1
-      security_level: SXID_IGNORE
     }
     arena_test {
       type: SIZE_T
       env_alias: MALLOC_ARENA_TEST
       minval: 1
-      security_level: SXID_IGNORE
     }
     tcache_max {
       type: SIZE_T
@@ -91,7 +86,6 @@ glibc {
     mxfast {
       type: SIZE_T
       minval: 0
-      security_level: SXID_IGNORE
     }
     hugetlb {
       type: SIZE_T
diff --git a/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c
index f0b92c97e7..79795cdce7 100644
--- a/elf/tst-env-setuid-tunables.c
+++ b/elf/tst-env-setuid-tunables.c
@@ -60,26 +60,6 @@ const char *teststrings[] =
   "glibc.not_valid.check=2",
 };
 
-const char *resultstrings[] =
-{
-  "glibc.malloc.mmap_threshold=4096",
-  "glibc.malloc.mmap_threshold=4096",
-  "glibc.malloc.mmap_threshold=4096",
-  "glibc.malloc.perturb=0x800",
-  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
-  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
-  "glibc.malloc.mmap_threshold=4096",
-  "glibc.malloc.mmap_threshold=4096",
-  "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096",
-  "",
-  "",
-  "",
-  "",
-  "",
-  "",
-  "",
-};
-
 static int
 test_child (int off)
 {
@@ -87,12 +67,11 @@ test_child (int off)
 
   printf ("    [%d] GLIBC_TUNABLES is %s\n", off, val);
   fflush (stdout);
-  if (val != NULL && strcmp (val, resultstrings[off]) == 0)
+  if (val != NULL && val[0] == '\0')
     return 0;
 
   if (val != NULL)
-    printf ("    [%d] Unexpected GLIBC_TUNABLES VALUE %s, expected %s\n",
-	    off, val, resultstrings[off]);
+    printf ("    [%d] Unexpected GLIBC_TUNABLES VALUE %s\n", off, val);
   else
     printf ("    [%d] GLIBC_TUNABLES environment variable absent\n", off);
 
diff --git a/elf/tst-env-setuid.c b/elf/tst-env-setuid.c
index 032ab44be2..100e2c6871 100644
--- a/elf/tst-env-setuid.c
+++ b/elf/tst-env-setuid.c
@@ -46,9 +46,9 @@ test_child (void)
       return 1;
     }
 
-  if (getenv ("MALLOC_MMAP_THRESHOLD_") == NULL)
+  if (getenv ("MALLOC_MMAP_THRESHOLD_") != NULL)
     {
-      printf ("MALLOC_MMAP_THRESHOLD_ lost\n");
+      printf ("MALLOC_MMAP_THRESHOLD_ is still set\n");
       return 1;
     }
 
diff --git a/sysdeps/generic/unsecvars.h b/sysdeps/generic/unsecvars.h
index 8278c50a84..ca70e2e989 100644
--- a/sysdeps/generic/unsecvars.h
+++ b/sysdeps/generic/unsecvars.h
@@ -17,7 +17,14 @@
   "LD_SHOW_AUXV\0"							      \
   "LOCALDOMAIN\0"							      \
   "LOCPATH\0"								      \
+  "MALLOC_ARENA_MAX\0"							      \
+  "MALLOC_ARENA_TEST\0"							      \
+  "MALLOC_MMAP_MAX_\0"							      \
+  "MALLOC_MMAP_THRESHOLD_\0"						      \
+  "MALLOC_PERTURB_\0"							      \
+  "MALLOC_TOP_PAD_\0"							      \
   "MALLOC_TRACE\0"							      \
+  "MALLOC_TRIM_THRESHOLD_\0"						      \
   "NIS_PATH\0"								      \
   "NLSPATH\0"								      \
   "RESOLV_HOST_CONF\0"							      \
-- 
2.41.0

[PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Siddhesh Poyarekar]

Limit effect of memory tagging to the same process and don't let it
bleed across privilege boundaries into non-setuid children of setuid
processes.

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 elf/dl-tunables.list | 1 -
 1 file changed, 1 deletion(-)

diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
index 42d8ffd06d..44baf10eaa 100644
--- a/elf/dl-tunables.list
+++ b/elf/dl-tunables.list
@@ -152,7 +152,6 @@ glibc {
       type: INT_32
       minval: 0
       maxval: 255
-      security_level: SXID_IGNORE
     }
   }
 
-- 
2.41.0

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Szabolcs Nagy]

The 10/03/2023 16:11, Siddhesh Poyarekar wrote:
> Limit effect of memory tagging to the same process and don't let it
> bleed across privilege boundaries into non-setuid children of setuid
> processes.
> 
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

the description does not match the documented behaviour of
SXID_IGNORE. (the setuid binary passes on the setting from
its parent, i don't see the privilege boundary crossing)

and it does not explain why would you want to turn a security
hardening feature off.

i'm not against the patch as the heap tagging feature is
very experimental at this point, but it needs a better
explanation.

> ---
>  elf/dl-tunables.list | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
> index 42d8ffd06d..44baf10eaa 100644
> --- a/elf/dl-tunables.list
> +++ b/elf/dl-tunables.list
> @@ -152,7 +152,6 @@ glibc {
>        type: INT_32
>        minval: 0
>        maxval: 255
> -      security_level: SXID_IGNORE
>      }
>    }
>  
> -- 
> 2.41.0
> 

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Siddhesh Poyarekar]

On 2023-10-04 03:29, Szabolcs Nagy wrote:
> The 10/03/2023 16:11, Siddhesh Poyarekar wrote:
>> Limit effect of memory tagging to the same process and don't let it
>> bleed across privilege boundaries into non-setuid children of setuid
>> processes.
>>
>> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> 
> the description does not match the documented behaviour of
> SXID_IGNORE. (the setuid binary passes on the setting from
> its parent, i don't see the privilege boundary crossing)

Maybe "privilege boundary crossing" is too strong a phrase, how about 
"bleed across different users or groups"?

> and it does not explain why would you want to turn a security
> hardening feature off.
> 
> i'm not against the patch as the heap tagging feature is
> very experimental at this point, but it needs a better
> explanation.

How about:

"""
Memory tagging is still an experimental feature, so limit propagation of 
tunables across setxid binaries.
"""

In future though, would you want SXID_IGNORE for memory tagging?  I 
would expect that once memory tagging becomes a stable feature you'd 
want it to be enabled by default and disabled by, e.g. a systemwide 
tunable.  I can't see why you'd want it to go across the setxid boundary.

Thanks,
Sid

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Szabolcs Nagy]

The 10/04/2023 07:59, Siddhesh Poyarekar wrote:
> On 2023-10-04 03:29, Szabolcs Nagy wrote:
> > The 10/03/2023 16:11, Siddhesh Poyarekar wrote:
> > > Limit effect of memory tagging to the same process and don't let it
> > > bleed across privilege boundaries into non-setuid children of setuid
> > > processes.
> > > 
> > > Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> > 
> > the description does not match the documented behaviour of
> > SXID_IGNORE. (the setuid binary passes on the setting from
> > its parent, i don't see the privilege boundary crossing)
> 
> Maybe "privilege boundary crossing" is too strong a phrase, how about "bleed
> across different users or groups"?
> 
> > and it does not explain why would you want to turn a security
> > hardening feature off.
> > 
> > i'm not against the patch as the heap tagging feature is
> > very experimental at this point, but it needs a better
> > explanation.
> 
> How about:
> 
> """
> Memory tagging is still an experimental feature, so limit propagation of
> tunables across setxid binaries.
> """

well i don't mind the wording, but i wanted to see
an actual justification.

"this is experiemental" is not useful.

"limit propagation across setxid binaries" answers
what the patch does, but not why.

is there an actual problem you are trying to solve?
do you think SXID_IGNORE is not suitable for security
hardening features? what is the intended usage of it?

i don't see anything immediately wrong with inheriting
env from a grandparent process if in between there was
a setuid process that ignored the env. (i also don't
see it as very useful/necessary)

> In future though, would you want SXID_IGNORE for memory tagging?  I would
> expect that once memory tagging becomes a stable feature you'd want it to be
> enabled by default and disabled by, e.g. a systemwide tunable.  I can't see
> why you'd want it to go across the setxid boundary.

it may not be enabled by default because of its overhead
(we need hw to decide).

i think it is unexpected that setxid drops env vars
(ignoring is ok, but dropping is weird).

i can live with the 'drop' semantics but then why do
we have SXID_IGNORE?

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Siddhesh Poyarekar]

On 2023-10-04 10:04, Szabolcs Nagy wrote:
> well i don't mind the wording, but i wanted to see
> an actual justification.
> 
> "this is experiemental" is not useful.
> 
> "limit propagation across setxid binaries" answers
> what the patch does, but not why.
> 
> is there an actual problem you are trying to solve?
> do you think SXID_IGNORE is not suitable for security
> hardening features? what is the intended usage of it?
> 
> i don't see anything immediately wrong with inheriting
> env from a grandparent process if in between there was
> a setuid process that ignored the env. (i also don't
> see it as very useful/necessary)

The actual problem I'm trying to solve is to get rid of SXID_IGNORE and 
SXID_NONE and the first step is to drop users of those levels. 
Supporting those levels requires additional processing in 
__libc_enable_secure, which is a source of bugs like CVE-2023-4911.  I'm 
not convinced that we really *need* to have tunables go across sxid 
boundaries, so I want to flip the question around: do you think it is 
*necessary* for memory tagging tunables to percolate sxid boundaries? 
If not, then they should stay at SXID_ERASE.

>> In future though, would you want SXID_IGNORE for memory tagging?  I would
>> expect that once memory tagging becomes a stable feature you'd want it to be
>> enabled by default and disabled by, e.g. a systemwide tunable.  I can't see
>> why you'd want it to go across the setxid boundary.
> 
> it may not be enabled by default because of its overhead
> (we need hw to decide).
> 
> i think it is unexpected that setxid drops env vars
> (ignoring is ok, but dropping is weird).
> 
> i can live with the 'drop' semantics but then why do
> we have SXID_IGNORE?

They were introduced for compatibility with the original semantics of 
MALLOC_MMAP_THRESHOLD_ and friends, that would go across sxid 
boundaries.  In those cases too I'm not convinced that the use case 
really needs to be supported and that users should find a way to achieve 
that effect in other ways, such as having a wrapper around the 
non-setuid child that needs to see these tunables and export the 
tunables in that wrapper.

The end goal here is to drop everything but SXID_ERASE so that we don't 
have to do string twiddling under __libc_enable_secure.

The other alternative to achieve the same effect (i.e. not twiddle 
strings in __libc_enable_secure) could be to make *everything* 
SXID_IGNORE, which would then leave GLIBC_TUNABLES untouched for 
non-setuid children to do what they please with it.

Sid

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Szabolcs Nagy]

The 10/04/2023 10:23, Siddhesh Poyarekar wrote:
> The actual problem I'm trying to solve is to get rid of SXID_IGNORE and
> SXID_NONE and the first step is to drop users of those levels. Supporting
> those levels requires additional processing in __libc_enable_secure, which
> is a source of bugs like CVE-2023-4911.  I'm not convinced that we really
> *need* to have tunables go across sxid boundaries, so I want to flip the
> question around: do you think it is *necessary* for memory tagging tunables
> to percolate sxid boundaries? If not, then they should stay at SXID_ERASE.

...

> The end goal here is to drop everything but SXID_ERASE so that we don't have
> to do string twiddling under __libc_enable_secure.
> 
> The other alternative to achieve the same effect (i.e. not twiddle strings
> in __libc_enable_secure) could be to make *everything* SXID_IGNORE, which
> would then leave GLIBC_TUNABLES untouched for non-setuid children to do what
> they please with it.

personally i think setuid binaries should not touch the
env, just ignore it, so everything SXID_IGNORE.

it's not just the string twiddling but tunables_strdup
is a problem too (it can fail, needs early alloc,..).
ideally tunables_init would be a nop with AT_SECURE and
otherwise parsed the env var without malloc and touching
the env[] array passed by the kernel.

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Siddhesh Poyarekar]

On 2023-10-04 10:51, Szabolcs Nagy wrote:
> personally i think setuid binaries should not touch the
> env, just ignore it, so everything SXID_IGNORE.
> 
> it's not just the string twiddling but tunables_strdup
> is a problem too (it can fail, needs early alloc,..).
> ideally tunables_init would be a nop with AT_SECURE and
> otherwise parsed the env var without malloc and touching
> the env[] array passed by the kernel.
> 

Sure, that could be a less contentious change.  I'll send v2 with it.

Thanks,
Sid

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Siddhesh Poyarekar]

On 2023-10-04 10:53, Siddhesh Poyarekar wrote:
> On 2023-10-04 10:51, Szabolcs Nagy wrote:
>> personally i think setuid binaries should not touch the
>> env, just ignore it, so everything SXID_IGNORE.
>>
>> it's not just the string twiddling but tunables_strdup
>> is a problem too (it can fail, needs early alloc,..).
>> ideally tunables_init would be a nop with AT_SECURE and
>> otherwise parsed the env var without malloc and touching
>> the env[] array passed by the kernel.
>>
> 
> Sure, that could be a less contentious change.  I'll send v2 with it.

I should clarify though, that I'm not removing tunables_strdup because 
it's still needed for regular tunables processing.  The v2 I'm testing 
will simply make all tunables SXID_IGNORE and tunables_init a nop with 
AT_SECURE.

Sid

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Adhemerval Zanella Netto]

On 04/10/23 11:51, Szabolcs Nagy wrote:
> The 10/04/2023 10:23, Siddhesh Poyarekar wrote:
>> The actual problem I'm trying to solve is to get rid of SXID_IGNORE and
>> SXID_NONE and the first step is to drop users of those levels. Supporting
>> those levels requires additional processing in __libc_enable_secure, which
>> is a source of bugs like CVE-2023-4911.  I'm not convinced that we really
>> *need* to have tunables go across sxid boundaries, so I want to flip the
>> question around: do you think it is *necessary* for memory tagging tunables
>> to percolate sxid boundaries? If not, then they should stay at SXID_ERASE.
> 
> ...
> 
>> The end goal here is to drop everything but SXID_ERASE so that we don't have
>> to do string twiddling under __libc_enable_secure.
>>
>> The other alternative to achieve the same effect (i.e. not twiddle strings
>> in __libc_enable_secure) could be to make *everything* SXID_IGNORE, which
>> would then leave GLIBC_TUNABLES untouched for non-setuid children to do what
>> they please with it.
> 
> personally i think setuid binaries should not touch the
> env, just ignore it, so everything SXID_IGNORE.

Do you mean just drop unsecvars and not filter out glibc environment variables
for AT_SECURE?

> 
> it's not just the string twiddling but tunables_strdup
> is a problem too (it can fail, needs early alloc,..).
> ideally tunables_init would be a nop with AT_SECURE and
> otherwise parsed the env var without malloc and touching
> the env[] array passed by the kernel.

I complete agree, tunables_strdup was a source of headache due its allocation
requirement and, now, for parsing tunables.

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Szabolcs Nagy]

The 10/04/2023 14:01, Adhemerval Zanella Netto wrote:
> On 04/10/23 11:51, Szabolcs Nagy wrote:
> > 
> > personally i think setuid binaries should not touch the
> > env, just ignore it, so everything SXID_IGNORE.
> 
> Do you mean just drop unsecvars and not filter out glibc environment variables
> for AT_SECURE?

i'd fix this in one place that makes the behaviour easy
to reason about: _dl_next_ld_env_entry in rtld should
just return empty in secure mode and same for getenv,
internally it should return empty.

then we know that nothing in libc can depend on the env.
(if something parses env directly that should be fixed)

if anything, there should be a whitelist, not blacklist
of env vars.

of course this changes behaviour, so if we want to be
bw compat then we have to live with the current unsetenv
logic.

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Siddhesh Poyarekar]

On 2023-10-05 04:19, Szabolcs Nagy wrote:
> i'd fix this in one place that makes the behaviour easy
> to reason about: _dl_next_ld_env_entry in rtld should
> just return empty in secure mode and same for getenv,
> internally it should return empty.
> 
> then we know that nothing in libc can depend on the env.
> (if something parses env directly that should be fixed)
> 
> if anything, there should be a whitelist, not blacklist
> of env vars.

That won't work because it would require knowledge of (or a mechanism to 
specify) safety of environment variables used by the application and its 
children.  The current unsecvars approach is probably the best option.

> of course this changes behaviour, so if we want to be
> bw compat then we have to live with the current unsetenv
> logic.

The current unsetenv logic is well reasoned IMO; the tunables layer made 
it complicated and it ought to be sufficient to just remove that.  But 
that would require dropping the memory tagging tunable from SXID_IGNORE 
and erasing GLIBC_TUNABLES by putting it in unsecvars.h.

Thanks,
Sid

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Szabolcs Nagy]

The 10/05/2023 08:55, Siddhesh Poyarekar wrote:
> On 2023-10-05 04:19, Szabolcs Nagy wrote:
> > i'd fix this in one place that makes the behaviour easy
> > to reason about: _dl_next_ld_env_entry in rtld should
> > just return empty in secure mode and same for getenv,
> > internally it should return empty.
> > 
> > then we know that nothing in libc can depend on the env.
> > (if something parses env directly that should be fixed)
> > 
> > if anything, there should be a whitelist, not blacklist
> > of env vars.
> 
> That won't work because it would require knowledge of (or a mechanism to
> specify) safety of environment variables used by the application and its
> children.  The current unsecvars approach is probably the best option.

why would you need a whitelist of application envvars?

if there is any env var usage *in libc* that is valid to
affect setuid binaries then those should be whitelisted.
(black list works too, but more error prone in imo)

> > of course this changes behaviour, so if we want to be
> > bw compat then we have to live with the current unsetenv
> > logic.
> 
> The current unsetenv logic is well reasoned IMO; the tunables layer made it
> complicated and it ought to be sufficient to just remove that.  But that
> would require dropping the memory tagging tunable from SXID_IGNORE and
> erasing GLIBC_TUNABLES by putting it in unsecvars.h.

i think it is broken to rewrite env[] that is passed by
the kernel. but since glibc always did this i guess it's
fine.

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Siddhesh Poyarekar]

On 2023-10-05 09:59, Szabolcs Nagy wrote:
> The 10/05/2023 08:55, Siddhesh Poyarekar wrote:
>> On 2023-10-05 04:19, Szabolcs Nagy wrote:
>>> i'd fix this in one place that makes the behaviour easy
>>> to reason about: _dl_next_ld_env_entry in rtld should
>>> just return empty in secure mode and same for getenv,
>>> internally it should return empty.
>>>
>>> then we know that nothing in libc can depend on the env.
>>> (if something parses env directly that should be fixed)
>>>
>>> if anything, there should be a whitelist, not blacklist
>>> of env vars.
>>
>> That won't work because it would require knowledge of (or a mechanism to
>> specify) safety of environment variables used by the application and its
>> children.  The current unsecvars approach is probably the best option.
> 
> why would you need a whitelist of application envvars?
> 
> if there is any env var usage *in libc* that is valid to
> affect setuid binaries then those should be whitelisted.
> (black list works too, but more error prone in imo)

So do you mean (1) maintain a full list of envvars recognized by glibc 
and then (2) designate specific ones as safe?  That would be a good idea.

Sid

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Zack Weinberg]

On Thu, Oct 5, 2023, at 9:59 AM, Szabolcs Nagy wrote:
> The 10/05/2023 08:55, Siddhesh Poyarekar wrote:
>> The current unsetenv logic is well reasoned IMO; the tunables layer made it
>> complicated and it ought to be sufficient to just remove that.  But that
>> would require dropping the memory tagging tunable from SXID_IGNORE and
>> erasing GLIBC_TUNABLES by putting it in unsecvars.h.
>
> i think it is broken to rewrite env[] that is passed by
> the kernel. but since glibc always did this i guess it's
> fine.

I think the CVE that prompted this discussion demonstrates that it's *insecure*
to allow children of setxid processes to inherit any environment variable that is
considered insecure to consult in the setxid process itself.

I also think we ought to be talking about a very short *whitelist* of environment
variables that are allowed to survive execve() of a setxid binary -- off the top
of my head, TERM, LANG, LANGUAGE, LC_*, and maybe *nothing else* -- and putting
that list into the kernel itself.

zw

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Siddhesh Poyarekar]

On 2023-10-05 14:31, Zack Weinberg wrote:
> On Thu, Oct 5, 2023, at 9:59 AM, Szabolcs Nagy wrote:
>> The 10/05/2023 08:55, Siddhesh Poyarekar wrote:
>>> The current unsetenv logic is well reasoned IMO; the tunables layer made it
>>> complicated and it ought to be sufficient to just remove that.  But that
>>> would require dropping the memory tagging tunable from SXID_IGNORE and
>>> erasing GLIBC_TUNABLES by putting it in unsecvars.h.
>>
>> i think it is broken to rewrite env[] that is passed by
>> the kernel. but since glibc always did this i guess it's
>> fine.
> 
> I think the CVE that prompted this discussion demonstrates that it's *insecure*
> to allow children of setxid processes to inherit any environment variable that is
> considered insecure to consult in the setxid process itself.

I don't completely disagree with the conclusion below, but the CVE that 
prompted this discussion doesn't say anything about environment 
inheritance because the vulnerability had nothing to do with environment 
processing and inheritance.  The issue there is limited to complex 
parsing of a particular environment variable in a setxid context and the 
main lesson there IMO is to keep any kind of processing to a bare 
minimum in a setxid context.

Processing for environment inheritance (specifically, cleaning out 
unsecvars) is fairly stable code that has stood the test of time.  It 
makes sense like you suggest below, to make it an inclusion list rather 
than an exclusion list, but IMO that's a separate hardening exercise 
from ripping tunables out of the setxid context.

> I also think we ought to be talking about a very short *whitelist* of environment
> variables that are allowed to survive execve() of a setxid binary -- off the top
> of my head, TERM, LANG, LANGUAGE, LC_*, and maybe *nothing else* -- and putting
> that list into the kernel itself.
> 
> zw
> 

Thanks,
Sid

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Zack Weinberg]

On Thu, Oct 5, 2023, at 3:11 PM, Siddhesh Poyarekar wrote:
> On 2023-10-05 14:31, Zack Weinberg wrote:
>> On Thu, Oct 5, 2023, at 9:59 AM, Szabolcs Nagy wrote:
>>> i think it is broken to rewrite env[] that is passed by the
>>> kernel. but since glibc always did this i guess it's fine.
>>
>> I think the CVE that prompted this discussion demonstrates that
>> it's *insecure* to allow children of setxid processes to inherit
>> any environment variable that is considered insecure to consult in
>> the setxid process itself.
>
> I don't completely disagree with the conclusion below, but the CVE
> that prompted this discussion doesn't say anything about environment
> inheritance because the vulnerability had nothing to do with
> environment processing and inheritance.

I may have misunderstood the CVE or mixed it up with another one.
I thought there was a recent CVE in which a SXID_IGNORE environment
variable was inherited by a child process, and that child process was
rendered vulnerable to further exploitation because it honored that
variable.

> The issue there is limited to complex parsing of a particular
> environment variable in a setxid context and the main lesson there
> IMO is to keep any kind of processing to a bare minimum in a setxid
> context.

Agreed.

> Processing for environment inheritance (specifically, cleaning out
> unsecvars) is fairly stable code that has stood the test of time.
> It makes sense like you suggest below, to make it an inclusion list
> rather than an exclusion list, but IMO that's a separate hardening
> exercise from ripping tunables out of the setxid context.

Also agreed.

zw

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Siddhesh Poyarekar]

On 2023-10-06 08:25, Zack Weinberg wrote:
>> I don't completely disagree with the conclusion below, but the CVE
>> that prompted this discussion doesn't say anything about environment
>> inheritance because the vulnerability had nothing to do with
>> environment processing and inheritance.
> 
> I may have misunderstood the CVE or mixed it up with another one.
> I thought there was a recent CVE in which a SXID_IGNORE environment
> variable was inherited by a child process, and that child process was
> rendered vulnerable to further exploitation because it honored that
> variable.

Hmm, I don't remember anything like this recently (but my memory is 
worse than my airplane flying skills), but something like that would be 
an interesting data point and further confirmation that we need to get 
rid of SXID_IGNORE and SXID_NONE.  In any case, I can't see a good 
reason anymore to keep these levels, especially if we drop memory 
tagging, malloc tuning and malloc debugging tunables from SXID_IGNORE. 
If memory tagging needs to persist across setxid programs then there 
needs to be a different way, maybe through systemwide tunables[1] that 
DJ is working on, or maybe even ELF markup.

Adhemerval has taken this patchset and is going to build on it to rip it 
all out, so we'll hopefully resolve all of this together.

Thanks,
Sid

[1] 
https://inbox.sourceware.org/libc-alpha/xn1qeayuo9.fsf@greed.delorie.com/T/#u

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Adhemerval Zanella Netto]

On 06/10/23 09:41, Siddhesh Poyarekar wrote:
> On 2023-10-06 08:25, Zack Weinberg wrote:
>>> I don't completely disagree with the conclusion below, but the CVE
>>> that prompted this discussion doesn't say anything about environment
>>> inheritance because the vulnerability had nothing to do with
>>> environment processing and inheritance.
>>
>> I may have misunderstood the CVE or mixed it up with another one.
>> I thought there was a recent CVE in which a SXID_IGNORE environment
>> variable was inherited by a child process, and that child process was
>> rendered vulnerable to further exploitation because it honored that
>> variable.
> 
> Hmm, I don't remember anything like this recently (but my memory is worse than my airplane flying skills), but something like that would be an interesting data point and further confirmation that we need to get rid of SXID_IGNORE and SXID_NONE.  In any case, I can't see a good reason anymore to keep these levels, especially if we drop memory tagging, malloc tuning and malloc debugging tunables from SXID_IGNORE. If memory tagging needs to persist across setxid programs then there needs to be a different way, maybe through systemwide tunables[1] that DJ is working on, or maybe even ELF markup.
> 
> Adhemerval has taken this patchset and is going to build on it to rip it all out, so we'll hopefully resolve all of this together.

I think the less contiguous change would to keep with the blacklist of
environment variables, since changing to whitelist will change the
current semantic (and add possible user-visible breakage), ignore any
tunable on AT_SECURE binaries (as some Linux distributions are already
doing [1]), add GLIBC_TUNABLES to unsecvars (so if AT_SECURE binaries
want to set any tunable, they should it explicit), and remove the 
requirement of duplice the GLIBC_TUNABLES tunable parsing (which
some memory allocation failure).

Yes, security hardening tunables won't be applied for AT_SECURE binaries,
but I think opt-in security features that have performance implications
should done by system administrator instead of relying on users.

[1] https://git.altlinux.org/gears/g/glibc.git?p=glibc.git;a=commitdiff;h=5d1686416ab766f3dd0780ab730650c4c0f76ca9

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Siddhesh Poyarekar]

On 2023-10-06 13:10, Adhemerval Zanella Netto wrote:
>> Adhemerval has taken this patchset and is going to build on it to rip it all out, so we'll hopefully resolve all of this together.
> 
> I think the less contiguous change would to keep with the blacklist of
> environment variables, ...

Yes of course, sorry I didn't mean to sneak up additional requirements 
on the work you're doing :)

Thanks,
Sid

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Michael Hudson-Doyle]

[-- Attachment #1: Type: text/plain, Size: 488 bytes --]

On Fri, 6 Oct 2023 at 07:32, Zack Weinberg <zack@owlfolio.org> wrote:

> I also think we ought to be talking about a very short *whitelist* of
> environment
> variables that are allowed to survive execve() of a setxid binary -- off
> the top
> of my head, TERM, LANG, LANGUAGE, LC_*, and maybe *nothing else* -- and
> putting
> that list into the kernel itself.
>

That would break at least one application I know about (snapd):
https://bugs.launchpad.net/snapd/+bug/1682308

Cheers,
mwh

Re: [PATCH 2/2] aarch64: Make glibc.mem.tagging SXID_ERASE

[Zack Weinberg]

On Sun, Oct 8, 2023, at 3:51 PM, Michael Hudson-Doyle wrote:
> On Fri, 6 Oct 2023 at 07:32, Zack Weinberg <zack@owlfolio.org> wrote:
>> I also think we ought to be talking about a very short *whitelist* of
>> environment variables that are allowed to survive execve() of a
>> setxid binary -- off the top of my head, TERM, LANG, LANGUAGE, LC_*,
>> and maybe *nothing else* -- and putting that list into the kernel
>> itself.
>
> That would break at least one application I know about (snapd):
> https://bugs.launchpad.net/snapd/+bug/1682308

Flip answer: I don't think snapd ought to exist in the first place
(because it violates the Highlander Principle of Package Management),
so I don't care if it gets broken.

More serious answer: The specific thing snapd is doing here could be
handled via some sort of client-server protocol, with a *non*-setxid
client.  This would allow the code running at elevated privilege to
treat the environment variables as an opaque blob of data to be copied
into the nascent sandboxed process, which would be safer.

Complementary serious answer: Are you sure there's no way to leverage
the ability to set arbitrary environment variables inside the sandbox
to carry out a sandbox escape?  Are you *certain*?

zw