On Sun, 2 Mar 2025 at 17:53, Casey Schaufler [off-list ref] wrote:
On 3/2/2025 8:06 AM, Christian Göttsche wrote:
quoted
From: Christian Göttsche <redacted>
capable() calls refer to enabled LSMs whether to permit or deny the
request. This is relevant in connection with SELinux, where a
capability check results in a policy decision and by default a denial
message on insufficient permission is issued.
It can lead to three undesired cases:
1. A denial message is generated, even in case the operation was an
unprivileged one and thus the syscall succeeded, creating noise.
2. To avoid the noise from 1. the policy writer adds a rule to ignore
those denial messages, hiding future syscalls, where the task
performs an actual privileged operation, leading to hidden limited
functionality of that task.
3. To avoid the noise from 1. the policy writer adds a rule to permit
the task the requested capability, while it does not need it,
violating the principle of least privilege.
What steps are you taking to ensure that these changes do not
negatively impact LSMs other than SELinux? At a glance, I don't
see that there is likely to be a problem. I do see a possibility
that changes in error returns could break test suites and, more
importantly, applications that are careful about using privileged
operations.
Checks are only reordered where the current right-hand side is
side-effect free, e.g. a comparison.
Whether a branch is taken or not (and thus possible return values)
should not be affected.
Here is the current output of the script with the false-positives not converted:
### begin ###
diff -u -p a/kernel/capability.c b/kernel/capability.c
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -491,8 +491,8 @@ bool capable_wrt_inode_uidgid(struct mnt
{
struct user_namespace *ns = current_user_ns();
- return ns_capable(ns, cap) &&
- privileged_wrt_inode_uidgid(ns, idmap, inode);
+ return privileged_wrt_inode_uidgid(ns, idmap, inode) && ns_capable(ns,
+ cap);
}
EXPORT_SYMBOL(capable_wrt_inode_uidgid);
diff -u -p a/kernel/pid.c b/kernel/pid.c--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -662,8 +662,7 @@ static int pid_table_root_permissions(st
container_of(head->set, struct pid_namespace, set);
int mode = table->mode;
- if (ns_capable(pidns->user_ns, CAP_SYS_ADMIN) ||
- uid_eq(current_euid(), make_kuid(pidns->user_ns, 0)))
+ if (uid_eq(current_euid(), make_kuid(pidns->user_ns, 0)) ||ns_capable(pidns->user_ns, CAP_SYS_ADMIN))
mode = (mode & S_IRWXU) >> 6;
else if (in_egroup_p(make_kgid(pidns->user_ns, 0)))
mode = (mode & S_IRWXG) >> 3;
diff -u -p a/kernel/bpf/token.c b/kernel/bpf/token.c--- a/kernel/bpf/token.c
+++ b/kernel/bpf/token.c
static bool bpf_ns_capable(struct user_namespace *ns, int cap)
{
- return ns_capable(ns, cap) || (cap != CAP_SYS_ADMIN &&
ns_capable(ns, CAP_SYS_ADMIN));
+ return (cap != CAP_SYS_ADMIN && ns_capable(ns, CAP_SYS_ADMIN))
|| ns_capable(ns,
+
cap);
}
bool bpf_token_capable(const struct bpf_token *token, int cap)
diff -u -p a/debian/linux-headers-6.9.0-rc2+/usr/src/linux-headers-6.9.0-rc2+/include/linux/bpf.h
b/debian/linux-headers-6.9.0-rc2+/usr/src/linux-headers-6.9.0-rc2+/include/linux/bpf.h--- a/debian/linux-headers-6.9.0-rc2+/usr/src/linux-headers-6.9.0-rc2+/include/linux/bpf.h
+++ b/debian/linux-headers-6.9.0-rc2+/usr/src/linux-headers-6.9.0-rc2+/include/linux/bpf.h
@@ -2715,7 +2715,7 @@ static inline int bpf_obj_get_user(const
static inline bool bpf_token_capable(const struct bpf_token *token, int cap)
{
- return capable(cap) || (cap != CAP_SYS_ADMIN && capable(CAP_SYS_ADMIN));
+ return (cap != CAP_SYS_ADMIN && capable(CAP_SYS_ADMIN)) || capable(cap);
}
static inline void bpf_token_inc(struct bpf_token *token)
diff -u -p a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1285,8 +1285,7 @@ static ssize_t scrub_show(struct device
&& !test_bit(ARS_CANCEL, &acpi_desc->scrub_flags);
rc = sysfs_emit(buf, "%d%s", acpi_desc->scrub_count, busy ?"+\n" : "\n");
/* Allow an admin to poll the busy state at a higher rate */
- if (busy && capable(CAP_SYS_RAWIO) && !test_and_set_bit(ARS_POLL,
- &acpi_desc->scrub_flags)) {
+ if (busy && !test_and_set_bit(ARS_POLL,
&acpi_desc->scrub_flags) && capable(CAP_SYS_RAWIO)) {
acpi_desc->scrub_tmo = 1;
mod_delayed_work(nfit_wq, &acpi_desc->dwork, HZ);
}
diff -u -p a/include/linux/bpf.h b/include/linux/bpf.h--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2830,7 +2830,7 @@ static inline int bpf_obj_get_user(const
static inline bool bpf_token_capable(const struct bpf_token *token, int cap)
{
- return capable(cap) || (cap != CAP_SYS_ADMIN && capable(CAP_SYS_ADMIN));
+ return (cap != CAP_SYS_ADMIN && capable(CAP_SYS_ADMIN)) || capable(cap);
}
static inline void bpf_token_inc(struct bpf_token *token)
diff -u -p a/kernel/user_namespace.c b/kernel/user_namespace.c--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -1194,8 +1194,7 @@ static bool new_idmap_permitted(const st
* (CAP_SETUID or CAP_SETGID) over the parent user namespace.
* And the opener of the id file also has the appropriate capability.
*/
- if (ns_capable(ns->parent, cap_setid) &&
- file_ns_capable(file, ns->parent, cap_setid))
+ if (file_ns_capable(file, ns->parent, cap_setid) &&ns_capable(ns->parent, cap_setid))
return true;
return false;
### end ###
quoted
Signed-off-by: Christian Göttsche <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
MAINTAINERS | 1 +
scripts/coccinelle/api/capable_order.cocci | 98 ++++++++++++++++++++++
2 files changed, 99 insertions(+)
create mode 100644 scripts/coccinelle/api/capable_order.cocci
diff --git a/MAINTAINERS b/MAINTAINERS
index 8e0736dc2ee0..b1d1c801765b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5196,6 +5196,7 @@ F: include/linux/capability.h
F: include/trace/events/capability.h
F: include/uapi/linux/capability.h
F: kernel/capability.c
+F: scripts/coccinelle/api/capable_order.cocci
F: security/commoncap.c
CAPELLA MICROSYSTEMS LIGHT SENSOR DRIVER
diff --git a/scripts/coccinelle/api/capable_order.cocci b/scripts/coccinelle/api/capable_order.cocci
new file mode 100644
index 000000000000..4150d91b0f33
--- /dev/null
+++ b/scripts/coccinelle/api/capable_order.cocci
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Checks for capable() calls of the left side of a binary expression.
+/// Reordering might avoid needless checks, LSM log messages, and more
+/// restrictive LSM security policies (e.g. SELinux).
+/// Can report false positives if the righthand side contains a nested
+/// capability check or has side effects.
+///
+// Confidence: Moderate
+// Copyright: (C) 2024 Christian Göttsche.
+// Options: --no-includes --include-headers
+// Keywords: capable, ns_capable, sockopt_ns_capable
+//
+
+virtual patch
+virtual context
+virtual org
+virtual report
+
+//----------------------------------------------------------
+// Pattern to ignore
+//----------------------------------------------------------
+
+@ignore@
+identifier F1 = { capable, ns_capable, sockopt_ns_capable };
+identifier F2 = { capable, ns_capable, sockopt_ns_capable };
+binary operator op,op1,op2;
+expression E;
+position p;
+@@
+
+(
+F1@p(...) op F2(...)
+|
+E op1 F1@p(...) op2 F2(...)
+)
+
+
+//----------------------------------------------------------
+// For patch mode
+//----------------------------------------------------------
+
+@ depends on patch@
+identifier F = { capable, ns_capable, sockopt_ns_capable };
+binary operator op,op1,op2;
+expression E,E1,E2;
+expression list EL;
+position p != ignore.p;
+@@
+
+(
+- F@p(EL) op E
++ E op F(EL)
+|
+- E1 op1 F@p(EL) op2 E2
++ E1 op1 E2 op2 F(EL)
+)
+
+
+//----------------------------------------------------------
+// For context mode
+//----------------------------------------------------------
+
+@r1 depends on !patch exists@
+identifier F = { capable, ns_capable, sockopt_ns_capable };
+binary operator op,op1,op2;
+expression E, E1, E2;
+position p != ignore.p;
+@@
+
+(
+* F@p(...) op E
+|
+* E1 op1 F@p(...) op2 E2
+)
+
+
+//----------------------------------------------------------
+// For org mode
+//----------------------------------------------------------
+
+@script:python depends on org@
+p << r1.p;
+@@
+
+cocci.print_main("WARNING opportunity for capable reordering",p)
+
+
+//----------------------------------------------------------
+// For report mode
+//----------------------------------------------------------
+
+@script:python depends on report@
+p << r1.p;
+@@
+
+msg = "WARNING opportunity for capable reordering"
+coccilib.report.print_report(p[0], msg)