Messages in this thread | | | Date | Tue, 29 Jun 2010 22:27:52 -0700 | From | Kees Cook <> | Subject | Re: [PATCH 2/2] Yama: add PTRACE exception tracking |
| |
Hi Serge,
On Tue, Jun 29, 2010 at 10:56:09PM -0500, Serge E. Hallyn wrote: > Quoting Kees Cook (kees.cook@canonical.com): > > Some application suites have external crash handlers that depend on > > being able to use PTRACE to generate crash reports (KDE, Chromium, etc). > > Since the inferior process generally knows the PID of the debugger, > > it can use PR_SET_PTRACER to allow a specific PID and its descendants > > to perform the PTRACE instead of only a direct ancestor. > > > > Signed-off-by: Kees Cook <kees.cook@canonical.com> > > --- > > Hi Kees - very nice, overall. One little note though:
Thanks for looking it over!
> > rc = cap_ptrace_access_check(child, mode); > > This means that if capable(CAP_SYS_PTRACE) we'll always shortcut > here, so > > > + if (mode == PTRACE_MODE_ATTACH && > > + ptrace_scope && > > + !capable(CAP_SYS_PTRACE) && > > + !task_is_descendant(current, child) && > > + !ptracer_exception_found(current, child)) > > + rc = -EPERM; > > You don't need the CAP_SYS_PTRACE check here AFAICS.
I don't think that's true -- the capable(CAP_SYS_PTRACE) tests are always done in the negative since we only ever abort with error instead of forcing an early "okay" (see also __ptrace_may_access() in kernel/ptrace.c, where capable(CAP_SYS_PTRACE) is called repeatedly while evaluating various negative conditions).
For cap_ptrace_access_check, capable(CAP_SYS_PTRACE) is only tested if the tracee's process permitted caps are not a subset of the tracer's. i.e. cap_ptrace_access_check will return 0 when either cap_issubset or capable. In the case of normal user processes or a tracer with greater caps, capable(CAP_SYS_PTRACE) will never be tested in cap_ptrace_access_check.
As a result, I have to include it in the test here too. I guess it's arguable that I should move it to the end of the series of &&s, but it logically doesn't really matter.
-Kees
(Interestingly, this means that having CAP_SYS_PTRACE means a process effectively has all capabilities...)
-- Kees Cook Ubuntu Security Team
| |