public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC] Allow introspection to already attached ptracer in __ptrace_may_access
@ 2014-12-24 13:28 Stijn Volckaert
  2014-12-24 18:06 ` Oleg Nesterov
  2015-01-05 23:47 ` Kees Cook
  0 siblings, 2 replies; 8+ messages in thread
From: Stijn Volckaert @ 2014-12-24 13:28 UTC (permalink / raw)
  To: Roland McGrath, Oleg Nesterov, Kees Cook; +Cc: linux-kernel

Hello,

I ran across the following problem recently but I'm not entirely sure 
whether this should be fixed in ptrace or in Yama. I'm working on a 
ptrace-based monitor that forks off its own tracee during startup. The 
monitor attaches to the tracee and then lets the tracee perform an 
execve call. This is much like running a program in gdb.

My monitor is multi-threaded and uses one monitor thread for every 
tracee thread so whenever the tracee forks/vforks/clones, I fire up a 
new monitor thread, detach the old monitor thread from the tracee thread 
and attach the new monitor thread to the tracee thread.

I have recently stumbled upon several applications in which the main 
process A forks off process B and then immediately exits. Under normal 
circumstances the following would happen:

Monitor[0]  ---   FORKS OFF   ---> Monitor[0]'
Monitor[0]  --- PTRACE_ATTACH ---> Monitor[0]'
Monitor[0]' ---    EXECVE     ---> Process A

Process A   ---   FORKS OFF   ---> Process B
Monitor[0]  --- PTRACE_DETACH ---> Process B
Monitor[1]  --- PTRACE_ATTACH ---> Process B

With Yama enabled (and the scope set to YAMA_SCOPE_RELATIONAL) however, 
a few interesting things can (and usually do) happen:

1) If Process A dies before Monitor[1] is attached to Process B, the 
attach will fail since from Yama's point of view, Process B is no longer 
a descendant of Monitor[1]. This problem is probably hard to fix
but I've circumvented it by delaying the death of Process A until 
Process B is attached to Monitor[1].

2) More interestingly though, even if Process B does get attached to 
Monitor[1], as soon as Process A dies, all process_vm_readv and 
process_vm_writev calls on Process B start failing. Any other ptrace 
operations peformed on Process B do succeed.

process_vm_readv|writev use __ptrace_may_access to check whether the 
operation is permitted, whereas other ptrace operations (with the 
exception of PTRACE_ATTACH) use ptrace_check_attach.

To fix this problem, __ptrace_may_access should be forced to return 0 if 
the calling process is already attached to the target process.

The question now is whether or not it's the security module's 
responsibility to check whether a tracee relationship is already in 
place or if ptrace itself should do it. For the latter case, which seems 
more logical to me, you could use the patch below.

What do you guys think?

Regards,
Stijn Volckaert

--

Signed-off-by: Stijn Volckaert <Stijn.Volckaert@elis.ugent.be>

