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

[Siddhesh Poyarekar <siddhesh at sourceware dot org>]

On Wednesday 01 February 2017 11:56 PM, Florian Weimer wrote:
> 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.

OK.  A bug is not necessary if the defect was introduced in unreleased
code btw.

> 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).

OK.

> 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.

I'll add a descriptive comment that includes the reason why we're using
NAME there.

>> 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.

Yes, it is lifted from tst-secure-getenv.c.  I didn't know there was a
custom of passing on copyright dates that way.  I don't mind changing it
though.

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

Agreed.

Siddhesh