lkml.org 
[lkml]   [2016]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [ISSUE] mm: Add a user_ns owner to mm_struct and fix ptrace_may_access
On Mon, Oct 24, 2016 at 06:11:04PM -0500, Eric W. Biederman wrote:
> Kees Cook <keescook@chromium.org> writes:
>
> > On Mon, Oct 24, 2016 at 1:29 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> >> On Mon, Oct 24, 2016 at 02:01:30PM -0500, Eric W. Biederman wrote:
> >>> So I am probably going to tweak the !mm case so that instead of failing
> >>> we perform the old capable check in that case. That seems the mot
> >>> certain way to avoid regressions. With that said, why is exit_code
> >>> behind a ptrace_may_access permission check?
> >>
> >> Yes, this would be great! And as to @exit_code I think better ask
> >> Kees, CC'ed.
> >
> > My concern was that this was an exposure in the sense that it is
> > internal program state that isn't visible through other means (without
> > being the parent, for example). Under the ptrace check, it has an
> > equivalency that seemed correct at the time.
> >
> > As already covered, I'd agree: it looks like ce99dd5fd5f6 accidentally
> > added a dependency on task->mm where it didn't before. That section of
> > logic was entirely around dumpability, not an mm existing. It should
> > be "EPERM if mm and dumpable != SUID_DUMP_USER".
> >
> > That said, I'd also agree that ptrace against no mm is crazy (though I
> > suppose that should return EINVAL or ESRCH or something), so perhaps a
> > better access control on @exit_code is needed here.
>
> I traced down the original logic of why we had that dumpable
> variable, and it was ancient conservative on my part when we started
> using the ptrace permission checks for proc files.
>
> That same conservatism has resulted in the regression under
> discussion.
>
> Given that we already have a very full set of permission checks
> separate from dumpable in ptrace_may_access I think it makes sense
> to simply ignore dumpable when there is no mm.
> AKA:
> mm = task->mm;
> if (mm &&
> ((get_dumpable(mm) != SUID_DUMP_USER) &&
> !ptrace_has_cap(mm->user_ns, mode)))
> return -EPERM;
>
> Because while it has been used for other things dumpable is
> fundamentally about do you have permission to read the mm.
> So there is no real point in permission checks that protect
> the mm if the mm has gone away.
>
> It also looks like I may need to update the check that sets
> PT_PTRACE_CAP to look at mm->user_ns as well.

Thanks a lot for informative explanations, Eric and Kees!
Eric, if you make some patch please ping me to test it.

\
 
 \ /
  Last update: 2016-10-25 11:03    [W:0.079 / U:1.856 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site