[PATCH RFC 0/4] coredump: mitigate privilege escalation of process coredump

[PATCH RFC 0/4] coredump: mitigate privilege escalation of process coredump

[Wander Lairson Costa]

A set-uid executable might be a vector to privilege escalation if the
system configures the coredump file name pattern as a relative
directory destiny. The full description of the vulnerability and
a demonstration of how we can exploit it can be found at [1].

This patch series adds a PF_SUID flag to the process in execve if it is
set-[ug]id binary and elevates the new image's privileges.

In the do_coredump function, we check if:

1) We have the SUID_FLAG set
2) We have CAP_SYS_ADMIN (the process might have decreased its
   privileges)
3) The current directory is owned by root (the current code already
   checks for core_pattern being a relative path).
4) non-privileged users don't have permission to write to the current
   directory.

If all four conditions match, we set the need_suid_safe flag.

An alternative implementation (and more elegant IMO) would be saving
the fsuid and fsgid of the process in the task_struct before loading the
new image to the memory. But this approach would add eight bytes to all
task_struct instances where only a tiny fraction of the processes need
it and under a configuration that not all (most?) distributions don't
adopt by default.

Wander Lairson Costa (4):
  exec: add a flag indicating if an exec file is a suid/sgid
  process: add the PF_SUID flag
  coredump: mitigate privilege escalation of process coredump
  exec: only set the suid flag if the current proc isn't root

 fs/coredump.c           | 15 +++++++++++++++
 fs/exec.c               | 10 ++++++++++
 include/linux/binfmts.h |  6 +++++-
 include/linux/sched.h   |  1 +
 kernel/fork.c           |  2 ++
 5 files changed, 33 insertions(+), 1 deletion(-)

-- 
2.27.0

[PATCH RFC 1/4] exec: add a flag indicating if an exec file is a suid/sgid

[Wander Lairson Costa]

We create an additional flag in the struct linux_bprm to indicate a
suid/sgid binary. We will use this information in a later commit to
set the task_struct flags accordingly.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 fs/exec.c               | 2 ++
 include/linux/binfmts.h | 6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/exec.c b/fs/exec.c
index 2bb8dd6a4e2a..3913b335b95f 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1621,11 +1621,13 @@ static void bprm_fill_uid(struct linux_binprm *bprm, struct file *file)
 	if (mode & S_ISUID) {
 		bprm->per_clear |= PER_CLEAR_ON_SETID;
 		bprm->cred->euid = uid;
+		bprm->suid_bin = 1;
 	}
 
 	if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
 		bprm->per_clear |= PER_CLEAR_ON_SETID;
 		bprm->cred->egid = gid;
+		bprm->suid_bin = 1;
 	}
 }
 
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 049cf9421d83..c4b41b9711d2 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -41,7 +41,11 @@ struct linux_binprm {
 		 * Set when errors can no longer be returned to the
 		 * original userspace.
 		 */
-		point_of_no_return:1;
+		point_of_no_return:1,
+		/*
+		 * Is this a suid/sgid binary?
+		 */
+		suid_bin:1;
 #ifdef __alpha__
 	unsigned int taso:1;
 #endif
-- 
2.27.0

[PATCH RFC 2/4] process: add the PF_SUID flag

[Wander Lairson Costa]

If the binary file in an execve system call is a suid executable, we add
the PF_SUID flag to the process and all its future new children and
threads.

In a later commit, we will use this information to determine if it is
safe to core dump such a process.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 fs/exec.c             | 4 ++++
 include/linux/sched.h | 1 +
 kernel/fork.c         | 2 ++
 3 files changed, 7 insertions(+)

diff --git a/fs/exec.c b/fs/exec.c
index 3913b335b95f..b4bd157a5282 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1311,6 +1311,10 @@ int begin_new_exec(struct linux_binprm * bprm)
 
 	me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
 					PF_NOFREEZE | PF_NO_SETAFFINITY);
+
+	if (bprm->suid_bin)
+		me->flags |= PF_SUID;
+
 	flush_thread();
 	me->personality &= ~bprm->per_clear;
 
diff --git a/include/linux/sched.h b/include/linux/sched.h
index e3b328b81ac0..f7811c42b004 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1651,6 +1651,7 @@ extern struct pid *cad_pid;
 #define PF_VCPU			0x00000001	/* I'm a virtual CPU */
 #define PF_IDLE			0x00000002	/* I am an IDLE thread */
 #define PF_EXITING		0x00000004	/* Getting shut down */
+#define PF_SUID			0x00000008	/* The process comes from a suid/sgid binary */
 #define PF_IO_WORKER		0x00000010	/* Task is an IO worker */
 #define PF_WQ_WORKER		0x00000020	/* I'm a workqueue worker */
 #define PF_FORKNOEXEC		0x00000040	/* Forked but didn't exec */
diff --git a/kernel/fork.c b/kernel/fork.c
index 231b1ba3ca9f..1e1ffff70d14 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2080,6 +2080,8 @@ static __latent_entropy struct task_struct *copy_process(
 	delayacct_tsk_init(p);	/* Must remain after dup_task_struct() */
 	p->flags &= ~(PF_SUPERPRIV | PF_WQ_WORKER | PF_IDLE | PF_NO_SETAFFINITY);
 	p->flags |= PF_FORKNOEXEC;
+	if (current->flags & PF_SUID)
+		p->flags |= PF_SUID;
 	INIT_LIST_HEAD(&p->children);
 	INIT_LIST_HEAD(&p->sibling);
 	rcu_copy_process(p);
-- 
2.27.0

[PATCH RFC 3/4] coredump: mitigate privilege escalation of process coredump

[Wander Lairson Costa]

A set-uid executable might be a vector to a privilege escalation if the
system configures the coredump file name pattern as a relative
directory destiny. The full description of the vulnerability and
a demonstration of how we can exploit it can be found at [1].

We now check if the core dump pattern is relative. If it is, then we
verify if root owns the current directory and if it does, we deny
writing the core file unless the directory is universally writable.

[1] https://www.openwall.com/lists/oss-security/2021/10/20/2

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 fs/coredump.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/fs/coredump.c b/fs/coredump.c
index 07afb5ddb1c4..74eae7bd144d 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -580,6 +580,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 	struct core_name cn;
 	struct mm_struct *mm = current->mm;
 	struct linux_binfmt * binfmt;
+	struct inode *pwd_inode;
 	const struct cred *old_cred;
 	struct cred *cred;
 	int retval = 0;
@@ -625,6 +626,20 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 		need_suid_safe = true;
 	}
 
+	/*
+	 * If we are a set-uid/gid root process and the current directory is
+	 * owned by root but not universally writable, prohibit dumps under
+	 * this path.
+	 *
+	 * Mitigate https://www.openwall.com/lists/oss-security/2021/10/20/2
+	 */
+	pwd_inode = current->fs->pwd.dentry->d_inode;
+	if (current->flags & PF_SUID &&
+	    capable(CAP_SYS_ADMIN) &&
+	    uid_eq(pwd_inode->i_uid, GLOBAL_ROOT_UID) &&
+	    !(pwd_inode->i_mode & 0002))
+		need_suid_safe = true;
+
 	retval = coredump_wait(siginfo->si_signo, &core_state);
 	if (retval < 0)
 		goto fail_creds;
-- 
2.27.0

[PATCH RFC 4/4] exec: only set the suid flag if the current proc isn't root

[Wander Lairson Costa]

The goal of PF_SUID flag is to check if it is safe to coredump the
process. If the current process is already privileged, there is no
point in performing security checks because the name image is a
set-uid process.

Because of that, we don't set the suid flag if the forked process
already runs as root.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 fs/exec.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/exec.c b/fs/exec.c
index b4bd157a5282..d73b21b6298c 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1312,7 +1312,11 @@ int begin_new_exec(struct linux_binprm * bprm)
 	me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
 					PF_NOFREEZE | PF_NO_SETAFFINITY);
 
-	if (bprm->suid_bin)
+	/*
+	 * We set the PF_SUID flags for security reasons. There is no
+	 * point in setting it if the parent is root.
+	 */
+	if (bprm->suid_bin && !capable(CAP_SYS_ADMIN))
 		me->flags |= PF_SUID;
 
 	flush_thread();
-- 
2.27.0