[PATCH 04/11] exec: Move uid/gid handling from creds_from_file into bprm_fill_uid

[Eric W. Biederman]

The logic in cap_bprm_creds_from_file is difficult to follow in part
because it handles both uids/gids and capabilities.  That difficulty
in following the code has resulted in several small bugs.  Move the
handling of uids/gids into bprm_fill_uid to make the code clearer.

A small bug is fixed where the ambient capabilities were unnecessarily
cleared when the presence of a ptracer or a shared fs_struct resulted
in the setuid or setgid not being honored.  This bug was not possible
to leave in place with the movement of the uids and gids handling
out of cap_bprm_repopultate_creds.

The rest of the bugs I have tried to make more apparent but left
in tact when moving the code into bprm_fill_uid.

Ref: ee67ae7ef6ff ("commoncap: Move cap_elevated calculation into bprm_set_creds")
Fixes: 58319057b784 ("capabilities: ambient capabilities")
Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
---
 fs/exec.c            | 49 ++++++++++++++++++++++++++++++++++++--------
 security/commoncap.c | 25 +++++++---------------
 2 files changed, 48 insertions(+), 26 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 091ff6269610..956ee3a0d824 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1590,21 +1590,23 @@ static void check_unsafe_exec(struct linux_binprm *bprm)
 static void bprm_fill_uid(struct linux_binprm *bprm)
 {
 	/* Handle suid and sgid on files */
+	struct cred *new = bprm->cred;
 	struct inode *inode;
 	unsigned int mode;
+	bool need_cap;
 	kuid_t uid;
 	kgid_t gid;
 
 	if (!mnt_may_suid(bprm->file->f_path.mnt))
-		return;
+		goto after_setid;
 
 	if (task_no_new_privs(current))
-		return;
+		goto after_setid;
 
 	inode = bprm->file->f_path.dentry->d_inode;
 	mode = READ_ONCE(inode->i_mode);
 	if (!(mode & (S_ISUID|S_ISGID)))
-		return;
+		goto after_setid;
 
 	/* Be careful if suid/sgid is set */
 	inode_lock(inode);
@@ -1616,19 +1618,50 @@ static void bprm_fill_uid(struct linux_binprm *bprm)
 	inode_unlock(inode);
 
 	/* We ignore suid/sgid if there are no mappings for them in the ns */
-	if (!kuid_has_mapping(bprm->cred->user_ns, uid) ||
-		 !kgid_has_mapping(bprm->cred->user_ns, gid))
-		return;
+	if (!kuid_has_mapping(new->user_ns, uid) ||
+	    !kgid_has_mapping(new->user_ns, gid))
+		goto after_setid;
 
 	if (mode & S_ISUID) {
 		bprm->per_clear = 1;
-		bprm->cred->euid = uid;
+		new->euid = uid;
 	}
 
 	if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
 		bprm->per_clear = 1;
-		bprm->cred->egid = gid;
+		new->egid = gid;
+	}
+
+after_setid:
+	/* Will the new creds have multiple uids or gids? */
+	if (!uid_eq(new->euid, new->uid) || !gid_eq(new->egid, new->gid)) {
+		bprm->secureexec = 1;
+
+		/*
+		 * Is the root directory and working directory shared or is
+		 * the process traced and the tracing process does not have
+		 * CAP_SYS_PTRACE?
+		 *
+		 * In either case it is not safe to change the euid or egid
+		 * unless the current process has the appropriate cap and so
+		 * chaning the euid or egid was already possible.
+		 */
+		need_cap = bprm->unsafe & LSM_UNSAFE_SHARE ||
+			!ptracer_capable(current, new->user_ns);
+		if (need_cap && !uid_eq(new->euid, new->uid) &&
+		    (!ns_capable(new->user_ns, CAP_SETUID) ||
+		     (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS))) {
+			new->euid = new->uid;
+		}
+		if (need_cap && !gid_eq(new->egid, new->gid) &&
+		    (!ns_capable(new->user_ns, CAP_SETUID) ||
+		     (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS))) {
+			new->egid = new->gid;
+		}
 	}
+
+	new->suid = new->fsuid = new->euid;
+	new->sgid = new->fsgid = new->egid;
 }
 
 /*
diff --git a/security/commoncap.c b/security/commoncap.c
index 2bd1f24f3796..b39c7511862e 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -809,7 +809,7 @@ int cap_bprm_creds_from_file(struct linux_binprm *bprm)
 	/* Process setpcap binaries and capabilities for uid 0 */
 	const struct cred *old = current_cred();
 	struct cred *new = bprm->cred;
-	bool effective = false, has_fcap = false, is_setid;
+	bool effective = false, has_fcap = false;
 	int ret;
 	kuid_t root_uid;
 
@@ -828,31 +828,21 @@ int cap_bprm_creds_from_file(struct linux_binprm *bprm)
 	if (__cap_gained(permitted, new, old))
 		bprm->per_clear = 1;
 
-	/* Don't let someone trace a set[ug]id/setpcap binary with the revised
+	/* Don't let someone trace a 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 = __is_setuid(new, old) || __is_setgid(new, old);
-
-	if ((is_setid || __cap_gained(permitted, new, old)) &&
+	if (__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 */
-		if (!ns_capable(new->user_ns, CAP_SETUID) ||
-		    (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)) {
-			new->euid = new->uid;
-			new->egid = new->gid;
-		}
 		new->cap_permitted = cap_intersect(new->cap_permitted,
 						   old->cap_permitted);
 	}
 
-	new->suid = new->fsuid = new->euid;
-	new->sgid = new->fsgid = new->egid;
-
 	/* File caps or setid cancels ambient. */
-	if (has_fcap || is_setid)
+	if (has_fcap || __is_setuid(new, old) || __is_setgid(new, old))
 		cap_clear(new->cap_ambient);
 
 	/*
@@ -885,10 +875,9 @@ int cap_bprm_creds_from_file(struct linux_binprm *bprm)
 		return -EPERM;
 
 	/* Check for privilege-elevated exec. */
-	if (is_setid ||
-	    (!__is_real(root_uid, new) &&
-	     (effective ||
-	      __cap_grew(permitted, ambient, new))))
+	if (!__is_real(root_uid, new) &&
+	    (effective ||
+	     __cap_grew(permitted, ambient, new)))
 		bprm->secureexec = 1;
 
 	return 0;
-- 
2.25.0