Message-ID: <e78085e4-74cd-52e1-bc0e-4709fac4458a@redhat.com>
Date:   Tue, 21 Dec 2021 13:01:29 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH] exec: Make suid_dumpable apply to SUID/SGID binaries
 irrespective of invoking users
Content-Language: en-US
To:     "Eric W. Biederman" <ebiederm@xmission.com>
Cc:     Alexander Viro <viro@zeniv.linux.org.uk>,
        Kees Cook <keescook@chromium.org>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Laurent Vivier <laurent@vivier.eu>,
        YunQiang Su <ysu@wavecomp.com>, Helge Deller <deller@gmx.de>,
        Willy Tarreau <w@1wt.eu>,
        Linus Torvalds <torvalds@linux-foundation.org>
References: <20211221021744.864115-1-longman@redhat.com>
 <87lf0e7y0k.fsf@email.froward.int.ebiederm.org>
 <4f67dc4c-7038-7dde-cad9-4feeaa6bc71b@redhat.com>
 <87czlp7tdu.fsf@email.froward.int.ebiederm.org>
From:   Waiman Long <longman@redhat.com>
In-Reply-To: <87czlp7tdu.fsf@email.froward.int.ebiederm.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/21/21 12:35, Eric W. Biederman wrote:
> Adding a couple of other people who have expressed opinions on how
> to mitigate this issue in the kernel.
>
> Waiman Long <longman@redhat.com> writes:
>
>> On 12/21/21 10:55, Eric W. Biederman wrote:
>>> Waiman Long <longman@redhat.com> writes:
>>>
>>>> The begin_new_exec() function checks for SUID or SGID binaries by
>>>> comparing effective uid and gid against real uid and gid and using
>>>> the suid_dumpable sysctl parameter setting only if either one of them
>>>> differs.
>>>>
>>>> In the special case that the uid and/or gid of the SUID/SGID binaries
>>>> matches the id's of the user invoking it, the suid_dumpable is not
>>>> used and SUID_DUMP_USER will be used instead. The documentation for the
>>>> suid_dumpable sysctl parameter does not include that exception and so
>>>> this will be an undocumented behavior.
>>>>
>>>> Eliminate this undocumented behavior by adding a flag in the linux_binprm
>>>> structure to designate a SUID/SGID binary and use it for determining
>>>> if the suid_dumpable setting should be applied or not.
>>> I see that you are making the code match the documentation.
>>> What harm/problems does this mismatch cause in practice?
>>> What is the motivation for this change?
>>>
>>> I am trying to see the motivation but all I can see is that
>>> in the case where suid and sgid do nothing in practice the code
>>> does not change dumpable.  The point of dumpable is to refuse to
>>> core dump when it is not safe.  In this case since nothing happened
>>> in practice it is safe.
>>>
>>> So how does this matter in practice.  If there isn't a good
>>> motivation my feel is that it is the documentation that needs to be
>>> updated rather than the code.
>>>
>>> There are a lot of warts to the suid/sgid handling during exec.  This
>>> just doesn't look like one of them
>> This patch is a minor mitigation in response to the security
>> vulnerability as posted in
>> https://www.openwall.com/lists/oss-security/2021/10/20/2 (aka
>> CVE-2021-3864). In particular, the Su PoC (tested on CentOS 7) showing
>> that the su invokes /usr/sbin/unix_chkpwd which is also a SUID
>> binary. The initial su invocation won't generate a core dump because
>> the real uid and euid differs, but the second unix_chkpwd invocation
>> will. This patch eliminates this hole by making sure that all SUID
>> binaries follow suid_dumpable setting.
> All that is required to take advantage of this vulnerability is
> for an suid program to exec something that will coredump.  That
> exec resets the dumpability.
>
> While the example exploit is execing a suid program it is not required
> that the exec'd program be suid.
>
> This makes your proposed change is not a particularly effective mitigation.

Yes, I am aware of that. That is why I said it is just a minor 
mitigation. This patch was inspired after investigating this problem, 
but I do think it is good to make the code consistent with the 
documentation. Of course, we can go either way. I prefer my approach to 
use a flag to indicate a suid binary instead of just comparing ruid and 
euid.


>
> The best idea I have seen to mitigate this from the kernel side is:
>
> 1) set RLIMIT_CORE to 0 during an suid exec
> 2) update do_coredump to honor an rlimit of 0 for pipes
>
> Anecdotally this should not effect the common systems that pipe
> coredumps into programs as those programs are reported to honor
> RLIMIT_CORE of 0.  This needs to be verified.
>
> If those programs do honor RLIMIT_CORE of 0 we won't have any user
> visible changes if they never see coredumps from a program with a
> RLIMIT_CORE of 0.
>
>
> I have been meaning to audit userspace and see if the common coredump
> catchers truly honor an RLIMIT_CORE of 0.  Unfortunately I have not
> found time to do that yet.

Default RLIMIT_CORE to 0 will likely mitigate this vulnerability. 
However, there are still some userspace impacts as existing behavior 
will be modified. For instance, we may need to modify su to restore a 
proper value for RLIMIT_CORE after successful authentication.

Cheers,
Longman