[PATCH 06/11] exec: Don't set secureexec when the uid or gid changes are abandoned

From: ebiederm@xmission.com (Eric W. Biederman)
To: <linux-kernel@vger.kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Oleg Nesterov <oleg@redhat.com>, Jann Horn <jannh@google.com>,
	Kees Cook <keescook@chromium.org>,
	Greg Ungerer <gerg@linux-m68k.org>, Rob Landley <rob@landley.net>,
	Bernd Edlinger <bernd.edlinger@hotmail.de>,
	<linux-fsdevel@vger.kernel.org>,
	Al Viro <viro@ZenIV.linux.org.uk>,
	Alexey Dobriyan <adobriyan@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Casey Schaufler <casey@schaufler-ca.com>,
	linux-security-module@vger.kernel.org,
	James Morris <jmorris@namei.org>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Andy Lutomirski <luto@amacapital.net>
Subject: [PATCH 06/11] exec: Don't set secureexec when the uid or gid changes are abandoned
Date: Thu, 28 May 2020 10:48:11 -0500	[thread overview]
Message-ID: <87ftbkxdqc.fsf_-_@x220.int.ebiederm.org> (raw)
In-Reply-To: <87k10wysqz.fsf_-_@x220.int.ebiederm.org> (Eric W. Biederman's message of "Thu, 28 May 2020 10:38:28 -0500")


When the is_secureexec test was removed from cap_bprm_set_creds the
test was modified so that it based the status of secureexec on a
version of the euid and egid before ptrace and shared fs tests
possibly reverted them.

The effect of which is that secureexec continued to be set when the
euid and egid change were abandoned because the executable was being
ptraced to secureexec being set in that same situation.

As far as I can tell it is just an oversight and very poor quality of
implementation to set AT_SECURE when it is not ncessary.  So improve
the quality of the implementation by only setting secureexec when there
will be multiple uids or gids in the final cred structure.

Fixes: ee67ae7ef6ff ("commoncap: Move cap_elevated calculation into bprm_set_creds")
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/exec.c | 48 +++++++++++++++++++++---------------------------
 1 file changed, 21 insertions(+), 27 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index bac8db14f30d..123402f218fe 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1622,44 +1622,38 @@ static void bprm_fill_uid(struct linux_binprm *bprm)
 	    !kgid_has_mapping(new->user_ns, gid))
 		goto after_setid;
 
+	/*
+	 * 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 (mode & S_ISUID) {
 		bprm->per_clear = 1;
-		new->euid = uid;
+		if (!need_cap ||
+		    (ns_capable(new->user_ns, CAP_SETUID) &&
+		     !(bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)))
+			new->euid = uid;
 	}
-
 	if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
 		bprm->per_clear = 1;
-		new->egid = gid;
+		if (!need_cap ||
+		    (ns_capable(new->user_ns, CAP_SETGID) &&
+		     !(bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)))
+			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)) {
+	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_SETGID) ||
-		     (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS))) {
-			new->egid = new->gid;
-		}
-	}
-
 	new->suid = new->fsuid = new->euid;
 	new->sgid = new->fsgid = new->egid;
 }
-- 
2.25.0