--- a/kernel/ptrace.c	2014-12-24 13:53:23.055346526 +0100
+++ b/kernel/ptrace.c	2014-12-24 14:17:20.617824840 +0100
@@ -232,6 +232,9 @@ static int __ptrace_may_access(struct ta
  	/* Don't let security modules deny introspection */
  	if (same_thread_group(task, current))
  		return 0;
+	/* Don't deny introspection to already attached ptracer */
+	if (!ptrace_check_attach(task, true))
+	        return 0;
  	rcu_read_lock();
  	tcred = __task_cred(task);
  	if (uid_eq(cred->uid, tcred->euid) &&


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFC] Allow introspection to already attached ptracer in __ptrace_may_access
  2014-12-24 13:28 [PATCH RFC] Allow introspection to already attached ptracer in __ptrace_may_access Stijn Volckaert
@ 2014-12-24 18:06 ` Oleg Nesterov
  2015-01-05 23:47 ` Kees Cook
  1 sibling, 0 replies; 8+ messages in thread
From: Oleg Nesterov @ 2014-12-24 18:06 UTC (permalink / raw)
  To: Stijn Volckaert; +Cc: Roland McGrath, Kees Cook, linux-kernel

On 12/24, Stijn Volckaert wrote:
>
> The question now is whether or not it's the security module's
> responsibility to check whether a tracee relationship is already in
> place or if ptrace itself should do it.

Honestly, I have no idea

> --- a/kernel/ptrace.c	2014-12-24 13:53:23.055346526 +0100
> +++ b/kernel/ptrace.c	2014-12-24 14:17:20.617824840 +0100
> @@ -232,6 +232,9 @@ static int __ptrace_may_access(struct ta
>  	/* Don't let security modules deny introspection */
>  	if (same_thread_group(task, current))
>  		return 0;
> +	/* Don't deny introspection to already attached ptracer */
> +	if (!ptrace_check_attach(task, true))
> +	        return 0;

Perhaps this makes sense, probably security checks do not make sense
if the target is traced.

But in this case I'd suggest to simply

	if (ptrace_parent(task) == current)
		return 0;


Oleg.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFC] Allow introspection to already attached ptracer in __ptrace_may_access
  2014-12-24 13:28 [PATCH RFC] Allow introspection to already attached ptracer in __ptrace_may_access Stijn Volckaert
  2014-12-24 18:06 ` Oleg Nesterov
@ 2015-01-05 23:47 ` Kees Cook
  2015-01-06  0:17   ` Casey Schaufler
                     ` (2 more replies)
  1 sibling, 3 replies; 8+ messages in thread
From: Kees Cook @ 2015-01-05 23:47 UTC (permalink / raw)
  To: Stijn Volckaert
  Cc: Roland McGrath, Oleg Nesterov, LKML, linux-security-module,
	Stephen Smalley, Casey Schaufler, John Johansen, Tetsuo Handa

On Wed, Dec 24, 2014 at 5:28 AM, Stijn Volckaert
<Stijn.Volckaert@elis.ugent.be> wrote:
> Hello,
>
> I ran across the following problem recently but I'm not entirely sure
> whether this should be fixed in ptrace or in Yama. I'm working on a
> ptrace-based monitor that forks off its own tracee during startup. The
> monitor attaches to the tracee and then lets the tracee perform an execve
> call. This is much like running a program in gdb.
>
> My monitor is multi-threaded and uses one monitor thread for every tracee
> thread so whenever the tracee forks/vforks/clones, I fire up a new monitor
> thread, detach the old monitor thread from the tracee thread and attach the
> new monitor thread to the tracee thread.
>
> I have recently stumbled upon several applications in which the main process
> A forks off process B and then immediately exits. Under normal circumstances
> the following would happen:
>
> Monitor[0]  ---   FORKS OFF   ---> Monitor[0]'
> Monitor[0]  --- PTRACE_ATTACH ---> Monitor[0]'
> Monitor[0]' ---    EXECVE     ---> Process A
>
> Process A   ---   FORKS OFF   ---> Process B
> Monitor[0]  --- PTRACE_DETACH ---> Process B
> Monitor[1]  --- PTRACE_ATTACH ---> Process B
>
> With Yama enabled (and the scope set to YAMA_SCOPE_RELATIONAL) however, a
> few interesting things can (and usually do) happen:
>
> 1) If Process A dies before Monitor[1] is attached to Process B, the attach
> will fail since from Yama's point of view, Process B is no longer a
> descendant of Monitor[1]. This problem is probably hard to fix
> but I've circumvented it by delaying the death of Process A until Process B
> is attached to Monitor[1].

Just to make sure I understand this better, "Monitor" is the initial
process, and [0] and [1] are separate threads within that process? I
would expect B to have Monitor as its parent after A died, but I must
be misunderstanding something.

Regardless, your "interesting thing 1" is certainly a side-effect of
YAMA_SCOPE_RELATIONAL trying to do its job.

