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/2022 9: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.

    Yeah, sort of. We still don't want to change it willy-nilly as it
    has multiple users from both ends.

    >
    > The general rule is we don't support cases without an in-tree user. I
    > don't see an in-tree user.

    Unfortunately, the BPF security module allows arbitrary out-of-tree programs
    in any hook. While returns other than -ENOMEM may be nonsensical, they are
    possible. This is driving the LSM infrastructure in the direction of being
    an API, in that users of BPF need to know what they are allowed to do in
    their hook programs.

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

    The more clearly we define how a function is to be used the more it looks
    like an API. The LSM security_ interfaces are not well designed. They have
    appeared, changed and disappeared organically. This was fine when there was
    one user and tolerable when there were a few, but is getting to be painful
    as the number of security modules increases and their assumptions and
    behavior diverges from subject/object mandatory access control.


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

    I agree with both sides to some extent. The caller shouldn't assume that
    the only possible error is -ENOMEM, but the LSM hook should never do anything
    else, either. If there is a legitimate case where an different error may
    be returned and a reasonable, different action the caller(s) would take in
    that case, the change makes sense. Otherwise, no.

    > Eric
    >

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