12:35 a.m.
On Thu, May 11, 2017 at 04:42:40PM -0400, Richard Guy Briggs wrote:
This change is intended to be logic-neutral and simply make the logic
easier to
read in natural language and verify without getting distracted by details.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
security/commoncap.c | 53 ++++++++++++++++++++++++++++++++-----------------
1 files changed, 34 insertions(+), 19 deletions(-)
diff --git a/security/commoncap.c b/security/commoncap.c
index 78b3783..9520f0a 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -497,6 +497,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
int ret;
kuid_t root_uid;
The #defines make me uncomfortable, especially the lack of parens around
them. The way they are used seems fine, but they seem like potential
future maintenance issues. I definately appreciate the way you broke
the functionality down, though. And I'm not sure I can improve on it.
+#define SROOT !issecure(SECURE_NOROOT) /* root is special */
maybe
static inline bool root_privileged() { return !issecure(SECURE_NOROOT); }
+#define RROOT uid_eq(new->uid, root_uid) /* real root */
+#define EROOT uid_eq(new->euid, root_uid) /* effective root */
+#define SETUIDROOT !RROOT && EROOT /* set uid root */
Yeah every time I start typing an alternative it doesn't look as good.
+#define SUID !uid_eq(new->euid, old->uid) /* set uid */
+#define SGID !gid_eq(new->egid, old->gid) /* set gid */
+#define pPADD !cap_issubset(new->cap_permitted, old->cap_permitted) /* process
permitted capabilities have been added */
+#define pESET !cap_issubset(new->cap_effective, new->cap_ambient) /* process
effective capabilities have been set */
+#define pEALL cap_issubset(CAP_FULL_SET, new->cap_effective) /* process effective
capabilities are full set */
+#define pAADD !cap_issubset(new->cap_ambient, old->cap_ambient) /* process ambient
capabilities have been added */
if (WARN_ON(!cap_ambient_invariant_ok(old)))
return -EPERM;
@@ -507,13 +517,13 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
root_uid = make_kuid(new->user_ns, 0);
- if (!issecure(SECURE_NOROOT)) {
+ if (SROOT) {
/*
* 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)) {
+ if (has_cap && SETUIDROOT) {
warn_setuid_and_fcaps_mixed(bprm->filename);
goto skip;
}
@@ -521,33 +531,32 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
* 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)) {
+ if (EROOT || RROOT) {
/* pP' = (cap_bset & ~0) | (pI & ~0) */
new->cap_permitted = cap_combine(old->cap_bset,
old->cap_inheritable);
}
- if (uid_eq(new->euid, root_uid))
+ /*
+ * If only the real uid is root, we do not set the effective bit.
+ */
+ if (EROOT)
effective = true;
}
skip:
/* if we have fs caps, clear dangerous personality flags */
- if (!cap_issubset(new->cap_permitted, old->cap_permitted))
+ if (pPADD)
bprm->per_clear |= PER_CLEAR_ON_SETID;
+ is_setid = SUID || SGID;
/* Don't let someone trace a set[ug]id/setpcap binary with the revised
* credentials unless they have the appropriate permit.
*
* 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);
-
- if ((is_setid ||
- !cap_issubset(new->cap_permitted, old->cap_permitted)) &&
+ if ((is_setid || pPADD) &&
((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
!ptracer_capable(current, new->user_ns))) {
/* downgrade; they get no more than they had, and maybe less */
@@ -599,14 +608,10 @@ 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) ||
- !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
- issecure(SECURE_NOROOT)) {
- ret = audit_log_bprm_fcaps(bprm, new, old);
- if (ret < 0)
- return ret;
- }
+ if (pESET && (!pEALL || !EROOT || !RROOT || !SROOT) ) {
This might be better served by a separate helper
if (nonroot_raised_e(new, root_uid)) {
ret = audit_log_bprm_fcaps(bprm, new, old);
if (ret < 0)
return ret;
}
+ ret = audit_log_bprm_fcaps(bprm, new, old);
+ if (ret < 0)
+ return ret;
}
new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
@@ -615,6 +620,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
return -EPERM;
return 0;
+#undef SROOT
+#undef RROOT
+#undef EROOT
+#undef SETUIDROOT
+#undef SUID
+#undef SGID
+#undef pPADD
+#undef pESET
+#undef pEALL
+#undef pAADD
}
/**
--
1.7.1