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 06/11] exec: Don't set secureexec when the uid or gid changes are abandoned
Date: Thu, 28 May 2020 10:48:11 -0500
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


  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
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         ` Eric W. Biederman [this message]
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=87ftbkxdqc.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