[PATCH GHAK16 V5 02/10] capabilities: intuitive names for cap gain status

Discussion:

Richard Guy Briggs

2017-10-12 00:57:06 UTC

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 <***@redhat.com>
Reviewed-by: Serge Hallyn <***@hallyn.com>
Acked-by: James Morris <***@oracle.com>
Acked-by: Kees Cook <***@chromium.org>
Okay-ished-by: Paul Moore <***@paul-moore.com>
---
security/commoncap.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index be9bca5..4c9af6e 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;
}

--
1.8.3.1

Richard Guy Briggs

2017-10-12 00:57:09 UTC

Permalink

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 <***@redhat.com>
Reviewed-by: Serge Hallyn <***@hallyn.com>
Acked-by: James Morris <***@oracle.com>
Acked-by: Kees Cook <***@chromium.org>
Okay-ished-by: Paul Moore <***@paul-moore.com>
---
security/commoncap.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 9b8a6e7..421f743 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -697,6 +697,15 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f

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;

--
1.8.3.1

Richard Guy Briggs

2017-10-12 00:57:08 UTC

Permalink

Introduce inline root_privileged() to make use of SECURE_NONROOT
easier to read.

Suggested-by: Serge Hallyn <***@hallyn.com>
Signed-off-by: Richard Guy Briggs <***@redhat.com>
Reviewed-by: Serge Hallyn <***@hallyn.com>
Acked-by: James Morris <***@oracle.com>
Acked-by: Kees Cook <***@chromium.org>
Okay-ished-by: Paul Moore <***@paul-moore.com>
---
security/commoncap.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 13661d3..9b8a6e7 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -695,6 +695,8 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f
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;

--
1.8.3.1

Richard Guy Briggs

2017-10-12 00:57:05 UTC

Permalink

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 <***@hallyn.com>
Signed-off-by: Richard Guy Briggs <***@redhat.com>
Reviewed-by: Serge Hallyn <***@hallyn.com>
Acked-by: James Morris <***@oracle.com>
Acked-by: Kees Cook <***@chromium.org>
Okay-ished-by: Paul Moore <***@paul-moore.com>
---
security/commoncap.c | 76 +++++++++++++++++++++++++++++++++-------------------
1 file changed, 48 insertions(+), 28 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index c25e0d2..be9bca5 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -695,6 +695,52 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
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))

--
1.8.3.1

Richard Guy Briggs

2017-10-12 00:57:07 UTC

Permalink

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 <***@redhat.com>
Reviewed-by: Serge Hallyn <***@hallyn.com>
Acked-by: James Morris <***@oracle.com>
Acked-by: Kees Cook <***@chromium.org>
Okay-ished-by: Paul Moore <***@paul-moore.com>
---
security/commoncap.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 4c9af6e..13661d3 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 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
* 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);

