From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751322AbdATCxw (ORCPT ); Thu, 19 Jan 2017 21:53:52 -0500 Received: from mx2.suse.de ([195.135.220.15]:39266 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751258AbdATCxu (ORCPT ); Thu, 19 Jan 2017 21:53:50 -0500 Subject: Re: [PATCH] procfs: change the owner of non-dumpable and writeable files To: "Eric W. Biederman" References: <20170118040159.4751-1-asarai@suse.de> <20170119092930.GJ30786@dhcp22.suse.cz> <87r33yv6gk.fsf@xmission.com> Cc: Michal Hocko , Andrew Morton , Oleg Nesterov , Kees Cook , Al Viro , John Stultz , Mateusz Guzik , Janis Danisevskis , linux-kernel@vger.kernel.org, dev@opencontainers.org, containers@lists.linux-foundation.org From: Aleksa Sarai Message-ID: <4025e285-9179-b98a-88c0-905f4f9c3ef8@suse.de> Date: Fri, 20 Jan 2017 13:35:29 +1100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <87r33yv6gk.fsf@xmission.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > 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/