[PATCH v3] cred: Propagate security_prepare_creds() error code

[Casey Schaufler]

On 6/15/2022 8:33 AM, Paul Moore wrote:
> On Wed, Jun 15, 2022 at 11:06 AM Ignat Korchagin <ignat at cloudflare.com> wrote:
>> On Wed, Jun 15, 2022 at 3:14 PM Paul Moore <paul at paul-moore.com> wrote:
>>> On Wed, Jun 15, 2022 at 6:30 AM Christian Brauner <brauner at kernel.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.