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

[Siddhesh Poyarekar <siddhesh@sourceware.org>]

On 4/7/21 1:17 AM, Carlos O'Donell wrote:
> 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 have read through bug 27471, thanks for referencing the bug.
>   
> I'm worried about the fact that we have existing examples from DJ's
> nsswitch changes with / changing, that there are real-world examples
> where we actively run a child process in a potentially newer glibc
> and stripping away tunables for the current hardware means the child
> process needs a way to add it back, and doing that isn't entirely
> clear e.g. sudo with hardcoded env.
> 
> This makes testing and producing tuning difficult or impossible
> and I that means to me that we should be more conservative with what
> we process.
> 
>> 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.
> 
> This prevents passing a tunable through a program to a child that runs
> with a newer glibc and accepts different tunables.
> 
>> - Unrecognized tunables get scrubbed off from the environment and
>>    subsequently from the child environment.
> 
> I think we need to be conservative here and allow unknown tunables
> to be "passed through" to the child without modification.
> 
>> - 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.
> 
> Why is it wrong?

That comment was based on a previous consensus[1] where we agreed that 
we eventually port all SXID_IGNORE tunables into SXID_ERASE but I never 
actually got around to doing it.  However you seem to arguing the 
opposite case, i.e. making tunables SXID_IGNORE by default and porting 
SXID_ERASE over to it.

As I've mentioned in the bug report, I don't see miscategorization of 
tunables resulting in any *new* security issues; at worst they may 
provide additional control over processes across setxid boundaries that 
may otherwise not be possible.  In that sense, sxid categorization is a 
security hardening feature and the discussion is about whether the 
hardening is going too far.

> We can have instances where we chroot or unshare into another glibc
> version and we may want the env vars to pass through because they are
> related to hardware changes.
> 
> If I step back and look at the bigger picture I see the following:
> 
> (1) Tunables we have removed.
> - Did we record these?
> - Did we record their security setting?
> - Are we avoiding reusing them by accident?
> - Are we cleaning legacy tunables based on their security setting to
>    keep the environment clean for a child running with an old glibc?

There was one namespace renaming from glibc.tune to glibc.cpu, but it 
was aarch64-specific.  It defaults to SXID_ERASE, as do most tunables. 
Only tunables that were ported from environment variables, e.g. 
glibc.malloc.mmap_threshold are SXID_IGNORE.

> (2) Tunables we don't know about.
> - The child process may need to consume these because it runs a newer glibc.
> - We lack /var/glibc.tunables handling of these tunables, or the child may be a
>    generic container, or generic chroot, maybe env vars are the only way to pass
>    this information.
> 
> (3) Tunables we know about.
> - These are the straight forward fix, we just fix the code.
> 
> The patch handles (1) and (2) and (3) but picks an easier to audit
> and verify solution for (3) at the expense of (1) and (2). I don't know if
> that is a good tradeoff.
> 
> With my developer hat on I expect the following:
> - All old legacy tunables are handled as their old security settings.
> - All new tunables are handled as their current security settings.
> - Unknown tunables are left alone.
> 
> Have I justified my expectation?

To summarize my understanding, you argue that there may be a legitimate 
need to be able to control environment of processes across the sxid 
boundary.  While I agree that there ought to be ways to control 
processes across sxid boundaries I don't think it should be as trivial 
as setting the program environment of an sxid program.

Maybe the use case you're referring to is better served by systemwide 
and userwide tunables.  For example, if I had to control the environment 
of httpd, it should be through $HTTPD_HOME/.glibc-tunables or 
/etc/glibc-tunables and not through $SIDDHESH_HOME/.glibc-tunables or 
the environment of user siddhesh.  This may not work for legacy tunables 
if the older environment doesn't support systemwide or userwide tunables 
but that shouldn't cause problems in practice.

Siddhesh

[1] https://sourceware.org/legacy-ml/libc-alpha/2017-02/msg00042.html