su / sudo doesn't work when enlightenment is running as the window manager

Quoted 15 lines · Takashi Iwai

Hi Eric,

we've got a bug report showing the weird behavior supposedly triggered by your commit 20523132ec5d ("exec: Test the ptracer's saved cred to see if the tracee can gain caps"). In short, user can't run su or sudo any longer when a terminal is started from Enlighentment window manager.

Some details are found in openSUSE Bugzilla:
https://bugzilla.suse.com/show_bug.cgi?id=1040041

and more in gentoo forum and Enlightenment bug tracker:
https://forums.gentoo.org/viewtopic-t-1063022-postdays-0-postorder-asc-start-0.html
https://phab.enlightenment.org/T5470

Quoted 5 lines

As the bug seems solely in Enlightenment, it might that be some its specific patch is broken. But a regression is a regression, after all...

Could you take a look at the issue?

I will.

I am wondering if the bisect did not quite go back far enough as that change really should have been had no effect and it was the introduction of the ptracer_capable test that is causing problems.

To be able to think this through clearly I really need to understand what enlightenment-start is doing.

At first glance the reported behavior does seem corect. If you are not root and you are ptracing a setuid-root application it should not have be run setuid.

So either I made a stilly mistake somewhere or enlightenment is doing something problematic. The ugly possibility is that I might have closed a security hole they were depending upon by accident.

I took a quick look at what I think is the code to enlightment_start. AKA src/bin/e_start_main.c

And all it does is:

	pid = fork();
        if (pid == 0) {
        	/* child */
                ptrace(PT_TRACE_ME, 0, NULL);
        } else {
        	ptrace(PT_ATTACH, pid, NULL, NULL);
                ...;
        }

So the tracer_cred should be the same on either path.

So if someone who understands what enlightenment is doing could boil this down or otherwise help me understand exactly what kind of ptrace interaction is causing this problem that would really help.

Hmm. You know I think I know what is going on and it is a bit embarrasing:

Can someone please test this change? I am guessing I just forgot to zero ptracer_cred on fork... Why ptrace_init_task lives in a header instead of in kernel/fork.c I do not know.

include/linux/ptrace.h +1 -0

index 422bc2e4cb6a..a31442c4b272 100644
@@ -202,6 +202,7 @@ static inline void ptrace_init_task(struct task_struct *child, bool ptrace)
        INIT_LIST_HEAD(&child->ptraced);
        child->jobctl = 0;
        child->ptrace = 0;
+       child->ptracer_cred = NULL;
        child->parent = child->real_parent;
 
        if (unlikely(ptrace) && current->ptrace) {

Eric

When I introduced ptracer_cred I failed to consider the weirdness of fork where the task_struct copies the old value by default. This winds up leaving ptracer_cred set even when a process forks and the child process does not wind up being ptraced.

Because ptracer_cred is not set on non-ptraced processes whose parents were ptraced this has broken the ability of the enlightenment window manager to start setuid children.

Fix this by properly initializing ptracer_cred in ptrace_init_task

This must be done with a little bit of care to preserve the current value of ptracer_cred when ptrace carries through fork. Re-reading the ptracer_cred from the ptracing process at this point is inconsistent with how PT_PTRACE_CAP has been maintained all of these years.

Fixes 64b875f7ac8a ("ptrace: Capture the ptracer's creds not PT_PTRACE_CAP")

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

---

If I could get some folks to test and verify this fixes the enlightenment issue I would really appreciate it.

 include/linux/ptrace.h |  7 +++++--
 kernel/ptrace.c        | 20 +++++++++++++-------
 2 files changed, 18 insertions(+), 9 deletions(-)

include/linux/ptrace.h +5 -2

index 422bc2e4cb6a..23c5716f7bd2 100644
@@ -54,7 +54,8 @@ extern int ptrace_request(struct task_struct *child, long request,
 			  unsigned long addr, unsigned long data);
 extern void ptrace_notify(int exit_code);
 extern void __ptrace_link(struct task_struct *child,
-			  struct task_struct *new_parent);
+			  struct task_struct *new_parent,
+			  struct cred *ptracer_cred);
 extern void __ptrace_unlink(struct task_struct *child);
 extern void exit_ptrace(struct task_struct *tracer, struct list_head *dead);
 #define PTRACE_MODE_READ	0x01

@@ -206,7 +207,7 @@ static inline void ptrace_init_task(struct task_struct *child, bool ptrace)
 
 	if (unlikely(ptrace) && current->ptrace) {
 		child->ptrace = current->ptrace;
-		__ptrace_link(child, current->parent);
+		__ptrace_link(child, current->parent, current->ptracer_cred);
 
 		if (child->ptrace & PT_SEIZED)
 			task_set_jobctl_pending(child, JOBCTL_TRAP_STOP);

@@ -215,6 +216,8 @@ static inline void ptrace_init_task(struct task_struct *child, bool ptrace)
 
 		set_tsk_thread_flag(child, TIF_SIGPENDING);
 	}
