From: ebiederm@xmission.com (Eric W. Biederman)
To: Kees Cook <keescook@chromium.org>
Cc: Cyrill Gorcunov <gorcunov@gmail.com>,
        Andrey Vagin <avagin@virtuozzo.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Pavel Emelyanov <xemul@virtuozzo.com>,
        Linux Containers <containers@lists.linux-foundation.org>
References: <20161024105959.GQ1847@uranus.lan> <8760oh8tbp.fsf@xmission.com>
        <20161024202925.GS1847@uranus.lan>
        <CAGXu5jK4p4bbQU1Bu-p9aM1GJ+CAR-FAHZcXXRH0De_M4VQ3wA@mail.gmail.com>
Date: Mon, 24 Oct 2016 18:11:04 -0500
In-Reply-To: <CAGXu5jK4p4bbQU1Bu-p9aM1GJ+CAR-FAHZcXXRH0De_M4VQ3wA@mail.gmail.com>
        (Kees Cook's message of "Mon, 24 Oct 2016 14:32:06 -0700")
Message-ID: <8760oh737b.fsf@xmission.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [ISSUE] mm: Add a user_ns owner to mm_struct and fix ptrace_may_access
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2182
Lines: 55

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.  

Eric