Messages in this thread | | | Date | Thu, 16 Jun 2022 10:04:07 -0500 | Subject | Re: [PATCH v3] cred: Propagate security_prepare_creds() error code | From | 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@cloudflare.com> wrote: >>> On Wed, Jun 15, 2022 at 3:14 PM Paul Moore <paul@paul-moore.com> wrote: >>>> On Wed, Jun 15, 2022 at 6:30 AM Christian Brauner >>>> <brauner@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. >
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
| |