Re: [PATCH RFC] Allow introspection to already attached ptracer in __ptrace_may_access

[Casey Schaufler <casey@schaufler-ca.com>]

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
>