> 2) More interestingly though, even if Process B does get attached to
> Monitor[1], as soon as Process A dies, all process_vm_readv and
> process_vm_writev calls on Process B start failing. Any other ptrace
> operations peformed on Process B do succeed.
>
> process_vm_readv|writev use __ptrace_may_access to check whether the
> operation is permitted, whereas other ptrace operations (with the exception
> of PTRACE_ATTACH) use ptrace_check_attach.

Right, process_vm_{read,write}v use PTRACE_MODE_ATTACH (which is what
Yama interposes via the LSM entry point in __ptrace_may_access).

> To fix this problem, __ptrace_may_access should be forced to return 0 if the
> calling process is already attached to the target process.
>
> The question now is whether or not it's the security module's responsibility
> to check whether a tracee relationship is already in place or if ptrace
> itself should do it. For the latter case, which seems more logical to me,
> you could use the patch below.
>
> What do you guys think?
>
> Regards,
> Stijn Volckaert
>
> --
>
> Signed-off-by: Stijn Volckaert <Stijn.Volckaert@elis.ugent.be>
>
> --- a/kernel/ptrace.c   2014-12-24 13:53:23.055346526 +0100
> +++ b/kernel/ptrace.c   2014-12-24 14:17:20.617824840 +0100
> @@ -232,6 +232,9 @@ static int __ptrace_may_access(struct ta
>         /* Don't let security modules deny introspection */
>         if (same_thread_group(task, current))
>                 return 0;
> +       /* Don't deny introspection to already attached ptracer */
> +       if (!ptrace_check_attach(task, true))
> +               return 0;
>         rcu_read_lock();
>         tcred = __task_cred(task);
>         if (uid_eq(cred->uid, tcred->euid) &&
>

I'm nervous to add this (or Oleg's suggestion) generally to
__ptrace_may_access, as it would mean other LSMs would stop seeing
access checks even when attached. It does seem silly to deny ptrace
checks when already attached, but it does change the behavior here.

If the other LSM folks don't see a problem here, then it should live
in the general case. Otherwise, I'm happy to add this check only in
Yama. The existing Yama scopes should ignore attach requests when
already attached.

-Kees

-- 
Kees Cook
Chrome OS Security

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFC] Allow introspection to already attached ptracer in __ptrace_may_access
  2015-01-05 23:47 ` Kees Cook
@ 2015-01-06  0:17   ` Casey Schaufler
  2015-01-06  9:07     ` Stijn Volckaert
  2015-01-06 18:44   ` Oleg Nesterov
  2015-01-08 10:40   ` Stijn Volckaert
  2 siblings, 1 reply; 8+ messages in thread
From: Casey Schaufler @ 2015-01-06  0:17 UTC (permalink / raw)
  To: Kees Cook, Stijn Volckaert
  Cc: Roland McGrath, Oleg Nesterov, LKML, linux-security-module,
	Stephen Smalley, John Johansen, Tetsuo Handa, Casey Schaufler

On 1/5/2015 3:47 PM, Kees Cook wrote:
> On Wed, Dec 24, 2014 at 5:28 AM, Stijn Volckaert
> <Stijn.Volckaert@elis.ugent.be> wrote:
>> Hello,
>>
>> I ran across the following problem recently but I'm not entirely sure
>> whether this should be fixed in ptrace or in Yama. I'm working on a
>> ptrace-based monitor that forks off its own tracee during startup. The
>> monitor attaches to the tracee and then lets the tracee perform an execve
>> call. This is much like running a program in gdb.
>>
>> My monitor is multi-threaded and uses one monitor thread for every tracee
>> thread so whenever the tracee forks/vforks/clones, I fire up a new monitor
>> thread, detach the old monitor thread from the tracee thread and attach the
>> new monitor thread to the tracee thread.
>>
>> I have recently stumbled upon several applications in which the main process
>> A forks off process B and then immediately exits. Under normal circumstances
>> the following would happen:
>>
>> Monitor[0]  ---   FORKS OFF   ---> Monitor[0]'
>> Monitor[0]  --- PTRACE_ATTACH ---> Monitor[0]'
>> Monitor[0]' ---    EXECVE     ---> Process A
>>
>> Process A   ---   FORKS OFF   ---> Process B
>> Monitor[0]  --- PTRACE_DETACH ---> Process B
>> Monitor[1]  --- PTRACE_ATTACH ---> Process B
>>
>> With Yama enabled (and the scope set to YAMA_SCOPE_RELATIONAL) however, a
>> few interesting things can (and usually do) happen:
>>
>> 1) If Process A dies before Monitor[1] is attached to Process B, the attach
>> will fail since from Yama's point of view, Process B is no longer a
>> descendant of Monitor[1]. This problem is probably hard to fix
>> but I've circumvented it by delaying the death of Process A until Process B
>> is attached to Monitor[1].
> Just to make sure I understand this better, "Monitor" is the initial
> process, and [0] and [1] are separate threads within that process? I
> would expect B to have Monitor as its parent after A died, but I must
> be misunderstanding something.
>
> Regardless, your "interesting thing 1" is certainly a side-effect of
> YAMA_SCOPE_RELATIONAL trying to do its job.
>
>> 2) More interestingly though, even if Process B does get attached to
>> Monitor[1], as soon as Process A dies, all process_vm_readv and
>> process_vm_writev calls on Process B start failing. Any other ptrace
>> operations peformed on Process B do succeed.
>>
>> process_vm_readv|writev use __ptrace_may_access to check whether the
>> operation is permitted, whereas other ptrace operations (with the exception
>> of PTRACE_ATTACH) use ptrace_check_attach.
> Right, process_vm_{read,write}v use PTRACE_MODE_ATTACH (which is what
> Yama interposes via the LSM entry point in __ptrace_may_access).
>
>> To fix this problem, __ptrace_may_access should be forced to return 0 if the
>> calling process is already attached to the target process.
>>
>> The question now is whether or not it's the security module's responsibility
>> to check whether a tracee relationship is already in place or if ptrace
>> itself should do it. For the latter case, which seems more logical to me,
>> you could use the patch below.
>>
>> What do you guys think?
>>
>> Regards,
>> Stijn Volckaert
>>
>> --
>>
>> Signed-off-by: Stijn Volckaert <Stijn.Volckaert@elis.ugent.be>
>>
>> --- a/kernel/ptrace.c   2014-12-24 13:53:23.055346526 +0100
>> +++ b/kernel/ptrace.c   2014-12-24 14:17:20.617824840 +0100
>> @@ -232,6 +232,9 @@ static int __ptrace_may_access(struct ta
>>         /* Don't let security modules deny introspection */
>>         if (same_thread_group(task, current))
>>                 return 0;
>> +       /* Don't deny introspection to already attached ptracer */
>> +       if (!ptrace_check_attach(task, true))
>> +               return 0;
>>         rcu_read_lock();
>>         tcred = __task_cred(task);
>>         if (uid_eq(cred->uid, tcred->euid) &&
>>
> I'm nervous to add this (or Oleg's suggestion) generally to
> __ptrace_may_access, as it would mean other LSMs would stop seeing
> access checks even when attached. It does seem silly to deny ptrace
> checks when already attached, but it does change the behavior here.

An LSM may chose to do checks on a per access basis. Think in terms
of access checks on read/write instead of open. Smack and SELinux
do this for some network checks. It is reasonable to think that there
is a case where a security attribute (or access rule) could change
between the attach and the access.

Example: You allow the access when the developer mode switch is
set, but not when it isn't. Someone flips the switch.

>
> If the other LSM folks don't see a problem here, then it should live
> in the general case. Otherwise, I'm happy to add this check only in
> Yama. The existing Yama scopes should ignore attach requests when
> already attached.
>
> -Kees
>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFC] Allow introspection to already attached ptracer in __ptrace_may_access
  2015-01-06  0:17   ` Casey Schaufler
