From: ebiederm@xmission.com (Eric W. Biederman)
To: Luis Chamberlain <mcgrof@kernel.org>
Cc: Radoslaw Burny <rburny@google.com>,
Seth Forshee <seth.forshee@canonical.com>,
Kees Cook <keescook@chromium.org>,
linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
jsperbeck@google.com
Subject: Re: [PATCH] fs: Make /proc/sys inodes be owned by global root.
Date: Mon, 26 Nov 2018 23:29:40 -0600 [thread overview]
Message-ID: <87k1kzjdff.fsf@xmission.com> (raw)
In-Reply-To: <20181127011627.GI4922@garbanzo.do-not-panic.com> (Luis Chamberlain's message of "Mon, 26 Nov 2018 17:16:27 -0800")
Luis Chamberlain <mcgrof@kernel.org> writes:
> On Mon, Nov 26, 2018 at 06:26:07PM +0100, Radoslaw Burny wrote:
>> Due to a recent commit (d151ddc00498 - fs: Update i_[ug]id_(read|write)
>> to translate relative to s_user_ns),
>
> Recent? This is commit is from 2014 and present upstream since v4.8.
> And the commit ID you mentioned in your commit log seems to be
> incorrect. I get:
>
> 81754357770ebd900801231e7bc8d151ddc00498a fs: Update i_[ug]id_(read|write) to translate relative to s_user_ns
>
>> inodes under /proc/sys have -1
>> written to their i_uid/i_gid members if a containing userns does not
>> have entries for root in the uid/gid_map.
>
> Thanks for the description of how to run into the issue described but
> is there also a practical use case today where this is happening? I ask
> as it would be good to know the severity of the issue in the real world
> today.
People trying to run containers without a root user in the container.
It atypical but something doable.
>> This wouldn't normally matter, because these values are not used for
>> access checks. However, a later change (0bd23d09b874 - Don't modify
>> inodes with a uid or gid unknown to the vfs) changes the kernel to
>> prevent opens for write if the i_uid/i_gid field in the inode is -1,
>> even if the /proc/sys-specific access checks would otherwise pass.
>>
>> This causes a problem: in a userns without root mapping, even the
>> namespace creator cannot write to e.g. /proc/sys/kernel/shmmax.
>> This change fixes the problem by overriding i_uid/i_gid back to
>> GLOBAL_ROOT_UID/GID.
>
> We really need Seth and Eric to provide guidance here as they were
> the ones devising this long ago, but to me your solution seems backward.
> Why allow any namespace to muck with /proc/sys/ seettings?
There are many per namespace sysctls. Most of them are in the
networking stack.
> Let's recall that this case was a corner case, and writeback was the
> biggest concern, and for that it was decided that you'd simply not get
> write access, and so its read only. Its not clear to me if things like
> proc were considered. For the regular file case the situation can be
> addressed with chown, however we can't chown proc files.
>
>> Tested: Used a repro program that creates a user namespace without any
>> mapping and stat'ed /proc/$PID/root/proc/sys/kernel/shmmax from outside.
>> Before the change, it shows uid/gid of 65534,
>
> I thought you said it would be uid/gid -1 without your patch?
It is INVALID_UID/INVALID_GID. It is an over simplifcation to call
them -1. As they are not a valid value and are never mapped in any
user namespace they are displayed as the overflow_uid or overflow_gid
which is 65534 by default.
>> with the change it's 0.
>
> Note that a good way to also test issues is with the lib/test_sysctl.c
> module and the tools/testing/selftests/sysctl/sysctl.sh script, so if
> you can device a test there, once we decide what to do that would be
> appreciated.
We spoke about this at LPC. And this is the correct behavioral change.
The problem is there is a default value for i_uid and i_gid that is
correct in the general case. That default value is not corect for
sysctl, because proc is weird. As the sysctl permission check in
test_perm are all against GLOBAL_ROOT_UID and GLOBAL_ROOT_GID we did not
notice that i_uid and i_gid were being set wrong.
So all this patch does is fix the default values i_uid and i_gid.
The commit comment seems worth cleaning up. But for the
content of the code.
I expect when I have a few moments I will pick this change up.
Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
Eric
>> Signed-off-by: Radoslaw Burny <rburny@google.com>
>> ---
>> fs/proc/proc_sysctl.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
>> index c5cbbdff3c3d..67379a389658 100644
>> --- a/fs/proc/proc_sysctl.c
>> +++ b/fs/proc/proc_sysctl.c
>> @@ -499,6 +499,10 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
>>
>> if (root->set_ownership)
>> root->set_ownership(head, table, &inode->i_uid, &inode->i_gid);
>> + else {
>> + inode->i_uid = GLOBAL_ROOT_UID;
>> + inode->i_gid = GLOBAL_ROOT_GID;
>> + }
>>
>> out:
>> return inode;
>> --
>> 2.20.0.rc0.387.gc7a69e6b6c-goog
>>
next prev parent reply other threads:[~2018-11-27 5:29 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-26 17:26 [PATCH] fs: Make /proc/sys inodes be owned by global root Radoslaw Burny
2018-11-27 1:16 ` Luis Chamberlain
2018-11-27 5:29 ` Eric W. Biederman [this message]
2018-11-30 1:09 ` Luis Chamberlain
2018-11-30 13:46 ` Radoslaw Burny
2018-11-30 14:48 ` Eric W. Biederman
2018-11-30 18:19 ` Luis Chamberlain
[not found] ` <CAFkxGoM_rjciQ0sRh7Lhf_XfJu-g4Tth6Yo0L_YRVUaOnzjZuA@mail.gmail.com>
2018-12-01 13:55 ` Eric W. Biederman
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=87k1kzjdff.fsf@xmission.com \
--to=ebiederm@xmission.com \
--cc=jsperbeck@google.com \
--cc=keescook@chromium.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mcgrof@kernel.org \
--cc=rburny@google.com \
--cc=seth.forshee@canonical.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;
as well as URLs for NNTP newsgroup(s).