[committed,2/2] tunables: Terminate if end of input is reached (CVE-2023-4911)

The string parsing routine may end up writing beyond bounds of tunestr
if the input tunable string is malformed, of the form name=name=val.
This gets processed twice, first as name=name=val and next as name=val,
resulting in tunestr being name=name=val:name=val, thus overflowing
tunestr.

Terminate the parsing loop at the first instance itself so that tunestr
does not overflow.

This also fixes up tst-env-setuid-tunables to actually handle failures
correct and add new tests to validate the fix for this CVE.

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
---
 NEWS                          |  5 +++++
 elf/dl-tunables.c             | 17 +++++++++-------
 elf/tst-env-setuid-tunables.c | 37 +++++++++++++++++++++++++++--------
 3 files changed, 44 insertions(+), 15 deletions(-)

On 03/10/23 14:08, Siddhesh Poyarekar wrote:
> The string parsing routine may end up writing beyond bounds of tunestr
> if the input tunable string is malformed, of the form name=name=val.
> This gets processed twice, first as name=name=val and next as name=val,
> resulting in tunestr being name=name=val:name=val, thus overflowing
> tunestr.
> 
> Terminate the parsing loop at the first instance itself so that tunestr
> does not overflow.
> 
> This also fixes up tst-env-setuid-tunables to actually handle failures
> correct and add new tests to validate the fix for this CVE.
> 
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> ---
>  NEWS                          |  5 +++++
>  elf/dl-tunables.c             | 17 +++++++++-------
>  elf/tst-env-setuid-tunables.c | 37 +++++++++++++++++++++++++++--------
>  3 files changed, 44 insertions(+), 15 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index a94650da64..cc4b81f0ac 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -64,6 +64,11 @@ Security related changes:
>    an application calls getaddrinfo for AF_INET6 with AI_CANONNAME,
>    AI_ALL and AI_V4MAPPED flags set.
>  
> +  CVE-2023-4911: If a tunable of the form NAME=NAME=VAL is passed in the
> +  environment of a setuid program and NAME is valid, it may result in a
> +  buffer overflow, which could be exploited to achieve escalated
> +  privileges.  This flaw was introduced in glibc 2.34.
> +
>  The following bugs are resolved with this release:
>  
>    [The release manager will add the list generated by
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 62b7332d95..cae67efa0a 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -180,11 +180,7 @@ parse_tunables (char *tunestr, char *valstring)
>        /* If we reach the end of the string before getting a valid name-value
>  	 pair, bail out.  */
>        if (p[len] == '\0')
> -	{
> -	  if (__libc_enable_secure)
> -	    tunestr[off] = '\0';
> -	  return;
> -	}
> +	break;
>  
>        /* We did not find a valid name-value pair before encountering the
>  	 colon.  */
> @@ -244,9 +240,16 @@ parse_tunables (char *tunestr, char *valstring)
>  	    }
>  	}
>  
> -      if (p[len] != '\0')
> -	p += len + 1;
> +      /* 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';
>  }
>  

So how should we handle what might be inconsistent invalid inputs like:

  glibc.malloc.mmap_threshold=4096=4096

Since glibc does not provide any TUNABLE_SECLEVEL_NONE, this tunables
will be ignored.  But for TUNABLE_SECLEVEL_NONE one, the value is
still parsed by _dl_strtoul or stored in the tunable.

>  /* Enable the glibc.malloc.check tunable in SETUID/SETGID programs only when
> diff --git a/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c
> index 7dfb0e073a..f0b92c97e7 100644
> --- a/elf/tst-env-setuid-tunables.c
> +++ b/elf/tst-env-setuid-tunables.c
> @@ -50,6 +50,8 @@ const char *teststrings[] =
>    "glibc.malloc.perturb=0x800:not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
>    "glibc.not_valid.check=2:glibc.malloc.mmap_threshold=4096",
>    "not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.check=2",
>    "glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096:glibc.malloc.check=2",
>    "glibc.malloc.check=4:glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096",
>    ":glibc.malloc.garbage=2:glibc.malloc.check=1",
> @@ -68,6 +70,8 @@ const char *resultstrings[] =
>    "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",
> +  "",
>    "",
>    "",
>    "",
> @@ -81,11 +85,18 @@ test_child (int off)
>  {
>    const char *val = getenv ("GLIBC_TUNABLES");
>  
> +  printf ("    [%d] GLIBC_TUNABLES is %s\n", off, val);
> +  fflush (stdout);
>    if (val != NULL && strcmp (val, resultstrings[off]) == 0)
>      return 0;
>  
>    if (val != NULL)
> -    printf ("[%d] Unexpected GLIBC_TUNABLES VALUE %s\n", off, val);
> +    printf ("    [%d] Unexpected GLIBC_TUNABLES VALUE %s, expected %s\n",
> +	    off, val, resultstrings[off]);
> +  else
> +    printf ("    [%d] GLIBC_TUNABLES environment variable absent\n", off);
> +
> +  fflush (stdout);
>  
>    return 1;
>  }
> @@ -106,21 +117,26 @@ do_test (int argc, char **argv)
>        if (ret != 0)
>  	exit (1);
>  
> -      exit (EXIT_SUCCESS);
> +      /* Special return code to make sure that the child executed all the way
> +	 through.  */
> +      exit (42);
>      }
>    else
>      {
> -      int ret = 0;
> -
>        /* Spawn tests.  */
>        for (int i = 0; i < array_length (teststrings); i++)
>  	{
>  	  char buf[INT_BUFSIZE_BOUND (int)];
>  
> -	  printf ("Spawned test for %s (%d)\n", teststrings[i], i);
> +	  printf ("[%d] Spawned test for %s\n", i, teststrings[i]);
>  	  snprintf (buf, sizeof (buf), "%d\n", i);
> +	  fflush (stdout);
>  	  if (setenv ("GLIBC_TUNABLES", teststrings[i], 1) != 0)
> -	    exit (1);
> +	    {
> +	      printf ("    [%d] Failed to set GLIBC_TUNABLES: %m", i);
> +	      support_record_failure ();
> +	      continue;
> +	    }
>  
>  	  int status = support_capture_subprogram_self_sgid (buf);
>  
> @@ -128,9 +144,14 @@ do_test (int argc, char **argv)
>  	  if (WEXITSTATUS (status) == EXIT_UNSUPPORTED)
>  	    return EXIT_UNSUPPORTED;
>  
> -	  ret |= status;
> +	  if (WEXITSTATUS (status) != 42)
> +	    {
> +	      printf ("    [%d] child failed with status %d\n", i,
> +		      WEXITSTATUS (status));
> +	      support_record_failure ();
> +	    }
>  	}
> -      return ret;
> +      return 0;
>      }
>  }
>

On 2023-10-03 14:07, Adhemerval Zanella Netto wrote:
> So how should we handle what might be inconsistent invalid inputs like:
> 
>    glibc.malloc.mmap_threshold=4096=4096
> 
> Since glibc does not provide any TUNABLE_SECLEVEL_NONE, this tunables
> will be ignored.  But for TUNABLE_SECLEVEL_NONE one, the value is
> still parsed by _dl_strtoul or stored in the tunable.
> 

Ack, it probably makes sense to drop all tunables that don't match at 
this stage.  Arjun is going to rework the parsing for pr#30683 and it 
probably makes sense to, in addition to enhancing the parsing, also weed 
out invalid inputs and harden around allocations for the tunable string 
to provide some resilience against overflows.

The other aspect to think about may be the utility of passing tunables 
to (or through) setuid programs.  I had done it to maintain 
compatibility with the malloc envvars that were getting passed through, 
but maybe it's a good idea to filter all of them out.  Perhaps with 
systemwide tunables we could even have a way for tunables to be read in 
sxid programs in a safer way.

Thanks,
Sid

On 03/10/23 15:16, Siddhesh Poyarekar wrote:
> On 2023-10-03 14:07, Adhemerval Zanella Netto wrote:
>> So how should we handle what might be inconsistent invalid inputs like:
>>
>>    glibc.malloc.mmap_threshold=4096=4096
>>
>> Since glibc does not provide any TUNABLE_SECLEVEL_NONE, this tunables
>> will be ignored.  But for TUNABLE_SECLEVEL_NONE one, the value is
>> still parsed by _dl_strtoul or stored in the tunable.
>>
> 
> Ack, it probably makes sense to drop all tunables that don't match at this stage.  Arjun is going to rework the parsing for pr#30683 and it probably makes sense to, in addition to enhancing the parsing, also weed out invalid inputs and harden around allocations for the tunable string to provide some resilience against overflows.
> 
> The other aspect to think about may be the utility of passing tunables to (or through) setuid programs.  I had done it to maintain compatibility with the malloc envvars that were getting passed through, but maybe it's a good idea to filter all of them out.  Perhaps with systemwide tunables we could even have a way for tunables to be read in sxid programs in a safer way.

I think it would be best to avoid changing AT_SECURE binaries semantic
through tunables or even environment setting (/etc/suid-debug).  It
means adding back GLIBC_TUNABLES to unsecvars (so even non AT_SECURE
binaries won't see GLIBC_TUNABLES), do not process GLIBC_TUNABLES for
AT_SECURE (including malloc mcheck), and dropping any ill-formed
GLIBC_TUNABLES strings (so first parse and only apply well-formatted
ones).

This would allow to just remove the tunables_strdup altogether,
simplifying the code a lot.  It also means that any security tunable
(such as the glibc.mem.tagging) would also stop appling to AT_SECURE,
but I think we should stopping giving users to change secure binares
semantics even in this way.

And I don't think we should make this changes iff we have a a trusted
system-wide tunable.

I am working on coming up with this ideas, and we can discuss whether
adding trusting system-wide tunable would be a good idea.

On 2023-10-04 08:20, Adhemerval Zanella Netto wrote:
> I think it would be best to avoid changing AT_SECURE binaries semantic
> through tunables or even environment setting (/etc/suid-debug).  It
> means adding back GLIBC_TUNABLES to unsecvars (so even non AT_SECURE
> binaries won't see GLIBC_TUNABLES), do not process GLIBC_TUNABLES for
> AT_SECURE (including malloc mcheck), and dropping any ill-formed
> GLIBC_TUNABLES strings (so first parse and only apply well-formatted
> ones).

I started that process with this patchset[1] that's probably the most 
contentious (or maybe not) issue.  If we get consensus on that, we can 
move to drop GLIBC_TUNABLES support completely for AT_SECURE in 2.39.

> This would allow to just remove the tunables_strdup altogether,
> simplifying the code a lot.  It also means that any security tunable
> (such as the glibc.mem.tagging) would also stop appling to AT_SECURE,
> but I think we should stopping giving users to change secure binares
> semantics even in this way.

We won't get to drop tunables_strdup; it's still needed to make a copy 
to delimit tunable values.  We can drop all the twiddling under the 
__libc_enable_secure though.

> And I don't think we should make this changes iff we have a a trusted
> system-wide tunable.

Ack, I can live with that.

Thanks,
Sid

[1] https://patchwork.sourceware.org/project/glibc/list/?series=25312