@ 2015-01-06  9:07     ` Stijn Volckaert
  0 siblings, 0 replies; 8+ messages in thread
From: Stijn Volckaert @ 2015-01-06  9:07 UTC (permalink / raw)
  To: Casey Schaufler, Kees Cook
  Cc: Roland McGrath, Oleg Nesterov, LKML, linux-security-module,
	Stephen Smalley, John Johansen, Tetsuo Handa

Casey Schaufler schreef op 6/01/2015 om 1:17:
> On 1/5/2015 3:47 PM, Kees Cook wrote:
>> On Wed, Dec 24, 2014 at 5:28 AM, Stijn Volckaert
>> <Stijn.Volckaert@elis.ugent.be> wrote:
>>> Hello,
>>>
>>> I ran across the following problem recently but I'm not entirely sure
>>> whether this should be fixed in ptrace or in Yama. I'm working on a
>>> ptrace-based monitor that forks off its own tracee during startup. The
>>> monitor attaches to the tracee and then lets the tracee perform an execve
>>> call. This is much like running a program in gdb.
>>>
>>> My monitor is multi-threaded and uses one monitor thread for every tracee
>>> thread so whenever the tracee forks/vforks/clones, I fire up a new monitor
>>> thread, detach the old monitor thread from the tracee thread and attach the
>>> new monitor thread to the tracee thread.
>>>
>>> I have recently stumbled upon several applications in which the main process
>>> A forks off process B and then immediately exits. Under normal circumstances
>>> the following would happen:
>>>
>>> Monitor[0]  ---   FORKS OFF   ---> Monitor[0]'
>>> Monitor[0]  --- PTRACE_ATTACH ---> Monitor[0]'
>>> Monitor[0]' ---    EXECVE     ---> Process A
>>>
>>> Process A   ---   FORKS OFF   ---> Process B
>>> Monitor[0]  --- PTRACE_DETACH ---> Process B
>>> Monitor[1]  --- PTRACE_ATTACH ---> Process B
>>>
>>> With Yama enabled (and the scope set to YAMA_SCOPE_RELATIONAL) however, a
>>> few interesting things can (and usually do) happen:
>>>
>>> 1) If Process A dies before Monitor[1] is attached to Process B, the attach
>>> will fail since from Yama's point of view, Process B is no longer a
>>> descendant of Monitor[1]. This problem is probably hard to fix
>>> but I've circumvented it by delaying the death of Process A until Process B
>>> is attached to Monitor[1].
>> Just to make sure I understand this better, "Monitor" is the initial
>> process, and [0] and [1] are separate threads within that process? I
>> would expect B to have Monitor as its parent after A died, but I must
>> be misunderstanding something.
>>
>> Regardless, your "interesting thing 1" is certainly a side-effect of
>> YAMA_SCOPE_RELATIONAL trying to do its job.
Correct. Process B is obviously still descendant from the process that 
is ptracing it so you'd
think that this would work with YAMA_SCOPE_RELATIONAL. However, after 
process A dies,
/sbin/init becomes the new parent of Process B. If you want to see this 
yourselves, you can
reproduce this problem with this little test program:

$ cat ptrace_bug.c
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <string.h>

void daemon_process() {
   printf("pre-open\n");
   int fd = open("test.txt", O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
   if (fd != -1) {
     write(fd, "post-open\n", strlen("post-open\n"));
     close(fd);
   }
   exit(0);
}

int main(int argc, char** argv) {
   if (fork() == 0)
       daemon_process();
   return 0;
}
$ gcc -o ptrace_bug ptrace_bug.c

If I strace this on a machine which I've patched with my proposed patch 
below, I see the following
output for the "daemon_process":

$ strace -o strace_out -ff ./ptrace_bug
$ cat strace_out.2815
...
write(1, "pre-open\n", 9)               = 9
open("test.txt", O_RDWR|O_CREAT, 0600)  = 3
write(3, "post-open\n", 10)             = 10
...

However, on a machine which I haven't patched and with 
YAMA_SCOPE_RELATIONAL, I see:
...
write(1, 0x7f74338c0000, 9)             = 9
open(0x4007bd, O_RDWR|O_CREAT, 0600)    = 3
write(3, 0x4007c6, 10)                  = 10
...

This is the exact same problem: you can still read the regs and 
stop/resume tracees but
you can't do process_vm_{read,write}v or the ptrace ops that depend on it.

If I now add a pause() call just before the exit of the daemon process, 
you'll see the following
after process A dies:

$ ps u
USER       PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
stijn    29772  0.0  0.0   4852   820 pts/0    S+   09:41   0:00 strace 
-o strace_out -ff ./ptrace_bug
stijn    29778  0.0  0.0   4300    92 pts/0    S+   09:41   0:00 
./ptrace_bug

$ cat /proc/29778/status
Name:    ptrace_bug
State:    S (sleeping)
Tgid:    29778
Ngid:    0
Pid:    29778
PPid:    1
TracerPid:    29772

PPid is 1 so /sbin/init is now the real parent. As far as I can tell, 
this whole reparenting goes on
in kernel/exit.c due to:

do_exit
   -> exit_notify
     -> forget_original_parent
       -> find_new_reaper

 From my viewpoint, it would make more sense to reparent Process B to 
the ptracer that originally
forked off Process A. However, looking at the exit.c code, what actually 
goes on does seem like
the intended behavior.

>>> 2) More interestingly though, even if Process B does get attached to
>>> Monitor[1], as soon as Process A dies, all process_vm_readv and
>>> process_vm_writev calls on Process B start failing. Any other ptrace
>>> operations peformed on Process B do succeed.
>>>
>>> process_vm_readv|writev use __ptrace_may_access to check whether the
>>> operation is permitted, whereas other ptrace operations (with the exception
>>> of PTRACE_ATTACH) use ptrace_check_attach.
>> Right, process_vm_{read,write}v use PTRACE_MODE_ATTACH (which is what
>> Yama interposes via the LSM entry point in __ptrace_may_access).
>>
>>> To fix this problem, __ptrace_may_access should be forced to return 0 if the
>>> calling process is already attached to the target process.
>>>
>>> The question now is whether or not it's the security module's responsibility
>>> to check whether a tracee relationship is already in place or if ptrace
>>> itself should do it. For the latter case, which seems more logical to me,
>>> you could use the patch below.
>>>
>>> What do you guys think?
>>>
>>> Regards,
>>> Stijn Volckaert
>>>
>>> --
>>>
>>> Signed-off-by: Stijn Volckaert <Stijn.Volckaert@elis.ugent.be>
>>>
>>> --- a/kernel/ptrace.c   2014-12-24 13:53:23.055346526 +0100
>>> +++ b/kernel/ptrace.c   2014-12-24 14:17:20.617824840 +0100
>>> @@ -232,6 +232,9 @@ static int __ptrace_may_access(struct ta
>>>          /* Don't let security modules deny introspection */
>>>          if (same_thread_group(task, current))
>>>                  return 0;
>>> +       /* Don't deny introspection to already attached ptracer */
>>> +       if (!ptrace_check_attach(task, true))
>>> +               return 0;
>>>          rcu_read_lock();
>>>          tcred = __task_cred(task);
>>>          if (uid_eq(cred->uid, tcred->euid) &&
>>>
>> I'm nervous to add this (or Oleg's suggestion) generally to
>> __ptrace_may_access, as it would mean other LSMs would stop seeing
>> access checks even when attached. It does seem silly to deny ptrace
>> checks when already attached, but it does change the behavior here.
> An LSM may chose to do checks on a per access basis. Think in terms
> of access checks on read/write instead of open. Smack and SELinux
> do this for some network checks. It is reasonable to think that there
> is a case where a security attribute (or access rule) could change
> between the attach and the access.
>
> Example: You allow the access when the developer mode switch is
> set, but not when it isn't. Someone flips the switch.
That makes sense I guess. However, the problem in my case was not that 
the security policy
had change between the "open" and the "read/write" check. Instead, after 
the death of
Process A, the security check now lacks the context to see the 
relationship between
the tracer and the tracee.

>> If the other LSM folks don't see a problem here, then it should live
>> in the general case. Otherwise, I'm happy to add this check only in
>> Yama. The existing Yama scopes should ignore attach requests when
>> already attached.
>>
>> -Kees
>>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFC] Allow introspection to already attached ptracer in __ptrace_may_access
  2015-01-05 23:47 ` Kees Cook
  2015-01-06  0:17   ` Casey Schaufler
@ 2015-01-06 18:44   ` Oleg Nesterov
  2015-01-08 10:40   ` Stijn Volckaert
  2 siblings, 0 replies; 8+ messages in thread
From: Oleg Nesterov @ 2015-01-06 18:44 UTC (permalink / raw)
  To: Kees Cook
  Cc: Stijn Volckaert, Roland McGrath, LKML, linux-security-module,
	Stephen Smalley, Casey Schaufler, John Johansen, Tetsuo Handa

On 01/05, Kees Cook wrote:
>
> I'm nervous to add this (or Oleg's suggestion) generally to
> __ptrace_may_access, as it would mean other LSMs would stop seeing
> access checks even when attached. It does seem silly to deny ptrace
> checks when already attached, but it does change the behavior here.

Same here.

> If the other LSM folks don't see a problem here, then it should live
> in the general case. Otherwise, I'm happy to add this check only in
> Yama.

In this case this check should probably go into ptracer_exception_found().

Btw it looks buggy... RCU protects ptrace_relation object, but not
relation->tracer (the final __put_task_struct() calls yama_task_free()
and frees task_struct without rcu gp).

This means that task_is_descendant(parent, tracer) can dereference the
already freed/unmapped parent, no?


And the usage of ->group_leader looks strange. tracee->group_leader can
point to nowhere if the task is already dead. In this case
"relation->tracee == tracee" can be false-positive (the same task_struct
can be re-allocated), but probably this is not that bad exactly because
the task is dead anyway.


prctl(PR_SET_PTRACER) looks strange too wrt group_leader. What if the caller
execs later? The comment says "we want process-level granularity of control",
but this is only true if the main thread does exec.

And get/out_task_struct(myself) look unneeded.


And it seems that task_is_descendant() doesn't need ->group_leader at all,
it could simply do

	static int task_is_descendant(struct task_struct *parent,
				      struct task_struct *child)
	{
		int rc = 0;
		struct task_struct *walker = child;

		if (!parent || !child)
			return 0;

		rcu_read_lock();
		do {
			if (same_thread_group(walker, parent)) {
				rc = 1;
				break;
			}
			walker = rcu_dereference(walker->real_parent);
		} while (walker != &init_task);
		rcu_read_unlock();

		return rc;
	}

Oleg.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFC] Allow introspection to already attached ptracer in __ptrace_may_access
  2015-01-05 23:47 ` Kees Cook
  2015-01-06  0:17   ` Casey Schaufler
  2015-01-06 18:44   ` Oleg Nesterov
