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

[Casey Schaufler <casey@schaufler-ca.com>]

On 6/15/2022 7:14 AM, Paul Moore wrote:
> On Wed, Jun 15, 2022 at 6:30 AM Christian Brauner <brauner@kernel.org> wrote:
>> On Tue, Jun 14, 2022 at 01:59:08PM -0500, Frederick Lawler wrote:
>>> On 6/14/22 11:30 AM, Eric W. Biederman wrote:
>>>> Frederick Lawler <fred@cloudflare.com> writes:
>>>>
>>>>> On 6/13/22 11:44 PM, Eric W. Biederman wrote:
>>>>>> Frederick Lawler <fred@cloudflare.com> writes:
>>>>>>
>>>>>>> Hi Eric,
>>>>>>>
>>>>>>> On 6/13/22 12:04 PM, Eric W. Biederman wrote:
>>>>>>>> Frederick Lawler <fred@cloudflare.com> writes:
>>>>>>>>
>>>>>>>>> While experimenting with the security_prepare_creds() LSM hook, we
>>>>>>>>> noticed that our EPERM error code was not propagated up the callstack.
>>>>>>>>> Instead ENOMEM is always returned.  As a result, some tools may send a
>>>>>>>>> confusing error message to the user:
>>>>>>>>>
>>>>>>>>> $ unshare -rU
>>>>>>>>> unshare: unshare failed: Cannot allocate memory
>>>>>>>>>
>>>>>>>>> A user would think that the system didn't have enough memory, when
>>>>>>>>> instead the action was denied.
>>>>>>>>>
>>>>>>>>> This problem occurs because prepare_creds() and prepare_kernel_cred()
>>>>>>>>> return NULL when security_prepare_creds() returns an error code. Later,
>>>>>>>>> functions calling prepare_creds() and prepare_kernel_cred() return
>>>>>>>>> ENOMEM because they assume that a NULL meant there was no memory
>>>>>>>>> allocated.
>>>>>>>>>
>>>>>>>>> Fix this by propagating an error code from security_prepare_creds() up
>>>>>>>>> the callstack.
>>>>>>>> Why would it make sense for security_prepare_creds to return an error
>>>>>>>> code other than ENOMEM?
>>>>>>>>     > That seems a bit of a violation of what that function is supposed to do
>>>>>>>>
>>>>>>> The API allows LSM authors to decide what error code is returned from the
>>>>>>> cred_prepare hook. security_task_alloc() is a similar hook, and has its return
>>>>>>> code propagated.
>>>>>> It is not an api.  It is an implementation detail of the linux kernel.
>>>>>> It is a set of convenient functions that do a job.
>>>>>> The general rule is we don't support cases without an in-tree user.  I
>>>>>> don't see an in-tree user.
>>>>>>
>>>>>>> I'm proposing we follow security_task_allocs() pattern, and add visibility for
>>>>>>> failure cases in prepare_creds().
>>>>>> I am asking why we would want to.  Especially as it is not an API, and I
>>>>>> don't see any good reason for anything but an -ENOMEM failure to be
>>>>>> supported.
>>>>>>
>>>>> We're writing a LSM BPF policy, and not a new LSM. Our policy aims to solve
>>>>> unprivileged unshare, similar to Debian's patch [1]. We're in a position such
>>>>> that we can't use that patch because we can't block _all_ of our applications
>>>>> from performing an unshare. We prefer a granular approach. LSM BPF seems like a
>>>>> good choice.
>>>> I am quite puzzled why doesn't /proc/sys/user/max_user_namespaces work
>>>> for you?
>>>>
>>> We have the following requirements:
>>>
>>> 1. Allow list criteria
>>> 2. root user must be able to create namespaces whenever
>>> 3. Everything else not in 1 & 2 must be denied
>>>
>>> We use per task attributes to determine whether or not we allow/deny the
>>> current call to unshare().
>>>
>>> /proc/sys/user/max_user_namespaces limits are a bit broad for this level of
>>> detail.
>>>
>>>>> Because LSM BPF exposes these hooks, we should probably treat them as an
>>>>> API. From that perspective, userspace expects unshare to return a EPERM
>>>>> when the call is denied permissions.
>>>> The BPF code gets to be treated as a out of tree kernel module.
>>>>
>>>>>> Without an in-tree user that cares it is probably better to go the
>>>>>> opposite direction and remove the possibility of return anything but
>>>>>> memory allocation failure.  That will make it clearer to implementors
>>>>>> that a general error code is not supported and this is not a location
>>>>>> to implement policy, this is only a hook to allocate state for the LSM.
>>>>>>
>>>>> That's a good point, and it's possible we're using the wrong hook for the
>>>>> policy. Do you know of other hooks we can look into?
>> 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.

Smack isn't going to be interested in such a hook with the current
user namespace behavior. User namespaces are a discretionary access
control and privilege (capabilities) feature. Smack implements only
mandatory access control. I would still endorse adding the hook
as I could see MAC aspects (e.g. general xattr mapping) being
implemented as part of user namespaces.