| Message ID | 20250302160657.127253-11-cgoettsche@seltendoof.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,01/11] coccinelle: Add script to reorder capable() calls | expand |
On 3/2/2025 8:06 AM, Christian Göttsche wrote: > From: Christian Göttsche <cgzones@googlemail.com> > > 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. > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > 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)
On Sun, 2 Mar 2025 at 17:53, Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 3/2/2025 8:06 AM, Christian Göttsche wrote: > > From: Christian Göttsche <cgzones@googlemail.com> > > > > 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 @@ -10,7 +10,8 @@ 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 ### > > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > 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)
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)