linux-kernel - Re: [PATCH] procfs: change the owner of non-dumpable and writeable files

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]

Date:   Fri, 20 Jan 2017 13:35:29 +1100
From:   Aleksa Sarai <asarai@...e.de>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Michal Hocko <mhocko@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Oleg Nesterov <oleg@...hat.com>,
        Kees Cook <keescook@...omium.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        John Stultz <john.stultz@...aro.org>,
        Mateusz Guzik <mguzik@...hat.com>,
        Janis Danisevskis <jdanis@...gle.com>,
        linux-kernel@...r.kernel.org, dev@...ncontainers.org,
        containers@...ts.linux-foundation.org
Subject: Re: [PATCH] procfs: change the owner of non-dumpable and writeable
 files

> Please verify but the ptrace issue that allowed processes in a container
> to call setns on our processes should be fixed as of 4.10-rc1.  And the
> change has been marked for backporting.

ptrace(2) is not the only issue, the issue that we had in runC is that a 
process joining a namespace may have file descriptors that refer to the 
host filesystem. If the process joining is dumpable, a racing process 
inside the container can access those file descriptors through the 
/proc/[pid]/fd/... mechanism.

See CVE-2016-9962.

> AKA it should be this fix that removes the need for your dumpable setting.
> Fixes: bfedb589252c ("mm: Add a user_ns owner to mm_struct and fix ptrace permission checks")

I will check, though from what I recall that patch doesn't fix the 
ptrace_may_access checks. Not to mention it won't help if the container 
doesn't have it's own user namespace.

> Now with that said I believe we want to add the following change now
> that dumpable is user namespace relative.  That will use not the
> GLOBAL_ROOT_UID/GID but instead uid and gid 0 in the namespace
> that dumpable is relative too.

Sure, but that's tangential to the issue under discussion.

> But ugh!  Your case is even more confused that I had first noticed.
> Saying that a processes is undumpable is completely unnecessary
> when you are entering into a new fresh user namespace.  Touching
> setgroups at any point where there are other processes in the namespace
> makes no sense whatsoever.

Currently in runC the ordering for mixed create-and-join namespaces is 
that we first join existing namespaces and _then_ create new ones. So we 
need to be non-dumpable to avoid the problem in CVE-2016-9962.

> Clearing dumpable is to help not leak things
> into a container when you call setns on a user namespace.

It is also to help not leak things into a container when you join other 
namespaces. Most notably the PID namespace.

> +	if (mode != (S_IFDIR|S_IRUGO|S_IXUGO)) {

I'd just like to draw your attention to this special case -- why is this 
special cased? What was the original reasoning behind it? Does it make 
sense for a non-dumpable process to allow someone to change the mode of 
some random /proc/[pid]/ directories?

I get the feeling that some of this logic is a bit iffy.

-- 
Aleksa Sarai
Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/