Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
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 04/11] exec: Move uid/gid handling from creds_from_file into bprm_fill_uid
Date: Thu, 28 May 2020 10:44:00 -0500
Message-ID: <87r1v4xdxb.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")


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@xmission.com>
---
 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


  parent reply index

Thread overview: 108+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <87h7wujhmz.fsf@x220.int.ebiederm.org>
     [not found] ` <87sgga6ze4.fsf@x220.int.ebiederm.org>
2020-05-09 19:40   ` [PATCH 0/5] exec: Control flow simplifications Eric W. Biederman
2020-05-09 19:40     ` [PATCH 1/5] exec: Call cap_bprm_set_creds directly from prepare_binprm Eric W. Biederman
2020-05-09 20:04       ` Linus Torvalds
2020-05-09 19:41     ` [PATCH 2/5] exec: Directly call security_bprm_set_creds from __do_execve_file Eric W. Biederman
2020-05-09 20:07       ` Linus Torvalds
2020-05-09 20:12         ` Eric W. Biederman
2020-05-09 20:19           ` Linus Torvalds
2020-05-11  3:15       ` Kees Cook
2020-05-11 16:52         ` Eric W. Biederman
2020-05-11 21:18           ` Kees Cook
2020-05-09 19:41     ` [PATCH 3/5] exec: Remove recursion from search_binary_handler Eric W. Biederman
2020-05-09 20:16       ` Linus Torvalds
2020-05-10  4:22       ` Tetsuo Handa
2020-05-10 19:38         ` Linus Torvalds
2020-05-11 14:33           ` Eric W. Biederman
2020-05-11 19:10             ` Rob Landley
2020-05-13 21:59               ` Eric W. Biederman
2020-05-14 18:46                 ` Rob Landley
2020-05-11 21:55             ` Kees Cook
2020-05-12 18:42               ` Eric W. Biederman
2020-05-12 19:25                 ` Kees Cook
2020-05-12 20:31                   ` Eric W. Biederman
2020-05-12 23:08                     ` Kees Cook
2020-05-12 23:47                       ` Kees Cook
2020-05-12 23:51                         ` Kees Cook
2020-05-14 14:56                           ` Eric W. Biederman
2020-05-14 16:56                             ` Casey Schaufler
2020-05-14 17:02                               ` Eric W. Biederman
2020-05-13  0:20                 ` Linus Torvalds
2020-05-13  2:39                   ` Rob Landley
2020-05-13 19:51                     ` Linus Torvalds
2020-05-14 16:49                   ` Eric W. Biederman
2020-05-09 19:42     ` [PATCH 4/5] exec: Allow load_misc_binary to call prepare_binfmt unconditionally Eric W. Biederman
2020-05-11 22:09       ` Kees Cook
2020-05-09 19:42     ` [PATCH 5/5] exec: Move the call of prepare_binprm into search_binary_handler Eric W. Biederman
2020-05-11 22:24       ` Kees Cook
2020-05-19  0:29     ` [PATCH v2 0/8] exec: Control flow simplifications Eric W. Biederman
2020-05-19  0:29       ` [PATCH v2 1/8] exec: Teach prepare_exec_creds how exec treats uids & gids Eric W. Biederman
2020-05-19 18:03         ` Kees Cook
2020-05-19 18:28           ` Linus Torvalds
2020-05-19 18:57             ` Eric W. Biederman
2020-05-19  0:30       ` [PATCH v2 2/8] exec: Factor security_bprm_creds_for_exec out of security_bprm_set_creds Eric W. Biederman
2020-05-19 15:34         ` Casey Schaufler
2020-05-19 18:10         ` Kees Cook
2020-05-19 21:28           ` James Morris
2020-05-19  0:31       ` [PATCH v2 3/8] exec: Convert security_bprm_set_creds into security_bprm_repopulate_creds Eric W. Biederman
2020-05-19 18:21         ` Kees Cook
2020-05-19 19:03           ` Eric W. Biederman
2020-05-19 19:14             ` Kees Cook
2020-05-20 20:22               ` Eric W. Biederman
2020-05-20 20:53                 ` Kees Cook
2020-05-19 21:52         ` James Morris
2020-05-20 12:40           ` Eric W. Biederman
2020-05-19  0:31       ` [PATCH v2 4/8] exec: Allow load_misc_binary to call prepare_binfmt unconditionally Eric W. Biederman
2020-05-19 18:27         ` Kees Cook
2020-05-19 19:08           ` Eric W. Biederman
2020-05-19 19:17             ` Kees Cook
2020-05-19  0:32       ` [PATCH v2 5/8] exec: Move the call of prepare_binprm into search_binary_handler Eric W. Biederman
2020-05-19 18:27         ` Kees Cook
2020-05-19 21:30         ` James Morris
2020-05-19  0:33       ` [PATCH v2 6/8] exec/binfmt_script: Don't modify bprm->buf and then return -ENOEXEC Eric W. Biederman
2020-05-19 19:08         ` Kees Cook
2020-05-19 19:19           ` Eric W. Biederman
2020-05-19  0:33       ` [PATCH v2 7/8] exec: Generic execfd support Eric W. Biederman
2020-05-19 19:46         ` Kees Cook
2020-05-19 19:54           ` Linus Torvalds
2020-05-19 20:20             ` Eric W. Biederman
2020-05-19 21:59         ` Rob Landley
2020-05-20 16:05           ` Eric W. Biederman
2020-05-21 22:50             ` Rob Landley
2020-05-22  3:28               ` Eric W. Biederman
2020-05-22  4:51                 ` Rob Landley
2020-05-22 13:35                   ` Eric W. Biederman
2020-05-19  0:34       ` [PATCH v2 8/8] exec: Remove recursion from search_binary_handler Eric W. Biederman
2020-05-19 20:37         ` Kees Cook
2020-05-19  1:25       ` [PATCH v2 0/8] exec: Control flow simplifications Linus Torvalds
2020-05-19 21:55       ` Kees Cook
2020-05-20 13:02         ` Eric W. Biederman
2020-05-20 22:12       ` Eric W. Biederman
2020-05-20 23:43         ` Kees Cook
2020-05-21 11:53           ` Eric W. Biederman
2020-05-28 15:38       ` [PATCH 0/11] exec: cred calculation simplifications Eric W. Biederman
2020-05-28 15:41         ` [PATCH 01/11] exec: Reduce bprm->per_clear to a single bit Eric W. Biederman
2020-05-28 19:04           ` Linus Torvalds
2020-05-28 19:17             ` Eric W. Biederman
2020-05-28 15:42         ` [PATCH 02/11] exec: Introduce active_per_clear the per file version of per_clear Eric W. Biederman
2020-05-28 19:05           ` Linus Torvalds
2020-05-28 15:42         ` [PATCH 03/11] exec: Compute file based creds only once Eric W. Biederman
2020-05-28 15:43         ` [PATCH 04/11] exec: Move uid/gid handling from creds_from_file into bprm_fill_uid Eric W. Biederman
2020-05-28 15:44         ` Eric W. Biederman [this message]
2020-05-28 15:44         ` [PATCH 05/11] exec: In bprm_fill_uid use CAP_SETGID to see if a gid change is safe Eric W. Biederman
2020-05-28 15:48         ` [PATCH 06/11] exec: Don't set secureexec when the uid or gid changes are abandoned Eric W. Biederman
2020-05-28 15:48         ` [PATCH 07/11] exec: Set saved, fs, and effective ids together in bprm_fill_uid Eric W. Biederman
2020-05-28 15:49         ` [PATCH 08/11] exec: In bprm_fill_uid remove unnecessary no new privs check Eric W. Biederman
2020-05-28 15:49         ` [PATCH 09/11] exec: In bprm_fill_uid only set per_clear when honoring suid or sgid Eric W. Biederman
2020-05-28 19:08           ` Linus Torvalds
2020-05-28 19:21             ` Eric W. Biederman
2020-05-28 15:50         ` [PATCH 10/11] exec: In bprm_fill_uid set secureexec at same time as per_clear Eric W. Biederman
2020-05-28 15:50         ` [PATCH 11/11] exec: Remove the label after_setid from bprm_fill_uid Eric W. Biederman
2020-05-29 16:45         ` [PATCH 0/2] exec: Remove the computation of bprm->cred Eric W. Biederman
2020-05-29 16:46           ` [PATCH 1/2] exec: Add a per bprm->file version of per_clear Eric W. Biederman
2020-05-29 21:06             ` Kees Cook
2020-05-30  3:23               ` Eric W. Biederman
2020-05-30  5:14                 ` Kees Cook
2020-05-29 16:47           ` [PATCH 2/2] exec: Compute file based creds only once Eric W. Biederman
2020-05-29 21:24             ` Kees Cook
2020-05-30  3:28               ` Eric W. Biederman
2020-05-30  5:18                 ` Kees Cook

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87r1v4xdxb.fsf_-_@x220.int.ebiederm.org \
    --to=ebiederm@xmission.com \
    --cc=adobriyan@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=bernd.edlinger@hotmail.de \
    --cc=casey@schaufler-ca.com \
    --cc=gerg@linux-m68k.org \
    --cc=jannh@google.com \
    --cc=jmorris@namei.org \
    --cc=keescook@chromium.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=oleg@redhat.com \
    --cc=rob@landley.net \
    --cc=serge@hallyn.com \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@ZenIV.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git