+	else
+		child->ptracer_cred = NULL;
 }
 
 /**

kernel/ptrace.c +13 -7

index 266ddcc1d8bb..79cbe00fe787 100644
@@ -60,19 +60,25 @@ int ptrace_access_vm(struct task_struct *tsk, unsigned long addr,
 }
 
 
+void __ptrace_link(struct task_struct *child, struct task_struct *new_parent,
+		    struct cred *ptracer_cred)
+{
+	BUG_ON(!list_empty(&child->ptrace_entry));
+	list_add(&child->ptrace_entry, &new_parent->ptraced);
+	child->parent = new_parent;
+	child->ptracer_cred = get_cred(ptracer_cred);
+}
+
 /*
  * ptrace a task: make the debugger its new parent and
  * move it to the ptrace list.
  *
  * Must be called with the tasklist lock write-held.
  */
-void __ptrace_link(struct task_struct *child, struct task_struct *new_parent)
+static void ptrace_link(struct task_struct *child, struct task_struct *new_parent)
 {
-	BUG_ON(!list_empty(&child->ptrace_entry));
-	list_add(&child->ptrace_entry, &new_parent->ptraced);
-	child->parent = new_parent;
 	rcu_read_lock();
-	child->ptracer_cred = get_cred(__task_cred(new_parent));
+	__ptrace_link(child, new_parent, __task_cred(new_parent));
 	rcu_read_unlock();
 }
 

@@ -386,7 +392,7 @@ static int ptrace_attach(struct task_struct *task, long request,
 		flags |= PT_SEIZED;
 	task->ptrace = flags;
 
-	__ptrace_link(task, current);
+	ptrace_link(task, current);
 
 	/* SEIZE doesn't trap tracee on attach */
 	if (!seize)

@@ -459,7 +465,7 @@ static int ptrace_traceme(void)
 		 */
 		if (!ret && !(current->real_parent->flags & PF_EXITING)) {
 			current->ptrace = PT_PTRACED;
-			__ptrace_link(current, current->real_parent);
+			ptrace_link(current, current->real_parent);
 		}
 	}
 	write_unlock_irq(&tasklist_lock);

Signature · 1 line

2.10.1

Quoted 18 lines · Eric W. Biederman

When I introduced ptracer_cred I failed to consider the weirdness of fork where the task_struct copies the old value by default. This winds up leaving ptracer_cred set even when a process forks and the child process does not wind up being ptraced.

Because ptracer_cred is not set on non-ptraced processes whose parents were ptraced this has broken the ability of the enlightenment window manager to start setuid children.

Fix this by properly initializing ptracer_cred in ptrace_init_task

This must be done with a little bit of care to preserve the current value of ptracer_cred when ptrace carries through fork. Re-reading the ptracer_cred from the ptracing process at this point is inconsistent with how PT_PTRACE_CAP has been maintained all of these years.

Quoted 6 lines

Fixes 64b875f7ac8a ("ptrace: Capture the ptracer's creds not PT_PTRACE_CAP")

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

---

If I could get some folks to test and verify this fixes the enlightenment issue I would really appreciate it.

This seems giving a compile warning and it becomes error in the following:

In file included from ./include/linux/mutex.h:13:0,

                 from ./include/linux/kernfs.h:13,
                 from ./include/linux/sysfs.h:15,
                 from ./include/linux/kobject.h:21,
                 from ./include/linux/device.h:17,
                 from drivers/gpu/drm/i915/gvt/kvmgt.c:32:
./include/linux/ptrace.h: In function ‘ptrace_init_task’:
./arch/x86/include/asm/current.h:17:17: error: passing argument 3 of ‘__ptrace_link’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
 #define current get_current()
                 ^
./include/linux/ptrace.h:210:41: note: in expansion of macro ‘current’
   __ptrace_link(child, current->parent, current->ptracer_cred);
                                         ^~~~~~~
In file included from ./arch/x86/include/asm/stacktrace.h:10:0,
                 from ./arch/x86/include/asm/perf_event.h:246,
                 from ./include/linux/perf_event.h:24,
                 from ./arch/x86/include/asm/kvm_host.h:24,
                 from ./include/linux/kvm_host.h:37,
                 from drivers/gpu/drm/i915/gvt/kvmgt.c:41:
