lkml.org 
[lkml]   [2022]   [Jun]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3] cred: Propagate security_prepare_creds() error code
From
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.

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.

> 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?

>>> I have probably missed a very interesting discussion where that was
>>> mentioned but I don't see link to the discussion or anything explaining
>>> why we want to do that in this change.
>>>
>>
>> AFAIK, this is the start of the discussion.
>
> You were on v3 and had an out of tree piece of code so I assumed someone
> had at least thought about why you want to implement policy in a piece
> of code whose only purpose is to allocate memory to store state.
>

No worries.

> Eric
>
>
>

Links:
1:
https://sources.debian.org/patches/linux/3.16.56-1+deb8u1/debian/add-sysctl-to-disallow-unprivileged-CLONE_NEWUSER-by-default.patch/

\
 
 \ /
  Last update: 2022-06-14 18:08    [W:0.221 / U:0.432 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site