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

[Siddhesh Poyarekar <siddhesh at sourceware dot org>]

On Tuesday 31 January 2017 10:21 PM, Florian Weimer wrote:
> On 01/29/2017 06:11 PM, Siddhesh Poyarekar wrote:
>>  #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.  */
>> +static char *
>> +copy_to_heap (const char *in, size_t len)
>> +{
>> +  static size_t heap_size = 0;
>> +  static char *heap = NULL;
>> +  char *out;
>> +
>> +  if (heap_size < len + 1)
>> +    {
>> +      size_t ext = ALIGN_UP (len + 1, ALLOC_SIZE);
> 
> Is this really necessary?  That means that the tunable allocations are
> fairly substantial.

It is just a minor optimization - instead of allocating for every copy,
we allocate 4K at a time and then copy as we see fit.  We will end up
copying a total length < 2xlength of the tunable string so in most cases
this should only be a single 4K allocation.

>> +          /* 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)
>> +            {
>> +              char *q = &p[len];
>> +              while (*q != '\0')
>> +            *name++ = *q++;
>> +              name[0] = '\0';
>> +              len = 0;
>> +            }
>> +
>> +          if (cur->security_level != TUNABLE_SECLEVEL_NONE)
>> +            continue;
>> +        }
> 
> This does not quite work because the next read position is not updated,
> although the tunable definition in the input string has been moved.  As
> a result, a subsequent tunable is not recognized and applied or filtered.

Ugh right, I'll post an updated patch.

Siddhesh