From db1a8922cf3f0b936595ba41774fe4b66adf091a Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Wed, 11 Oct 2017 20:57:05 -0400 Subject: capabilities: factor out cap_bprm_set_creds privileged root Factor out the case of privileged root from the function cap_bprm_set_creds() to make the latter easier to read and analyse. Suggested-by: Serge Hallyn Signed-off-by: Richard Guy Briggs Reviewed-by: Serge Hallyn Acked-by: James Morris Acked-by: Kees Cook Okay-ished-by: Paul Moore Signed-off-by: James Morris --- security/commoncap.c | 76 +++++++++++++++++++++++++++++++++------------------- 1 file changed, 48 insertions(+), 28 deletions(-) (limited to 'security/commoncap.c') diff --git a/security/commoncap.c b/security/commoncap.c index c25e0d27537f..be9bca50c312 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -695,6 +695,52 @@ out: return rc; } +/* + * handle_privileged_root - Handle case of privileged root + * @bprm: The execution parameters, including the proposed creds + * @has_fcap: Are any file capabilities set? + * @effective: Do we have effective root privilege? + * @root_uid: This namespace' root UID WRT initial USER namespace + * + * Handle the case where root is privileged and hasn't been neutered by + * SECURE_NOROOT. If file capabilities are set, they won't be combined with + * set UID root and nothing is changed. If we are root, cap_permitted is + * updated. If we have become set UID root, the effective bit is set. + */ +static void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, + bool *effective, kuid_t root_uid) +{ + const struct cred *old = current_cred(); + struct cred *new = bprm->cred; + + if (issecure(SECURE_NOROOT)) + return; + /* + * If the legacy file capability is set, then don't set privs + * for a setuid root binary run by a non-root user. Do set it + * for a root user just to cause least surprise to an admin. + */ + if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { + warn_setuid_and_fcaps_mixed(bprm->filename); + return; + } + /* + * To support inheritance of root-permissions and suid-root + * executables under compatibility mode, we override the + * capability sets for the file. + */ + if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) { + /* pP' = (cap_bset & ~0) | (pI & ~0) */ + new->cap_permitted = cap_combine(old->cap_bset, + old->cap_inheritable); + } + /* + * If only the real uid is 0, we do not set the effective bit. + */ + if (uid_eq(new->euid, root_uid)) + *effective = true; +} + /** * cap_bprm_set_creds - Set up the proposed credentials for execve(). * @bprm: The execution parameters, including the proposed creds @@ -707,46 +753,20 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) { const struct cred *old = current_cred(); struct cred *new = bprm->cred; - bool effective, has_cap = false, is_setid; + bool effective = false, has_cap = false, is_setid; int ret; kuid_t root_uid; if (WARN_ON(!cap_ambient_invariant_ok(old))) return -EPERM; - effective = false; ret = get_file_caps(bprm, &effective, &has_cap); if (ret < 0) return ret; root_uid = make_kuid(new->user_ns, 0); - if (!issecure(SECURE_NOROOT)) { - /* - * If the legacy file capability is set, then don't set privs - * for a setuid root binary run by a non-root user. Do set it - * for a root user just to cause least surprise to an admin. - */ - if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { - warn_setuid_and_fcaps_mixed(bprm->filename); - goto skip; - } - /* - * To support inheritance of root-permissions and suid-root - * executables under compatibility mode, we override the - * capability sets for the file. - * - * If only the real uid is 0, we do not set the effective bit. - */ - if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) { - /* pP' = (cap_bset & ~0) | (pI & ~0) */ - new->cap_permitted = cap_combine(old->cap_bset, - old->cap_inheritable); - } - if (uid_eq(new->euid, root_uid)) - effective = true; - } -skip: + handle_privileged_root(bprm, has_cap, &effective, root_uid); /* if we have fs caps, clear dangerous personality flags */ if (!cap_issubset(new->cap_permitted, old->cap_permitted)) -- cgit From 4c7e715fc87b6f8b652363b3515b48b3822c5b5f Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Wed, 11 Oct 2017 20:57:06 -0400 Subject: capabilities: intuitive names for cap gain status Introduce macros cap_gained, cap_grew, cap_full to make the use of the negation of is_subset() easier to read and analyse. Signed-off-by: Richard Guy Briggs Reviewed-by: Serge Hallyn Acked-by: James Morris Acked-by: Kees Cook Okay-ished-by: Paul Moore Signed-off-by: James Morris --- security/commoncap.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) (limited to 'security/commoncap.c') diff --git a/security/commoncap.c b/security/commoncap.c index be9bca50c312..4c9af6ef24b6 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -741,6 +741,12 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, *effective = true; } +#define __cap_gained(field, target, source) \ + !cap_issubset(target->cap_##field, source->cap_##field) +#define __cap_grew(target, source, cred) \ + !cap_issubset(cred->cap_##target, cred->cap_##source) +#define __cap_full(field, cred) \ + cap_issubset(CAP_FULL_SET, cred->cap_##field) /** * cap_bprm_set_creds - Set up the proposed credentials for execve(). * @bprm: The execution parameters, including the proposed creds @@ -769,10 +775,9 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) handle_privileged_root(bprm, has_cap, &effective, root_uid); /* if we have fs caps, clear dangerous personality flags */ - if (!cap_issubset(new->cap_permitted, old->cap_permitted)) + if (__cap_gained(permitted, new, old)) bprm->per_clear |= PER_CLEAR_ON_SETID; - /* Don't let someone trace a set[ug]id/setpcap binary with the revised * credentials unless they have the appropriate permit. * @@ -780,8 +785,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) */ is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid); - if ((is_setid || - !cap_issubset(new->cap_permitted, old->cap_permitted)) && + if ((is_setid || __cap_gained(permitted, new, old)) && ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) || !ptracer_capable(current, new->user_ns))) { /* downgrade; they get no more than they had, and maybe less */ @@ -831,8 +835,8 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) * Number 1 above might fail if you don't have a full bset, but I think * that is interesting information to audit. */ - if (!cap_issubset(new->cap_effective, new->cap_ambient)) { - if (!cap_issubset(CAP_FULL_SET, new->cap_effective) || + if (__cap_grew(effective, ambient, new)) { + if (!__cap_full(effective, new) || !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) || issecure(SECURE_NOROOT)) { ret = audit_log_bprm_fcaps(bprm, new, old); @@ -852,7 +856,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) bprm->cap_elevated = 1; } else if (!uid_eq(new->uid, root_uid)) { if (effective || - !cap_issubset(new->cap_permitted, new->cap_ambient)) + __cap_grew(permitted, ambient, new)) bprm->cap_elevated = 1; } -- cgit From fc7eadf768a3e2c062e52eea89b52a0076d53b0c Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Wed, 11 Oct 2017 20:57:07 -0400 Subject: capabilities: rename has_cap to has_fcap Rename has_cap to has_fcap to clarify it applies to file capabilities since the entire source file is about capabilities. Signed-off-by: Richard Guy Briggs Reviewed-by: Serge Hallyn Acked-by: James Morris Acked-by: Kees Cook Okay-ished-by: Paul Moore Signed-off-by: James Morris --- security/commoncap.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) (limited to 'security/commoncap.c') diff --git a/security/commoncap.c b/security/commoncap.c index 4c9af6ef24b6..13661d34f842 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -536,7 +536,7 @@ int cap_convert_nscap(struct dentry *dentry, void **ivalue, size_t size) static inline int bprm_caps_from_vfs_caps(struct cpu_vfs_cap_data *caps, struct linux_binprm *bprm, bool *effective, - bool *has_cap) + bool *has_fcap) { struct cred *new = bprm->cred; unsigned i; @@ -546,7 +546,7 @@ static inline int bprm_caps_from_vfs_caps(struct cpu_vfs_cap_data *caps, *effective = true; if (caps->magic_etc & VFS_CAP_REVISION_MASK) - *has_cap = true; + *has_fcap = true; CAP_FOR_EACH_U32(i) { __u32 permitted = caps->permitted.cap[i]; @@ -652,7 +652,7 @@ int get_vfs_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data * its xattrs and, if present, apply them to the proposed credentials being * constructed by execve(). */ -static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_cap) +static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_fcap) { int rc = 0; struct cpu_vfs_cap_data vcaps; @@ -683,7 +683,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c goto out; } - rc = bprm_caps_from_vfs_caps(&vcaps, bprm, effective, has_cap); + rc = bprm_caps_from_vfs_caps(&vcaps, bprm, effective, has_fcap); if (rc == -EINVAL) printk(KERN_NOTICE "%s: cap_from_disk returned %d for %s\n", __func__, rc, bprm->filename); @@ -707,7 +707,7 @@ out: * set UID root and nothing is changed. If we are root, cap_permitted is * updated. If we have become set UID root, the effective bit is set. */ -static void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, +static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effective, kuid_t root_uid) { const struct cred *old = current_cred(); @@ -720,7 +720,7 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, * for a setuid root binary run by a non-root user. Do set it * for a root user just to cause least surprise to an admin. */ - if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { + if (has_fcap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { warn_setuid_and_fcaps_mixed(bprm->filename); return; } @@ -759,20 +759,20 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) { const struct cred *old = current_cred(); struct cred *new = bprm->cred; - bool effective = false, has_cap = false, is_setid; + bool effective = false, has_fcap = false, is_setid; int ret; kuid_t root_uid; if (WARN_ON(!cap_ambient_invariant_ok(old))) return -EPERM; - ret = get_file_caps(bprm, &effective, &has_cap); + ret = get_file_caps(bprm, &effective, &has_fcap); if (ret < 0) return ret; root_uid = make_kuid(new->user_ns, 0); - handle_privileged_root(bprm, has_cap, &effective, root_uid); + handle_privileged_root(bprm, has_fcap, &effective, root_uid); /* if we have fs caps, clear dangerous personality flags */ if (__cap_gained(permitted, new, old)) @@ -802,7 +802,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) new->sgid = new->fsgid = new->egid; /* File caps or setid cancels ambient. */ - if (has_cap || is_setid) + if (has_fcap || is_setid) cap_clear(new->cap_ambient); /* -- cgit From 9304b46c912d65a103a68f093b456ba3c02dca3b Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Wed, 11 Oct 2017 20:57:08 -0400 Subject: capabilities: use root_priveleged inline to clarify logic Introduce inline root_privileged() to make use of SECURE_NONROOT easier to read. Suggested-by: Serge Hallyn Signed-off-by: Richard Guy Briggs Reviewed-by: Serge Hallyn Acked-by: James Morris Acked-by: Kees Cook Okay-ished-by: Paul Moore Signed-off-by: James Morris --- security/commoncap.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'security/commoncap.c') diff --git a/security/commoncap.c b/security/commoncap.c index 13661d34f842..9b8a6e79d858 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -695,6 +695,8 @@ out: return rc; } +static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); } + /* * handle_privileged_root - Handle case of privileged root * @bprm: The execution parameters, including the proposed creds @@ -713,7 +715,7 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, const struct cred *old = current_cred(); struct cred *new = bprm->cred; - if (issecure(SECURE_NOROOT)) + if (!root_privileged()) return; /* * If the legacy file capability is set, then don't set privs @@ -838,7 +840,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) if (__cap_grew(effective, ambient, new)) { if (!__cap_full(effective, new) || !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) || - issecure(SECURE_NOROOT)) { + !root_privileged()) { ret = audit_log_bprm_fcaps(bprm, new, old); if (ret < 0) return ret; -- cgit From 81a6a012996b3fd47608d87b16e79412dd73578e Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Wed, 11 Oct 2017 20:57:09 -0400 Subject: capabilities: use intuitive names for id changes Introduce a number of inlines to make the use of the negation of uid_eq() easier to read and analyse. Signed-off-by: Richard Guy Briggs Reviewed-by: Serge Hallyn Acked-by: James Morris Acked-by: Kees Cook Okay-ished-by: Paul Moore Signed-off-by: James Morris --- security/commoncap.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) (limited to 'security/commoncap.c') diff --git a/security/commoncap.c b/security/commoncap.c index 9b8a6e79d858..421f7438d3c8 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -697,6 +697,15 @@ out: static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); } +static inline bool __is_real(kuid_t uid, struct cred *cred) +{ return uid_eq(cred->uid, uid); } + +static inline bool __is_eff(kuid_t uid, struct cred *cred) +{ return uid_eq(cred->euid, uid); } + +static inline bool __is_suid(kuid_t uid, struct cred *cred) +{ return !__is_real(uid, cred) && __is_eff(uid, cred); } + /* * handle_privileged_root - Handle case of privileged root * @bprm: The execution parameters, including the proposed creds @@ -722,7 +731,7 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, * for a setuid root binary run by a non-root user. Do set it * for a root user just to cause least surprise to an admin. */ - if (has_fcap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { + if (has_fcap && __is_suid(root_uid, new)) { warn_setuid_and_fcaps_mixed(bprm->filename); return; } @@ -731,7 +740,7 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, * executables under compatibility mode, we override the * capability sets for the file. */ - if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) { + if (__is_eff(root_uid, new) || __is_real(root_uid, new)) { /* pP' = (cap_bset & ~0) | (pI & ~0) */ new->cap_permitted = cap_combine(old->cap_bset, old->cap_inheritable); @@ -739,7 +748,7 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, /* * If only the real uid is 0, we do not set the effective bit. */ - if (uid_eq(new->euid, root_uid)) + if (__is_eff(root_uid, new)) *effective = true; } @@ -749,6 +758,13 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, !cap_issubset(cred->cap_##target, cred->cap_##source) #define __cap_full(field, cred) \ cap_issubset(CAP_FULL_SET, cred->cap_##field) + +static inline bool __is_setuid(struct cred *new, const struct cred *old) +{ return !uid_eq(new->euid, old->uid); } + +static inline bool __is_setgid(struct cred *new, const struct cred *old) +{ return !gid_eq(new->egid, old->gid); } + /** * cap_bprm_set_creds - Set up the proposed credentials for execve(). * @bprm: The execution parameters, including the proposed creds @@ -785,7 +801,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) * * In addition, if NO_NEW_PRIVS, then ensure we get no new privs. */ - is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid); + is_setid = __is_setuid(new, old) || __is_setgid(new, old); if ((is_setid || __cap_gained(permitted, new, old)) && ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) || @@ -839,7 +855,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) */ if (__cap_grew(effective, ambient, new)) { if (!__cap_full(effective, new) || - !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) || + !__is_eff(root_uid, new) || !__is_real(root_uid, new) || !root_privileged()) { ret = audit_log_bprm_fcaps(bprm, new, old); if (ret < 0) @@ -856,7 +872,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) bprm->cap_elevated = 0; if (is_setid) { bprm->cap_elevated = 1; - } else if (!uid_eq(new->uid, root_uid)) { + } else if (!__is_real(root_uid, new)) { if (effective || __cap_grew(permitted, ambient, new)) bprm->cap_elevated = 1; -- cgit From 9fbc2c79644a88a1cc40a2628ccff1bbbbc9ecc5 Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Wed, 11 Oct 2017 20:57:10 -0400 Subject: capabilities: move audit log decision to function Move the audit log decision logic to its own function to isolate the complexity in one place. Suggested-by: Serge Hallyn Signed-off-by: Richard Guy Briggs Reviewed-by: Serge Hallyn Acked-by: James Morris Acked-by: Kees Cook Okay-ished-by: Paul Moore Signed-off-by: James Morris --- security/commoncap.c | 50 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 20 deletions(-) (limited to 'security/commoncap.c') diff --git a/security/commoncap.c b/security/commoncap.c index 421f7438d3c8..d7f0cbdf04c4 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -765,6 +765,32 @@ static inline bool __is_setuid(struct cred *new, const struct cred *old) static inline bool __is_setgid(struct cred *new, const struct cred *old) { return !gid_eq(new->egid, old->gid); } +/* + * Audit candidate if current->cap_effective is set + * + * We do not bother to audit if 3 things are true: + * 1) cap_effective has all caps + * 2) we are root + * 3) root is supposed to have all caps (SECURE_NOROOT) + * Since this is just a normal root execing a process. + * + * Number 1 above might fail if you don't have a full bset, but I think + * that is interesting information to audit. + */ +static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root) +{ + bool ret = false; + + if (__cap_grew(effective, ambient, cred)) { + if (!__cap_full(effective, cred) || + !__is_eff(root, cred) || !__is_real(root, cred) || + !root_privileged()) { + ret = true; + } + } + return ret; +} + /** * cap_bprm_set_creds - Set up the proposed credentials for execve(). * @bprm: The execution parameters, including the proposed creds @@ -841,26 +867,10 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) if (WARN_ON(!cap_ambient_invariant_ok(new))) return -EPERM; - /* - * Audit candidate if current->cap_effective is set - * - * We do not bother to audit if 3 things are true: - * 1) cap_effective has all caps - * 2) we are root - * 3) root is supposed to have all caps (SECURE_NOROOT) - * Since this is just a normal root execing a process. - * - * Number 1 above might fail if you don't have a full bset, but I think - * that is interesting information to audit. - */ - if (__cap_grew(effective, ambient, new)) { - if (!__cap_full(effective, new) || - !__is_eff(root_uid, new) || !__is_real(root_uid, new) || - !root_privileged()) { - ret = audit_log_bprm_fcaps(bprm, new, old); - if (ret < 0) - return ret; - } + if (nonroot_raised_pE(new, root_uid)) { + ret = audit_log_bprm_fcaps(bprm, new, old); + if (ret < 0) + return ret; } new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS); -- cgit From 02ebbaf48cf211498a9bd2c6b65e7d1b0a901807 Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Wed, 11 Oct 2017 20:57:11 -0400 Subject: capabilities: remove a layer of conditional logic Remove a layer of conditional logic to make the use of conditions easier to read and analyse. Signed-off-by: Richard Guy Briggs Reviewed-by: Serge Hallyn Acked-by: James Morris Acked-by: Kees Cook Okay-ished-by: Paul Moore Signed-off-by: James Morris --- security/commoncap.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) (limited to 'security/commoncap.c') diff --git a/security/commoncap.c b/security/commoncap.c index d7f0cbdf04c4..eac70e2b400b 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -781,13 +781,12 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root) { bool ret = false; - if (__cap_grew(effective, ambient, cred)) { - if (!__cap_full(effective, cred) || - !__is_eff(root, cred) || !__is_real(root, cred) || - !root_privileged()) { - ret = true; - } - } + if (__cap_grew(effective, ambient, cred) && + (!__cap_full(effective, cred) || + !__is_eff(root, cred) || + !__is_real(root, cred) || + !root_privileged())) + ret = true; return ret; } @@ -880,13 +879,11 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) /* Check for privilege-elevated exec. */ bprm->cap_elevated = 0; - if (is_setid) { + if (is_setid || + (!__is_real(root_uid, new) && + (effective || + __cap_grew(permitted, ambient, new)))) bprm->cap_elevated = 1; - } else if (!__is_real(root_uid, new)) { - if (effective || - __cap_grew(permitted, ambient, new)) - bprm->cap_elevated = 1; - } return 0; } -- cgit From c0d1adefe0a3775cc16374dc9ebdfd8504afa14b Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Wed, 11 Oct 2017 20:57:12 -0400 Subject: capabilities: invert logic for clarity The way the logic was presented, it was awkward to read and verify. Invert the logic using DeMorgan's Law to be more easily able to read and understand. Signed-off-by: Richard Guy Briggs Reviewed-by: Serge Hallyn Acked-by: James Morris Acked-by: Kees Cook Okay-ished-by: Paul Moore Signed-off-by: James Morris --- security/commoncap.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'security/commoncap.c') diff --git a/security/commoncap.c b/security/commoncap.c index eac70e2b400b..0bd94d36e635 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -782,10 +782,10 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root) bool ret = false; if (__cap_grew(effective, ambient, cred) && - (!__cap_full(effective, cred) || - !__is_eff(root, cred) || - !__is_real(root, cred) || - !root_privileged())) + !(__cap_full(effective, cred) && + __is_eff(root, cred) && + __is_real(root, cred) && + root_privileged())) ret = true; return ret; } -- cgit From 588fb2c7e294753d3090a1dc2e7c34e7e3ce5aff Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Wed, 11 Oct 2017 20:57:13 -0400 Subject: capabilities: fix logic for effective root or real root Now that the logic is inverted, it is much easier to see that both real root and effective root conditions had to be met to avoid printing the BPRM_FCAPS record with audit syscalls. This meant that any setuid root applications would print a full BPRM_FCAPS record when it wasn't necessary, cluttering the event output, since the SYSCALL and PATH records indicated the presence of the setuid bit and effective root user id. Require only one of effective root or real root to avoid printing the unnecessary record. Ref: commit 3fc689e96c0c ("Add audit_log_bprm_fcaps/AUDIT_BPRM_FCAPS") See: https://github.com/linux-audit/audit-kernel/issues/16 Signed-off-by: Richard Guy Briggs Reviewed-by: Serge Hallyn Acked-by: James Morris Acked-by: Kees Cook Acked-by: Paul Moore Signed-off-by: James Morris --- security/commoncap.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'security/commoncap.c') diff --git a/security/commoncap.c b/security/commoncap.c index 0bd94d36e635..ad7536d76820 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -770,7 +770,7 @@ static inline bool __is_setgid(struct cred *new, const struct cred *old) * * We do not bother to audit if 3 things are true: * 1) cap_effective has all caps - * 2) we are root + * 2) we became root *OR* are were already root * 3) root is supposed to have all caps (SECURE_NOROOT) * Since this is just a normal root execing a process. * @@ -783,8 +783,7 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root) if (__cap_grew(effective, ambient, cred) && !(__cap_full(effective, cred) && - __is_eff(root, cred) && - __is_real(root, cred) && + (__is_eff(root, cred) || __is_real(root, cred)) && root_privileged())) ret = true; return ret; -- cgit From dbbbe1105ea6aa0c49d78a4ea0d924e0c02307eb Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Wed, 11 Oct 2017 20:57:14 -0400 Subject: capabilities: audit log other surprising conditions The existing condition tested for process effective capabilities set by file attributes but intended to ignore the change if the result was unsurprisingly an effective full set in the case root is special with a setuid root executable file and we are root. Stated again: - When you execute a setuid root application, it is no surprise and expected that it got all capabilities, so we do not want capabilities recorded. if (pE_grew && !(pE_fullset && (eff_root || real_root) && root_priveleged) ) Now make sure we cover other cases: - If something prevented a setuid root app getting all capabilities and it wound up with one capability only, then it is a surprise and should be logged. When it is a setuid root file, we only want capabilities when the process does not get full capabilities.. root_priveleged && setuid_root && !pE_fullset - Similarly if a non-setuid program does pick up capabilities due to file system based capabilities, then we want to know what capabilities were picked up. When it has file system based capabilities we want the capabilities. !is_setuid && (has_fcap && pP_gained) - If it is a non-setuid file and it gets ambient capabilities, we want the capabilities. !is_setuid && pA_gained - These last two are combined into one due to the common first parameter. Related: https://github.com/linux-audit/audit-kernel/issues/16 Signed-off-by: Richard Guy Briggs Reviewed-by: Serge Hallyn Acked-by: James Morris Acked-by: Kees Cook Acked-by: Paul Moore Signed-off-by: James Morris --- security/commoncap.c | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) (limited to 'security/commoncap.c') diff --git a/security/commoncap.c b/security/commoncap.c index ad7536d76820..5fa839c7fb3f 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -766,7 +766,7 @@ static inline bool __is_setgid(struct cred *new, const struct cred *old) { return !gid_eq(new->egid, old->gid); } /* - * Audit candidate if current->cap_effective is set + * 1) Audit candidate if current->cap_effective is set * * We do not bother to audit if 3 things are true: * 1) cap_effective has all caps @@ -776,16 +776,31 @@ static inline bool __is_setgid(struct cred *new, const struct cred *old) * * Number 1 above might fail if you don't have a full bset, but I think * that is interesting information to audit. + * + * A number of other conditions require logging: + * 2) something prevented setuid root getting all caps + * 3) non-setuid root gets fcaps + * 4) non-setuid root gets ambient */ -static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root) +static inline bool nonroot_raised_pE(struct cred *new, const struct cred *old, + kuid_t root, bool has_fcap) { bool ret = false; - if (__cap_grew(effective, ambient, cred) && - !(__cap_full(effective, cred) && - (__is_eff(root, cred) || __is_real(root, cred)) && - root_privileged())) + if ((__cap_grew(effective, ambient, new) && + !(__cap_full(effective, new) && + (__is_eff(root, new) || __is_real(root, new)) && + root_privileged())) || + (root_privileged() && + __is_suid(root, new) && + !__cap_full(effective, new)) || + (!__is_setuid(new, old) && + ((has_fcap && + __cap_gained(permitted, new, old)) || + __cap_gained(ambient, new, old)))) + ret = true; + return ret; } @@ -865,7 +880,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) if (WARN_ON(!cap_ambient_invariant_ok(new))) return -EPERM; - if (nonroot_raised_pE(new, root_uid)) { + if (nonroot_raised_pE(new, old, root_uid, has_fcap)) { ret = audit_log_bprm_fcaps(bprm, new, old); if (ret < 0) return ret; -- cgit