From: ebiederm@xmission.com (Eric W. Biederman)
To: Jann Horn <jann@thejh.net>
Cc: Michal Hocko <mhocko@kernel.org>,
linux-kernel@vger.kernel.org,
Linux Containers <containers@lists.linux-foundation.org>,
Oleg Nesterov <oleg@redhat.com>,
Andy Lutomirski <luto@amacapital.net>,
linux-mm@kvack.org, linux-fsdevel@vger.kernel.org
Subject: Re: [REVIEW][PATCH] mm: Add a user_ns owner to mm_struct and fix ptrace_may_access
Date: Tue, 18 Oct 2016 10:35:23 -0500 [thread overview]
Message-ID: <87twc9656s.fsf@xmission.com> (raw)
In-Reply-To: <20161018150507.GP14666@pc.thejh.net> (Jann Horn's message of "Tue, 18 Oct 2016 17:05:07 +0200")
Jann Horn <jann@thejh.net> writes:
> On Tue, Oct 18, 2016 at 09:56:53AM -0500, Eric W. Biederman wrote:
>> Michal Hocko <mhocko@kernel.org> writes:
>>
>> > On Mon 17-10-16 11:39:49, Eric W. Biederman wrote:
>> >>
>> >> During exec dumpable is cleared if the file that is being executed is
>> >> not readable by the user executing the file. A bug in
>> >> ptrace_may_access allows reading the file if the executable happens to
>> >> enter into a subordinate user namespace (aka clone(CLONE_NEWUSER),
>> >> unshare(CLONE_NEWUSER), or setns(fd, CLONE_NEWUSER).
>> >>
>> >> This problem is fixed with only necessary userspace breakage by adding
>> >> a user namespace owner to mm_struct, captured at the time of exec,
>> >> so it is clear in which user namespace CAP_SYS_PTRACE must be present
>> >> in to be able to safely give read permission to the executable.
>> >>
>> >> The function ptrace_may_access is modified to verify that the ptracer
>> >> has CAP_SYS_ADMIN in task->mm->user_ns instead of task->cred->user_ns.
>> >> This ensures that if the task changes it's cred into a subordinate
>> >> user namespace it does not become ptraceable.
>> >
>> > I haven't studied your patch too deeply but one thing that immediately
>> > raised a red flag was that mm might be shared between processes (aka
>> > thread groups). What prevents those two to sit in different user
>> > namespaces?
>> >
>> > I am primarily asking because this generated a lot of headache for the
>> > memcg handling as those processes might sit in different cgroups while
>> > there is only one correct memcg for them which can disagree with the
>> > cgroup associated with one of the processes.
>>
>> That is a legitimate concern, but I do not see any of those kinds of
>> issues here.
>>
>> Part of the memcg pain comes from the fact that control groups are
>> process centric, and part of the pain comes from the fact that it is
>> possible to change control groups. What I am doing is making the mm
>> owned by a user namespace (at creation time), and I am not allowing
>> changes to that ownership. The credentials of the tasks that use that mm
>> may be in the same user namespace or descendent user namespaces.
>>
>> The core goal is to enforce the unreadability of an mm when an
>> non-readable file is executed. This is a time of mm creation property.
>> The enforcement of which fits very well with the security/permission
>> checking role of the user namespace.
>
> How is that going to work? I thought the core goal was better security for
> entering containers.
The better security when entering containers came from fixing the the
check for unreadable files. Because that is fundamentally what
the mm dumpable settings are for.
> If I want to dump a non-readable file, afaik, I can just make a new user
> namespace, then run the file in there and dump its memory.
> I guess you could fix that by entirely prohibiting the execution of a
> non-readable file whose owner UID is not mapped. (Adding more dumping
> restrictions wouldn't help much because you could still e.g. supply a
> malicious dynamic linker if you control the mount namespace.)
That seems to be a part of this puzzle I have incompletely addressed,
thank you.
It looks like I need to change either the owning user namespace or
fail the exec. Malicious dynamic linkers are doubly interesting.
As mount name spaces are also owned if I have privileges I can address
the possibility of a malicious dynamic linker that way. AKA who cares
about the link if the owner of the mount namespace has permissions to
read the file.
I am going to look at failing the exec if the owning user namespace
of the mm would not have permissions to read the file. That should just
be a couple of lines of code and easy to maintain. Plus it does not
appear that non-readable executables are particularly common.
Eric
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2016-10-18 15:37 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-17 16:39 Eric W. Biederman
2016-10-17 17:25 ` Jann Horn
2016-10-17 17:33 ` Eric W. Biederman
2016-10-18 13:50 ` Michal Hocko
2016-10-18 13:57 ` Jann Horn
2016-10-18 14:56 ` Eric W. Biederman
2016-10-18 15:05 ` Jann Horn
2016-10-18 15:35 ` Eric W. Biederman [this message]
2016-10-18 19:12 ` Jann Horn
2016-10-18 21:07 ` Eric W. Biederman
2016-10-18 21:15 ` [REVIEW][PATCH] exec: Don't exec files the userns root can not read Eric W. Biederman
2016-10-19 6:13 ` Amir Goldstein
2016-10-19 13:33 ` Eric W. Biederman
2016-10-19 17:04 ` Eric W. Biederman
2016-10-19 15:30 ` Andy Lutomirski
2016-10-19 16:52 ` Eric W. Biederman
2016-10-19 17:29 ` Jann Horn
2016-10-19 17:32 ` Andy Lutomirski
2016-10-19 17:55 ` Eric W. Biederman
2016-10-19 18:38 ` Andy Lutomirski
2016-10-19 21:26 ` Eric W. Biederman
2016-10-19 23:17 ` Andy Lutomirski
2016-11-17 17:02 ` [REVIEW][PATCH 0/3] Fixing ptrace vs exec vs userns interactions Eric W. Biederman
2016-11-17 17:05 ` [REVIEW][PATCH 1/3] ptrace: Capture the ptracer's creds not PT_PTRACE_CAP Eric W. Biederman
2016-11-17 23:14 ` Kees Cook
2016-11-18 18:56 ` Eric W. Biederman
2016-11-17 23:27 ` Andy Lutomirski
2016-11-17 23:44 ` Eric W. Biederman
2016-11-17 17:08 ` [REVIEW][PATCH 2/3] exec: Don't allow ptracing an exec of an unreadable file Eric W. Biederman
2016-11-17 20:47 ` Willy Tarreau
2016-11-17 21:07 ` Kees Cook
2016-11-17 21:32 ` Willy Tarreau
2016-11-17 21:51 ` Eric W. Biederman
2016-11-17 22:50 ` [REVIEW][PATCH 2/3] ptrace: Don't allow accessing an undumpable mm Eric W. Biederman
2016-11-17 23:17 ` Kees Cook
2016-11-17 23:28 ` [REVIEW][PATCH 2/3] exec: Don't allow ptracing an exec of an unreadable file Andy Lutomirski
2016-11-17 23:29 ` Andy Lutomirski
2016-11-17 23:55 ` Eric W. Biederman
2016-11-18 0:10 ` Andy Lutomirski
2016-11-18 0:35 ` Eric W. Biederman
2016-11-17 17:10 ` [REVIEW][PATCH 3/3] exec: Ensure mm->user_ns contains the execed files Eric W. Biederman
2016-11-19 7:17 ` [REVIEW][PATCH 0/3] Fixing ptrace vs exec vs userns interactions Willy Tarreau
2016-11-19 9:28 ` Willy Tarreau
2016-11-19 9:33 ` Willy Tarreau
2016-11-19 18:44 ` Eric W. Biederman
2016-11-19 18:35 ` Eric W. Biederman
2016-11-19 18:37 ` Eric W. Biederman
2016-10-19 18:36 ` [REVIEW][PATCH] exec: Don't exec files the userns root can not read Andy Lutomirski
2016-10-18 18:06 ` [REVIEW][PATCH] mm: Add a user_ns owner to mm_struct and fix ptrace_may_access Michal Hocko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87twc9656s.fsf@xmission.com \
--to=ebiederm@xmission.com \
--cc=containers@lists.linux-foundation.org \
--cc=jann@thejh.net \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=luto@amacapital.net \
--cc=mhocko@kernel.org \
--cc=oleg@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox