mm: Add a user_ns owner to mm_struct and fix ptrace_may_access

During exec dumpable is cleared if the file that is being executed is not readable by the user executing the file. A bug in ptrace_may_access allows reading the file if the executable happens to enter into a subordinate user namespace (aka clone(CLONE_NEWUSER), unshare(CLONE_NEWUSER), or setns(fd, CLONE_NEWUSER).

This problem is fixed with only necessary userspace breakage by adding a user namespace owner to mm_struct, captured at the time of exec, so it is clear in which user namespace CAP_SYS_PTRACE must be present in to be able to safely give read permission to the executable.

The function ptrace_may_access is modified to verify that the ptracer has CAP_SYS_ADMIN in task->mm->user_ns instead of task->cred->user_ns. This ensures that if the task changes it's cred into a subordinate user namespace it does not become ptraceable.

Cc [email protected]

Fixes 8409cca70561 ("userns: allow ptrace from non-init user namespaces")

Signed-off-by "Eric W. Biederman" <[email protected]>

---

It turns out that dumpable needs to be fixed to be user namespace aware to fix this issue. When this patch is ready I plan to place it in my userns tree and send it to Linus, hopefully for -rc2.

 include/linux/mm_types.h |  1 +
 kernel/fork.c            |  9 ++++++---
 kernel/ptrace.c          | 17 ++++++-----------
 mm/init-mm.c             |  2 ++
 4 files changed, 15 insertions(+), 14 deletions(-)

include/linux/mm_types.h +1 -0

index 4a8acedf4b7d..08d947fc4c59 100644
@@ -473,6 +473,7 @@ struct mm_struct {
 	 */
 	struct task_struct __rcu *owner;
 #endif
+	struct user_namespace *user_ns;
 
 	/* store ref to file /proc/<pid>/exe symlink points to */
 	struct file __rcu *exe_file;

kernel/fork.c +6 -3

index 623259fc794d..fd85c68c2791 100644
@@ -742,7 +742,8 @@ static void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
 #endif
 }
 
-static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p)
+static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p,
+	struct user_namespace *user_ns)
 {
 	mm->mmap = NULL;
 	mm->mm_rb = RB_ROOT;

@@ -782,6 +783,7 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p)
 	if (init_new_context(p, mm))
 		goto fail_nocontext;
 
+	mm->user_ns = get_user_ns(user_ns);
 	return mm;
 
 fail_nocontext:

@@ -827,7 +829,7 @@ struct mm_struct *mm_alloc(void)
 		return NULL;
 
 	memset(mm, 0, sizeof(*mm));
-	return mm_init(mm, current);
+	return mm_init(mm, current, current_user_ns());
 }
 
 /*

@@ -842,6 +844,7 @@ void __mmdrop(struct mm_struct *mm)
 	destroy_context(mm);
 	mmu_notifier_mm_destroy(mm);
 	check_mm(mm);
+	put_user_ns(mm->user_ns);
 	free_mm(mm);
 }
 EXPORT_SYMBOL_GPL(__mmdrop);

@@ -1123,7 +1126,7 @@ static struct mm_struct *dup_mm(struct task_struct *tsk)
 
 	memcpy(mm, oldmm, sizeof(*mm));
 
-	if (!mm_init(mm, tsk))
+	if (!mm_init(mm, tsk, mm->user_ns))
 		goto fail_nomem;
 
 	err = dup_mmap(mm, oldmm);

kernel/ptrace.c +6 -11

index 2a99027312a6..f2d1b9afb3f8 100644
@@ -220,7 +220,7 @@ static int ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
 static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
 {
 	const struct cred *cred = current_cred(), *tcred;
-	int dumpable = 0;
+	struct mm_struct *mm;
 	kuid_t caller_uid;
 	kgid_t caller_gid;
 

@@ -271,16 +271,11 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
 	return -EPERM;
 ok:
 	rcu_read_unlock();
-	smp_rmb();
-	if (task->mm)
-		dumpable = get_dumpable(task->mm);
-	rcu_read_lock();
-	if (dumpable != SUID_DUMP_USER &&
-	    !ptrace_has_cap(__task_cred(task)->user_ns, mode)) {
-		rcu_read_unlock();
-		return -EPERM;
-	}
-	rcu_read_unlock();
+	mm = task->mm;
+	if (!mm ||
+	    ((get_dumpable(mm) != SUID_DUMP_USER) &&
+	     !ptrace_has_cap(mm->user_ns, mode)))
+	    return -EPERM;
 
 	return security_ptrace_access_check(task, mode);
 }

mm/init-mm.c +2 -0

index a56a851908d2..975e49f00f34 100644
@@ -6,6 +6,7 @@
 #include <linux/cpumask.h>
 
 #include <linux/atomic.h>
+#include <linux/user_namespace.h>
 #include <asm/pgtable.h>
 #include <asm/mmu.h>
 

@@ -21,5 +22,6 @@ struct mm_struct init_mm = {
 	.mmap_sem	= __RWSEM_INITIALIZER(init_mm.mmap_sem),
 	.page_table_lock =  __SPIN_LOCK_UNLOCKED(init_mm.page_table_lock),
 	.mmlist		= LIST_HEAD_INIT(init_mm.mmlist),
+	.user_ns	= &init_user_ns,
 	INIT_MM_CONTEXT(init_mm)
 };

Signature · 1 line

2.8.3

Quoted 12 lines · Eric W. Biederman

During exec dumpable is cleared if the file that is being executed is not readable by the user executing the file. A bug in ptrace_may_access allows reading the file if the executable happens to enter into a subordinate user namespace (aka clone(CLONE_NEWUSER), unshare(CLONE_NEWUSER), or setns(fd, CLONE_NEWUSER).

This problem is fixed with only necessary userspace breakage by adding a user namespace owner to mm_struct, captured at the time of exec, so it is clear in which user namespace CAP_SYS_PTRACE must be present in to be able to safely give read permission to the executable.

Quoted 4 lines

The function ptrace_may_access is modified to verify that the ptracer has CAP_SYS_ADMIN in task->mm->user_ns instead of task->cred->user_ns. This ensures that if the task changes it's cred into a subordinate user namespace it does not become ptraceable.

This looks good! Basically applies the same rules that already apply to EUID/... changes to namespace changes, and anyone entering a user namespace can now safely drop UIDs and GIDs to namespace root.

This integrates better in the existing security concept than my old patch "ptrace: being capable wrt a process requires mapped uids/gids", and it has less issues in cases where e.g. the extra privileges of an entering process are the filesystem root or so.

FWIW, if you want, you can add "Reviewed-by: Jann Horn <[email protected]>".

During exec dumpable is cleared if the file that is being executed is not readable by the user executing the file. A bug in ptrace_may_access allows reading the file if the executable happens to enter into a subordinate user namespace (aka clone(CLONE_NEWUSER), unshare(CLONE_NEWUSER), or setns(fd, CLONE_NEWUSER).

This problem is fixed with only necessary userspace breakage by adding a user namespace owner to mm_struct, captured at the time of exec, so it is clear in which user namespace CAP_SYS_PTRACE must be present in to be able to safely give read permission to the executable.

The function ptrace_may_access is modified to verify that the ptracer has CAP_SYS_ADMIN in task->mm->user_ns instead of task->cred->user_ns. This ensures that if the task changes it's cred into a subordinate user namespace it does not become ptraceable.

Cc [email protected]

Fixes 8409cca70561 ("userns: allow ptrace from non-init user namespaces")

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---

It turns out that dumpable needs to be fixed to be user namespace aware to fix this issue. When this patch is ready I plan to place it in my userns tree and send it to Linus, hopefully for -rc2.

 include/linux/mm_types.h |  1 +
 kernel/fork.c            |  9 ++++++---
 kernel/ptrace.c          | 17 ++++++-----------
 mm/init-mm.c             |  2 ++
 4 files changed, 15 insertions(+), 14 deletions(-)

include/linux/mm_types.h +1 -0

index 4a8acedf4b7d..08d947fc4c59 100644
@@ -473,6 +473,7 @@ struct mm_struct {
 	 */
 	struct task_struct __rcu *owner;
 #endif
+	struct user_namespace *user_ns;
 
 	/* store ref to file /proc/<pid>/exe symlink points to */
 	struct file __rcu *exe_file;

kernel/fork.c +6 -3

index 623259fc794d..fd85c68c2791 100644
@@ -742,7 +742,8 @@ static void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
 #endif
 }
 
-static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p)
+static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p,
+	struct user_namespace *user_ns)
 {
 	mm->mmap = NULL;
 	mm->mm_rb = RB_ROOT;

@@ -782,6 +783,7 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p)
 	if (init_new_context(p, mm))
 		goto fail_nocontext;
 
+	mm->user_ns = get_user_ns(user_ns);
 	return mm;
 
 fail_nocontext:

@@ -827,7 +829,7 @@ struct mm_struct *mm_alloc(void)
 		return NULL;
 
 	memset(mm, 0, sizeof(*mm));
