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

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

On 4/8/21 12:38 AM, Siddhesh Poyarekar wrote:
> 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.

If a deprecated SXID_IGNORE variable existed, which it doesn't, then
I would have expected it to cross the boundary.

I'm not arguing about defaults just behaviour of existing code and
how it might be used by users.

My argument is that it is not "wrong" for tunables to cross the SXID
boundary if they were marked as being allowed to cross that boundary.

However, I need to review this patch again because:

* By default we set tunables to SXID_ERASE.

* We have no deprecated SXID_IGNORE tunables that I know about.

If we had the latter then we would need to do more work to keep them,
but we don't so your patch might be OK.
 
> 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.

The hardening, SXID_ERASE, should follow the documented behaviour
for a given variable, including deprecated ones.

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

Right, which is why today your patch should be OK, and I need to redo
the review.

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

I think it should.

Setting values in a file for a container or cross-mount-namespace
requires considerable amount of work.

In k8s and even Red Hat OCP there is *significant* use of environment
variables to express tunables for containers.

Consider a read-only runtime... how do you impact the behaviour? How
do you test a tuning to a hardware feature that only the container
runtime knows about?

In practice I'd like the tuning to be automatic e.g. ifunc, but some
options are difficult and workload specific, and so may need to pass
to a child process across an SXID boundary.

It is *more* dangerous IMO to agree to mount a read-write filesystem
into which you have to write configuration data.

However, I don't think this discussion should or needs to be resolved
today since we have no deprecated SXID_IGNORE env vars that would need
special handling.
 
> 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.

Some tunables I think can be set in that way because you have:

* A known workload tuning.

* A known parameter for your runtime.

But there are much harder parameters to set that depend on the runtime
knowing something more about the host. Today this happens to be
environment variables.

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


-- 
Cheers,
Carlos.