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
