Re: [REVIEW][PATCH] mm: Add a user_ns owner to mm_struct and fix ptrace_may_access

[Eric W. Biederman]

Jann Horn <jann@xxxxxxxxx> writes:

> On Mon, Oct 17, 2016 at 11:39:49AM -0500, 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.
>
> This looks good! Basically applies the same rules that already apply to
> EUID/... changes to namespace changes, and anyone entering a user
> namespace can now safely drop UIDs and GIDs to namespace root.

Yes.  It just required the right perspective and it turned out to be
straight forward to solve.  Especially since it is buggy today for
unreadable executables.

> This integrates better in the existing security concept than my old
> patch "ptrace: being capable wrt a process requires mapped uids/gids",
> and it has less issues in cases where e.g. the extra privileges of an
> entering process are the filesystem root or so.
>
> FWIW, if you want, you can add "Reviewed-by: Jann Horn
> <jann@xxxxxxxxx>".

Will do. Thank you.

Eric