linux-kernel - Re: [ISSUE] mm: Add a user_ns owner to mm_struct and fix ptrace_may

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161025090213.GX1847@uranus.lan>
Date:   Tue, 25 Oct 2016 12:02:13 +0300
From:   Cyrill Gorcunov <gorcunov@...il.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Kees Cook <keescook@...omium.org>,
        Andrey Vagin <avagin@...tuozzo.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Pavel Emelyanov <xemul@...tuozzo.com>,
        Linux Containers <containers@...ts.linux-foundation.org>
Subject: Re: [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@...omium.org> writes:
> 
> > On Mon, Oct 24, 2016 at 1:29 PM, Cyrill Gorcunov <gorcunov@...il.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.