Re: [PATCH 1/2] tunables: Fix environment variable processing for setuid binaries

[Florian Weimer <fweimer at redhat dot com>]

On 02/01/2017 12:37 PM, Siddhesh Poyarekar wrote:

> 	* elf/dl-tunable-types.h (tunable_seclevel_t): New enum.
> 	* elf/dl-tunables.c (tunables_strdup): Remove.
> 	(get_next_env): Also return the previous envp.
> 	(copy_to_heap): New function.
> 	(parse_tunables): Erase tunables of category
> 	TUNABLES_SECLEVEL_SXID_ERASE.
> 	(maybe_enable_malloc_check): Make MALLOC_CHECK_
> 	TUNABLE_SECLEVEL_NONE if /etc/setuid-debug is accessible.
> 	(__tunables_init)[TUNABLES_FRONTEND ==
> 	TUNABLES_FRONTEND_valstring]: Update GLIBC_TUNABLES envvar
> 	after parsing.
> 	[TUNABLES_FRONTEND != TUNABLES_FRONTEND_valstring]: Erase
> 	tunable envvars of category TUNABLES_SECLEVEL_SXID_ERASE.
> 	* elf/dl-tunables.h (struct _tunable): Change member is_secure
> 	to security_level.
> 	* elf/dl-tunables.list: Add security_level annotations for all
> 	tunables.
> 	* scripts/gen-tunables.awk: Recognize and generate enum values
> 	for security_level.
> 	* elf/tst-env-setuid.c: New test case.
> 	* elf/tst-env-setuid-tunables: new test case.
> 	* elf/Makefile (tests-static): Add them.

Changelog needs reference to bug 21073.

>  #if TUNABLES_FRONTEND == TUNABLES_FRONTEND_valstring
> +# define ALLOC_SIZE 4096
> +/* Allocate bytes on heap to store tunable values copied over from the
> +   valstring.  We use a hardcoded ALLOC_SIZE to avoid querying the page size,
> +   since it may not be available this early in the startup process.  */

I still don't think this micro-optimization is worthwhile.  If we want 
to avoid allocations, we should avoid the copies (they would only be 
needed for strings and for variable rewriting under AT_SECURE).

> -	      tunable_initialize (cur, value);
> +	      /* If we are in a secure context (AT_SECURE) then ignore the tunable
> +		 unless it is explicitly marked as secure.  Tunable values take
> +		 precendence over their envvar aliases.  */
> +	      if (__libc_enable_secure)
> +		{
> +		  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
> +			{
> +			  char *q = &p[len + 1];
> +			  p = name;
> +			  while (*q != '\0')
> +			    *name++ = *q++;
> +			  name[0] = '\0';
> +			  len = 0;
> +			}
> +		    }
> +
> +		  if (cur->security_level != TUNABLE_SECLEVEL_NONE)
> +		    break;
> +		}
> +
> +	      char *val = copy_to_heap (value, len);
> +	      if (val != NULL)
> +		tunable_initialize (cur, val);
>  	      break;
>  	    }
>  	}
>
> -      if (end == ':')
> +      if (p[len] == '\0')
> +	return;
> +      else
>  	p += len + 1;
> -      else
> -	return;
>      }
>  }

I believe this correctly rewrites the string.  But reusing the “name” 
variable in the move-forward loop is confusing.  Maybe also add a 
comment to explain what the loop is doing.

> diff --git a/elf/tst-env-setuid.c b/elf/tst-env-setuid.c
> new file mode 100644
> index 0000000..7def663
> --- /dev/null
> +++ b/elf/tst-env-setuid.c
> @@ -0,0 +1,282 @@
> +/* Copyright (C) 2017 Free Software Foundation, Inc.


Copyright should start in, uh, 2012 because …

> +/* Copies the executable into a restricted directory, so that we can
> +   safely make it SGID with the TARGET group ID.  Then runs the
> +   executable.  */
> +static int
> +run_executable_sgid (gid_t target)

… this code looks familiar.

We should probably consolidate this functionality in support/ for 2.26.

Thanks,
Florian