linux-kernel - Re: [PATCH v3] cred: Propagate security_prepare

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9fe9cd9f-1ded-a179-8ded-5fde8960a586@cloudflare.com>
Date:   Thu, 16 Jun 2022 10:04:07 -0500
From:   Frederick Lawler <fred@...udflare.com>
To:     Casey Schaufler <casey@...aufler-ca.com>,
        Paul Moore <paul@...l-moore.com>,
        Ignat Korchagin <ignat@...udflare.com>
Cc:     Christian Brauner <brauner@...nel.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        linux-doc@...r.kernel.org,
        linux-kernel <linux-kernel@...r.kernel.org>, linux-aio@...ck.org,
        linux-fsdevel@...r.kernel.org, linux-cachefs@...hat.com,
        linux-cifs@...r.kernel.org, samba-technical@...ts.samba.org,
        linux-mm@...ck.org, linux-nfs@...r.kernel.org,
        linux-unionfs@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        netdev <netdev@...r.kernel.org>, keyrings@...r.kernel.org,
        selinux@...r.kernel.org, serge@...lyn.com, amir73il@...il.com,
        kernel-team <kernel-team@...udflare.com>,
        Jeff Moyer <jmoyer@...hat.com>
Subject: Re: [PATCH v3] cred: Propagate security_prepare_creds() error code

On 6/15/22 10:55 AM, Casey Schaufler wrote:
> On 6/15/2022 8:33 AM, Paul Moore wrote:
>> On Wed, Jun 15, 2022 at 11:06 AM Ignat Korchagin 
>> <ignat@...udflare.com> wrote:
>>> On Wed, Jun 15, 2022 at 3:14 PM Paul Moore <paul@...l-moore.com> wrote:
>>>> On Wed, Jun 15, 2022 at 6:30 AM Christian Brauner 
>>>> <brauner@...nel.org> wrote:
>> ...
>>
>>>>> Fwiw, from this commit it wasn't very clear what you wanted to achieve
>>>>> with this. It might be worth considering adding a new security hook 
>>>>> for
>>>>> this. Within msft it recently came up SELinux might have an 
>>>>> interest in
>>>>> something like this as well.
>>>> Just to clarify things a bit, I believe SELinux would have an interest
>>>> in a LSM hook capable of implementing an access control point for user
>>>> namespaces regardless of Microsoft's current needs.  I suspect due to
>>>> the security relevant nature of user namespaces most other LSMs would
>>>> be interested as well; it seems like a well crafted hook would be
>>>> welcome by most folks I think.
>>> Just to get the full picture: is there actually a good reason not to
>>> make this hook support this scenario? I understand it was not
>>> originally intended for this, but it is well positioned in the code,
>>> covers multiple subsystems (not only user namespaces), doesn't require
>>> changing the LSM interface and it already does the job - just the
>>> kernel internals need to respect the error code better. What bad
>>> things can happen if we extend its use case to not only allocate
>>> resources in LSMs?
>> My concern is that the security_prepare_creds() hook, while only
>> called from two different functions, ends up being called for a
>> variety of different uses (look at the prepare_creds() and
>> perpare_kernel_cred() callers) and I think it would be a challenge to
>> identify the proper calling context in the LSM hook implementation
>> given the current hook parameters.  One might be able to modify the
>> hook to pass the necessary information, but I don't think that would
>> be any cleaner than adding a userns specific hook.  I'm also guessing
>> that the modified security_prepare_creds() hook implementations would
>> also be more likely to encounter future maintenance issues as
>> overriding credentials in the kernel seems only to be increasing, and
>> each future caller would risk using the modified hook wrong by passing
>> the wrong context and triggering the wrong behavior in the LSM.
> 
> We don't usually have hooks that do both attribute management and
> access control. Some people seem excessively concerned about "cluttering"
> calling code with security_something() instances, but for the most
> part I think we're past that. I agree that making security_prepare_creds()
> multi-purpose is a bad idea. Shared cred management isn't simple, and
> adding access checks there is only going to make it worse.
> 

Sounds like we've reached the conclusion not to proceed with a v4 of 
this patch. I'll pivot to propose a new hook instead.

Thanks for the feedback everyone :)

Fred