/*

--
1.8.3.1

Richard Guy Briggs

2017-10-12 00:57:12 UTC

Permalink

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 <***@redhat.com>
Reviewed-by: Serge Hallyn <***@hallyn.com>
Acked-by: James Morris <***@oracle.com>
Acked-by: Kees Cook <***@chromium.org>
Okay-ished-by: Paul Moore <***@paul-moore.com>
---
security/commoncap.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index eac70e2..0bd94d3 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;
}

--
1.8.3.1

Richard Guy Briggs

2017-10-12 00:57:13 UTC

Permalink

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 <***@redhat.com>
Reviewed-by: Serge Hallyn <***@hallyn.com>
Acked-by: James Morris <***@oracle.com>
Acked-by: Kees Cook <***@chromium.org>
Acked-by: Paul Moore <***@paul-moore.com>
---
security/commoncap.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 0bd94d3..ad7536d 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;

--
1.8.3.1

Richard Guy Briggs

2017-10-12 00:57:10 UTC

Permalink

Move the audit log decision logic to its own function to isolate the
complexity in one place.

Suggested-by: Serge Hallyn <***@hallyn.com>
Signed-off-by: Richard Guy Briggs <***@redhat.com>
Reviewed-by: Serge Hallyn <***@hallyn.com>
Acked-by: James Morris <***@oracle.com>
Acked-by: Kees Cook <***@chromium.org>
Okay-ished-by: Paul Moore <***@paul-moore.com>
---
security/commoncap.c | 50 ++++++++++++++++++++++++++++++--------------------
1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 421f743..d7f0cbd 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);

--
1.8.3.1

Richard Guy Briggs

2017-10-12 00:57:11 UTC

Permalink

Remove a layer of conditional logic to make the use of conditions
easier to read and analyse.

Signed-off-by: Richard Guy Briggs <***@redhat.com>
Reviewed-by: Serge Hallyn <***@hallyn.com>
Acked-by: James Morris <***@oracle.com>
Acked-by: Kees Cook <***@chromium.org>
Okay-ished-by: Paul Moore <***@paul-moore.com>
---
security/commoncap.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index d7f0cbd..eac70e2 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;
}

--
1.8.3.1

Richard Guy Briggs

2017-10-12 00:57:14 UTC

Permalink

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 <***@redhat.com>
Reviewed-by: Serge Hallyn <***@hallyn.com>
Acked-by: James Morris <***@oracle.com>
Acked-by: Kees Cook <***@chromium.org>
Acked-by: Paul Moore <***@paul-moore.com>
---
security/commoncap.c | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index ad7536d..5fa839c 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;

--
1.8.3.1

Richard Guy Briggs

2017-10-19 13:08:10 UTC

Permalink

The audit subsystem is adding a BPRM_FCAPS record when auditing setuid
application execution (SYSCALL execve). This is not expected as it was
supposed to be limited to when the file system actually had capabilities
in an extended attribute. It lists all capabilities making the event
really ugly to parse what is happening. The PATH record correctly
records the setuid bit and owner. Suppress the BPRM_FCAPS record on
set*id.

<crickets>

Serge? James? Can one of you two take this via your trees since Paul
has backed down citing (reasonably) that it is mostly capabilities
patches rather than audit?

See: https://github.com/linux-audit/audit-kernel/issues/16
The first to eighth patches just massage the logic to make it easier to
understand. Some of them could be squashed together.
The patch that resolves this issue is the ninth.
It would be possible to address the original issue with a change of
"!uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid)"
to
"!(uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid))"
but it took me long enough to understand this logic that I don't think
I'd be doing any favours by leaving it this difficult to understand.
The final patch attempts to address all the conditions that need logging
based on mailing list conversations, recoginizing there is probably some
duplication in the logic.
Passes: (ltp 20170516)
./runltp -f syscalls -s cap
./runltp -f securebits
./runltp -f cap_bounds
./runltp -f filecaps
make TARGETS=capabilities kselftest (when run locally, fails over nfs)
Since this is mostly capabilities related rather than audit, could this go
through the capabilites (Serge) or security (James) trees please? Thanks!
v5
rebase on linux-security/next 4.14-rc2
added comment block header to handle_privileged_root()
moved comment in handle_privileged_root()
moved root_privileged() check back into handle_privileged_root()
v4
rebase on kees' 4.13 commoncap changes
minor local func renames
v3
refactor into several sub-functions
convert most macros to inline funcs
v2
use macros to clarify intent of calculations
fix original logic error
address additional audit logging conditions
capabilities: factor out cap_bprm_set_creds privileged root
capabilities: intuitive names for cap gain status
capabilities: rename has_cap to has_fcap
capabilities: use root_priveleged inline to clarify logic
capabilities: use intuitive names for id changes
capabilities: move audit log decision to function
capabilities: remove a layer of conditional logic
capabilities: invert logic for clarity
capabilities: fix logic for effective root or real root
capabilities: audit log other surprising conditions
security/commoncap.c | 193 ++++++++++++++++++++++++++++++++++-----------------
1 file changed, 128 insertions(+), 65 deletions(-)
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

James Morris

2017-10-20 01:29:50 UTC

Permalink

Post by Richard Guy Briggs

<crickets>
Serge? James? Can one of you two take this via your trees since Paul
has backed down citing (reasonably) that it is mostly capabilities
patches rather than audit?

Sure, I will take it.

Post by Richard Guy Briggs

- RGB
--
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
James Morris
<***@oracle.com>

Richard Guy Briggs

2017-10-20 02:34:37 UTC

Permalink

Post by James Morris

Post by Richard Guy Briggs

<crickets>
Serge? James? Can one of you two take this via your trees since Paul
has backed down citing (reasonably) that it is mostly capabilities
patches rather than audit?

Sure, I will take it.

Thanks Jaume!

Post by James Morris

Post by Richard Guy Briggs

- RGB
--
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
James Morris

- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

James Morris

2017-10-20 05:15:00 UTC

Permalink

Post by Richard Guy Briggs

<crickets>
Serge? James? Can one of you two take this via your trees since Paul
has backed down citing (reasonably) that it is mostly capabilities
patches rather than audit?

Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-general

--
James Morris
<***@oracle.com>