[PATCH linux-next] security: Fix side effects of default BPF LSM hooks

[Casey Schaufler]

On 6/10/2022 4:49 PM, KP Singh wrote:
> On Fri, Jun 10, 2022 at 8:50 PM Casey Schaufler <casey at schaufler-ca.com> wrote:
>> On 6/9/2022 4:46 PM, KP Singh wrote:
>>> BPF LSM currently has a default implementation for each LSM hooks which
>>> return a default value defined in include/linux/lsm_hook_defs.h. These
>>> hooks should have no functional effect when there is no BPF program
>>> loaded to implement the hook logic.
>>>
>>> Some LSM hooks treat any return value of the hook as policy decision
>>> which results in destructive side effects.
>>>
>>> This issue and the effects were reported to me by Jann Horn:
>>>
>>> For a system configured with CONFIG_BPF_LSM and the bpf lsm is enabled
>>> (via lsm= or CONFIG_LSM) an unprivileged user can vandalize the system
>>> by removing the security.capability xattrs from binaries, preventing
>>> them from working normally:
>>>
>>> $ getfattr -d -m- /bin/ping
>>> getfattr: Removing leading '/' from absolute path names
>>> security.capability=0sAQAAAgAgAAAAAAAAAAAAAAAAAAA=
>>>
>>> $ setfattr -x security.capability /bin/ping
>>> $ getfattr -d -m- /bin/ping
>>> $ ping 1.2.3.4
>>> $ ping google.com
>>> $ echo $?
>>> 2
>>>
>>> The above reproduces with:
>>>
>>> cat /sys/kernel/security/lsm
>>> capability,apparmor,bpf
>>>
>>> But not with SELinux as SELinux does the required check in its LSM hook:
>>>
>>> cat /sys/kernel/security/lsm
>>> capability,selinux,bpf
>>>
>>> In this case security_inode_removexattr() calls
>>> call_int_hook(inode_removexattr, 1, mnt_userns, dentry, name), which
>>> expects a return value of 1 to mean "no LSM hooks hit" and 0 is
>>> supposed to mean "the LSM decided to permit the access and checked
>>> cap_inode_removexattr"
>>>
>>> There are other security hooks that are similarly effected.
>> This shouldn't come as a surprise.
>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2281257.html
>> is just one place where this sort of issue has been discussed.
>>
>>> In order to reliably fix this issue and also allow LSM Hooks and BPF
>>> programs which implement hook logic to choose to not make a decision
>>> in certain conditions (e.g. when BPF programs are used for auditing),
>>> introduce a special return value LSM_HOOK_NO_EFFECT which can be used
>>> by the hook to indicate to the framework that it does not intend to
>>> make a decision.
>> The LSM infrastructure already has a convention of returning
>> -EOPNOTSUPP for this condition. Why add another value to check?'
> This is not the case in call_int_hook currently.
>
> If we can update the LSM infra to imply that  -EOPNOTSUPP means
> that the hook iteration can continue as that implies "no decision"
> this would be okay as well.

It would be really nice if there was sufficient consistency in
the LSM infrastructure for this to make sense. The cases where
a module supplies a hook but only cares about the data some of
the time are rare, excepting for BPF. As I mention elsewhere,
general stacking is what you need.