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

[Frederick Lawler]

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@xxxxxxxxxxxxxx> wrote:
> > > On Wed, Jun 15, 2022 at 3:14 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > > On Wed, Jun 15, 2022 at 6:30 AM Christian Brauner 
> > > > <brauner@xxxxxxxxxx> 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