Introduce a new capable flag, CAP_OPT_NODENYAUDIT, to not generate
an audit event if the requested capability is not granted. This will be
used in a new capable_any() functionality to reduce the number of
necessary capable calls.
Handle the flag accordingly in AppArmor and SELinux.
Suggested-by: Paul Moore <[email protected]>
Signed-off-by: Christian Göttsche <[email protected]>
---
include/linux/security.h | 2 ++
security/apparmor/capability.c | 8 +++++---
security/selinux/hooks.c | 14 ++++++++------
3 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index e2734e9e44d5..629c775ec297 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -67,6 +67,8 @@ struct watch_notification;
#define CAP_OPT_NOAUDIT BIT(1)
/* If capable is being called by a setid function */
#define CAP_OPT_INSETID BIT(2)
+/* If capable should audit the security request for authorized requests only */
+#define CAP_OPT_NODENYAUDIT BIT(3)
/* LSM Agnostic defines for security_sb_set_mnt_opts() flags */
#define SECURITY_LSM_NATIVE_LABELS 1
diff --git a/security/apparmor/capability.c b/security/apparmor/capability.c
index 326a51838ef2..98120dd62ca7 100644
--- a/security/apparmor/capability.c
+++ b/security/apparmor/capability.c
@@ -108,7 +108,8 @@ static int audit_caps(struct common_audit_data *sa, struct aa_profile *profile,
* profile_capable - test if profile allows use of capability @cap
* @profile: profile being enforced (NOT NULL, NOT unconfined)
* @cap: capability to test if allowed
- * @opts: CAP_OPT_NOAUDIT bit determines whether audit record is generated
+ * @opts: CAP_OPT_NOAUDIT/CAP_OPT_NODENYAUDIT bit determines whether audit
+ * record is generated
* @sa: audit data (MAY BE NULL indicating no auditing)
*
* Returns: 0 if allowed else -EPERM
@@ -126,7 +127,7 @@ static int profile_capable(struct aa_profile *profile, int cap,
else
error = -EPERM;
- if (opts & CAP_OPT_NOAUDIT) {
+ if ((opts & CAP_OPT_NOAUDIT) || ((opts & CAP_OPT_NODENYAUDIT) && error)) {
if (!COMPLAIN_MODE(profile))
return error;
/* audit the cap request in complain mode but note that it
@@ -142,7 +143,8 @@ static int profile_capable(struct aa_profile *profile, int cap,
* aa_capable - test permission to use capability
* @label: label being tested for capability (NOT NULL)
* @cap: capability to be tested
- * @opts: CAP_OPT_NOAUDIT bit determines whether audit record is generated
+ * @opts: CAP_OPT_NOAUDIT/CAP_OPT_NODENYAUDIT bit determines whether audit
+ * record is generated
*
* Look up capability in profile capability set.
*
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 79b4890e9936..0730edf2f5f1 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1571,7 +1571,7 @@ static int cred_has_capability(const struct cred *cred,
u16 sclass;
u32 sid = cred_sid(cred);
u32 av = CAP_TO_MASK(cap);
- int rc;
+ int rc, rc2;
ad.type = LSM_AUDIT_DATA_CAP;
ad.u.cap = cap;
@@ -1590,11 +1590,13 @@ static int cred_has_capability(const struct cred *cred,
}
rc = avc_has_perm_noaudit(sid, sid, sclass, av, 0, &avd);
- if (!(opts & CAP_OPT_NOAUDIT)) {
- int rc2 = avc_audit(sid, sid, sclass, av, &avd, rc, &ad);
- if (rc2)
- return rc2;
- }
+ if ((opts & CAP_OPT_NOAUDIT) || ((opts & CAP_OPT_NODENYAUDIT) && rc))
+ return rc;
+
+ rc2 = avc_audit(sid, sid, sclass, av, &avd, rc, &ad);
+ if (rc2)
+ return rc2;
+
return rc;
}
--
2.40.1
Use the new added capable_any function in appropriate cases, where a
task is required to have any of two capabilities.
Signed-off-by: Christian Göttsche <[email protected]>
---
v3:
rename to capable_any()
---
kernel/bpf/syscall.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 14f39c1e573e..1bd50da05a22 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2539,7 +2539,7 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
!bpf_capable())
return -EPERM;
- if (is_net_admin_prog_type(type) && !capable(CAP_NET_ADMIN) && !capable(CAP_SYS_ADMIN))
+ if (is_net_admin_prog_type(type) && !capable_any(CAP_NET_ADMIN, CAP_SYS_ADMIN))
return -EPERM;
if (is_perfmon_prog_type(type) && !perfmon_capable())
return -EPERM;
--
2.40.1
Use the new added capable_any function in appropriate cases, where a
task is required to have any of two capabilities.
Signed-off-by: Christian Göttsche <[email protected]>
---
v3:
rename to capable_any()
---
fs/pipe.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/pipe.c b/fs/pipe.c
index ceb17d2dfa19..05c64494d37b 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -776,7 +776,7 @@ bool too_many_pipe_buffers_hard(unsigned long user_bufs)
bool pipe_is_unprivileged_user(void)
{
- return !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN);
+ return !capable_any(CAP_SYS_RESOURCE, CAP_SYS_ADMIN);
}
struct pipe_inode_info *alloc_pipe_info(void)
--
2.40.1
Add the interfaces `capable_any()` and `ns_capable_any()` as an
alternative to multiple `capable()`/`ns_capable()` calls, like
`capable_any(CAP_SYS_NICE, CAP_SYS_ADMIN)` instead of
`capable(CAP_SYS_NICE) || capable(CAP_SYS_ADMIN)`.
`capable_any()`/`ns_capable_any()` will in particular generate exactly
one audit message, either for the left most capability in effect or, if
the task has none, the first one.
This is especially helpful with regard to SELinux, where each audit
message about a not allowed capability request will create a denial
message. Using this new wrapper with the least invasive capability as
left most argument (e.g. CAP_SYS_NICE before CAP_SYS_ADMIN) enables
policy writers to only grant the least invasive one for the particular
subject instead of both.
Signed-off-by: Christian Göttsche <[email protected]>
---
v4:
Use CAP_OPT_NODENYAUDIT via added ns_capable_nodenyaudit()
v3:
- rename to capable_any()
- fix typo in function documentation
- add ns_capable_any()
v2:
avoid varargs and fix to two capabilities; capable_or3() can be added
later if needed
---
include/linux/capability.h | 10 ++++++
kernel/capability.c | 70 ++++++++++++++++++++++++++++++++++++++
2 files changed, 80 insertions(+)
diff --git a/include/linux/capability.h b/include/linux/capability.h
index 0c356a517991..eeb958440656 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -146,7 +146,9 @@ extern bool has_capability_noaudit(struct task_struct *t, int cap);
extern bool has_ns_capability_noaudit(struct task_struct *t,
struct user_namespace *ns, int cap);
extern bool capable(int cap);
+extern bool capable_any(int cap1, int cap2);
extern bool ns_capable(struct user_namespace *ns, int cap);
+extern bool ns_capable_any(struct user_namespace *ns, int cap1, int cap2);
extern bool ns_capable_noaudit(struct user_namespace *ns, int cap);
extern bool ns_capable_setid(struct user_namespace *ns, int cap);
#else
@@ -172,10 +174,18 @@ static inline bool capable(int cap)
{
return true;
}
+static inline bool capable_any(int cap1, int cap2)
+{
+ return true;
+}
static inline bool ns_capable(struct user_namespace *ns, int cap)
{
return true;
}
+static inline bool ns_capable_any(struct user_namespace *ns, int cap1, int cap2)
+{
+ return true;
+}
static inline bool ns_capable_noaudit(struct user_namespace *ns, int cap)
{
return true;
diff --git a/kernel/capability.c b/kernel/capability.c
index 3e058f41df32..d50544063920 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -402,6 +402,23 @@ bool ns_capable_noaudit(struct user_namespace *ns, int cap)
}
EXPORT_SYMBOL(ns_capable_noaudit);
+/**
+ * ns_capable_nodenyaudit - Determine if the current task has a superior capability
+ * (unaudited when not authorized) in effect
+ * @ns: The usernamespace we want the capability in
+ * @cap: The capability to be tested for
+ *
+ * Return true if the current task has the given superior capability currently
+ * available for use, false if not.
+ *
+ * This sets PF_SUPERPRIV on the task if the capability is available on the
+ * assumption that it's about to be used.
+ */
+static bool ns_capable_nodenyaudit(struct user_namespace *ns, int cap)
+{
+ return ns_capable_common(ns, cap, CAP_OPT_NODENYAUDIT);
+}
+
/**
* ns_capable_setid - Determine if the current task has a superior capability
* in effect, while signalling that this check is being done from within a
@@ -421,6 +438,59 @@ bool ns_capable_setid(struct user_namespace *ns, int cap)
}
EXPORT_SYMBOL(ns_capable_setid);
+/**
+ * ns_capable_any - Determine if the current task has one of two superior capabilities in effect
+ * @ns: The usernamespace we want the capability in
+ * @cap1: The capabilities to be tested for first
+ * @cap2: The capabilities to be tested for secondly
+ *
+ * Return true if the current task has at least one of the two given superior
+ * capabilities currently available for use, false if not.
+ *
+ * In contrast to or'ing capable() this call will create exactly one audit
+ * message, either for @cap1, if it is granted or both are not permitted,
+ * or @cap2, if it is granted while the other one is not.
+ *
+ * The capabilities should be ordered from least to most invasive, i.e. CAP_SYS_ADMIN last.
+ *
+ * This sets PF_SUPERPRIV on the task if the capability is available on the
+ * assumption that it's about to be used.
+ */
+bool ns_capable_any(struct user_namespace *ns, int cap1, int cap2)
+{
+ if (ns_capable_nodenyaudit(ns, cap1))
+ return true;
+
+ if (ns_capable_nodenyaudit(ns, cap2))
+ return true;
+
+ return ns_capable(ns, cap1);
+}
+EXPORT_SYMBOL(ns_capable_any);
+
+/**
+ * capable_any - Determine if the current task has one of two superior capabilities in effect
+ * @cap1: The capabilities to be tested for first
+ * @cap2: The capabilities to be tested for secondly
+ *
+ * Return true if the current task has at least one of the two given superior
+ * capabilities currently available for use, false if not.
+ *
+ * In contrast to or'ing capable() this call will create exactly one audit
+ * message, either for @cap1, if it is granted or both are not permitted,
+ * or @cap2, if it is granted while the other one is not.
+ *
+ * The capabilities should be ordered from least to most invasive, i.e. CAP_SYS_ADMIN last.
+ *
+ * This sets PF_SUPERPRIV on the task if the capability is available on the
+ * assumption that it's about to be used.
+ */
+bool capable_any(int cap1, int cap2)
+{
+ return ns_capable_any(&init_user_ns, cap1, cap2);
+}
+EXPORT_SYMBOL(capable_any);
+
/**
* capable - Determine if the current task has a superior capability in effect
* @cap: The capability to be tested for
--
2.40.1
Use the new added capable_any function in appropriate cases, where a
task is required to have any of two capabilities.
Signed-off-by: Christian Göttsche <[email protected]>
---
v3:
- rename to capable_any()
- simplify checkpoint_restore_ns_capable()
---
include/linux/capability.h | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/include/linux/capability.h b/include/linux/capability.h
index eeb958440656..4db0ffb47271 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -204,18 +204,17 @@ extern bool file_ns_capable(const struct file *file, struct user_namespace *ns,
extern bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns);
static inline bool perfmon_capable(void)
{
- return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
+ return capable_any(CAP_PERFMON, CAP_SYS_ADMIN);
}
static inline bool bpf_capable(void)
{
- return capable(CAP_BPF) || capable(CAP_SYS_ADMIN);
+ return capable_any(CAP_BPF, CAP_SYS_ADMIN);
}
static inline bool checkpoint_restore_ns_capable(struct user_namespace *ns)
{
- return ns_capable(ns, CAP_CHECKPOINT_RESTORE) ||
- ns_capable(ns, CAP_SYS_ADMIN);
+ return ns_capable_any(ns, CAP_CHECKPOINT_RESTORE, CAP_SYS_ADMIN);
}
/* audit system wants to get cap info from files as well */
--
2.40.1
Use the new added capable_any function in appropriate cases, where a
task is required to have any of two capabilities.
Reorder CAP_SYS_ADMIN last.
Signed-off-by: Christian Göttsche <[email protected]>
---
v4:
Additional usage in kfd_ioctl()
v3:
rename to capable_any()
---
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 3 +--
drivers/net/caif/caif_serial.c | 2 +-
drivers/s390/block/dasd_eckd.c | 2 +-
3 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 1b54a9aaae70..d21fb9d1556b 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -2896,8 +2896,7 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
* more priviledged access.
*/
if (unlikely(ioctl->flags & KFD_IOC_FLAG_CHECKPOINT_RESTORE)) {
- if (!capable(CAP_CHECKPOINT_RESTORE) &&
- !capable(CAP_SYS_ADMIN)) {
+ if (!capable_any(CAP_CHECKPOINT_RESTORE, CAP_SYS_ADMIN)) {
retcode = -EACCES;
goto err_i1;
}
diff --git a/drivers/net/caif/caif_serial.c b/drivers/net/caif/caif_serial.c
index 688075859ae4..ca3f82a0e3a6 100644
--- a/drivers/net/caif/caif_serial.c
+++ b/drivers/net/caif/caif_serial.c
@@ -326,7 +326,7 @@ static int ldisc_open(struct tty_struct *tty)
/* No write no play */
if (tty->ops->write == NULL)
return -EOPNOTSUPP;
- if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_TTY_CONFIG))
+ if (!capable_any(CAP_SYS_TTY_CONFIG, CAP_SYS_ADMIN))
return -EPERM;
/* release devices to avoid name collision */
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index ade1369fe5ed..67d1058bce1b 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -5370,7 +5370,7 @@ static int dasd_symm_io(struct dasd_device *device, void __user *argp)
char psf0, psf1;
int rc;
- if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_RAWIO))
+ if (!capable_any(CAP_SYS_RAWIO, CAP_SYS_ADMIN))
return -EACCES;
psf0 = psf1 = 0;
--
2.40.1
Use the new added capable_any function in appropriate cases, where a
task is required to have any of two capabilities.
Reorder CAP_SYS_ADMIN last.
Fixes: 94c4b4fd25e6 ("block: Check ADMIN before NICE for IOPRIO_CLASS_RT")
Signed-off-by: Christian Göttsche <[email protected]>
---
v3:
rename to capable_any()
---
block/ioprio.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/block/ioprio.c b/block/ioprio.c
index 32a456b45804..0a7df88bf6d9 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -37,14 +37,7 @@ int ioprio_check_cap(int ioprio)
switch (class) {
case IOPRIO_CLASS_RT:
- /*
- * Originally this only checked for CAP_SYS_ADMIN,
- * which was implicitly allowed for pid 0 by security
- * modules such as SELinux. Make sure we check
- * CAP_SYS_ADMIN first to avoid a denial/avc for
- * possibly missing CAP_SYS_NICE permission.
- */
- if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_NICE))
+ if (!capable_any(CAP_SYS_NICE, CAP_SYS_ADMIN))
return -EPERM;
fallthrough;
/* rt has prio field too */
--
2.40.1
Use the new added capable_any function in appropriate cases, where a
task is required to have any of two capabilities.
Signed-off-by: Christian Göttsche <[email protected]>
---
v3:
rename to capable_any()
---
kernel/fork.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index ed4e01daccaa..6e00933e8ef4 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2371,7 +2371,7 @@ __latent_entropy struct task_struct *copy_process(
retval = -EAGAIN;
if (is_rlimit_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
if (p->real_cred->user != INIT_USER &&
- !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
+ !capable_any(CAP_SYS_RESOURCE, CAP_SYS_ADMIN))
goto bad_fork_cleanup_count;
}
current->flags &= ~PF_NPROC_EXCEEDED;
--
2.40.1
Use the new added capable_any function in appropriate cases, where a
task is required to have any of two capabilities.
Add sock_ns_capable_any() wrapper similar to existing sock_ns_capable()
one.
Reorder CAP_SYS_ADMIN last.
Signed-off-by: Christian Göttsche <[email protected]>
---
v4:
- introduce sockopt_ns_capable_any()
v3:
- rename to capable_any()
- make use of ns_capable_any
Signed-off-by: Christian Göttsche <[email protected]>
---
include/net/sock.h | 1 +
net/caif/caif_socket.c | 2 +-
net/core/sock.c | 18 ++++++++++--------
net/ieee802154/socket.c | 6 ++----
net/ipv4/ip_sockglue.c | 4 ++--
net/ipv6/ipv6_sockglue.c | 3 +--
net/unix/scm.c | 2 +-
7 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index 8b7ed7167243..a17178e31e91 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1762,6 +1762,7 @@ static inline void unlock_sock_fast(struct sock *sk, bool slow)
void sockopt_lock_sock(struct sock *sk);
void sockopt_release_sock(struct sock *sk);
bool sockopt_ns_capable(struct user_namespace *ns, int cap);
+bool sockopt_ns_capable_any(struct user_namespace *ns, int cap1, int cap2);
bool sockopt_capable(int cap);
/* Used by processes to "lock" a socket state, so that
diff --git a/net/caif/caif_socket.c b/net/caif/caif_socket.c
index 4eebcc66c19a..6dcc08f9da3b 100644
--- a/net/caif/caif_socket.c
+++ b/net/caif/caif_socket.c
@@ -1027,7 +1027,7 @@ static int caif_create(struct net *net, struct socket *sock, int protocol,
.usersize = sizeof_field(struct caifsock, conn_req.param)
};
- if (!capable(CAP_SYS_ADMIN) && !capable(CAP_NET_ADMIN))
+ if (!capable_any(CAP_NET_ADMIN, CAP_SYS_ADMIN))
return -EPERM;
/*
* The sock->type specifies the socket type to use.
diff --git a/net/core/sock.c b/net/core/sock.c
index 5440e67bcfe3..6a236d649bec 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1073,6 +1073,12 @@ bool sockopt_ns_capable(struct user_namespace *ns, int cap)
}
EXPORT_SYMBOL(sockopt_ns_capable);
+bool sockopt_ns_capable_any(struct user_namespace *ns, int cap1, int cap2)
+{
+ return has_current_bpf_ctx() || ns_capable_any(ns, cap1, cap2);
+}
+EXPORT_SYMBOL(sockopt_ns_capable_any);
+
bool sockopt_capable(int cap)
{
return has_current_bpf_ctx() || capable(cap);
@@ -1207,8 +1213,7 @@ int sk_setsockopt(struct sock *sk, int level, int optname,
case SO_PRIORITY:
if ((val >= 0 && val <= 6) ||
- sockopt_ns_capable(sock_net(sk)->user_ns, CAP_NET_RAW) ||
- sockopt_ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN))
+ sockopt_ns_capable_any(sock_net(sk)->user_ns, CAP_NET_RAW, CAP_NET_ADMIN))
sk->sk_priority = val;
else
ret = -EPERM;
@@ -1353,8 +1358,7 @@ int sk_setsockopt(struct sock *sk, int level, int optname,
clear_bit(SOCK_PASSSEC, &sock->flags);
break;
case SO_MARK:
- if (!sockopt_ns_capable(sock_net(sk)->user_ns, CAP_NET_RAW) &&
- !sockopt_ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN)) {
+ if (!sockopt_ns_capable_any(sock_net(sk)->user_ns, CAP_NET_RAW, CAP_NET_ADMIN)) {
ret = -EPERM;
break;
}
@@ -1362,8 +1366,7 @@ int sk_setsockopt(struct sock *sk, int level, int optname,
__sock_set_mark(sk, val);
break;
case SO_RCVMARK:
- if (!sockopt_ns_capable(sock_net(sk)->user_ns, CAP_NET_RAW) &&
- !sockopt_ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN)) {
+ if (!sockopt_ns_capable_any(sock_net(sk)->user_ns, CAP_NET_RAW, CAP_NET_ADMIN)) {
ret = -EPERM;
break;
}
@@ -2747,8 +2750,7 @@ int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
switch (cmsg->cmsg_type) {
case SO_MARK:
- if (!ns_capable(sock_net(sk)->user_ns, CAP_NET_RAW) &&
- !ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN))
+ if (!ns_capable_any(sock_net(sk)->user_ns, CAP_NET_RAW, CAP_NET_ADMIN))
return -EPERM;
if (cmsg->cmsg_len != CMSG_LEN(sizeof(u32)))
return -EINVAL;
diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
index 1fa2fe041ec0..f9bc6cae4af9 100644
--- a/net/ieee802154/socket.c
+++ b/net/ieee802154/socket.c
@@ -904,8 +904,7 @@ static int dgram_setsockopt(struct sock *sk, int level, int optname,
ro->want_lqi = !!val;
break;
case WPAN_SECURITY:
- if (!ns_capable(net->user_ns, CAP_NET_ADMIN) &&
- !ns_capable(net->user_ns, CAP_NET_RAW)) {
+ if (!ns_capable_any(net->user_ns, CAP_NET_ADMIN, CAP_NET_RAW)) {
err = -EPERM;
break;
}
@@ -928,8 +927,7 @@ static int dgram_setsockopt(struct sock *sk, int level, int optname,
}
break;
case WPAN_SECURITY_LEVEL:
- if (!ns_capable(net->user_ns, CAP_NET_ADMIN) &&
- !ns_capable(net->user_ns, CAP_NET_RAW)) {
+ if (!ns_capable_any(net->user_ns, CAP_NET_ADMIN, CAP_NET_RAW)) {
err = -EPERM;
break;
}
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index b511ff0adc0a..4dd752743b84 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -1341,8 +1341,8 @@ int do_ip_setsockopt(struct sock *sk, int level, int optname,
break;
case IP_TRANSPARENT:
- if (!!val && !sockopt_ns_capable(sock_net(sk)->user_ns, CAP_NET_RAW) &&
- !sockopt_ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN)) {
+ if (!!val && !sockopt_ns_capable_any(sock_net(sk)->user_ns, CAP_NET_RAW,
+ CAP_NET_ADMIN)) {
err = -EPERM;
break;
}
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index ae818ff46224..38aad44547e4 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -625,8 +625,7 @@ int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
break;
case IPV6_TRANSPARENT:
- if (valbool && !sockopt_ns_capable(net->user_ns, CAP_NET_RAW) &&
- !sockopt_ns_capable(net->user_ns, CAP_NET_ADMIN)) {
+ if (valbool && !sockopt_ns_capable_any(net->user_ns, CAP_NET_RAW, CAP_NET_ADMIN)) {
retv = -EPERM;
break;
}
diff --git a/net/unix/scm.c b/net/unix/scm.c
index f9152881d77f..4d18187a5349 100644
--- a/net/unix/scm.c
+++ b/net/unix/scm.c
@@ -99,7 +99,7 @@ static inline bool too_many_unix_fds(struct task_struct *p)
struct user_struct *user = current_user();
if (unlikely(user->unix_inflight > task_rlimit(p, RLIMIT_NOFILE)))
- return !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN);
+ return !capable_any(CAP_SYS_RESOURCE, CAP_SYS_ADMIN);
return false;
}
--
2.40.1
On Thu, May 11, 2023 at 04:25:27PM +0200, Christian G?ttsche wrote:
> Use the new added capable_any function in appropriate cases, where a
> task is required to have any of two capabilities.
What is this new function and why should we using it?
Your also forgot to Cc the block list on the entire series, making this
page completely unreviewable.
On Thu, 11 May 2023 at 17:35, Christoph Hellwig <[email protected]> wrote:
>
> On Thu, May 11, 2023 at 04:25:27PM +0200, Christian Göttsche wrote:
> > Use the new added capable_any function in appropriate cases, where a
> > task is required to have any of two capabilities.
>
> What is this new function and why should we using it?
Quoting the description from
https://lore.kernel.org/all/[email protected]/
:
Add the interfaces `capable_any()` and `ns_capable_any()` as an
alternative to multiple `capable()`/`ns_capable()` calls, like
`capable_any(CAP_SYS_NICE, CAP_SYS_ADMIN)` instead of
`capable(CAP_SYS_NICE) || capable(CAP_SYS_ADMIN)`.
`capable_any()`/`ns_capable_any()` will in particular generate exactly
one audit message, either for the left most capability in effect or, if
the task has none, the first one.
This is especially helpful with regard to SELinux, where each audit
message about a not allowed capability request will create a denial
message. Using this new wrapper with the least invasive capability as
left most argument (e.g. CAP_SYS_NICE before CAP_SYS_ADMIN) enables
policy writers to only grant the least invasive one for the particular
subject instead of both.
> Your also forgot to Cc the block list on the entire series, making this
> page completely unreviewable.
On Thu, May 11, 2023 at 04:25:30PM +0200, Christian Göttsche wrote:
> Use the new added capable_any function in appropriate cases, where a
> task is required to have any of two capabilities.
>
> Signed-off-by: Christian Göttsche <[email protected]>
> ---
Looks good to me,
Reviewed-by: Christian Brauner <[email protected]>
On Thu, May 11, 2023 at 04:25:29PM +0200, Christian Göttsche wrote:
> Use the new added capable_any function in appropriate cases, where a
> task is required to have any of two capabilities.
>
> Signed-off-by: Christian Göttsche <[email protected]>
> ---
Acked-by: Christian Brauner <[email protected]>
On Thu, May 11, 2023 at 04:25:28PM +0200, Christian G?ttsche wrote:
> Use the new added capable_any function in appropriate cases, where a
> task is required to have any of two capabilities.
>
> Reorder CAP_SYS_ADMIN last.
>
> Signed-off-by: Christian G?ttsche <[email protected]>
> ---
> v4:
> Additional usage in kfd_ioctl()
> v3:
> rename to capable_any()
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 3 +--
> drivers/net/caif/caif_serial.c | 2 +-
> drivers/s390/block/dasd_eckd.c | 2 +-
> 3 files changed, 3 insertions(+), 4 deletions(-)
...
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index ade1369fe5ed..67d1058bce1b 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -5370,7 +5370,7 @@ static int dasd_symm_io(struct dasd_device *device, void __user *argp)
> char psf0, psf1;
> int rc;
>
> - if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_RAWIO))
> + if (!capable_any(CAP_SYS_RAWIO, CAP_SYS_ADMIN))
> return -EACCES;
> psf0 = psf1 = 0;
For s390 part:
Acked-by: Alexander Gordeev <[email protected]>
On Thu, May 11, 2023 at 7:26 AM Christian Göttsche
<[email protected]> wrote:
>
> Use the new added capable_any function in appropriate cases, where a
> task is required to have any of two capabilities.
>
> Signed-off-by: Christian Göttsche <[email protected]>
> ---
> v3:
> rename to capable_any()
> ---
> kernel/bpf/syscall.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Acked-by: Andrii Nakryiko <[email protected]>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 14f39c1e573e..1bd50da05a22 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2539,7 +2539,7 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
> !bpf_capable())
> return -EPERM;
>
> - if (is_net_admin_prog_type(type) && !capable(CAP_NET_ADMIN) && !capable(CAP_SYS_ADMIN))
> + if (is_net_admin_prog_type(type) && !capable_any(CAP_NET_ADMIN, CAP_SYS_ADMIN))
> return -EPERM;
> if (is_perfmon_prog_type(type) && !perfmon_capable())
> return -EPERM;
> --
> 2.40.1
>
On Thu, May 11, 2023 at 7:27 AM Christian Göttsche
<[email protected]> wrote:
>
> Use the new added capable_any function in appropriate cases, where a
> task is required to have any of two capabilities.
>
> Signed-off-by: Christian Göttsche <[email protected]>
> ---
> v3:
> - rename to capable_any()
> - simplify checkpoint_restore_ns_capable()
> ---
> include/linux/capability.h | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index eeb958440656..4db0ffb47271 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -204,18 +204,17 @@ extern bool file_ns_capable(const struct file *file, struct user_namespace *ns,
> extern bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns);
> static inline bool perfmon_capable(void)
> {
> - return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
> + return capable_any(CAP_PERFMON, CAP_SYS_ADMIN);
> }
>
> static inline bool bpf_capable(void)
> {
> - return capable(CAP_BPF) || capable(CAP_SYS_ADMIN);
> + return capable_any(CAP_BPF, CAP_SYS_ADMIN);
> }
>
For bpf parts:
Acked-by: Andrii Nakryiko <[email protected]>
> static inline bool checkpoint_restore_ns_capable(struct user_namespace *ns)
> {
> - return ns_capable(ns, CAP_CHECKPOINT_RESTORE) ||
> - ns_capable(ns, CAP_SYS_ADMIN);
> + return ns_capable_any(ns, CAP_CHECKPOINT_RESTORE, CAP_SYS_ADMIN);
> }
>
> /* audit system wants to get cap info from files as well */
> --
> 2.40.1
>
>
Hi Christian,
[email protected] wrote on Thu, 11 May 2023 16:25:32 +0200:
> Use the new added capable_any function in appropriate cases, where a
> task is required to have any of two capabilities.
>
> Add sock_ns_capable_any() wrapper similar to existing sock_ns_capable()
> one.
>
> Reorder CAP_SYS_ADMIN last.
>
> Signed-off-by: Christian Göttsche <[email protected]>
> ---
> v4:
> - introduce sockopt_ns_capable_any()
> v3:
> - rename to capable_any()
> - make use of ns_capable_any
> Signed-off-by: Christian Göttsche <[email protected]>
> ---
> include/net/sock.h | 1 +
> net/caif/caif_socket.c | 2 +-
> net/core/sock.c | 18 ++++++++++--------
> net/ieee802154/socket.c | 6 ++----
> net/ipv4/ip_sockglue.c | 4 ++--
> net/ipv6/ipv6_sockglue.c | 3 +--
> net/unix/scm.c | 2 +-
> 7 files changed, 18 insertions(+), 18 deletions(-)
>
[...]
> diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
> index 1fa2fe041ec0..f9bc6cae4af9 100644
> --- a/net/ieee802154/socket.c
> +++ b/net/ieee802154/socket.c
> @@ -904,8 +904,7 @@ static int dgram_setsockopt(struct sock *sk, int level, int optname,
> ro->want_lqi = !!val;
> break;
> case WPAN_SECURITY:
> - if (!ns_capable(net->user_ns, CAP_NET_ADMIN) &&
> - !ns_capable(net->user_ns, CAP_NET_RAW)) {
> + if (!ns_capable_any(net->user_ns, CAP_NET_ADMIN, CAP_NET_RAW)) {
> err = -EPERM;
> break;
> }
> @@ -928,8 +927,7 @@ static int dgram_setsockopt(struct sock *sk, int level, int optname,
> }
> break;
> case WPAN_SECURITY_LEVEL:
> - if (!ns_capable(net->user_ns, CAP_NET_ADMIN) &&
> - !ns_capable(net->user_ns, CAP_NET_RAW)) {
> + if (!ns_capable_any(net->user_ns, CAP_NET_ADMIN, CAP_NET_RAW)) {
> err = -EPERM;
> break;
> }
I was not noticed this was applied already, so, for ieee802154:
Reviewed-by: Miquel Raynal <[email protected]>
Thanks,
Miquèl
On Thu, May 11, 2023 at 04:25:24PM +0200, Christian G?ttsche wrote:
> Introduce a new capable flag, CAP_OPT_NODENYAUDIT, to not generate
> an audit event if the requested capability is not granted. This will be
> used in a new capable_any() functionality to reduce the number of
> necessary capable calls.
>
> Handle the flag accordingly in AppArmor and SELinux.
>
> Suggested-by: Paul Moore <[email protected]>
> Signed-off-by: Christian G?ttsche <[email protected]>
Reviewed-by: Serge Hallyn <[email protected]>
> ---
> include/linux/security.h | 2 ++
> security/apparmor/capability.c | 8 +++++---
> security/selinux/hooks.c | 14 ++++++++------
> 3 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index e2734e9e44d5..629c775ec297 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -67,6 +67,8 @@ struct watch_notification;
> #define CAP_OPT_NOAUDIT BIT(1)
> /* If capable is being called by a setid function */
> #define CAP_OPT_INSETID BIT(2)
> +/* If capable should audit the security request for authorized requests only */
> +#define CAP_OPT_NODENYAUDIT BIT(3)
>
> /* LSM Agnostic defines for security_sb_set_mnt_opts() flags */
> #define SECURITY_LSM_NATIVE_LABELS 1
> diff --git a/security/apparmor/capability.c b/security/apparmor/capability.c
> index 326a51838ef2..98120dd62ca7 100644
> --- a/security/apparmor/capability.c
> +++ b/security/apparmor/capability.c
> @@ -108,7 +108,8 @@ static int audit_caps(struct common_audit_data *sa, struct aa_profile *profile,
> * profile_capable - test if profile allows use of capability @cap
> * @profile: profile being enforced (NOT NULL, NOT unconfined)
> * @cap: capability to test if allowed
> - * @opts: CAP_OPT_NOAUDIT bit determines whether audit record is generated
> + * @opts: CAP_OPT_NOAUDIT/CAP_OPT_NODENYAUDIT bit determines whether audit
> + * record is generated
> * @sa: audit data (MAY BE NULL indicating no auditing)
> *
> * Returns: 0 if allowed else -EPERM
> @@ -126,7 +127,7 @@ static int profile_capable(struct aa_profile *profile, int cap,
> else
> error = -EPERM;
>
> - if (opts & CAP_OPT_NOAUDIT) {
> + if ((opts & CAP_OPT_NOAUDIT) || ((opts & CAP_OPT_NODENYAUDIT) && error)) {
> if (!COMPLAIN_MODE(profile))
> return error;
> /* audit the cap request in complain mode but note that it
> @@ -142,7 +143,8 @@ static int profile_capable(struct aa_profile *profile, int cap,
> * aa_capable - test permission to use capability
> * @label: label being tested for capability (NOT NULL)
> * @cap: capability to be tested
> - * @opts: CAP_OPT_NOAUDIT bit determines whether audit record is generated
> + * @opts: CAP_OPT_NOAUDIT/CAP_OPT_NODENYAUDIT bit determines whether audit
> + * record is generated
> *
> * Look up capability in profile capability set.
> *
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 79b4890e9936..0730edf2f5f1 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1571,7 +1571,7 @@ static int cred_has_capability(const struct cred *cred,
> u16 sclass;
> u32 sid = cred_sid(cred);
> u32 av = CAP_TO_MASK(cap);
> - int rc;
> + int rc, rc2;
>
> ad.type = LSM_AUDIT_DATA_CAP;
> ad.u.cap = cap;
> @@ -1590,11 +1590,13 @@ static int cred_has_capability(const struct cred *cred,
> }
>
> rc = avc_has_perm_noaudit(sid, sid, sclass, av, 0, &avd);
> - if (!(opts & CAP_OPT_NOAUDIT)) {
> - int rc2 = avc_audit(sid, sid, sclass, av, &avd, rc, &ad);
> - if (rc2)
> - return rc2;
> - }
> + if ((opts & CAP_OPT_NOAUDIT) || ((opts & CAP_OPT_NODENYAUDIT) && rc))
> + return rc;
Hm, if the caller passes only CAP_OPT_NODENYAUDIT, and rc == 0, then
you will audit the allow. Is that what you want, or did you want, or
did you want CAP_OPT_NODENYAUDIT to imply CAP_OPT_NOAUDIT?
> +
> + rc2 = avc_audit(sid, sid, sclass, av, &avd, rc, &ad);
> + if (rc2)
> + return rc2;
> +
> return rc;
> }
>
> --
> 2.40.1
On Wed, May 31, 2023 at 09:07:34AM -0500, Serge E. Hallyn wrote:
> On Thu, May 11, 2023 at 04:25:24PM +0200, Christian G?ttsche wrote:
> > Introduce a new capable flag, CAP_OPT_NODENYAUDIT, to not generate
> > an audit event if the requested capability is not granted. This will be
> > used in a new capable_any() functionality to reduce the number of
> > necessary capable calls.
> >
> > Handle the flag accordingly in AppArmor and SELinux.
> >
> > Suggested-by: Paul Moore <[email protected]>
> > Signed-off-by: Christian G?ttsche <[email protected]>
>
> Reviewed-by: Serge Hallyn <[email protected]>
Sorry, obviously I should have removed this, until the comment below was
answered :)
> > ---
> > include/linux/security.h | 2 ++
> > security/apparmor/capability.c | 8 +++++---
> > security/selinux/hooks.c | 14 ++++++++------
> > 3 files changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index e2734e9e44d5..629c775ec297 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -67,6 +67,8 @@ struct watch_notification;
> > #define CAP_OPT_NOAUDIT BIT(1)
> > /* If capable is being called by a setid function */
> > #define CAP_OPT_INSETID BIT(2)
> > +/* If capable should audit the security request for authorized requests only */
> > +#define CAP_OPT_NODENYAUDIT BIT(3)
> >
> > /* LSM Agnostic defines for security_sb_set_mnt_opts() flags */
> > #define SECURITY_LSM_NATIVE_LABELS 1
> > diff --git a/security/apparmor/capability.c b/security/apparmor/capability.c
> > index 326a51838ef2..98120dd62ca7 100644
> > --- a/security/apparmor/capability.c
> > +++ b/security/apparmor/capability.c
> > @@ -108,7 +108,8 @@ static int audit_caps(struct common_audit_data *sa, struct aa_profile *profile,
> > * profile_capable - test if profile allows use of capability @cap
> > * @profile: profile being enforced (NOT NULL, NOT unconfined)
> > * @cap: capability to test if allowed
> > - * @opts: CAP_OPT_NOAUDIT bit determines whether audit record is generated
> > + * @opts: CAP_OPT_NOAUDIT/CAP_OPT_NODENYAUDIT bit determines whether audit
> > + * record is generated
> > * @sa: audit data (MAY BE NULL indicating no auditing)
> > *
> > * Returns: 0 if allowed else -EPERM
> > @@ -126,7 +127,7 @@ static int profile_capable(struct aa_profile *profile, int cap,
> > else
> > error = -EPERM;
> >
> > - if (opts & CAP_OPT_NOAUDIT) {
> > + if ((opts & CAP_OPT_NOAUDIT) || ((opts & CAP_OPT_NODENYAUDIT) && error)) {
> > if (!COMPLAIN_MODE(profile))
> > return error;
> > /* audit the cap request in complain mode but note that it
> > @@ -142,7 +143,8 @@ static int profile_capable(struct aa_profile *profile, int cap,
> > * aa_capable - test permission to use capability
> > * @label: label being tested for capability (NOT NULL)
> > * @cap: capability to be tested
> > - * @opts: CAP_OPT_NOAUDIT bit determines whether audit record is generated
> > + * @opts: CAP_OPT_NOAUDIT/CAP_OPT_NODENYAUDIT bit determines whether audit
> > + * record is generated
> > *
> > * Look up capability in profile capability set.
> > *
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 79b4890e9936..0730edf2f5f1 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -1571,7 +1571,7 @@ static int cred_has_capability(const struct cred *cred,
> > u16 sclass;
> > u32 sid = cred_sid(cred);
> > u32 av = CAP_TO_MASK(cap);
> > - int rc;
> > + int rc, rc2;
> >
> > ad.type = LSM_AUDIT_DATA_CAP;
> > ad.u.cap = cap;
> > @@ -1590,11 +1590,13 @@ static int cred_has_capability(const struct cred *cred,
> > }
> >
> > rc = avc_has_perm_noaudit(sid, sid, sclass, av, 0, &avd);
> > - if (!(opts & CAP_OPT_NOAUDIT)) {
> > - int rc2 = avc_audit(sid, sid, sclass, av, &avd, rc, &ad);
> > - if (rc2)
> > - return rc2;
> > - }
> > + if ((opts & CAP_OPT_NOAUDIT) || ((opts & CAP_OPT_NODENYAUDIT) && rc))
> > + return rc;
>
> Hm, if the caller passes only CAP_OPT_NODENYAUDIT, and rc == 0, then
> you will audit the allow. Is that what you want, or did you want, or
> did you want CAP_OPT_NODENYAUDIT to imply CAP_OPT_NOAUDIT?
>
> > +
> > + rc2 = avc_audit(sid, sid, sclass, av, &avd, rc, &ad);
> > + if (rc2)
> > + return rc2;
> > +
> > return rc;
> > }
> >
> > --
> > 2.40.1
On Wed, 31 May 2023 at 16:08, Serge E. Hallyn <[email protected]> wrote:
>
> On Wed, May 31, 2023 at 09:07:34AM -0500, Serge E. Hallyn wrote:
> > On Thu, May 11, 2023 at 04:25:24PM +0200, Christian Göttsche wrote:
> > > Introduce a new capable flag, CAP_OPT_NODENYAUDIT, to not generate
> > > an audit event if the requested capability is not granted. This will be
> > > used in a new capable_any() functionality to reduce the number of
> > > necessary capable calls.
> > >
> > > Handle the flag accordingly in AppArmor and SELinux.
> > >
> > > Suggested-by: Paul Moore <[email protected]>
> > > Signed-off-by: Christian Göttsche <[email protected]>
> >
> > Reviewed-by: Serge Hallyn <[email protected]>
>
> Sorry, obviously I should have removed this, until the comment below was
> answered :)
>
> > > ---
> > > include/linux/security.h | 2 ++
> > > security/apparmor/capability.c | 8 +++++---
> > > security/selinux/hooks.c | 14 ++++++++------
> > > 3 files changed, 15 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/include/linux/security.h b/include/linux/security.h
> > > index e2734e9e44d5..629c775ec297 100644
> > > --- a/include/linux/security.h
> > > +++ b/include/linux/security.h
> > > @@ -67,6 +67,8 @@ struct watch_notification;
> > > #define CAP_OPT_NOAUDIT BIT(1)
> > > /* If capable is being called by a setid function */
> > > #define CAP_OPT_INSETID BIT(2)
> > > +/* If capable should audit the security request for authorized requests only */
> > > +#define CAP_OPT_NODENYAUDIT BIT(3)
> > >
> > > /* LSM Agnostic defines for security_sb_set_mnt_opts() flags */
> > > #define SECURITY_LSM_NATIVE_LABELS 1
> > > diff --git a/security/apparmor/capability.c b/security/apparmor/capability.c
> > > index 326a51838ef2..98120dd62ca7 100644
> > > --- a/security/apparmor/capability.c
> > > +++ b/security/apparmor/capability.c
> > > @@ -108,7 +108,8 @@ static int audit_caps(struct common_audit_data *sa, struct aa_profile *profile,
> > > * profile_capable - test if profile allows use of capability @cap
> > > * @profile: profile being enforced (NOT NULL, NOT unconfined)
> > > * @cap: capability to test if allowed
> > > - * @opts: CAP_OPT_NOAUDIT bit determines whether audit record is generated
> > > + * @opts: CAP_OPT_NOAUDIT/CAP_OPT_NODENYAUDIT bit determines whether audit
> > > + * record is generated
> > > * @sa: audit data (MAY BE NULL indicating no auditing)
> > > *
> > > * Returns: 0 if allowed else -EPERM
> > > @@ -126,7 +127,7 @@ static int profile_capable(struct aa_profile *profile, int cap,
> > > else
> > > error = -EPERM;
> > >
> > > - if (opts & CAP_OPT_NOAUDIT) {
> > > + if ((opts & CAP_OPT_NOAUDIT) || ((opts & CAP_OPT_NODENYAUDIT) && error)) {
> > > if (!COMPLAIN_MODE(profile))
> > > return error;
> > > /* audit the cap request in complain mode but note that it
> > > @@ -142,7 +143,8 @@ static int profile_capable(struct aa_profile *profile, int cap,
> > > * aa_capable - test permission to use capability
> > > * @label: label being tested for capability (NOT NULL)
> > > * @cap: capability to be tested
> > > - * @opts: CAP_OPT_NOAUDIT bit determines whether audit record is generated
> > > + * @opts: CAP_OPT_NOAUDIT/CAP_OPT_NODENYAUDIT bit determines whether audit
> > > + * record is generated
> > > *
> > > * Look up capability in profile capability set.
> > > *
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 79b4890e9936..0730edf2f5f1 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -1571,7 +1571,7 @@ static int cred_has_capability(const struct cred *cred,
> > > u16 sclass;
> > > u32 sid = cred_sid(cred);
> > > u32 av = CAP_TO_MASK(cap);
> > > - int rc;
> > > + int rc, rc2;
> > >
> > > ad.type = LSM_AUDIT_DATA_CAP;
> > > ad.u.cap = cap;
> > > @@ -1590,11 +1590,13 @@ static int cred_has_capability(const struct cred *cred,
> > > }
> > >
> > > rc = avc_has_perm_noaudit(sid, sid, sclass, av, 0, &avd);
> > > - if (!(opts & CAP_OPT_NOAUDIT)) {
> > > - int rc2 = avc_audit(sid, sid, sclass, av, &avd, rc, &ad);
> > > - if (rc2)
> > > - return rc2;
> > > - }
> > > + if ((opts & CAP_OPT_NOAUDIT) || ((opts & CAP_OPT_NODENYAUDIT) && rc))
> > > + return rc;
> >
> > Hm, if the caller passes only CAP_OPT_NODENYAUDIT, and rc == 0, then
> > you will audit the allow. Is that what you want, or did you want, or
> > did you want CAP_OPT_NODENYAUDIT to imply CAP_OPT_NOAUDIT?
> >
The new option should cause to issue an audit event if (and only if)
the requested capability is in effect for the current task. If the
task does not have the capability no audit event should be issued.
The new option should not imply CAP_OPT_NOAUDIT since we want an audit
event in the case the capability is in effect.
I admit the naming is a bit confusing as CAP_OPT_NODENYAUDIT as well
as the commit description contains a double negation (while the inline
comment for the macro definition does not).
Do you prefer naming the constant CAP_OPT_ALLOWAUDIT or CAP_OPT_AUDIT_ON_ALLOW?
> > > +
> > > + rc2 = avc_audit(sid, sid, sclass, av, &avd, rc, &ad);
> > > + if (rc2)
> > > + return rc2;
> > > +
> > > return rc;
> > > }
> > >
> > > --
> > > 2.40.1
On Wed, May 31, 2023 at 2:34 PM Christian Göttsche
<[email protected]> wrote:
> On Wed, 31 May 2023 at 16:08, Serge E. Hallyn <[email protected]> wrote:
> >
> > On Wed, May 31, 2023 at 09:07:34AM -0500, Serge E. Hallyn wrote:
> > > On Thu, May 11, 2023 at 04:25:24PM +0200, Christian Göttsche wrote:
> > > > Introduce a new capable flag, CAP_OPT_NODENYAUDIT, to not generate
> > > > an audit event if the requested capability is not granted. This will be
> > > > used in a new capable_any() functionality to reduce the number of
> > > > necessary capable calls.
> > > >
> > > > Handle the flag accordingly in AppArmor and SELinux.
> > > >
> > > > Suggested-by: Paul Moore <[email protected]>
> > > > Signed-off-by: Christian Göttsche <[email protected]>
> > >
> > > Reviewed-by: Serge Hallyn <[email protected]>
> >
> > Sorry, obviously I should have removed this, until the comment below was
> > answered :)
> >
> > > > ---
> > > > include/linux/security.h | 2 ++
> > > > security/apparmor/capability.c | 8 +++++---
> > > > security/selinux/hooks.c | 14 ++++++++------
> > > > 3 files changed, 15 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/include/linux/security.h b/include/linux/security.h
> > > > index e2734e9e44d5..629c775ec297 100644
> > > > --- a/include/linux/security.h
> > > > +++ b/include/linux/security.h
> > > > @@ -67,6 +67,8 @@ struct watch_notification;
> > > > #define CAP_OPT_NOAUDIT BIT(1)
> > > > /* If capable is being called by a setid function */
> > > > #define CAP_OPT_INSETID BIT(2)
> > > > +/* If capable should audit the security request for authorized requests only */
> > > > +#define CAP_OPT_NODENYAUDIT BIT(3)
> > > >
> > > > /* LSM Agnostic defines for security_sb_set_mnt_opts() flags */
> > > > #define SECURITY_LSM_NATIVE_LABELS 1
> > > > diff --git a/security/apparmor/capability.c b/security/apparmor/capability.c
> > > > index 326a51838ef2..98120dd62ca7 100644
> > > > --- a/security/apparmor/capability.c
> > > > +++ b/security/apparmor/capability.c
> > > > @@ -108,7 +108,8 @@ static int audit_caps(struct common_audit_data *sa, struct aa_profile *profile,
> > > > * profile_capable - test if profile allows use of capability @cap
> > > > * @profile: profile being enforced (NOT NULL, NOT unconfined)
> > > > * @cap: capability to test if allowed
> > > > - * @opts: CAP_OPT_NOAUDIT bit determines whether audit record is generated
> > > > + * @opts: CAP_OPT_NOAUDIT/CAP_OPT_NODENYAUDIT bit determines whether audit
> > > > + * record is generated
> > > > * @sa: audit data (MAY BE NULL indicating no auditing)
> > > > *
> > > > * Returns: 0 if allowed else -EPERM
> > > > @@ -126,7 +127,7 @@ static int profile_capable(struct aa_profile *profile, int cap,
> > > > else
> > > > error = -EPERM;
> > > >
> > > > - if (opts & CAP_OPT_NOAUDIT) {
> > > > + if ((opts & CAP_OPT_NOAUDIT) || ((opts & CAP_OPT_NODENYAUDIT) && error)) {
> > > > if (!COMPLAIN_MODE(profile))
> > > > return error;
> > > > /* audit the cap request in complain mode but note that it
> > > > @@ -142,7 +143,8 @@ static int profile_capable(struct aa_profile *profile, int cap,
> > > > * aa_capable - test permission to use capability
> > > > * @label: label being tested for capability (NOT NULL)
> > > > * @cap: capability to be tested
> > > > - * @opts: CAP_OPT_NOAUDIT bit determines whether audit record is generated
> > > > + * @opts: CAP_OPT_NOAUDIT/CAP_OPT_NODENYAUDIT bit determines whether audit
> > > > + * record is generated
> > > > *
> > > > * Look up capability in profile capability set.
> > > > *
> > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > index 79b4890e9936..0730edf2f5f1 100644
> > > > --- a/security/selinux/hooks.c
> > > > +++ b/security/selinux/hooks.c
> > > > @@ -1571,7 +1571,7 @@ static int cred_has_capability(const struct cred *cred,
> > > > u16 sclass;
> > > > u32 sid = cred_sid(cred);
> > > > u32 av = CAP_TO_MASK(cap);
> > > > - int rc;
> > > > + int rc, rc2;
> > > >
> > > > ad.type = LSM_AUDIT_DATA_CAP;
> > > > ad.u.cap = cap;
> > > > @@ -1590,11 +1590,13 @@ static int cred_has_capability(const struct cred *cred,
> > > > }
> > > >
> > > > rc = avc_has_perm_noaudit(sid, sid, sclass, av, 0, &avd);
> > > > - if (!(opts & CAP_OPT_NOAUDIT)) {
> > > > - int rc2 = avc_audit(sid, sid, sclass, av, &avd, rc, &ad);
> > > > - if (rc2)
> > > > - return rc2;
> > > > - }
> > > > + if ((opts & CAP_OPT_NOAUDIT) || ((opts & CAP_OPT_NODENYAUDIT) && rc))
> > > > + return rc;
> > >
> > > Hm, if the caller passes only CAP_OPT_NODENYAUDIT, and rc == 0, then
> > > you will audit the allow. Is that what you want, or did you want, or
> > > did you want CAP_OPT_NODENYAUDIT to imply CAP_OPT_NOAUDIT?
> > >
>
> The new option should cause to issue an audit event if (and only if)
> the requested capability is in effect for the current task. If the
> task does not have the capability no audit event should be issued.
>
> The new option should not imply CAP_OPT_NOAUDIT since we want an audit
> event in the case the capability is in effect.
>
> I admit the naming is a bit confusing as CAP_OPT_NODENYAUDIT as well
> as the commit description contains a double negation (while the inline
> comment for the macro definition does not).
>
> Do you prefer naming the constant CAP_OPT_ALLOWAUDIT or CAP_OPT_AUDIT_ON_ALLOW?
I think we need a different name, although I'm struggling to think of
something ... I don't think ALLOWAUDIT is right, as I believe it
implies that it is needed to "allow" auditing to take place for the
operation. AUDIT_ON_ALLOW is better, but it still seems like it would
be required if you wanted to generate audit records on a successful
operation, which isn't correct. I think we need to focus on the idea
that the flag blocks auditing for denials.
CAP_OPT_NOAUDITDENY is pretty much what you have, but in my mind the
NOAUDITDENY shares enough with the existing NOAUDIT flag that it makes
a bit more sense.
I honestly don't know. However, whatever you pick, make sure you
update patch 2/X so that the name of ns_capable_nodenyaudit() is kept
close to the flag's name.
--
paul-moore.com
On Wed, May 31, 2023 at 06:13:55PM -0400, Paul Moore wrote:
> On Wed, May 31, 2023 at 2:34 PM Christian Göttsche
> <[email protected]> wrote:
> > On Wed, 31 May 2023 at 16:08, Serge E. Hallyn <[email protected]> wrote:
> > >
> > > On Wed, May 31, 2023 at 09:07:34AM -0500, Serge E. Hallyn wrote:
> > > > On Thu, May 11, 2023 at 04:25:24PM +0200, Christian Göttsche wrote:
> > > > > Introduce a new capable flag, CAP_OPT_NODENYAUDIT, to not generate
> > > > > an audit event if the requested capability is not granted. This will be
> > > > > used in a new capable_any() functionality to reduce the number of
> > > > > necessary capable calls.
> > > > >
> > > > > Handle the flag accordingly in AppArmor and SELinux.
> > > > >
> > > > > Suggested-by: Paul Moore <[email protected]>
> > > > > Signed-off-by: Christian Göttsche <[email protected]>
> > > >
> > > > Reviewed-by: Serge Hallyn <[email protected]>
> > >
> > > Sorry, obviously I should have removed this, until the comment below was
> > > answered :)
> > >
> > > > > ---
> > > > > include/linux/security.h | 2 ++
> > > > > security/apparmor/capability.c | 8 +++++---
> > > > > security/selinux/hooks.c | 14 ++++++++------
> > > > > 3 files changed, 15 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/security.h b/include/linux/security.h
> > > > > index e2734e9e44d5..629c775ec297 100644
> > > > > --- a/include/linux/security.h
> > > > > +++ b/include/linux/security.h
> > > > > @@ -67,6 +67,8 @@ struct watch_notification;
> > > > > #define CAP_OPT_NOAUDIT BIT(1)
> > > > > /* If capable is being called by a setid function */
> > > > > #define CAP_OPT_INSETID BIT(2)
> > > > > +/* If capable should audit the security request for authorized requests only */
> > > > > +#define CAP_OPT_NODENYAUDIT BIT(3)
> > > > >
> > > > > /* LSM Agnostic defines for security_sb_set_mnt_opts() flags */
> > > > > #define SECURITY_LSM_NATIVE_LABELS 1
> > > > > diff --git a/security/apparmor/capability.c b/security/apparmor/capability.c
> > > > > index 326a51838ef2..98120dd62ca7 100644
> > > > > --- a/security/apparmor/capability.c
> > > > > +++ b/security/apparmor/capability.c
> > > > > @@ -108,7 +108,8 @@ static int audit_caps(struct common_audit_data *sa, struct aa_profile *profile,
> > > > > * profile_capable - test if profile allows use of capability @cap
> > > > > * @profile: profile being enforced (NOT NULL, NOT unconfined)
> > > > > * @cap: capability to test if allowed
> > > > > - * @opts: CAP_OPT_NOAUDIT bit determines whether audit record is generated
> > > > > + * @opts: CAP_OPT_NOAUDIT/CAP_OPT_NODENYAUDIT bit determines whether audit
> > > > > + * record is generated
> > > > > * @sa: audit data (MAY BE NULL indicating no auditing)
> > > > > *
> > > > > * Returns: 0 if allowed else -EPERM
> > > > > @@ -126,7 +127,7 @@ static int profile_capable(struct aa_profile *profile, int cap,
> > > > > else
> > > > > error = -EPERM;
> > > > >
> > > > > - if (opts & CAP_OPT_NOAUDIT) {
> > > > > + if ((opts & CAP_OPT_NOAUDIT) || ((opts & CAP_OPT_NODENYAUDIT) && error)) {
> > > > > if (!COMPLAIN_MODE(profile))
> > > > > return error;
> > > > > /* audit the cap request in complain mode but note that it
> > > > > @@ -142,7 +143,8 @@ static int profile_capable(struct aa_profile *profile, int cap,
> > > > > * aa_capable - test permission to use capability
> > > > > * @label: label being tested for capability (NOT NULL)
> > > > > * @cap: capability to be tested
> > > > > - * @opts: CAP_OPT_NOAUDIT bit determines whether audit record is generated
> > > > > + * @opts: CAP_OPT_NOAUDIT/CAP_OPT_NODENYAUDIT bit determines whether audit
> > > > > + * record is generated
> > > > > *
> > > > > * Look up capability in profile capability set.
> > > > > *
> > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > > index 79b4890e9936..0730edf2f5f1 100644
> > > > > --- a/security/selinux/hooks.c
> > > > > +++ b/security/selinux/hooks.c
> > > > > @@ -1571,7 +1571,7 @@ static int cred_has_capability(const struct cred *cred,
> > > > > u16 sclass;
> > > > > u32 sid = cred_sid(cred);
> > > > > u32 av = CAP_TO_MASK(cap);
> > > > > - int rc;
> > > > > + int rc, rc2;
> > > > >
> > > > > ad.type = LSM_AUDIT_DATA_CAP;
> > > > > ad.u.cap = cap;
> > > > > @@ -1590,11 +1590,13 @@ static int cred_has_capability(const struct cred *cred,
> > > > > }
> > > > >
> > > > > rc = avc_has_perm_noaudit(sid, sid, sclass, av, 0, &avd);
> > > > > - if (!(opts & CAP_OPT_NOAUDIT)) {
> > > > > - int rc2 = avc_audit(sid, sid, sclass, av, &avd, rc, &ad);
> > > > > - if (rc2)
> > > > > - return rc2;
> > > > > - }
> > > > > + if ((opts & CAP_OPT_NOAUDIT) || ((opts & CAP_OPT_NODENYAUDIT) && rc))
> > > > > + return rc;
> > > >
> > > > Hm, if the caller passes only CAP_OPT_NODENYAUDIT, and rc == 0, then
> > > > you will audit the allow. Is that what you want, or did you want, or
> > > > did you want CAP_OPT_NODENYAUDIT to imply CAP_OPT_NOAUDIT?
> > > >
> >
> > The new option should cause to issue an audit event if (and only if)
> > the requested capability is in effect for the current task. If the
> > task does not have the capability no audit event should be issued.
> >
> > The new option should not imply CAP_OPT_NOAUDIT since we want an audit
> > event in the case the capability is in effect.
> >
> > I admit the naming is a bit confusing as CAP_OPT_NODENYAUDIT as well
> > as the commit description contains a double negation (while the inline
> > comment for the macro definition does not).
> >
> > Do you prefer naming the constant CAP_OPT_ALLOWAUDIT or CAP_OPT_AUDIT_ON_ALLOW?
>
> I think we need a different name, although I'm struggling to think of
> something ... I don't think ALLOWAUDIT is right, as I believe it
> implies that it is needed to "allow" auditing to take place for the
> operation. AUDIT_ON_ALLOW is better, but it still seems like it would
> be required if you wanted to generate audit records on a successful
> operation, which isn't correct. I think we need to focus on the idea
> that the flag blocks auditing for denials.
>
> CAP_OPT_NOAUDITDENY is pretty much what you have, but in my mind the
> NOAUDITDENY shares enough with the existing NOAUDIT flag that it makes
> a bit more sense.
>
> I honestly don't know. However, whatever you pick, make sure you
> update patch 2/X so that the name of ns_capable_nodenyaudit() is kept
> close to the flag's name.
(Sorry for the late response. I still need to fix my filters)
Is CAP_OPT_NOAUDIT_ONDENY or CAP_OPT_AUDIT_ONLY_ONALLOW too long? :)
Anyway, Christian, I leave the final choice to you, then please feel
free to add my Reviewed-by.
thanks,
-serge