@ 2015-01-08 10:40   ` Stijn Volckaert
  2015-01-08 19:18     ` Oleg Nesterov
  2 siblings, 1 reply; 8+ messages in thread
From: Stijn Volckaert @ 2015-01-08 10:40 UTC (permalink / raw)
  To: Kees Cook
  Cc: Roland McGrath, Oleg Nesterov, LKML, linux-security-module,
	Stephen Smalley, Casey Schaufler, John Johansen, Tetsuo Handa

Kees Cook schreef op 6/01/2015 om 0:47:
> Just to make sure I understand this better, "Monitor" is the initial
> process, and [0] and [1] are separate threads within that process? I
> would expect B to have Monitor as its parent after A died, but I must
> be misunderstanding something.
>
> Regardless, your "interesting thing 1" is certainly a side-effect of
> YAMA_SCOPE_RELATIONAL trying to do its job.

I've thought about it some more and there's actually a much easier way to show
what is going on here. Suppose that you have a debugger that forks off its own
tracee and then traces that tracee PLUS any of that tracee's future childs.
At some point you may get:

+----------+                      +----------+
| DEBUGGER | -- FORK + ATTACH --> | TRACEE A |
+----------+                      +----------+
      |                                 |
      |                                 |
      |                                FORK
    ATTACH                              |
      |                                 |
      |                                 V
      |                            +----------+
      +--------------------------> | TRACEE B |
                                   +----------+

