| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJ2a_DdrhV0pGj1WQ9N201KCY3bp_70zd1eSZ2PD1VOOd6SBrg@mail.gmail.com>
Date: Sun, 2 Mar 2025 19:35:55 +0100
From: Christian Göttsche <cgzones@...glemail.com>
To: Casey Schaufler <casey@...aufler-ca.com>
Cc: Serge Hallyn <serge@...lyn.com>, Jan Kara <jack@...e.com>,
Julia Lawall <Julia.Lawall@...ia.fr>, Nicolas Palix <nicolas.palix@...g.fr>,
linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org,
cocci@...ia.fr
Subject: Re: [PATCH v2 01/11] coccinelle: Add script to reorder capable() calls
On Sun, 2 Mar 2025 at 17:53, Casey Schaufler <casey@...aufler-ca.com> wrote:
>
> On 3/2/2025 8:06 AM, Christian Göttsche wrote:
> > From: Christian Göttsche <cgzones@...glemail.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@...glemail.com>
> > Reviewed-by: Serge Hallyn <serge@...lyn.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
> > +//----------------------------------------------------------
> > +
> > +@...ore@
> > +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
> > +//----------------------------------------------------------
> > +
> > +@...ipt:python depends on org@
> > +p << r1.p;
> > +@@
> > +
> > +cocci.print_main("WARNING opportunity for capable reordering",p)
> > +
> > +
> > +//----------------------------------------------------------
> > +// For report mode
> > +//----------------------------------------------------------
> > +
> > +@...ipt:python depends on report@
> > +p << r1.p;
> > +@@
> > +
> > +msg = "WARNING opportunity for capable reordering"
> > +coccilib.report.print_report(p[0], msg)
Powered by blists - more mailing lists