Re: [RESEND][RFC][PATCH 2/3] bpf-lsm: Limit values that can be returned by security modules

[Roberto Sassu]

On Fri, 2022-11-04 at 17:42 -0700, Alexei Starovoitov wrote:
> On Fri, Nov 4, 2022 at 8:29 AM Roberto Sassu
> <roberto.sassu@xxxxxxxxxxxxxxx> wrote:
> > On Thu, 2022-11-03 at 16:09 +0100, KP Singh wrote:
> > > On Fri, Oct 28, 2022 at 6:55 PM Roberto Sassu
> > > <roberto.sassu@xxxxxxxxxxxxxxx> wrote:
> > > > From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > > > 
> > > > BPF LSM defines a bpf_lsm_*() function for each LSM hook, so that
> > > > security modules can define their own implementation for the desired hooks.
> > > > 
> > > > Unfortunately, BPF LSM does not restrict which values security modules can
> > > > return (for non-void LSM hooks). Security modules might follow the
> > > > conventions stated in include/linux/lsm_hooks.h, or put arbitrary values.
> > > > 
> > > > This could cause big troubles, as the kernel is not ready to handle
> > > > possibly malicious return values from LSMs. Until now, it was not the
> > > 
> > > I am not sure I would call this malicious. This would be incorrect, if
> > > someone is writing a BPF LSM program they already have the powers
> > > to willingly do a lot of malicious stuff.
> > > 
> > > It's about unknowingly returning values that can break the system.
> > 
> > Maybe it is possible to return specific values that lead to acquire
> > more information/do actions that the eBPF program is not supposed to
> > cause.
> > 
> > I don't have a concrete example, so I will use the word you suggested.
> > 
> > > > case, as each LSM is carefully reviewed and it won't be accepted if it
> > > > does not meet the return value conventions.
> > > > 
> > > > The biggest problem is when an LSM returns a positive value, instead of a
> > > > negative value, as it could be converted to a pointer. Since such pointer
> > > > escapes the IS_ERR() check, its use later in the code can cause
> > > > unpredictable consequences (e.g. invalid memory access).
> > > > 
> > > > Another problem is returning zero when an LSM is supposed to have done some
> > > > operations. For example, the inode_init_security hook expects that their
> > > > implementations return zero only if they set the name and value of the new
> > > > xattr to be added to the new inode. Otherwise, other kernel subsystems
> > > > might encounter unexpected conditions leading to a crash (e.g.
> > > > evm_protected_xattr_common() getting NULL as argument).
> > > > 
> > > > Finally, there are LSM hooks which are supposed to return just one as
> > > > positive value, or non-negative values. Also in these cases, although it
> > > > seems less critical, it is safer to return to callers of the LSM
> > > > infrastructure more precisely what they expect.
> > > > 
> > > > As eBPF allows code outside the kernel to run, it is its responsibility
> > > > to ensure that only expected values are returned to LSM infrastructure
> > > > callers.
> > > > 
> > > > Create four new BTF ID sets, respectively for hooks that can return
> > > > positive values, only one as positive value, that cannot return zero, and
> > > > that cannot return negative values. Create also corresponding functions to
> > > > check if the hook a security module is attached to belongs to one of the
> > > > defined sets.
> > > > 
> > > > Finally, check in the eBPF verifier the value returned by security modules
> > > > for each attached LSM hook, and return -EINVAL (the security module cannot
> > > > run) if the hook implementation does not satisfy the hook return value
> > > > policy.
> > > > 
> > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > Fixes: 9d3fdea789c8 ("bpf: lsm: Provide attachment points for BPF LSM programs")
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > > > ---
> > > >  include/linux/bpf_lsm.h | 24 ++++++++++++++++++
> > > >  kernel/bpf/bpf_lsm.c    | 56 +++++++++++++++++++++++++++++++++++++++++
> > > >  kernel/bpf/verifier.c   | 35 +++++++++++++++++++++++---
> > > >  3 files changed, 112 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> > > > index 4bcf76a9bb06..cd38aca4cfc0 100644
> > > > --- a/include/linux/bpf_lsm.h
> > > > +++ b/include/linux/bpf_lsm.h
> > > > @@ -28,6 +28,10 @@ int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> > > >                         const struct bpf_prog *prog);
> > > > 
> > > >  bool bpf_lsm_is_sleepable_hook(u32 btf_id);
> > > > +bool bpf_lsm_can_ret_pos_value(u32 btf_id);
> > > > +bool bpf_lsm_can_ret_only_one_as_pos_value(u32 btf_id);
> > > > +bool bpf_lsm_cannot_ret_zero(u32 btf_id);
> > > > +bool bpf_lsm_cannot_ret_neg_value(u32 btf_id);
> > > > 
> > > 
> > > This does not need to be exported to the rest of the kernel. Please
> > > have this logic in bpf_lsm.c and export a single verify function.
> > > 
> > > Also, these really don't need to be such scattered logic, Could we
> > > somehow encode this into the LSM_HOOK definition?
> > 
> > The problem is that a new LSM_HOOK definition would apply to every LSM
> > hook, while we need the ability to select subsets.
> > 
> > I was thinking, but I didn't check yet, what about using BTF_ID_FLAGS,
> > introducing a flag for each interval (<0, 0, 1, >1) and setting the
> > appropriate flags for each LSM hook?
> 
> Before adding infra to all hooks, let's analyze all hooks first.
> I thought the number of exceptions is very small.
> 99% of hooks will be fine with IS_ERR.
> If so, adding an extra flag to every hook will cause too much churn.

If I counted them correctly, there are 12 hooks which can return a
positive value. Among them, 6 can return only 1. 3 should not return a
negative value.

A reason for making this change in the LSM infrastructure would be that
the return values provided there would be the official reference for
anyone dealing with LSM hooks (e.g. redefining the LSM_HOOK macro).

Another reason would be that for new hooks, the developer introducing
them would have to provide the information. BPF LSM would use that
automatically (otherwise it might get out of sync).

The idea would be to use BTF_ID_FLAGS() with the flags coming from
lsm_hook_defs.h, and to check if a flag is set depending on the
interval of the return value provided by the eBPF program.

Roberto