Now at this point, if tracee A dies, tracee B will get reparented to the init
process. From that point onwards, the debugger can no longer perform any
operations that go through __ptrace_may_access. These include
process_vm_{read,write}v but also PTRACE_{POKE,PEEK}{TEXT,DATA}. I don't see
how Yama can possibly tell that tracee B and the debugger are still related
at this point so I see no easy fix for this. Patching __ptrace_may_access
might indeed not be a good idea as it is used to check for credentials to
perform a bunch of other non-ptrace operations throughout the kernel.

The possible solutions that I can see right now are:
1) Adding some sort of original_parent field to task_struct, just for the
sake of relationship tracking
2) Changing the credentials check in process_vm_{read,write}v only so that
you don't go all the way into __ptrace_may_access if you're already attached
as a ptracer

-- Stijn


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFC] Allow introspection to already attached ptracer in __ptrace_may_access
  2015-01-08 10:40   ` Stijn Volckaert
@ 2015-01-08 19:18     ` Oleg Nesterov
  0 siblings, 0 replies; 8+ messages in thread
From: Oleg Nesterov @ 2015-01-08 19:18 UTC (permalink / raw)
  To: Stijn Volckaert
  Cc: Kees Cook, Roland McGrath, LKML, linux-security-module,
	Stephen Smalley, Casey Schaufler, John Johansen, Tetsuo Handa