-	return mm_init(mm, current);
+	return mm_init(mm, current, current_user_ns());
 }
 
 /*

@@ -842,6 +844,7 @@ void __mmdrop(struct mm_struct *mm)
 	destroy_context(mm);
 	mmu_notifier_mm_destroy(mm);
 	check_mm(mm);
+	put_user_ns(mm->user_ns);
 	free_mm(mm);
 }
 EXPORT_SYMBOL_GPL(__mmdrop);

@@ -1123,7 +1126,7 @@ static struct mm_struct *dup_mm(struct task_struct *tsk)
 
 	memcpy(mm, oldmm, sizeof(*mm));
 
-	if (!mm_init(mm, tsk))
+	if (!mm_init(mm, tsk, mm->user_ns))
 		goto fail_nomem;
 
 	err = dup_mmap(mm, oldmm);

kernel/ptrace.c +6 -11

index 2a99027312a6..f2d1b9afb3f8 100644
@@ -220,7 +220,7 @@ static int ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
 static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
 {
 	const struct cred *cred = current_cred(), *tcred;
-	int dumpable = 0;
+	struct mm_struct *mm;
 	kuid_t caller_uid;
 	kgid_t caller_gid;
 

@@ -271,16 +271,11 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
 	return -EPERM;
 ok:
 	rcu_read_unlock();
-	smp_rmb();
-	if (task->mm)
-		dumpable = get_dumpable(task->mm);
-	rcu_read_lock();
-	if (dumpable != SUID_DUMP_USER &&
-	    !ptrace_has_cap(__task_cred(task)->user_ns, mode)) {
-		rcu_read_unlock();
-		return -EPERM;
-	}
-	rcu_read_unlock();
+	mm = task->mm;
+	if (!mm ||
+	    ((get_dumpable(mm) != SUID_DUMP_USER) &&
+	     !ptrace_has_cap(mm->user_ns, mode)))
+	    return -EPERM;
 
 	return security_ptrace_access_check(task, mode);
 }

mm/init-mm.c +2 -0

index a56a851908d2..975e49f00f34 100644
@@ -6,6 +6,7 @@
 #include <linux/cpumask.h>
 
 #include <linux/atomic.h>
+#include <linux/user_namespace.h>
 #include <asm/pgtable.h>
 #include <asm/mmu.h>
 

@@ -21,5 +22,6 @@ struct mm_struct init_mm = {
 	.mmap_sem	= __RWSEM_INITIALIZER(init_mm.mmap_sem),
 	.page_table_lock =  __SPIN_LOCK_UNLOCKED(init_mm.page_table_lock),
 	.mmlist		= LIST_HEAD_INIT(init_mm.mmlist),
+	.user_ns	= &init_user_ns,
 	INIT_MM_CONTEXT(init_mm)
 };

Signature · 1 line

2.8.3

During exec dumpable is cleared if the file that is being executed is not readable by the user executing the file. A bug in ptrace_may_access allows reading the file if the executable happens to enter into a subordinate user namespace (aka clone(CLONE_NEWUSER), unshare(CLONE_NEWUSER), or setns(fd, CLONE_NEWUSER).

This problem is fixed with only necessary userspace breakage by adding a user namespace owner to mm_struct, captured at the time of exec, so it is clear in which user namespace CAP_SYS_PTRACE must be present in to be able to safely give read permission to the executable.

The function ptrace_may_access is modified to verify that the ptracer has CAP_SYS_ADMIN in task->mm->user_ns instead of task->cred->user_ns. This ensures that if the task changes it's cred into a subordinate user namespace it does not become ptraceable.

Cc [email protected]

Fixes 8409cca70561 ("userns: allow ptrace from non-init user namespaces")

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---

It turns out that dumpable needs to be fixed to be user namespace aware to fix this issue. When this patch is ready I plan to place it in my userns tree and send it to Linus, hopefully for -rc2.

 include/linux/mm_types.h |  1 +
 kernel/fork.c            |  9 ++++++---
 kernel/ptrace.c          | 17 ++++++-----------
 mm/init-mm.c             |  2 ++
 4 files changed, 15 insertions(+), 14 deletions(-)

include/linux/mm_types.h +1 -0

index 4a8acedf4b7d..08d947fc4c59 100644
@@ -473,6 +473,7 @@ struct mm_struct {
 	 */
 	struct task_struct __rcu *owner;
 #endif
+	struct user_namespace *user_ns;
 
 	/* store ref to file /proc/<pid>/exe symlink points to */
 	struct file __rcu *exe_file;

kernel/fork.c +6 -3

index 623259fc794d..fd85c68c2791 100644
@@ -742,7 +742,8 @@ static void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
 #endif
 }
 
-static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p)
+static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p,
+	struct user_namespace *user_ns)
 {
 	mm->mmap = NULL;
 	mm->mm_rb = RB_ROOT;

@@ -782,6 +783,7 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p)
 	if (init_new_context(p, mm))
 		goto fail_nocontext;
 
+	mm->user_ns = get_user_ns(user_ns);
 	return mm;
 
 fail_nocontext:

@@ -827,7 +829,7 @@ struct mm_struct *mm_alloc(void)
 		return NULL;
 
 	memset(mm, 0, sizeof(*mm));
-	return mm_init(mm, current);
+	return mm_init(mm, current, current_user_ns());
 }
 
 /*

@@ -842,6 +844,7 @@ void __mmdrop(struct mm_struct *mm)
 	destroy_context(mm);
 	mmu_notifier_mm_destroy(mm);
 	check_mm(mm);
+	put_user_ns(mm->user_ns);
 	free_mm(mm);
 }
 EXPORT_SYMBOL_GPL(__mmdrop);

@@ -1123,7 +1126,7 @@ static struct mm_struct *dup_mm(struct task_struct *tsk)
 
 	memcpy(mm, oldmm, sizeof(*mm));
 
-	if (!mm_init(mm, tsk))
+	if (!mm_init(mm, tsk, mm->user_ns))
 		goto fail_nomem;
 
 	err = dup_mmap(mm, oldmm);

kernel/ptrace.c +6 -11

index 2a99027312a6..f2d1b9afb3f8 100644
@@ -220,7 +220,7 @@ static int ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
 static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
 {
 	const struct cred *cred = current_cred(), *tcred;
-	int dumpable = 0;
+	struct mm_struct *mm;
 	kuid_t caller_uid;
 	kgid_t caller_gid;
 

@@ -271,16 +271,11 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
 	return -EPERM;
 ok:
 	rcu_read_unlock();
-	smp_rmb();
-	if (task->mm)
-		dumpable = get_dumpable(task->mm);
-	rcu_read_lock();
-	if (dumpable != SUID_DUMP_USER &&
-	    !ptrace_has_cap(__task_cred(task)->user_ns, mode)) {
-		rcu_read_unlock();
-		return -EPERM;
-	}
-	rcu_read_unlock();
+	mm = task->mm;
+	if (!mm ||
+	    ((get_dumpable(mm) != SUID_DUMP_USER) &&
+	     !ptrace_has_cap(mm->user_ns, mode)))
+	    return -EPERM;
 
 	return security_ptrace_access_check(task, mode);
 }

mm/init-mm.c +2 -0

index a56a851908d2..975e49f00f34 100644
@@ -6,6 +6,7 @@
 #include <linux/cpumask.h>
 
 #include <linux/atomic.h>
+#include <linux/user_namespace.h>
 #include <asm/pgtable.h>
 #include <asm/mmu.h>
 

@@ -21,5 +22,6 @@ struct mm_struct init_mm = {
 	.mmap_sem	= __RWSEM_INITIALIZER(init_mm.mmap_sem),
 	.page_table_lock =  __SPIN_LOCK_UNLOCKED(init_mm.page_table_lock),
 	.mmlist		= LIST_HEAD_INIT(init_mm.mmlist),
+	.user_ns	= &init_user_ns,
 	INIT_MM_CONTEXT(init_mm)
 };

Signature · 7 lines

2.8.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

During exec dumpable is cleared if the file that is being executed is not readable by the user executing the file. A bug in ptrace_may_access allows reading the file if the executable happens to enter into a subordinate user namespace (aka clone(CLONE_NEWUSER), unshare(CLONE_NEWUSER), or setns(fd, CLONE_NEWUSER).

This problem is fixed with only necessary userspace breakage by adding a user namespace owner to mm_struct, captured at the time of exec, so it is clear in which user namespace CAP_SYS_PTRACE must be present in to be able to safely give read permission to the executable.

The function ptrace_may_access is modified to verify that the ptracer has CAP_SYS_ADMIN in task->mm->user_ns instead of task->cred->user_ns. This ensures that if the task changes it's cred into a subordinate user namespace it does not become ptraceable.

Cc [email protected]

Fixes 8409cca70561 ("userns: allow ptrace from non-init user namespaces")

Signed-off-by "Eric W. Biederman" <[email protected]>

---

It turns out that dumpable needs to be fixed to be user namespace aware to fix this issue. When this patch is ready I plan to place it in my userns tree and send it to Linus, hopefully for -rc2.

 include/linux/mm_types.h |  1 +
 kernel/fork.c            |  9 ++++++---
 kernel/ptrace.c          | 17 ++++++-----------
 mm/init-mm.c             |  2 ++
 4 files changed, 15 insertions(+), 14 deletions(-)

include/linux/mm_types.h +1 -0

index 4a8acedf4b7d..08d947fc4c59 100644
@@ -473,6 +473,7 @@ struct mm_struct {
 	 */
 	struct task_struct __rcu *owner;
 #endif
+	struct user_namespace *user_ns;
 
 	/* store ref to file /proc/<pid>/exe symlink points to */
 	struct file __rcu *exe_file;

kernel/fork.c +6 -3

index 623259fc794d..fd85c68c2791 100644
@@ -742,7 +742,8 @@ static void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
 #endif
 }
 
-static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p)
+static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p,
+	struct user_namespace *user_ns)
 {
 	mm->mmap = NULL;
 	mm->mm_rb = RB_ROOT;

@@ -782,6 +783,7 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p)
 	if (init_new_context(p, mm))
 		goto fail_nocontext;
 
+	mm->user_ns = get_user_ns(user_ns);
 	return mm;
 
 fail_nocontext:

@@ -827,7 +829,7 @@ struct mm_struct *mm_alloc(void)
 		return NULL;
 
 	memset(mm, 0, sizeof(*mm));
-	return mm_init(mm, current);
+	return mm_init(mm, current, current_user_ns());
 }
 
 /*

@@ -842,6 +844,7 @@ void __mmdrop(struct mm_struct *mm)
 	destroy_context(mm);
 	mmu_notifier_mm_destroy(mm);
 	check_mm(mm);
+	put_user_ns(mm->user_ns);
 	free_mm(mm);
 }
 EXPORT_SYMBOL_GPL(__mmdrop);

@@ -1123,7 +1126,7 @@ static struct mm_struct *dup_mm(struct task_struct *tsk)
 
 	memcpy(mm, oldmm, sizeof(*mm));
 
-	if (!mm_init(mm, tsk))
+	if (!mm_init(mm, tsk, mm->user_ns))
 		goto fail_nomem;
 
 	err = dup_mmap(mm, oldmm);

kernel/ptrace.c +6 -11

index 2a99027312a6..f2d1b9afb3f8 100644
@@ -220,7 +220,7 @@ static int ptrace_has_cap(struct user_namespace *ns, unsigned int mode)
 static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
 {
 	const struct cred *cred = current_cred(), *tcred;
-	int dumpable = 0;
+	struct mm_struct *mm;
 	kuid_t caller_uid;
 	kgid_t caller_gid;
 

@@ -271,16 +271,11 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
 	return -EPERM;
 ok:
 	rcu_read_unlock();
-	smp_rmb();
-	if (task->mm)
-		dumpable = get_dumpable(task->mm);
-	rcu_read_lock();
-	if (dumpable != SUID_DUMP_USER &&
-	    !ptrace_has_cap(__task_cred(task)->user_ns, mode)) {
-		rcu_read_unlock();
-		return -EPERM;
-	}
-	rcu_read_unlock();
+	mm = task->mm;
+	if (!mm ||
+	    ((get_dumpable(mm) != SUID_DUMP_USER) &&
+	     !ptrace_has_cap(mm->user_ns, mode)))
+	    return -EPERM;
 
 	return security_ptrace_access_check(task, mode);
 }

mm/init-mm.c +2 -0

index a56a851908d2..975e49f00f34 100644
@@ -6,6 +6,7 @@
 #include <linux/cpumask.h>
 
 #include <linux/atomic.h>
+#include <linux/user_namespace.h>
 #include <asm/pgtable.h>
 #include <asm/mmu.h>
 

@@ -21,5 +22,6 @@ struct mm_struct init_mm = {
 	.mmap_sem	= __RWSEM_INITIALIZER(init_mm.mmap_sem),
 	.page_table_lock =  __SPIN_LOCK_UNLOCKED(init_mm.page_table_lock),
 	.mmlist		= LIST_HEAD_INIT(init_mm.mmlist),
+	.user_ns	= &init_user_ns,
 	INIT_MM_CONTEXT(init_mm)
 };

Signature · 7 lines

2.8.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Quoted 12 lines · Eric W. Biederman

During exec dumpable is cleared if the file that is being executed is not readable by the user executing the file. A bug in ptrace_may_access allows reading the file if the executable happens to enter into a subordinate user namespace (aka clone(CLONE_NEWUSER), unshare(CLONE_NEWUSER), or setns(fd, CLONE_NEWUSER).

This problem is fixed with only necessary userspace breakage by adding a user namespace owner to mm_struct, captured at the time of exec, so it is clear in which user namespace CAP_SYS_PTRACE must be present in to be able to safely give read permission to the executable.

Quoted 4 lines

The function ptrace_may_access is modified to verify that the ptracer has CAP_SYS_ADMIN in task->mm->user_ns instead of task->cred->user_ns. This ensures that if the task changes it's cred into a subordinate user namespace it does not become ptraceable.

I haven't studied your patch too deeply but one thing that immediately raised a red flag was that mm might be shared between processes (aka thread groups).

What prevents those two to sit in different user
namespaces?

Quoted 12 lines · Eric W. Biederman

During exec dumpable is cleared if the file that is being executed is not readable by the user executing the file. A bug in ptrace_may_access allows reading the file if the executable happens to enter into a subordinate user namespace (aka clone(CLONE_NEWUSER), unshare(CLONE_NEWUSER), or setns(fd, CLONE_NEWUSER).

This problem is fixed with only necessary userspace breakage by adding a user namespace owner to mm_struct, captured at the time of exec, so it is clear in which user namespace CAP_SYS_PTRACE must be present in to be able to safely give read permission to the executable.

Quoted 4 lines

The function ptrace_may_access is modified to verify that the ptracer has CAP_SYS_ADMIN in task->mm->user_ns instead of task->cred->user_ns. This ensures that if the task changes it's cred into a subordinate user namespace it does not become ptraceable.

I haven't studied your patch too deeply but one thing that immediately raised a red flag was that mm might be shared between processes (aka thread groups). What prevents those two to sit in different user namespaces?

I am primarily asking because this generated a lot of headache for the memcg handling as those processes might sit in different cgroups while there is only one correct memcg for them which can disagree with the cgroup associated with one of the processes.

Quoted 23 lines · Michal Hocko

Quoted 12 lines · Eric W. Biederman

During exec dumpable is cleared if the file that is being executed is not readable by the user executing the file. A bug in ptrace_may_access allows reading the file if the executable happens to enter into a subordinate user namespace (aka clone(CLONE_NEWUSER), unshare(CLONE_NEWUSER), or setns(fd, CLONE_NEWUSER).

This problem is fixed with only necessary userspace breakage by adding a user namespace owner to mm_struct, captured at the time of exec, so it is clear in which user namespace CAP_SYS_PTRACE must be present in to be able to safely give read permission to the executable.

Quoted 4 lines

The function ptrace_may_access is modified to verify that the ptracer has CAP_SYS_ADMIN in task->mm->user_ns instead of task->cred->user_ns. This ensures that if the task changes it's cred into a subordinate user namespace it does not become ptraceable.

I haven't studied your patch too deeply but one thing that immediately raised a red flag was that mm might be shared between processes (aka thread groups). What prevents those two to sit in different user namespaces?

Quoted 4 lines

I am primarily asking because this generated a lot of headache for the memcg handling as those processes might sit in different cgroups while there is only one correct memcg for them which can disagree with the cgroup associated with one of the processes.

That is a legitimate concern, but I do not see any of those kinds of issues here.

Part of the memcg pain comes from the fact that control groups are process centric, and part of the pain comes from the fact that it is possible to change control groups. What I am doing is making the mm owned by a user namespace (at creation time), and I am not allowing changes to that ownership. The credentials of the tasks that use that mm may be in the same user namespace or descendent user namespaces.

The core goal is to enforce the unreadability of an mm when an non-readable file is executed. This is a time of mm creation property. The enforcement of which fits very well with the security/permission checking role of the user namespace.

Quoted 40 lines · Eric W. Biederman

Quoted 23 lines · Michal Hocko

Quoted 12 lines · Eric W. Biederman

During exec dumpable is cleared if the file that is being executed is not readable by the user executing the file. A bug in ptrace_may_access allows reading the file if the executable happens to enter into a subordinate user namespace (aka clone(CLONE_NEWUSER), unshare(CLONE_NEWUSER), or setns(fd, CLONE_NEWUSER).

This problem is fixed with only necessary userspace breakage by adding a user namespace owner to mm_struct, captured at the time of exec, so it is clear in which user namespace CAP_SYS_PTRACE must be present in to be able to safely give read permission to the executable.

Quoted 4 lines

The function ptrace_may_access is modified to verify that the ptracer has CAP_SYS_ADMIN in task->mm->user_ns instead of task->cred->user_ns. This ensures that if the task changes it's cred into a subordinate user namespace it does not become ptraceable.

I haven't studied your patch too deeply but one thing that immediately raised a red flag was that mm might be shared between processes (aka thread groups). What prevents those two to sit in different user namespaces?

Quoted 4 lines

I am primarily asking because this generated a lot of headache for the memcg handling as those processes might sit in different cgroups while there is only one correct memcg for them which can disagree with the cgroup associated with one of the processes.

That is a legitimate concern, but I do not see any of those kinds of issues here.

Part of the memcg pain comes from the fact that control groups are process centric, and part of the pain comes from the fact that it is possible to change control groups. What I am doing is making the mm owned by a user namespace (at creation time), and I am not allowing changes to that ownership. The credentials of the tasks that use that mm may be in the same user namespace or descendent user namespaces.

Quoted 4 lines

The core goal is to enforce the unreadability of an mm when an non-readable file is executed. This is a time of mm creation property. The enforcement of which fits very well with the security/permission checking role of the user namespace.

How is that going to work? I thought the core goal was better security for entering containers.

If I want to dump a non-readable file, afaik, I can just make a new user namespace, then run the file in there and dump its memory.
I guess you could fix that by entirely prohibiting the execution of a non-readable file whose owner UID is not mapped. (Adding more dumping restrictions wouldn't help much because you could still e.g. supply a malicious dynamic linker if you control the mount namespace.)

Quoted 46 lines · Jann Horn

Quoted 44 lines · Eric W. Biederman

Quoted 23 lines · Michal Hocko

Quoted 12 lines · Eric W. Biederman

During exec dumpable is cleared if the file that is being executed is not readable by the user executing the file. A bug in ptrace_may_access allows reading the file if the executable happens to enter into a subordinate user namespace (aka clone(CLONE_NEWUSER), unshare(CLONE_NEWUSER), or setns(fd, CLONE_NEWUSER).

This problem is fixed with only necessary userspace breakage by adding a user namespace owner to mm_struct, captured at the time of exec, so it is clear in which user namespace CAP_SYS_PTRACE must be present in to be able to safely give read permission to the executable.

Quoted 4 lines

The function ptrace_may_access is modified to verify that the ptracer has CAP_SYS_ADMIN in task->mm->user_ns instead of task->cred->user_ns. This ensures that if the task changes it's cred into a subordinate user namespace it does not become ptraceable.

I haven't studied your patch too deeply but one thing that immediately raised a red flag was that mm might be shared between processes (aka thread groups). What prevents those two to sit in different user namespaces?

Quoted 4 lines

I am primarily asking because this generated a lot of headache for the memcg handling as those processes might sit in different cgroups while there is only one correct memcg for them which can disagree with the cgroup associated with one of the processes.

That is a legitimate concern, but I do not see any of those kinds of issues here.

Part of the memcg pain comes from the fact that control groups are process centric, and part of the pain comes from the fact that it is possible to change control groups. What I am doing is making the mm owned by a user namespace (at creation time), and I am not allowing changes to that ownership. The credentials of the tasks that use that mm may be in the same user namespace or descendent user namespaces.

The core goal is to enforce the unreadability of an mm when an non-readable file is executed. This is a time of mm creation property. The enforcement of which fits very well with the security/permission checking role of the user namespace.

Quoted 2 lines

How is that going to work? I thought the core goal was better security for entering containers.

The better security when entering containers came from fixing the the check for unreadable files. Because that is fundamentally what the mm dumpable settings are for.

Quoted 6 lines

If I want to dump a non-readable file, afaik, I can just make a new user namespace, then run the file in there and dump its memory.
I guess you could fix that by entirely prohibiting the execution of a non-readable file whose owner UID is not mapped. (Adding more dumping restrictions wouldn't help much because you could still e.g. supply a malicious dynamic linker if you control the mount namespace.)

That seems to be a part of this puzzle I have incompletely addressed, thank you.

It looks like I need to change either the owning user namespace or fail the exec. Malicious dynamic linkers are doubly interesting.

As mount name spaces are also owned if I have privileges I can address the possibility of a malicious dynamic linker that way. AKA who cares about the link if the owner of the mount namespace has permissions to read the file.

I am going to look at failing the exec if the owning user namespace of the mm would not have permissions to read the file. That should just be a couple of lines of code and easy to maintain. Plus it does not appear that non-readable executables are particularly common.

Quoted 48 lines · Eric W. Biederman

Quoted 46 lines · Jann Horn

Quoted 44 lines · Eric W. Biederman

Quoted 23 lines · Michal Hocko

Quoted 12 lines · Eric W. Biederman

During exec dumpable is cleared if the file that is being executed is not readable by the user executing the file. A bug in ptrace_may_access allows reading the file if the executable happens to enter into a subordinate user namespace (aka clone(CLONE_NEWUSER), unshare(CLONE_NEWUSER), or setns(fd, CLONE_NEWUSER).

This problem is fixed with only necessary userspace breakage by adding a user namespace owner to mm_struct, captured at the time of exec, so it is clear in which user namespace CAP_SYS_PTRACE must be present in to be able to safely give read permission to the executable.

Quoted 4 lines

The function ptrace_may_access is modified to verify that the ptracer has CAP_SYS_ADMIN in task->mm->user_ns instead of task->cred->user_ns. This ensures that if the task changes it's cred into a subordinate user namespace it does not become ptraceable.

I haven't studied your patch too deeply but one thing that immediately raised a red flag was that mm might be shared between processes (aka thread groups). What prevents those two to sit in different user namespaces?

Quoted 4 lines

I am primarily asking because this generated a lot of headache for the memcg handling as those processes might sit in different cgroups while there is only one correct memcg for them which can disagree with the cgroup associated with one of the processes.

That is a legitimate concern, but I do not see any of those kinds of issues here.

Part of the memcg pain comes from the fact that control groups are process centric, and part of the pain comes from the fact that it is possible to change control groups. What I am doing is making the mm owned by a user namespace (at creation time), and I am not allowing changes to that ownership. The credentials of the tasks that use that mm may be in the same user namespace or descendent user namespaces.

The core goal is to enforce the unreadability of an mm when an non-readable file is executed. This is a time of mm creation property. The enforcement of which fits very well with the security/permission checking role of the user namespace.

Quoted 6 lines

Quoted 2 lines

How is that going to work? I thought the core goal was better security for entering containers.

The better security when entering containers came from fixing the the check for unreadable files. Because that is fundamentally what the mm dumpable settings are for.

Oh, interesting.

Quoted 10 lines

Quoted 6 lines

If I want to dump a non-readable file, afaik, I can just make a new user namespace, then run the file in there and dump its memory.
I guess you could fix that by entirely prohibiting the execution of a non-readable file whose owner UID is not mapped. (Adding more dumping restrictions wouldn't help much because you could still e.g. supply a malicious dynamic linker if you control the mount namespace.)

That seems to be a part of this puzzle I have incompletely addressed, thank you.

Quoted 7 lines

It looks like I need to change either the owning user namespace or fail the exec. Malicious dynamic linkers are doubly interesting.

As mount name spaces are also owned if I have privileges I can address the possibility of a malicious dynamic linker that way. AKA who cares about the link if the owner of the mount namespace has permissions to read the file.

If you just check the owner of the mount namespace, someone could still use a user namespace to chroot() the process. That should also be sufficient to get the evil linker in. I think it really needs to be the user namespace of the executing process that's checked, not the user namespace associated with some mount namespace.

Quoted 4 lines

I am going to look at failing the exec if the owning user namespace of the mm would not have permissions to read the file. That should just be a couple of lines of code and easy to maintain. Plus it does not appear that non-readable executables are particularly common.

Hm. Yeah, I guess mode 04111 probably isn't sooo common. From a security perspective, I think that should work.

When the user namespace support was merged the need to prevent ptracing an executable that is not readable was overlooked.

Correct this oversight by not letting exec succeed if during exec an executable is not readable and the current user namespace capabilities do not apply to the executable's file.

While it happens that distros install some files setuid and non-readable I have not found any executable files just installed non-readalbe. Executables that are setuid to a user not mapped in a user namespace are worthless, so I don't expect this to introduce any problems in practice.

There may be a way to allow this execution to happen by setting mm->user_ns to a more privileged user namespace and watching out for the possibility of using dynamic linkers or other shared libraries that the kernel loads into the mm to bypass the read-only restriction. But the analysis is more difficult and it would require more code churn so I don't think the effort is worth it.

Cc [email protected]

Reported-by Jann Horn <[email protected]>

Fixes 9e4a36ece652 ("userns: Fail exec for suid and sgid binaries with ids outside our user namespace.")

Signed-off-by "Eric W. Biederman" <[email protected]>

---

Tossing this out for review in case I missed something silly but this patch seems pretty trivial.

 arch/x86/ia32/ia32_aout.c |  4 +++-
 fs/binfmt_aout.c          |  4 +++-
 fs/binfmt_elf.c           |  4 +++-
 fs/binfmt_elf_fdpic.c     |  4 +++-
 fs/binfmt_flat.c          |  4 +++-
 fs/exec.c                 | 19 ++++++++++++++++---
 include/linux/binfmts.h   |  6 +++++-
 7 files changed, 36 insertions(+), 9 deletions(-)

arch/x86/ia32/ia32_aout.c +3 -1

index cb26f18d43af..7ad20dedd929 100644
@@ -294,7 +294,9 @@ static int load_aout_binary(struct linux_binprm *bprm)
        set_personality(PER_LINUX);
        set_personality_ia32(false);
 
-       setup_new_exec(bprm);
+       retval = setup_new_exec(bprm);
+       if (retval)
+               return retval;
 
        regs->cs = __USER32_CS;
        regs->r8 = regs->r9 = regs->r10 = regs->r11 = regs->r12 =

fs/binfmt_aout.c +3 -1

index ae1b5404fced..b7b8aa03ccd0 100644
@@ -242,7 +242,9 @@ static int load_aout_binary(struct linux_binprm * bprm)
 #else
        set_personality(PER_LINUX);
 #endif
-       setup_new_exec(bprm);
+       retval = setup_new_exec(bprm);
+       if (retval)
+               return retval;
 
        current->mm->end_code = ex.a_text +
                (current->mm->start_code = N_TXTADDR(ex));

fs/binfmt_elf.c +3 -1

index 2472af2798c7..423fece0b8c4 100644
@@ -852,7 +852,9 @@ static int load_elf_binary(struct linux_binprm *bprm)
        if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
                current->flags |= PF_RANDOMIZE;
 
-       setup_new_exec(bprm);
+       retval = setup_new_exec(bprm);
+       if (retval)
+               goto out_free_dentry;
        install_exec_creds(bprm);
 
        /* Do this so that we can load the interpreter, if need be.  We will

fs/binfmt_elf_fdpic.c +3 -1

index 464a972e88c1..d3099caff96d 100644
@@ -352,7 +352,9 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
        if (elf_read_implies_exec(&exec_params.hdr, executable_stack))
                current->personality |= READ_IMPLIES_EXEC;
 
-       setup_new_exec(bprm);
+       retval = setup_new_exec(bprm);
+       if (retval)
+               goto error;
 
        set_binfmt(&elf_fdpic_format);
 

fs/binfmt_flat.c +3 -1

index 9b2917a30294..25ca68940ad4 100644
@@ -524,7 +524,9 @@ static int load_flat_file(struct linux_binprm *bprm,
 
                /* OK, This is the point of no return */
                set_personality(PER_LINUX_32BIT);
-               setup_new_exec(bprm);
+               ret = setup_new_exec(bprm);
+               if (ret)
+                       goto err;
        }
 
        /*

fs/exec.c +0 -1

index 6fcfb3f7b137..f724ed94ba7a 100644
@@ -1270,12 +1270,21 @@ EXPORT_SYMBOL(flush_old_exec);
 
 void would_dump(struct linux_binprm *bprm, struct file *file)
 {
-       if (inode_permission(file_inode(file), MAY_READ) < 0)

fs/exec.c +7 -2

+       }
 }
 EXPORT_SYMBOL(would_dump);
 
-void setup_new_exec(struct linux_binprm * bprm)
+int setup_new_exec(struct linux_binprm * bprm)
 {
        arch_pick_mmap_layout(current->mm);
 

@@ -1296,12 +1305,15 @@ void setup_new_exec(struct linux_binprm * bprm)
         */
        current->mm->task_size = TASK_SIZE;
 
+       would_dump(bprm, bprm->file);
+       if (bprm->interp_flags & BINPRM_FLAGS_EXEC_INACCESSIBLE)
+               return -EPERM;
+
        /* install the new credentials */
        if (!uid_eq(bprm->cred->uid, current_euid()) ||
            !gid_eq(bprm->cred->gid, current_egid())) {
                current->pdeath_signal = 0;
        } else {
-               would_dump(bprm, bprm->file);
                if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP)
                        set_dumpable(current->mm, suid_dumpable);
        }

@@ -1311,6 +1323,7 @@ void setup_new_exec(struct linux_binprm * bprm)
        current->self_exec_id++;
        flush_signal_handlers(current, 0);
        do_close_on_exec(current->files);
+       return 0;
 }
 EXPORT_SYMBOL(setup_new_exec);
 

include/linux/binfmts.h +5 -2

