Re: [PATCH 4/4] Fix SXID_ERASE behavior in setuid programs (BZ #27471)

[Carlos O'Donell <carlos@redhat.com>]

On 3/16/21 3:07 AM, Siddhesh Poyarekar via Libc-alpha wrote:
> When parse_tunables tries to erase a tunable marked as SXID_ERASE for
> setuid programs, it ends up setting the envvar string iterator
> incorrectly, because of which it may parse the next tunable
> incorrectly.  Given that currently the implementation allows malformed
> and unrecognized tunables pass through, it may even allow SXID_ERASE
> tunables to go through.

I am reviewing this a second time after our downthread discussion that
there are no SXID_IGNORE tunables that are deprecated and that would
actually need to be handled in a special way. Any deprecated SXID_ERASE
variables would be unknown to a later runtime and be erased anyway
matching the SXID_ERASE requirement.
 
> This change revamps the SXID_ERASE implementation so that:
> 
> - Only valid tunables are written back to the tunestr string, because
>   of which children of SXID programs will only inherit a clean list of
>   identified tunables that are not SXID_ERASE.
> 
> - Unrecognized tunables get scrubbed off from the environment and
>   subsequently from the child environment.
> 
> - This has the side-effect that a tunable that is not identified by
>   the setxid binary, will not be passed on to a non-setxid child even
>   if the child could have identified that tunable.  This may break
>   applications that expect this behaviour but expecting such tunables
>   to cross the SXID boundary is wrong.

Re-review in the context of a limited change to fix the bug.

OK to commit.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  elf/dl-tunables.c             | 56 ++++++++++++++++-------------------
>  elf/tst-env-setuid-tunables.c | 26 ++++++++++++++++
>  2 files changed, 52 insertions(+), 30 deletions(-)
> 
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index a2be9cde2f..1aedb9bd36 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -171,6 +171,7 @@ parse_tunables (char *tunestr, char *valstring)
>      return;
>  
>    char *p = tunestr;
> +  size_t off = 0;

OK.

>  
>    while (true)
>      {
> @@ -184,7 +185,11 @@ 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')
> -	return;
> +	{
> +	  if (__libc_enable_secure)
> +	    tunestr[off] = '\0';
> +	  return;

OK. Terminate the tunable if we are secure.

> +	}
>  
>        /* We did not find a valid name-value pair before encountering the
>  	 colon.  */
> @@ -210,35 +215,28 @@ 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.  */
> +	      /* 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.  */

OK. For today there are no SXID_IGNORE tunables that have been deprecated, and so
all currently deprecated tunables were always SXID_ERASE and so this doesn't
change the current behaviour. Future changes will need to take this into account.

>  	      if (__libc_enable_secure)
>  		{
> -		  if (cur->security_level == TUNABLE_SECLEVEL_SXID_ERASE)
> +		  if (cur->security_level != TUNABLE_SECLEVEL_SXID_ERASE)
>  		    {
> -		      if (p[len] == '\0')
> -			{
> -			  /* Last tunable in the valstring.  Null-terminate and
> -			     return.  */
> -			  *name = '\0';
> -			  return;
> -			}
> -		      else
> -			{
> -			  /* Remove the current tunable from the string.  We do
> -			     this by overwriting the string starting from NAME
> -			     (which is where the current tunable begins) with
> -			     the remainder of the string.  We then have P point
> -			     to NAME so that we continue in the correct
> -			     position in the valstring.  */
> -			  char *q = &p[len + 1];
> -			  p = name;
> -			  while (*q != '\0')
> -			    *name++ = *q++;
> -			  name[0] = '\0';
> -			  len = 0;
> -			}
> +		      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];

OK.

>  		    }
>  
>  		  if (cur->security_level != TUNABLE_SECLEVEL_NONE)
> @@ -251,9 +249,7 @@ parse_tunables (char *tunestr, char *valstring)
>  	    }
>  	}
>  
> -      if (p[len] == '\0')
> -	return;
> -      else
> +      if (p[len] != '\0')

OK.

>  	p += len + 1;
>      }
>  }
> diff --git a/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c
> index 3d523875b1..05619c9adc 100644
> --- a/elf/tst-env-setuid-tunables.c
> +++ b/elf/tst-env-setuid-tunables.c
> @@ -45,11 +45,37 @@
>  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",
> +  "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2",
> +  "glibc.malloc.perturb=0x800",
> +  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
> +  "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.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",
> +  "glibc.malloc.check=1:glibc.malloc.check=2",
> +  "not_valid.malloc.check=2",
> +  "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",
> +  "",
> +  "",
> +  "",
> +  "",
> +  "",
> +  "",

OK. Adds many more tests, which is great!

>  };
>  
>  static int
> 


-- 
Cheers,
Carlos.