On 01/08, Stijn Volckaert wrote:
>
> From that point onwards, the debugger can no longer perform any
> operations that go through __ptrace_may_access. These include
> process_vm_{read,write}v but also PTRACE_{POKE,PEEK}{TEXT,DATA}.

PTRACE_POKE/PEEK should work? not that this really matters...

> I don't see
> how Yama can possibly tell that tracee B and the debugger are still related
> at this point so I see no easy fix for this.

Well, perhaps yama can also have PR_SET_PTRACEE, I dunno.

> Patching __ptrace_may_access
> might indeed not be a good idea as it is used to check for credentials to
> perform a bunch of other non-ptrace operations throughout the kernel.

Just add the "ptrace_parent(task) == current" check into
ptracer_exception_found() ?

> The possible solutions that I can see right now are:
> 1) Adding some sort of original_parent field to task_struct, just for the
> sake of relationship tracking

And what should we do with this pointer if original_parent exits?

> 2) Changing the credentials check in process_vm_{read,write}v only so that
> you don't go all the way into __ptrace_may_access if you're already attached
> as a ptracer

This really doesn't look good, imo.

Oleg.


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2015-01-08 19:20 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-24 13:28 [PATCH RFC] Allow introspection to already attached ptracer in __ptrace_may_access Stijn Volckaert
2014-12-24 18:06 ` Oleg Nesterov
2015-01-05 23:47 ` Kees Cook
2015-01-06  0:17   ` Casey Schaufler
2015-01-06  9:07     ` Stijn Volckaert
2015-01-06 18:44   ` Oleg Nesterov
2015-01-08 10:40   ` Stijn Volckaert
2015-01-08 19:18     ` Oleg Nesterov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox