From: Stephen Smalley <sds@tycho.nsa.gov>
To: Vivek Goyal <vgoyal@redhat.com>, Miklos Szeredi <miklos@szeredi.hu>
Cc: Ondrej Mosnacek <omosnace@redhat.com>,
"J. Bruce Fields" <bfields@fieldses.org>,
Mark Salyzyn <salyzyn@android.com>,
Paul Moore <paul@paul-moore.com>,
linux-kernel@vger.kernel.org,
overlayfs <linux-unionfs@vger.kernel.org>,
linux-fsdevel@vger.kernel.org, selinux@vger.kernel.org,
Daniel J Walsh <dwalsh@redhat.com>
Subject: Re: overlayfs access checks on underlying layers
Date: Wed, 28 Nov 2018 14:34:42 -0500 [thread overview]
Message-ID: <377b7d4f-eb1d-c281-5c67-8ab6de77c881@tycho.nsa.gov> (raw)
In-Reply-To: <20181128170302.GA12405@redhat.com>
On 11/28/18 12:03 PM, Vivek Goyal wrote:
> On Wed, Nov 28, 2018 at 11:00:09AM +0100, Miklos Szeredi wrote:
>> On Tue, Nov 27, 2018 at 10:05 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>>>
>>> On Tue, Nov 27, 2018 at 08:58:06PM +0100, Miklos Szeredi wrote:
>>>> [resending with fixed email address for Paul Moore]
>>>>
>>>> Moving discussion from github[1] to here.
>>>>
>>>> To summarize: commit 007ea44892e6 ("ovl: relax permission checking on
>>>> underlying layers") was added in 4.20-rc1 to make overlayfs access
>>>> checks on underlying "real" filesystems more consistent. The
>>>> discussion leading up to this commit can be found at [2]. The commit
>>>> broke some selinux-testsuite cases, possibly indicating a security
>>>> hole opened by this commit.
>>>>
>>>> The model this patch tries to follow is that if "cp --preserve=all"
>>>> was allowed to the mounter from underlying layer to the overlay layer,
>>>> then operation is allowed. That means even if mounter's creds doesn't
>>>> provide permission to for example execute underying file X, if
>>>> mounter's creds provide sufficient permission to perform "cp
>>>> --preserve=all X Y" and original creds allow execute on Y, then the
>>>> operation is allowed. This provides consistency in the face of
>>>> copy-ups. Consistency is only provided in sane setups, where mounter
>>>> has sufficient privileges to access both the lower and upper layers.
>>>
>>> [cc daniel walsh]
>>>
>>> I think current selinux testsuite tests are written keeping these
>>> rules in mind.
>>>
>>> 1. Check overlay inode creds in the context of task and underlying
>>> inode creds (lower/upper), in the context of mounter.
>>>
>>> 2. For a lower inode, if said file is being copied up, then only
>>> check MAY_READ on lower. This is equivalent to mounter creating
>>> a copy of file and providing caller access to it (context mount).
>>>
>>> For the case of special devices, we do not copy up these. So should
>>> we continue to do check on lower inode in the context of mounter
>>> (instead of not doing any check on lower at all).
>>
>> Hmm, I'm trying to understand the logic... If we follow the "cp
>> --preserve=all" thing, than mounter needs to have CREATE permission
>> for the special file, not READ or WRITE. Does that make sense? Would
>> that help with the context= mount use case?
>
> Ok. If we follow "cp --preserve=all" methodology, then checking for
> mounter CREATE permission on upper for special files makes sense. Or
> change logic to copy up this special file during open. I am assuming
> we don't copy up special file during open as it is not necessary
> for things to work but copying up will work as well?
>
> So rules will become.
>
> - Two levels of checks.
> - For lower level inode, check MAY_READ for regular files. (including
> exec).
> - For special files, only make sure mounter can CREATE object in upper.
>
> - What about checks on files on upper/. As of now we seem to check
> access in mounter's context if it is regular file. Skip the checks
> completely for special files and for executables.
>
> While non-context mount should still be ok, but this means lot of
> privilige granting to unprivileged process using context mounts. So
> unprivileged process which could not open a device/socket/fifo for
> read/write on host fs, can open it for those operations for context
> mounts.
>
> IOW, for context mount case, an unprivileged user will gain lot of
> privileges. But that seems to be the point of context mount anyway
> on regular disks. If a disk is mounted using context mount option,
> then all real labels are ignored and all access checking happens
> using context label. We are doing similar thing. With one step extra
> and that is making sure if mounter itself can not do certain operation
> on host, that will still be denied.
>
> This probably means that context= mounts should be used very carefully.
> It will grant lot of priviliges to the process (and allow operations
> which process could not do on host without overlayfs mount).
>
> Case of device file still baffels me though. We don't do any mounter's
> checks on device files. So if a device file is on upper which mounter
> can't open for read but mounter is still granting priviliges to client
> to open that device file. That's unintutive to me and seems counter
> to the principle of that mounter can't give more priviliges than what
> it itself can't do on host.
>
> Dan, stephen, paul moore, does this sound ok to you folks from selinux
> point of view.
It seems wrong to check CREATE when no file is being created, and doing
so could lead to over-privileging of the mounter context, requiring one
to allow a mounter context to create device nodes just to allow a client
task context to read/write via already existing device nodes through an
overlay.
Checking READ but not EXECUTE upon an execute check could permit a
mounter to execute unauthorized code, if it can context mount from a
readable-but-not-executable context to an executable context.
Note btw that cp --preserve=all doesn't quite operate as expected if
dealing with a context mount. You can't preserve the original security
context if copying to a context mount unless the two contexts happen to
already match. So I'm not sure how that model applies in the case of a
context mount.
Does the breaking commit (007ea44892e6) fix a real bug affecting users?
If not, I'd recommend just reverting it.
next prev parent reply other threads:[~2018-11-28 19:32 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-27 19:55 overlayfs access checks on underlying layers Miklos Szeredi
2018-11-27 19:58 ` Miklos Szeredi
2018-11-27 21:05 ` Vivek Goyal
2018-11-28 10:00 ` Miklos Szeredi
2018-11-28 17:03 ` Vivek Goyal
2018-11-28 19:34 ` Stephen Smalley [this message]
2018-11-28 20:24 ` Miklos Szeredi
2018-11-28 21:46 ` Stephen Smalley
2018-11-29 11:04 ` Miklos Szeredi
2018-11-29 13:49 ` Vivek Goyal
2019-03-04 17:01 ` Mark Salyzyn
2019-03-04 17:56 ` Casey Schaufler
2019-03-04 18:44 ` Stephen Smalley
2019-03-04 19:21 ` Amir Goldstein
2018-11-29 16:16 ` Stephen Smalley
2018-11-29 16:22 ` Stephen Smalley
2018-11-29 19:47 ` Miklos Szeredi
2018-11-29 21:03 ` Stephen Smalley
2018-11-29 21:19 ` Stephen Smalley
2018-12-04 13:32 ` Miklos Szeredi
2018-12-04 14:30 ` Stephen Smalley
2018-12-04 14:45 ` Miklos Szeredi
2018-12-04 15:35 ` Stephen Smalley
2018-12-04 15:39 ` Miklos Szeredi
2018-12-11 15:50 ` Paul Moore
2018-12-04 15:15 ` Vivek Goyal
2018-12-04 15:22 ` Vivek Goyal
2018-12-04 15:31 ` Miklos Szeredi
2018-12-04 15:42 ` Vivek Goyal
2018-12-04 16:05 ` Stephen Smalley
2018-12-04 16:17 ` Vivek Goyal
2018-12-04 16:49 ` Stephen Smalley
2018-12-05 13:43 ` Vivek Goyal
2018-12-06 20:26 ` Stephen Smalley
2018-12-11 21:48 ` Vivek Goyal
2018-12-12 14:51 ` Stephen Smalley
2018-12-13 14:58 ` Vivek Goyal
2018-12-13 16:12 ` Stephen Smalley
2018-12-13 18:54 ` Vivek Goyal
2018-12-13 20:09 ` Stephen Smalley
2018-12-13 20:26 ` Vivek Goyal
2018-12-04 15:42 ` Stephen Smalley
2018-12-04 16:15 ` Vivek Goyal
2018-11-29 22:22 ` Daniel Walsh
2018-12-03 23:27 ` Paul Moore
2018-12-04 14:43 ` Stephen Smalley
2018-12-04 23:01 ` Paul Moore
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=377b7d4f-eb1d-c281-5c67-8ab6de77c881@tycho.nsa.gov \
--to=sds@tycho.nsa.gov \
--cc=bfields@fieldses.org \
--cc=dwalsh@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-unionfs@vger.kernel.org \
--cc=miklos@szeredi.hu \
--cc=omosnace@redhat.com \
--cc=paul@paul-moore.com \
--cc=salyzyn@android.com \
--cc=selinux@vger.kernel.org \
--cc=vgoyal@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;
as well as URLs for NNTP newsgroup(s).