[RFC,0/4] security: Ensure LSMs return expected values

Message

Roberto Sassu Nov. 15, 2022, 5:56 p.m. UTC

From: Roberto Sassu <[e-mail]>

LSMs should follow the conventions stated in include/linux/lsm_hooks.h for
return values, as these conventions are followed by callers of the LSM
infrastructure for error handling.

The ability of an LSM to return arbitrary values could cause big troubles.
For LSMs aiming at being upstreamed, this event is unlikely, as each LSM is
carefully reviewed and it won't be accepted if it does not meet the return
value conventions. However, the recent introduction of BPF LSM allows
security modules (not part of the kernel) to inject arbitrary values,
without BPF LSM verifying them.

The initial idea was to fix BPF LSM itself. However, due to technical
difficulties to determine the precise interval of return values from a
static code analysis of eBPF programs, the new approach was to put the
fix in the LSM infrastructure, so that all LSMs can benefit from this work
as well.

The biggest problem of allowing arbitrary return values 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 fields 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). This problem is
addressed separately in another patch set.

Finally, there are LSM hooks which are supposed to return just 1 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.

Patches 1 and 2 ensure that the documentation of LSM return values is
complete and accurate. Then, patch 3 introduces four flags (LSM_RET_NEG,
LSM_RET_ZERO, LSM_RET_ONE, LSM_RET_GT_ONE), one for each interval of
interest (< 0, = 0, = 1, > 1), and sets the correct flags for each LSM
hook. Finally, patch 4 verifies for each return value from LSMs that it is
an expected one.

Roberto Sassu (4):
  lsm: Clarify documentation of vm_enough_memory hook
  lsm: Add missing return values doc in lsm_hooks.h and fix formatting
  lsm: Redefine LSM_HOOK() macro to add return value flags as argument
  security: Enforce limitations on return values from LSMs

 include/linux/bpf_lsm.h       |   2 +-
 include/linux/lsm_hook_defs.h | 779 ++++++++++++++++++++--------------
 include/linux/lsm_hooks.h     | 136 ++++--
 kernel/bpf/bpf_lsm.c          |   5 +-
 security/bpf/hooks.c          |   2 +-
 security/security.c           |  38 +-
 6 files changed, 589 insertions(+), 373 deletions(-)

Comments

Casey Schaufler Nov. 15, 2022, 6:41 p.m. UTC | #1

On 11/15/2022 9:56 AM, Roberto Sassu wrote:
> From: Roberto Sassu <[e-mail]>
>
> LSMs should follow the conventions stated in include/linux/lsm_hooks.h for
> return values, as these conventions are followed by callers of the LSM
> infrastructure for error handling.
>
> The ability of an LSM to return arbitrary values could cause big troubles.
> For LSMs aiming at being upstreamed, this event is unlikely, as each LSM is
> carefully reviewed and it won't be accepted if it does not meet the return
> value conventions. However, the recent introduction of BPF LSM allows
> security modules (not part of the kernel) to inject arbitrary values,
> without BPF LSM verifying them.
>
> The initial idea was to fix BPF LSM itself. However, due to technical
> difficulties to determine the precise interval of return values from a
> static code analysis of eBPF programs, the new approach was to put the
> fix in the LSM infrastructure, so that all LSMs can benefit from this work
> as well.

The LSM infrastructure is a hodgepodge of inconsistently defined functions.
That makes them difficult to deal with in any sort of consistent way.
Your attempt to make them more rational is laudable, but falls short of
being useful, while slowing all security processing to no end.

I don't consider this a "fix", and I don't see how it benefits any LSM
other than BPF. Adding return code checks into the infrastructure code is
insufficient. If someone adds a bpf_secid_to_secctx() eBPF program you're
going to have a whole new set of problems that this clutter isn't going to
help with at all. Return values are an important part of the problem, but
nowhere near the whole of it.

Please take this back into the BPF security module, where it belongs.
BPF isn't playing by the admittedly complex, inconsistent and arbitrary
rules of the LSM infrastructure. Your proposal doesn't change that, nor
does it sufficiently change the infrastructure to make BPF safe.

>
> The biggest problem of allowing arbitrary return values 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 fields 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). This problem is
> addressed separately in another patch set.
>
> Finally, there are LSM hooks which are supposed to return just 1 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.
>
> Patches 1 and 2 ensure that the documentation of LSM return values is
> complete and accurate. Then, patch 3 introduces four flags (LSM_RET_NEG,
> LSM_RET_ZERO, LSM_RET_ONE, LSM_RET_GT_ONE), one for each interval of
> interest (< 0, = 0, = 1, > 1), and sets the correct flags for each LSM
> hook. Finally, patch 4 verifies for each return value from LSMs that it is
> an expected one.
>
> Roberto Sassu (4):
>   lsm: Clarify documentation of vm_enough_memory hook
>   lsm: Add missing return values doc in lsm_hooks.h and fix formatting
>   lsm: Redefine LSM_HOOK() macro to add return value flags as argument
>   security: Enforce limitations on return values from LSMs
>
>  include/linux/bpf_lsm.h       |   2 +-
>  include/linux/lsm_hook_defs.h | 779 ++++++++++++++++++++--------------
>  include/linux/lsm_hooks.h     | 136 ++++--
>  kernel/bpf/bpf_lsm.c          |   5 +-
>  security/bpf/hooks.c          |   2 +-
>  security/security.c           |  38 +-
>  6 files changed, 589 insertions(+), 373 deletions(-)
>