./include/linux/ptrace.h:56:13: note: expected ‘struct cred *’ but argument is of type ‘const struct cred *’
 extern void __ptrace_link(struct task_struct *child,
             ^~~~~~~~~~~~~

cc1: all warnings being treated as errors

Quoted 53 lines · Takashi Iwai

Quoted 18 lines · Eric W. Biederman

When I introduced ptracer_cred I failed to consider the weirdness of fork where the task_struct copies the old value by default. This winds up leaving ptracer_cred set even when a process forks and the child process does not wind up being ptraced.

Because ptracer_cred is not set on non-ptraced processes whose parents were ptraced this has broken the ability of the enlightenment window manager to start setuid children.

Fix this by properly initializing ptracer_cred in ptrace_init_task

This must be done with a little bit of care to preserve the current value of ptracer_cred when ptrace carries through fork. Re-reading the ptracer_cred from the ptracing process at this point is inconsistent with how PT_PTRACE_CAP has been maintained all of these years.

Quoted 6 lines

Fixes 64b875f7ac8a ("ptrace: Capture the ptracer's creds not PT_PTRACE_CAP")

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

---

If I could get some folks to test and verify this fixes the enlightenment issue I would really appreciate it.

This seems giving a compile warning and it becomes error in the following:

In file included from ./include/linux/mutex.h:13:0,

                 from ./include/linux/kernfs.h:13,
                 from ./include/linux/sysfs.h:15,
                 from ./include/linux/kobject.h:21,
                 from ./include/linux/device.h:17,
                 from drivers/gpu/drm/i915/gvt/kvmgt.c:32:
./include/linux/ptrace.h: In function ‘ptrace_init_task’:
./arch/x86/include/asm/current.h:17:17: error: passing argument 3 of ‘__ptrace_link’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
 #define current get_current()
                 ^
./include/linux/ptrace.h:210:41: note: in expansion of macro ‘current’
   __ptrace_link(child, current->parent, current->ptracer_cred);
                                         ^~~~~~~
In file included from ./arch/x86/include/asm/stacktrace.h:10:0,
                 from ./arch/x86/include/asm/perf_event.h:246,
                 from ./include/linux/perf_event.h:24,
                 from ./arch/x86/include/asm/kvm_host.h:24,
                 from ./include/linux/kvm_host.h:37,
                 from drivers/gpu/drm/i915/gvt/kvmgt.c:41:
./include/linux/ptrace.h:56:13: note: expected ‘struct cred *’ but argument is of type ‘const struct cred *’
 extern void __ptrace_link(struct task_struct *child,
             ^~~~~~~~~~~~~

Quoted 1 line

cc1: all warnings being treated as errors

Through a quick test on VM (fixed patch by adding const to __ptrace_link() argument), it seems working fine.

Tested-by Takashi Iwai <[email protected]>

Quoted 57 lines · Takashi Iwai

Quoted 53 lines · Takashi Iwai

Quoted 18 lines · Eric W. Biederman

When I introduced ptracer_cred I failed to consider the weirdness of fork where the task_struct copies the old value by default. This winds up leaving ptracer_cred set even when a process forks and the child process does not wind up being ptraced.

Because ptracer_cred is not set on non-ptraced processes whose parents were ptraced this has broken the ability of the enlightenment window manager to start setuid children.

Fix this by properly initializing ptracer_cred in ptrace_init_task

This must be done with a little bit of care to preserve the current value of ptracer_cred when ptrace carries through fork. Re-reading the ptracer_cred from the ptracing process at this point is inconsistent with how PT_PTRACE_CAP has been maintained all of these years.

Quoted 6 lines

Fixes 64b875f7ac8a ("ptrace: Capture the ptracer's creds not PT_PTRACE_CAP")

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

---

If I could get some folks to test and verify this fixes the enlightenment issue I would really appreciate it.

This seems giving a compile warning and it becomes error in the following:

In file included from ./include/linux/mutex.h:13:0,

                 from ./include/linux/kernfs.h:13,
                 from ./include/linux/sysfs.h:15,
                 from ./include/linux/kobject.h:21,
                 from ./include/linux/device.h:17,
                 from drivers/gpu/drm/i915/gvt/kvmgt.c:32:
./include/linux/ptrace.h: In function ‘ptrace_init_task’:
./arch/x86/include/asm/current.h:17:17: error: passing argument 3 of ‘__ptrace_link’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
 #define current get_current()
                 ^
./include/linux/ptrace.h:210:41: note: in expansion of macro ‘current’
   __ptrace_link(child, current->parent, current->ptracer_cred);
                                         ^~~~~~~
In file included from ./arch/x86/include/asm/stacktrace.h:10:0,
                 from ./arch/x86/include/asm/perf_event.h:246,
                 from ./include/linux/perf_event.h:24,
                 from ./arch/x86/include/asm/kvm_host.h:24,
                 from ./include/linux/kvm_host.h:37,
                 from drivers/gpu/drm/i915/gvt/kvmgt.c:41:
./include/linux/ptrace.h:56:13: note: expected ‘struct cred *’ but argument is of type ‘const struct cred *’
 extern void __ptrace_link(struct task_struct *child,
             ^~~~~~~~~~~~~

Quoted 1 line

cc1: all warnings being treated as errors

Quoted 2 lines

Through a quick test on VM (fixed patch by adding const to __ptrace_link() argument), it seems working fine.

Thank you. It seems when I fixed the const issue in my tree I forget commit --amend so the fix didn't make it into the patch I sent out.

Quoted 1 line

Tested-by Takashi Iwai <[email protected]>

Just to confirm. You were able to reproduce the enlightenment failure and this fixes it?