index 1303b570b18c..8e5fb9eca2ee 100644
@@ -57,6 +57,10 @@ struct linux_binprm {
 #define BINPRM_FLAGS_PATH_INACCESSIBLE_BIT 2
 #define BINPRM_FLAGS_PATH_INACCESSIBLE (1 << BINPRM_FLAGS_PATH_INACCESSIBLE_BIT)
 
+/* executable is inaccessible for performing exec */
+#define BINPRM_FLAGS_EXEC_INACCESSIBLE_BIT 3
+#define BINPRM_FLAGS_EXEC_INACCESSIBLE (1 << BINPRM_FLAGS_EXEC_INACCESSIBLE_BIT)
+
 /* Function parameter for binfmt->coredump */
 struct coredump_params {
        const siginfo_t *siginfo;

@@ -100,7 +104,7 @@ extern int prepare_binprm(struct linux_binprm *);
 extern int __must_check remove_arg_zero(struct linux_binprm *);
 extern int search_binary_handler(struct linux_binprm *);
 extern int flush_old_exec(struct linux_binprm * bprm);
-extern void setup_new_exec(struct linux_binprm * bprm);
+extern int setup_new_exec(struct linux_binprm * bprm);
 extern void would_dump(struct linux_binprm *, struct file *);
 
 extern int suid_dumpable;
--

2.8.3

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 15 lines

fs/exec.c +5 -1

index 6fcfb3f7b137..f724ed94ba7a 100644
@@ -1270,12 +1270,21 @@ EXPORT_SYMBOL(flush_old_exec);
 
 void would_dump(struct linux_binprm *bprm, struct file *file)
 {
-       if (inode_permission(file_inode(file), MAY_READ) < 0)
+       struct inode *inode = file_inode(file);
+       if (inode_permission(inode, MAY_READ) < 0) {
+               struct user_namespace *user_ns = current->mm->user_ns;
                bprm->interp_flags |= BINPRM_FLAGS_ENFORCE_NONDUMP;
+
+               /* May the user_ns root read the executable? */

fs/exec.c EXPORT_SYMBOL(flush_old_exec); · line 1282

Quoted 4 lines

fs/exec.c +4 -0

+               if (!kuid_has_mapping(user_ns, inode->i_uid) ||
+                   !kgid_has_mapping(user_ns, inode->i_gid)) {
+                       bprm->interp_flags |= BINPRM_FLAGS_EXEC_INACCESSIBLE;
+               }

This feels like it should belong inside inode_permission(file_inode(file), MAY_EXEC)
which hopefully should be checked long before getting here??

Quoted 15 lines · Amir Goldstein

Quoted 15 lines

fs/exec.c +5 -1

index 6fcfb3f7b137..f724ed94ba7a 100644
@@ -1270,12 +1270,21 @@ EXPORT_SYMBOL(flush_old_exec);
 
 void would_dump(struct linux_binprm *bprm, struct file *file)
 {
-       if (inode_permission(file_inode(file), MAY_READ) < 0)
+       struct inode *inode = file_inode(file);
+       if (inode_permission(inode, MAY_READ) < 0) {
+               struct user_namespace *user_ns = current->mm->user_ns;
                bprm->interp_flags |= BINPRM_FLAGS_ENFORCE_NONDUMP;
+
+               /* May the user_ns root read the executable? */

Quoted 8 lines

fs/exec.c EXPORT_SYMBOL(flush_old_exec); · line 1282

Quoted 4 lines

fs/exec.c +4 -0

+               if (!kuid_has_mapping(user_ns, inode->i_uid) ||
+                   !kgid_has_mapping(user_ns, inode->i_gid)) {
+                       bprm->interp_flags |= BINPRM_FLAGS_EXEC_INACCESSIBLE;
+               }

This feels like it should belong inside inode_permission(file_inode(file), MAY_EXEC)
which hopefully should be checked long before getting here??

It is the active ingredient in capable_wrt_inode_uidgid and is indeed inside of inode_permission.

What I am testing for here is if I have a process with a full set of capabilities in current->mm->user_ns will the inode be readable.

I can see an argument for calling prepare_creds stuffing the new cred full of capabilities. Calling override_cred. Calling inode_permission, restoring the credentials. But it seems very much like overkill and more error prone because of the more code involved.

So I have done the simple thing that doesn't hide what is really going on.

Quoted 15 lines

fs/exec.c +5 -1

index 6fcfb3f7b137..f724ed94ba7a 100644
@@ -1270,12 +1270,21 @@ EXPORT_SYMBOL(flush_old_exec);
 
 void would_dump(struct linux_binprm *bprm, struct file *file)
 {
-       if (inode_permission(file_inode(file), MAY_READ) < 0)
+       struct inode *inode = file_inode(file);
+       if (inode_permission(inode, MAY_READ) < 0) {
+               struct user_namespace *user_ns = current->mm->user_ns;
                bprm->interp_flags |= BINPRM_FLAGS_ENFORCE_NONDUMP;
+
+               /* May the user_ns root read the executable? */

fs/exec.c EXPORT_SYMBOL(flush_old_exec); · line 1282

Quoted 4 lines

fs/exec.c +4 -0

+               if (!kuid_has_mapping(user_ns, inode->i_uid) ||
+                   !kgid_has_mapping(user_ns, inode->i_gid)) {
+                       bprm->interp_flags |= BINPRM_FLAGS_EXEC_INACCESSIBLE;
+               }

This feels like it should belong inside inode_permission(file_inode(file), MAY_EXEC)
which hopefully should be checked long before getting here??

When the user namespace support was merged the need to prevent ptracing an executable that is not readable was overlooked.

Correct this oversight by not letting exec succeed if during exec an executable is not readable and the current user namespace capabilities do not apply to the executable's file.

While it happens that distros install some files setuid and non-readable I have not found any executable files just installed non-readalbe. Executables that are setuid to a user not mapped in a user namespace are worthless, so I don't expect this to introduce any problems in practice.

There may be a way to allow this execution to happen by setting mm->user_ns to a more privileged user namespace and watching out for the possibility of using dynamic linkers or other shared libraries that the kernel loads into the mm to bypass the read-only restriction. But the analysis is more difficult and it would require more code churn so I don't think the effort is worth it.

Cc [email protected]

Reported-by Jann Horn <[email protected]>

Fixes 9e4a36ece652 ("userns: Fail exec for suid and sgid binaries with ids outside our user namespace.")

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---

Tossing this out for review in case I missed something silly but this patch seems pretty trivial.

 arch/x86/ia32/ia32_aout.c |  4 +++-
 fs/binfmt_aout.c          |  4 +++-
 fs/binfmt_elf.c           |  4 +++-
 fs/binfmt_elf_fdpic.c     |  4 +++-
 fs/binfmt_flat.c          |  4 +++-
 fs/exec.c                 | 19 ++++++++++++++++---
 include/linux/binfmts.h   |  6 +++++-
 7 files changed, 36 insertions(+), 9 deletions(-)

arch/x86/ia32/ia32_aout.c +3 -1

index cb26f18d43af..7ad20dedd929 100644
@@ -294,7 +294,9 @@ static int load_aout_binary(struct linux_binprm *bprm)
        set_personality(PER_LINUX);
        set_personality_ia32(false);
 
-       setup_new_exec(bprm);
+       retval = setup_new_exec(bprm);
+       if (retval)
+               return retval;
 
        regs->cs = __USER32_CS;
        regs->r8 = regs->r9 = regs->r10 = regs->r11 = regs->r12 =

fs/binfmt_aout.c +3 -1

index ae1b5404fced..b7b8aa03ccd0 100644
@@ -242,7 +242,9 @@ static int load_aout_binary(struct linux_binprm * bprm)
 #else
        set_personality(PER_LINUX);
 #endif
-       setup_new_exec(bprm);
+       retval = setup_new_exec(bprm);
+       if (retval)
+               return retval;
 
        current->mm->end_code = ex.a_text +
                (current->mm->start_code = N_TXTADDR(ex));

fs/binfmt_elf.c +3 -1

index 2472af2798c7..423fece0b8c4 100644
@@ -852,7 +852,9 @@ static int load_elf_binary(struct linux_binprm *bprm)
        if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
                current->flags |= PF_RANDOMIZE;
 
-       setup_new_exec(bprm);
+       retval = setup_new_exec(bprm);
+       if (retval)
+               goto out_free_dentry;
        install_exec_creds(bprm);
 
        /* Do this so that we can load the interpreter, if need be.  We will

fs/binfmt_elf_fdpic.c +3 -1

index 464a972e88c1..d3099caff96d 100644
@@ -352,7 +352,9 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
        if (elf_read_implies_exec(&exec_params.hdr, executable_stack))
                current->personality |= READ_IMPLIES_EXEC;
 
-       setup_new_exec(bprm);
+       retval = setup_new_exec(bprm);
+       if (retval)
+               goto error;
 
        set_binfmt(&elf_fdpic_format);
 

fs/binfmt_flat.c +3 -1

index 9b2917a30294..25ca68940ad4 100644
@@ -524,7 +524,9 @@ static int load_flat_file(struct linux_binprm *bprm,
 
                /* OK, This is the point of no return */
                set_personality(PER_LINUX_32BIT);
-               setup_new_exec(bprm);
+               ret = setup_new_exec(bprm);
+               if (ret)
+                       goto err;
        }
 
        /*

fs/exec.c +0 -1

index 6fcfb3f7b137..f724ed94ba7a 100644
@@ -1270,12 +1270,21 @@ EXPORT_SYMBOL(flush_old_exec);
 
 void would_dump(struct linux_binprm *bprm, struct file *file)
 {
-       if (inode_permission(file_inode(file), MAY_READ) < 0)

fs/exec.c +7 -2

+       }
 }
 EXPORT_SYMBOL(would_dump);
 
-void setup_new_exec(struct linux_binprm * bprm)
+int setup_new_exec(struct linux_binprm * bprm)
 {
        arch_pick_mmap_layout(current->mm);
 

@@ -1296,12 +1305,15 @@ void setup_new_exec(struct linux_binprm * bprm)
         */
        current->mm->task_size = TASK_SIZE;
 
+       would_dump(bprm, bprm->file);
+       if (bprm->interp_flags & BINPRM_FLAGS_EXEC_INACCESSIBLE)
+               return -EPERM;
+
        /* install the new credentials */
        if (!uid_eq(bprm->cred->uid, current_euid()) ||
            !gid_eq(bprm->cred->gid, current_egid())) {
                current->pdeath_signal = 0;
        } else {
-               would_dump(bprm, bprm->file);
                if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP)
                        set_dumpable(current->mm, suid_dumpable);
        }

@@ -1311,6 +1323,7 @@ void setup_new_exec(struct linux_binprm * bprm)
        current->self_exec_id++;
        flush_signal_handlers(current, 0);
        do_close_on_exec(current->files);
+       return 0;
 }
 EXPORT_SYMBOL(setup_new_exec);
 

include/linux/binfmts.h +5 -2

index 1303b570b18c..8e5fb9eca2ee 100644
@@ -57,6 +57,10 @@ struct linux_binprm {
 #define BINPRM_FLAGS_PATH_INACCESSIBLE_BIT 2
 #define BINPRM_FLAGS_PATH_INACCESSIBLE (1 << BINPRM_FLAGS_PATH_INACCESSIBLE_BIT)
 
+/* executable is inaccessible for performing exec */
+#define BINPRM_FLAGS_EXEC_INACCESSIBLE_BIT 3
+#define BINPRM_FLAGS_EXEC_INACCESSIBLE (1 << BINPRM_FLAGS_EXEC_INACCESSIBLE_BIT)
+
 /* Function parameter for binfmt->coredump */
 struct coredump_params {
        const siginfo_t *siginfo;

@@ -100,7 +104,7 @@ extern int prepare_binprm(struct linux_binprm *);
 extern int __must_check remove_arg_zero(struct linux_binprm *);
 extern int search_binary_handler(struct linux_binprm *);
 extern int flush_old_exec(struct linux_binprm * bprm);
-extern void setup_new_exec(struct linux_binprm * bprm);
+extern int setup_new_exec(struct linux_binprm * bprm);
 extern void would_dump(struct linux_binprm *, struct file *);
 
 extern int suid_dumpable;
--

2.8.3

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

When the user namespace support was merged the need to prevent ptracing an executable that is not readable was overlooked.

When the user namespace support was merged the need to prevent ptracing an executable that is not readable was overlooked.

Quoted 3 lines · Eric W. Biederman

When the user namespace support was merged the need to prevent ptracing an executable that is not readable was overlooked.

Before getting too excited about this fix, isn't there a much bigger hole that's been there forever?

Simply ptrace yourself, exec the
program, and then dump the program out. A program that really wants to be unreadable should have a stub: the stub is setuid and readable, but all the stub does is to exec the real program, and the real program should have mode 0500 or similar.

ISTM the "right" check would be to enforce that the program's new creds can read the program, but that will break backwards compatibility.

Quoted 9 lines · Andy Lutomirski

Simply ptrace yourself, exec the
program, and then dump the program out. A program that really wants to be unreadable should have a stub: the stub is setuid and readable, but all the stub does is to exec the real program, and the real program should have mode 0500 or similar.

ISTM the "right" check would be to enforce that the program's new creds can read the program, but that will break backwards compatibility.

Last I looked I had the impression that exec of a setuid program kills the ptrace.

If we are talking about a exec of a simple unreadable executable (aka something that sets undumpable but is not setuid or setgid). Then I agree it should break the ptrace as well and since those programs are as rare as hens teeth I don't see any problem with changing the ptrace behavior in that case.

Quoted 11 lines · Eric W. Biederman

Quoted 9 lines · Andy Lutomirski

Simply ptrace yourself, exec the
program, and then dump the program out. A program that really wants to be unreadable should have a stub: the stub is setuid and readable, but all the stub does is to exec the real program, and the real program should have mode 0500 or similar.

ISTM the "right" check would be to enforce that the program's new creds can read the program, but that will break backwards compatibility.

Quoted 8 lines

Last I looked I had the impression that exec of a setuid program kills the ptrace.

If we are talking about a exec of a simple unreadable executable (aka something that sets undumpable but is not setuid or setgid). Then I agree it should break the ptrace as well and since those programs are as rare as hens teeth I don't see any problem with changing the ptrace behavior in that case.

Nope. check_unsafe_exec() sets LSM_UNSAFE_* flags in bprm->unsafe, and then the flags are checked by the LSMs and cap_bprm_set_creds() in commoncap.c. cap_bprm_set_creds() just degrades the execution to a non-setuid-ish one, and e.g. ptracers stay attached.

Quoted 21 lines · Jann Horn

Quoted 11 lines · Eric W. Biederman

Quoted 9 lines · Andy Lutomirski

Simply ptrace yourself, exec the
program, and then dump the program out. A program that really wants to be unreadable should have a stub: the stub is setuid and readable, but all the stub does is to exec the real program, and the real program should have mode 0500 or similar.

ISTM the "right" check would be to enforce that the program's new creds can read the program, but that will break backwards compatibility.

Quoted 8 lines

Last I looked I had the impression that exec of a setuid program kills the ptrace.

If we are talking about a exec of a simple unreadable executable (aka something that sets undumpable but is not setuid or setgid). Then I agree it should break the ptrace as well and since those programs are as rare as hens teeth I don't see any problem with changing the ptrace behavior in that case.

Quoted 4 lines

Nope. check_unsafe_exec() sets LSM_UNSAFE_* flags in bprm->unsafe, and then the flags are checked by the LSMs and cap_bprm_set_creds() in commoncap.c. cap_bprm_set_creds() just degrades the execution to a non-setuid-ish one, and e.g. ptracers stay attached.

I think you're right. I ought to be completely sure because I rewrote that code back in 2005 or so back when I thought kernel programming was only for the cool kids. It was probably my first kernel patch ever and it closed an awkward-to-exploit root hole. But it's been a while. (Too bad my second (IIRC) kernel patch was more mundane and fixed the mute button on "new" Lenovo X60-era laptops and spend several years in limbo...)

Quoted 27 lines · Andy Lutomirski

Quoted 21 lines · Jann Horn

Quoted 11 lines · Eric W. Biederman

Quoted 9 lines · Andy Lutomirski

Simply ptrace yourself, exec the
program, and then dump the program out. A program that really wants to be unreadable should have a stub: the stub is setuid and readable, but all the stub does is to exec the real program, and the real program should have mode 0500 or similar.

ISTM the "right" check would be to enforce that the program's new creds can read the program, but that will break backwards compatibility.

Quoted 8 lines

Last I looked I had the impression that exec of a setuid program kills the ptrace.

If we are talking about a exec of a simple unreadable executable (aka something that sets undumpable but is not setuid or setgid). Then I agree it should break the ptrace as well and since those programs are as rare as hens teeth I don't see any problem with changing the ptrace behavior in that case.

Quoted 4 lines

Nope. check_unsafe_exec() sets LSM_UNSAFE_* flags in bprm->unsafe, and then the flags are checked by the LSMs and cap_bprm_set_creds() in commoncap.c. cap_bprm_set_creds() just degrades the execution to a non-setuid-ish one, and e.g. ptracers stay attached.

Quoted 7 lines

I think you're right. I ought to be completely sure because I rewrote that code back in 2005 or so back when I thought kernel programming was only for the cool kids. It was probably my first kernel patch ever and it closed an awkward-to-exploit root hole. But it's been a while. (Too bad my second (IIRC) kernel patch was more mundane and fixed the mute button on "new" Lenovo X60-era laptops and spend several years in limbo...)

Ah yes and this is only a problem if the ptracer does not have CAP_SYS_PTRACE.

If the tracer does not have sufficient permissions any opinions on failing the exec or kicking out the ptracer? I am leaning towards failing the exec as it is more obvious if someone cares. Dropping the ptracer could be a major mystery.

Quoted 37 lines · Eric W. Biederman

Quoted 27 lines · Andy Lutomirski

Quoted 21 lines · Jann Horn

Quoted 11 lines · Eric W. Biederman

Quoted 9 lines · Andy Lutomirski

Simply ptrace yourself, exec the
program, and then dump the program out. A program that really wants to be unreadable should have a stub: the stub is setuid and readable, but all the stub does is to exec the real program, and the real program should have mode 0500 or similar.

ISTM the "right" check would be to enforce that the program's new creds can read the program, but that will break backwards compatibility.

Quoted 8 lines

Last I looked I had the impression that exec of a setuid program kills the ptrace.

If we are talking about a exec of a simple unreadable executable (aka something that sets undumpable but is not setuid or setgid). Then I agree it should break the ptrace as well and since those programs are as rare as hens teeth I don't see any problem with changing the ptrace behavior in that case.

Quoted 4 lines

Nope. check_unsafe_exec() sets LSM_UNSAFE_* flags in bprm->unsafe, and then the flags are checked by the LSMs and cap_bprm_set_creds() in commoncap.c. cap_bprm_set_creds() just degrades the execution to a non-setuid-ish one, and e.g. ptracers stay attached.

Quoted 7 lines

I think you're right. I ought to be completely sure because I rewrote that code back in 2005 or so back when I thought kernel programming was only for the cool kids. It was probably my first kernel patch ever and it closed an awkward-to-exploit root hole. But it's been a while. (Too bad my second (IIRC) kernel patch was more mundane and fixed the mute button on "new" Lenovo X60-era laptops and spend several years in limbo...)

Quoted 7 lines

Ah yes and this is only a problem if the ptracer does not have CAP_SYS_PTRACE.

If the tracer does not have sufficient permissions any opinions on failing the exec or kicking out the ptracer? I am leaning towards failing the exec as it is more obvious if someone cares. Dropping the ptracer could be a major mystery.

I would suggest leaving it alone. Changing it could break enough things that a sysctl would be needed, and I just don't see how this is a significant issue, especially since it's been insecure forever. Anyone who cares should do the stub executable trick:

/sbin/foo: 04755, literally just does execve("/sbin/foo-helper");

/sbin/foo-helper: 0500.

Quoted 52 lines · Andy Lutomirski

Quoted 37 lines · Eric W. Biederman

Quoted 27 lines · Andy Lutomirski

Quoted 21 lines · Jann Horn

Quoted 11 lines · Eric W. Biederman

Quoted 9 lines · Andy Lutomirski

Simply ptrace yourself, exec the
program, and then dump the program out. A program that really wants to be unreadable should have a stub: the stub is setuid and readable, but all the stub does is to exec the real program, and the real program should have mode 0500 or similar.

ISTM the "right" check would be to enforce that the program's new creds can read the program, but that will break backwards compatibility.

Quoted 8 lines

Last I looked I had the impression that exec of a setuid program kills the ptrace.

If we are talking about a exec of a simple unreadable executable (aka something that sets undumpable but is not setuid or setgid). Then I agree it should break the ptrace as well and since those programs are as rare as hens teeth I don't see any problem with changing the ptrace behavior in that case.

Quoted 4 lines

Nope. check_unsafe_exec() sets LSM_UNSAFE_* flags in bprm->unsafe, and then the flags are checked by the LSMs and cap_bprm_set_creds() in commoncap.c. cap_bprm_set_creds() just degrades the execution to a non-setuid-ish one, and e.g. ptracers stay attached.

Quoted 7 lines

I think you're right. I ought to be completely sure because I rewrote that code back in 2005 or so back when I thought kernel programming was only for the cool kids. It was probably my first kernel patch ever and it closed an awkward-to-exploit root hole. But it's been a while. (Too bad my second (IIRC) kernel patch was more mundane and fixed the mute button on "new" Lenovo X60-era laptops and spend several years in limbo...)

Quoted 7 lines

Ah yes and this is only a problem if the ptracer does not have CAP_SYS_PTRACE.

If the tracer does not have sufficient permissions any opinions on failing the exec or kicking out the ptracer? I am leaning towards failing the exec as it is more obvious if someone cares. Dropping the ptracer could be a major mystery.

I would suggest leaving it alone. Changing it could break enough things that a sysctl would be needed, and I just don't see how this is a significant issue, especially since it's been insecure forever. Anyone who cares should do the stub executable trick:

Quoted 3 lines

/sbin/foo: 04755, literally just does execve("/sbin/foo-helper");

/sbin/foo-helper: 0500.

I can't imagine what non-malware would depend on being able to circumvent file permissions and ptrace a read-only executable. Is there something you are thinking of?

When the flag PT_PTRACE_CAP was added the PTRACE_TRACEME path was overlooked. This can result in incorrect behavior when an application like strace traces an exec of a setuid executable.

Further PT_PTRACE_CAP does not have enough information for making good security decisions as it does not report which user namespace the capability is in. This has already allowed one mistake through insufficient granulariy.

I found this issue when I was testing another corner case of exec and discovered that I could not get strace to set PT_PTRACE_CAP even when running strace as root with a full set of caps.

This change fixes the above issue with strace allowing stracing as root a setuid executable without disabling setuid. More fundamentaly this change allows what is allowable at all times, by using the correct information in it's decision.

Cc [email protected]

Fixes 4214e42f96d4 ("v2.4.9.11 -> v2.4.9.12")

Signed-off-by "Eric W. Biederman" <[email protected]>

---
[...]

include/linux/sched.h +0 -0

index 348f51b0ec92..8fe58255d219 100644

Quoted 31 lines · Eric W. Biederman

When the flag PT_PTRACE_CAP was added the PTRACE_TRACEME path was overlooked. This can result in incorrect behavior when an application like strace traces an exec of a setuid executable.

Further PT_PTRACE_CAP does not have enough information for making good security decisions as it does not report which user namespace the capability is in. This has already allowed one mistake through insufficient granulariy.

I found this issue when I was testing another corner case of exec and discovered that I could not get strace to set PT_PTRACE_CAP even when running strace as root with a full set of caps.

This change fixes the above issue with strace allowing stracing as root a setuid executable without disabling setuid. More fundamentaly this change allows what is allowable at all times, by using the correct information in it's decision.

Cc [email protected]

Fixes 4214e42f96d4 ("v2.4.9.11 -> v2.4.9.12")

Signed-off-by "Eric W. Biederman" <[email protected]>

---
[...]

include/linux/sched.h +0 -0

index 348f51b0ec92..8fe58255d219 100644
@@ -1656,6 +1656,7 @@ struct task_struct {
        struct list_head cpu_timers[3];
 

include/linux/sched.h struct task_struct { · line 1659

Quoted 2 lines

include/linux/sched.h +1 -0

 /* process credentials */
+       const struct cred __rcu *ptracer_cred; /* Tracer's dredentials at attach */

Typo: credentials.

Quoted 31 lines · Eric W. Biederman

When the flag PT_PTRACE_CAP was added the PTRACE_TRACEME path was overlooked. This can result in incorrect behavior when an application like strace traces an exec of a setuid executable.

Further PT_PTRACE_CAP does not have enough information for making good security decisions as it does not report which user namespace the capability is in. This has already allowed one mistake through insufficient granulariy.

I found this issue when I was testing another corner case of exec and discovered that I could not get strace to set PT_PTRACE_CAP even when running strace as root with a full set of caps.

This change fixes the above issue with strace allowing stracing as root a setuid executable without disabling setuid. More fundamentaly this change allows what is allowable at all times, by using the correct information in it's decision.

Cc [email protected]

Fixes 4214e42f96d4 ("v2.4.9.11 -> v2.4.9.12")

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
[...]

include/linux/sched.h +0 -0

index 348f51b0ec92..8fe58255d219 100644
@@ -1656,6 +1656,7 @@ struct task_struct {
        struct list_head cpu_timers[3];
 

include/linux/sched.h struct task_struct { · line 1659

Quoted 2 lines

include/linux/sched.h +1 -0

 /* process credentials */
+       const struct cred __rcu *ptracer_cred; /* Tracer's dredentials at attach */

Typo: credentials.

When the flag PT_PTRACE_CAP was added the PTRACE_TRACEME path was overlooked. This can result in incorrect behavior when an application like strace traces an exec of a setuid executable.

Further PT_PTRACE_CAP does not have enough information for making good security decisions as it does not report which user namespace the capability is in. This has already allowed one mistake through insufficient granulariy.

I found this issue when I was testing another corner case of exec and discovered that I could not get strace to set PT_PTRACE_CAP even when running strace as root with a full set of caps.

This change fixes the above issue with strace allowing stracing as root a setuid executable without disabling setuid. More fundamentaly this change allows what is allowable at all times, by using the correct information in it's decision.

Cc [email protected]

Fixes 4214e42f96d4 ("v2.4.9.11 -> v2.4.9.12")

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
[...]

include/linux/sched.h +0 -0

index 348f51b0ec92..8fe58255d219 100644

When the flag PT_PTRACE_CAP was added the PTRACE_TRACEME path was overlooked. This can result in incorrect behavior when an application like strace traces an exec of a setuid executable.

Further PT_PTRACE_CAP does not have enough information for making good security decisions as it does not report which user namespace the capability is in. This has already allowed one mistake through insufficient granulariy.

I found this issue when I was testing another corner case of exec and discovered that I could not get strace to set PT_PTRACE_CAP even when running strace as root with a full set of caps.

This change fixes the above issue with strace allowing stracing as root a setuid executable without disabling setuid. More fundamentaly this change allows what is allowable at all times, by using the correct information in it's decision.

Cc [email protected]

Fixes 4214e42f96d4 ("v2.4.9.11 -> v2.4.9.12")

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 fs/exec.c                  |  2 +-
 include/linux/capability.h |  1 +
 include/linux/ptrace.h     |  1 -
 include/linux/sched.h      |  1 +
 kernel/capability.c        | 20 ++++++++++++++++++++
 kernel/ptrace.c            | 12 +++++++-----
 6 files changed, 30 insertions(+), 7 deletions(-)

fs/exec.c +0 -0

index 6fcfb3f7b137..fdec760bfac3 100644

When the flag PT_PTRACE_CAP was added the PTRACE_TRACEME path was overlooked. This can result in incorrect behavior when an application like strace traces an exec of a setuid executable.

Further PT_PTRACE_CAP does not have enough information for making good security decisions as it does not report which user namespace the capability is in. This has already allowed one mistake through insufficient granulariy.

I found this issue when I was testing another corner case of exec and discovered that I could not get strace to set PT_PTRACE_CAP even when running strace as root with a full set of caps.

This change fixes the above issue with strace allowing stracing as root a setuid executable without disabling setuid. More fundamentaly this change allows what is allowable at all times, by using the correct information in it's decision.

Cc [email protected]

Fixes 4214e42f96d4 ("v2.4.9.11 -> v2.4.9.12")

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/exec.c                  |  2 +-
 include/linux/capability.h |  1 +
 include/linux/ptrace.h     |  1 -
 include/linux/sched.h      |  1 +
 kernel/capability.c        | 20 ++++++++++++++++++++
 kernel/ptrace.c            | 12 +++++++-----
 6 files changed, 30 insertions(+), 7 deletions(-)

fs/exec.c +0 -0

index 6fcfb3f7b137..fdec760bfac3 100644

Quoted 38 lines · Eric W. Biederman

When the flag PT_PTRACE_CAP was added the PTRACE_TRACEME path was overlooked. This can result in incorrect behavior when an application like strace traces an exec of a setuid executable.

Further PT_PTRACE_CAP does not have enough information for making good security decisions as it does not report which user namespace the capability is in. This has already allowed one mistake through insufficient granulariy.

I found this issue when I was testing another corner case of exec and discovered that I could not get strace to set PT_PTRACE_CAP even when running strace as root with a full set of caps.

This change fixes the above issue with strace allowing stracing as root a setuid executable without disabling setuid. More fundamentaly this change allows what is allowable at all times, by using the correct information in it's decision.

Cc [email protected]

Fixes 4214e42f96d4 ("v2.4.9.11 -> v2.4.9.12")

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/exec.c                  |  2 +-
 include/linux/capability.h |  1 +
 include/linux/ptrace.h     |  1 -
 include/linux/sched.h      |  1 +
 kernel/capability.c        | 20 ++++++++++++++++++++
 kernel/ptrace.c            | 12 +++++++-----
 6 files changed, 30 insertions(+), 7 deletions(-)

fs/exec.c +0 -0

index 6fcfb3f7b137..fdec760bfac3 100644
@@ -1401,7 +1401,7 @@ static void check_unsafe_exec(struct linux_binprm *bprm)
        unsigned n_fs;
 

fs/exec.c static void check_unsafe_exec(struct linux_binprm *bprm) · line 1404

Quoted 3 lines

fs/exec.c +1 -1

        if (p->ptrace) {
-               if (p->ptrace & PT_PTRACE_CAP)
+               if (ptracer_capable(p, current_user_ns()))

IIRC PT_PTRACE_CAP was added to prevent TOCTOU races. What prevents that type of race now? For that matter, what guarantees that we've already switched to new creds here and will continue to do so in the future?

Quoted 38 lines · Eric W. Biederman

When the flag PT_PTRACE_CAP was added the PTRACE_TRACEME path was overlooked. This can result in incorrect behavior when an application like strace traces an exec of a setuid executable.

Further PT_PTRACE_CAP does not have enough information for making good security decisions as it does not report which user namespace the capability is in. This has already allowed one mistake through insufficient granulariy.

I found this issue when I was testing another corner case of exec and discovered that I could not get strace to set PT_PTRACE_CAP even when running strace as root with a full set of caps.

This change fixes the above issue with strace allowing stracing as root a setuid executable without disabling setuid. More fundamentaly this change allows what is allowable at all times, by using the correct information in it's decision.

Cc [email protected]

Fixes 4214e42f96d4 ("v2.4.9.11 -> v2.4.9.12")

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 fs/exec.c                  |  2 +-
 include/linux/capability.h |  1 +
 include/linux/ptrace.h     |  1 -
 include/linux/sched.h      |  1 +
 kernel/capability.c        | 20 ++++++++++++++++++++
 kernel/ptrace.c            | 12 +++++++-----
 6 files changed, 30 insertions(+), 7 deletions(-)

fs/exec.c +0 -0

index 6fcfb3f7b137..fdec760bfac3 100644
@@ -1401,7 +1401,7 @@ static void check_unsafe_exec(struct linux_binprm *bprm)
        unsigned n_fs;
 

fs/exec.c static void check_unsafe_exec(struct linux_binprm *bprm) · line 1404

Quoted 3 lines

fs/exec.c +1 -1

        if (p->ptrace) {
-               if (p->ptrace & PT_PTRACE_CAP)
+               if (ptracer_capable(p, current_user_ns()))

IIRC PT_PTRACE_CAP was added to prevent TOCTOU races. What prevents that type of race now? For that matter, what guarantees that we've already switched to new creds here and will continue to do so in the future?

With everyone heading to Kernel Summit and Plumbers I put this set of
patches down temporarily. Now is the time to take it back up and to
make certain I am not missing something stupid in this set of patches.

It is the reasonable expectation that if an executable file is not readable there will be no way for a user without special privileges to read the file. This is enforced in ptrace_attach but if we are already attached there is no enforcement if a readonly executable is exec'd.

Quoted 6 lines · Eric W. Biederman

It is the reasonable expectation that if an executable file is not readable there will be no way for a user without special privileges to read the file. This is enforced in ptrace_attach but if we are already attached there is no enforcement if a readonly executable is exec'd.

I'm really scared by this Eric. At least you want to make it a hardening option that can be disabled at run time, otherwise it can easily break a lot of userspace :

admin@aloha:~$ ll /bin/bash /bin/coreutils /bin/ls /usr/bin/telnet

(quoted diff) +0 -2

-r-xr-x--x 1 root adm 549272 Oct 28 16:25 /bin/bash
-rwx--x--x 1 root adm 765624 Oct 28 16:27 /bin/coreutils

lrwxrwxrwx 1 root root 9 Oct 28 16:27 /bin/ls -> coreutils -r-xr-x--x 1 root adm 70344 Oct 28 16:34 /usr/bin/telnet

And I've not invented it, I've being taught to do this more than 20 years ago and been doing this since on any slightly hardened server just because in pratice it's efficient at stopping quite a bunch of rootkits which require to copy and modify your executables. Sure they could get the contents using ptrace, but using cp is much more common than ptrace in scripts and that works. This has prooven quite efficient in field at stopping some rootkits several times over the last two decades and I know I'm not the only one to do it. In fact I *never* install an executable with read permissions for users if there's no need for random users to copy it. Does it mean that nobody should be able to see why their favorite utility doesn't work anymore ? Not in my opinion, at least not by default.

So here I fear that we'll break strace at many places where strace precisely matters to debug things.

However I'd love to have this feature controlled by a sysctl (to enforce it by default where possible).

I'm not opposed to a sysctl for this. Regardless, I think we need to embrace this idea now, though, since we'll soon end up with architectures that enforce executable-only memory, in which case ptrace will again fail. Almost better to get started here and then not have more surprises later.

Quoted 5 lines · Kees Cook

I'm not opposed to a sysctl for this. Regardless, I think we need to embrace this idea now, though, since we'll soon end up with architectures that enforce executable-only memory, in which case ptrace will again fail. Almost better to get started here and then not have more surprises later.

Also that makes me realize that by far the largest use case of ptrace is strace and that strace needs very little capabilities. I guess that most users would be fine with having only pointers and not contents for addresses or read/write of data, as they have on some other OSes, when the process is not readable. But in my opinion when a process is executable we should be able to trace its execution (even without memory read access).

It is the reasonable expectation that if an executable file is not readable there will be no way for a user without special privileges to read the file. This is enforced in ptrace_attach but if ptrace is already attached before exec there is no enforcement for read-only executables.

It is the reasonable expectation that if an executable file is not readable there will be no way for a user without special privileges to read the file. This is enforced in ptrace_attach but if ptrace is already attached before exec there is no enforcement for read-only executables.

Quoted 5 lines · Kees Cook

I'm not opposed to a sysctl for this. Regardless, I think we need to embrace this idea now, though, since we'll soon end up with architectures that enforce executable-only memory, in which case ptrace will again fail. Almost better to get started here and then not have more surprises later.

Also that makes me realize that by far the largest use case of ptrace is strace and that strace needs very little capabilities. I guess that most users would be fine with having only pointers and not contents for addresses or read/write of data, as they have on some other OSes, when the process is not readable. But in my opinion when a process is executable we should be able to trace its execution (even without memory read access).

I'm not opposed to a sysctl for this. Regardless, I think we need to embrace this idea now, though, since we'll soon end up with architectures that enforce executable-only memory, in which case ptrace will again fail. Almost better to get started here and then not have more surprises later.

Quoted 34 lines · Willy Tarreau

Quoted 6 lines · Eric W. Biederman

It is the reasonable expectation that if an executable file is not readable there will be no way for a user without special privileges to read the file. This is enforced in ptrace_attach but if we are already attached there is no enforcement if a readonly executable is exec'd.

I'm really scared by this Eric. At least you want to make it a hardening option that can be disabled at run time, otherwise it can easily break a lot of userspace :

admin@aloha:~$ ll /bin/bash /bin/coreutils /bin/ls /usr/bin/telnet

(quoted diff) +0 -2

-r-xr-x--x 1 root adm 549272 Oct 28 16:25 /bin/bash
-rwx--x--x 1 root adm 765624 Oct 28 16:27 /bin/coreutils

lrwxrwxrwx 1 root root 9 Oct 28 16:27 /bin/ls -> coreutils -r-xr-x--x 1 root adm 70344 Oct 28 16:34 /usr/bin/telnet

And I've not invented it, I've being taught to do this more than 20 years ago and been doing this since on any slightly hardened server just because in pratice it's efficient at stopping quite a bunch of rootkits which require to copy and modify your executables. Sure they could get the contents using ptrace, but using cp is much more common than ptrace in scripts and that works. This has prooven quite efficient in field at stopping some rootkits several times over the last two decades and I know I'm not the only one to do it. In fact I *never* install an executable with read permissions for users if there's no need for random users to copy it. Does it mean that nobody should be able to see why their favorite utility doesn't work anymore ? Not in my opinion, at least not by default.

So here I fear that we'll break strace at many places where strace precisely matters to debug things.

Quoted 2 lines

However I'd love to have this feature controlled by a sysctl (to enforce it by default where possible).

I'm not opposed to a sysctl for this. Regardless, I think we need to embrace this idea now, though, since we'll soon end up with architectures that enforce executable-only memory, in which case ptrace will again fail. Almost better to get started here and then not have more surprises later.

Quoted 6 lines · Eric W. Biederman

It is the reasonable expectation that if an executable file is not readable there will be no way for a user without special privileges to read the file. This is enforced in ptrace_attach but if we are already attached there is no enforcement if a readonly executable is exec'd.

I'm really scared by this Eric. At least you want to make it a hardening option that can be disabled at run time, otherwise it can easily break a lot of userspace :

admin@aloha:~$ ll /bin/bash /bin/coreutils /bin/ls /usr/bin/telnet

(quoted diff) +0 -2

-r-xr-x--x 1 root adm 549272 Oct 28 16:25 /bin/bash
-rwx--x--x 1 root adm 765624 Oct 28 16:27 /bin/coreutils

lrwxrwxrwx 1 root root 9 Oct 28 16:27 /bin/ls -> coreutils -r-xr-x--x 1 root adm 70344 Oct 28 16:34 /usr/bin/telnet

And I've not invented it, I've being taught to do this more than 20 years ago and been doing this since on any slightly hardened server just because in pratice it's efficient at stopping quite a bunch of rootkits which require to copy and modify your executables. Sure they could get the contents using ptrace, but using cp is much more common than ptrace in scripts and that works. This has prooven quite efficient in field at stopping some rootkits several times over the last two decades and I know I'm not the only one to do it. In fact I *never* install an executable with read permissions for users if there's no need for random users to copy it. Does it mean that nobody should be able to see why their favorite utility doesn't work anymore ? Not in my opinion, at least not by default.

So here I fear that we'll break strace at many places where strace precisely matters to debug things.

However I'd love to have this feature controlled by a sysctl (to enforce it by default where possible).

It is the reasonable expectation that if an executable file is not readable there will be no way for a user without special privileges to read the file. This is enforced in ptrace_attach but if we are already attached there is no enforcement if a readonly executable is exec'd.

It is the reasonable expectation that if an executable file is not readable there will be no way for a user without special privileges to read the file. This is enforced in ptrace_attach but if we are already attached there is no enforcement if a readonly executable is exec'd.

Therefore do the simple thing and if there is a non-dumpable executable that we are tracing without privilege fail to exec it.

Fixes v1.0

Cc [email protected]

Reported-by Andy Lutomirski <[email protected]>

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 fs/exec.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

fs/exec.c +0 -0

index fdec760bfac3..de107f74e055 100644

It is the reasonable expectation that if an executable file is not readable there will be no way for a user without special privileges to read the file. This is enforced in ptrace_attach but if we are already attached there is no enforcement if a readonly executable is exec'd.

Therefore do the simple thing and if there is a non-dumpable executable that we are tracing without privilege fail to exec it.

Fixes v1.0

Cc [email protected]

Reported-by Andy Lutomirski <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/exec.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

fs/exec.c +0 -0

index fdec760bfac3..de107f74e055 100644

Quoted 23 lines · Eric W. Biederman

It is the reasonable expectation that if an executable file is not readable there will be no way for a user without special privileges to read the file. This is enforced in ptrace_attach but if we are already attached there is no enforcement if a readonly executable is exec'd.

Therefore do the simple thing and if there is a non-dumpable executable that we are tracing without privilege fail to exec it.

Fixes v1.0

Cc [email protected]

Reported-by Andy Lutomirski <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/exec.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

fs/exec.c +0 -0

index fdec760bfac3..de107f74e055 100644

fs/exec.c int flush_old_exec(struct linux_binprm * bprm) · line 1237

Quoted 8 lines

fs/exec.c +5 -0

@@ -1230,6 +1230,11 @@ int flush_old_exec(struct linux_binprm * bprm)
 {
        int retval;
 
+       /* Fail if the tracer can't read the executable */
+       if ((bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP) &&
+           !ptracer_capable(current, bprm->mm->user_ns))
+               return -EPERM;
+

At the very least, I think that BINPRM_FLAGS_ENFORCE_NONDUMP needs to check capable_wrt_inode_uidgid too. Otherwise we risk breaking:

$ gcc whatever.c
$ chmod 400 a.out
$ strace a.out

Quoted 37 lines · Andy Lutomirski

Quoted 23 lines · Eric W. Biederman

It is the reasonable expectation that if an executable file is not readable there will be no way for a user without special privileges to read the file. This is enforced in ptrace_attach but if we are already attached there is no enforcement if a readonly executable is exec'd.

Therefore do the simple thing and if there is a non-dumpable executable that we are tracing without privilege fail to exec it.

Fixes v1.0

Cc [email protected]

Reported-by Andy Lutomirski <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/exec.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

fs/exec.c +0 -0

index fdec760bfac3..de107f74e055 100644

fs/exec.c int flush_old_exec(struct linux_binprm * bprm) · line 1237

Quoted 8 lines

fs/exec.c +5 -0

@@ -1230,6 +1230,11 @@ int flush_old_exec(struct linux_binprm * bprm)
 {
        int retval;
 
+       /* Fail if the tracer can't read the executable */
+       if ((bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP) &&
+           !ptracer_capable(current, bprm->mm->user_ns))
+               return -EPERM;
+

At the very least, I think that BINPRM_FLAGS_ENFORCE_NONDUMP needs to check capable_wrt_inode_uidgid too. Otherwise we risk breaking:

Quoted 3 lines

$ gcc whatever.c
$ chmod 400 a.out
$ strace a.out

It is an invariant that if you have caps in mm->user_ns you will also be capable_write_inode_uidgid of all files that a process exec's.

My third patch winds up changing mm->user_ns to maintain this invariant.

It is also true that Willy convinced me while this check is trivial it will break historic uses so I have replaced this patch with:
"ptrace: Don't allow accessing an undumpable mm.

Eric

Quoted 39 lines · Eric W. Biederman

Quoted 37 lines · Andy Lutomirski

Quoted 23 lines · Eric W. Biederman

It is the reasonable expectation that if an executable file is not readable there will be no way for a user without special privileges to read the file. This is enforced in ptrace_attach but if we are already attached there is no enforcement if a readonly executable is exec'd.

Therefore do the simple thing and if there is a non-dumpable executable that we are tracing without privilege fail to exec it.

Fixes v1.0

Cc [email protected]

Reported-by Andy Lutomirski <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/exec.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

fs/exec.c +0 -0

index fdec760bfac3..de107f74e055 100644

fs/exec.c int flush_old_exec(struct linux_binprm * bprm) · line 1237

Quoted 8 lines

fs/exec.c +5 -0

@@ -1230,6 +1230,11 @@ int flush_old_exec(struct linux_binprm * bprm)
 {
        int retval;
 
+       /* Fail if the tracer can't read the executable */
+       if ((bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP) &&
+           !ptracer_capable(current, bprm->mm->user_ns))
+               return -EPERM;
+

At the very least, I think that BINPRM_FLAGS_ENFORCE_NONDUMP needs to check capable_wrt_inode_uidgid too. Otherwise we risk breaking:

Quoted 6 lines

Quoted 3 lines

$ gcc whatever.c
$ chmod 400 a.out
$ strace a.out

It is an invariant that if you have caps in mm->user_ns you will also be capable_write_inode_uidgid of all files that a process exec's.

I meant to check whether you *are* the owner, too.

Quoted 5 lines

My third patch winds up changing mm->user_ns to maintain this invariant.

It is also true that Willy convinced me while this check is trivial it will break historic uses so I have replaced this patch with:
"ptrace: Don't allow accessing an undumpable mm.

I think that's better.

Quoted 39 lines · Eric W. Biederman

Quoted 37 lines · Andy Lutomirski

Quoted 23 lines · Eric W. Biederman

It is the reasonable expectation that if an executable file is not readable there will be no way for a user without special privileges to read the file. This is enforced in ptrace_attach but if we are already attached there is no enforcement if a readonly executable is exec'd.

Therefore do the simple thing and if there is a non-dumpable executable that we are tracing without privilege fail to exec it.

Fixes v1.0

Cc [email protected]

Reported-by Andy Lutomirski <[email protected]>

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 fs/exec.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

fs/exec.c +0 -0

index fdec760bfac3..de107f74e055 100644

fs/exec.c int flush_old_exec(struct linux_binprm * bprm) · line 1237

Quoted 8 lines

fs/exec.c +5 -0

@@ -1230,6 +1230,11 @@ int flush_old_exec(struct linux_binprm * bprm)
 {
        int retval;
 
+       /* Fail if the tracer can't read the executable */
+       if ((bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP) &&
+           !ptracer_capable(current, bprm->mm->user_ns))
+               return -EPERM;
+

At the very least, I think that BINPRM_FLAGS_ENFORCE_NONDUMP needs to check capable_wrt_inode_uidgid too. Otherwise we risk breaking: