From: Vivek Goyal <vgoyal@redhat.com>
To: Miklos Szeredi <mszeredi@redhat.com>
Cc: linux-unionfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, Al Viro <viro@zeniv.linux.org.uk>,
linux-security-module@vger.kernel.org,
Daniel J Walsh <dwalsh@redhat.com>,
Paul Moore <pmoore@redhat.com>,
Stephen Smalley <sds@tycho.nsa.gov>
Subject: Re: [PATCH v2 22/35] vfs: don't open real
Date: Fri, 11 May 2018 15:42:48 -0400 [thread overview]
Message-ID: <20180511194248.GF6044@redhat.com> (raw)
In-Reply-To: <20180511185430.GE6044@redhat.com>
On Fri, May 11, 2018 at 02:54:30PM -0400, Vivek Goyal wrote:
> On Mon, May 07, 2018 at 10:37:54AM +0200, Miklos Szeredi wrote:
> > Let overlayfs do its thing when opening a file.
> >
> > This enables stacking and fixes the corner case when a file is opened for
> > read, modified through a writable open, and data is read from the read-only
> > file. After this patch the read-only open will not return stale data even
> > in this case.
>
> [CC Dan, Steven, Paul, linux-security-module list]
>
> Hi Miklos,
>
> I was running selinux-testsuite and one of the tests seems to fail. I
> think this is side effect of installing overlay inode in file->f_inode
> instead of real underlying inode.
>
> Following test is failing.
>
> sub test_90_1 {
> print "Attempting to enter domain with bad entrypoint, should fail.\n";
> $result = system(
> "runcon -t test_overlay_client_t -l s0:c10,c20 $basedir/container1/merged/badentrypoint >/dev/null 2>&1"
> );
> ok($result);
> return;
> }
I am wondering, shouldn't do_open_execat() have failed. It should have called
into inode_permission(MAY_EXEC). And then ovl_inode_permission()
will in turn call inode_permission(realinode, MAY_EXEC) with mounter's
creds. Shouldn't selinux_inode_permission() have returned that mounter
does not have MAY_EXEC permission on inode.
Dan, I am wondering if this is a selinux policy issue? In my testing
on upstream kernel, do_open_execat() succeeds and it fails much later.
I am wondering why that's the case. Is it expected.
Thanks
Vivek
>
> Basically, this test has an executable named "badentrypoint" with selinux
> label "unconfined_u:object_r:test_overlay_files_ro_t:s0". And we mount
> overlay with context=unconfined_u:object_r:test_overlay_files_rwx_t:s0:c10,c20
>
> So effectively overlay inode of "badentrypoint" now gets the label
> specified by "context=".
>
> I think intent of test is that this file's real label is "...ro_t". That
> means this file is not supposed to be executed and any attempt to execute
> it should be denied.
>
> Currently test works and execution fails with following avc.
>
> AVC avc: denied { entrypoint } for pid=1425 comm="runcon" path="/root/git/selinux-testsuite/tests/overlay/container1/merged/badentrypoint" dev="dm-0" ino=34515261 scontext=unconfined_u:unconfined_r:test_overlay_client_t:s0:c10,c20 tcontext=unconfined_u:object_r:test_overlay_files_ro_t:s0 tclass=file permissive=0
>
> But with new patches, this test starts passing.
>
> I think currently selinux_bprm_set_creds() returns error. It does
> checks on inode returned by file_inode() and as of now that inode is
> real inode and that inode has real lable of "...ro_t" and permission
> to execute that file is denied.
>
> But after the patches file_inode() returns overlay inode. Which has
> the label specified by context= mount option "...rwx_t". And that
> label allows executing file, so file execution is not blocked by
> selinux.
>
> I feel that even now code is working accidently. Ideally our theme was
> that task's credential as checked against overlay inode and mounter's
> creds are checked against underlying inode to determine if certain
> permission is allowed. So ideally mounter should not have been allwed
> to execute a file of type "...ro_t". But we don't have that workflow
> and VFS calls into selinux and selinux checks the underlying file's
> label against task.
>
> It worked so far but the moment we install overlay inode in file, selinux
> checks it against overlay inode label and allows permission to execute and
> mounter is never checked against real inode.
>
> I am not sure what's the right solution. So far selinux is not aware of
> two levels of checks and if two levels of checks are to be performed, it
> somehow needs to be enforced by overlay and call same hook on two levels.
>
> Thought of atleast starting a conversation on this.
>
> Thanks
> Vivek
>
>
> >
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
> > fs/open.c | 7 +------
> > 1 file changed, 1 insertion(+), 6 deletions(-)
> >
> > diff --git a/fs/open.c b/fs/open.c
> > index 6e52fd6fea7c..244cd2ecfefd 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -897,13 +897,8 @@ EXPORT_SYMBOL(file_path);
> > int vfs_open(const struct path *path, struct file *file,
> > const struct cred *cred)
> > {
> > - struct dentry *dentry = d_real(path->dentry, NULL, file->f_flags, 0);
> > -
> > - if (IS_ERR(dentry))
> > - return PTR_ERR(dentry);
> > -
> > file->f_path = *path;
> > - return do_dentry_open(file, d_backing_inode(dentry), NULL, cred);
> > + return do_dentry_open(file, d_backing_inode(path->dentry), NULL, cred);
> > }
> >
> > /**
> > --
> > 2.14.3
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2018-05-11 19:42 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-07 8:37 [PATCH v2 00/35] overlayfs: stack file operations Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 01/35] vfs: add path_open() Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 02/35] vfs: optionally don't account file in nr_files Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 03/35] vfs: add f_op->pre_mmap() Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 04/35] vfs: export vfs_ioctl() to modules Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 05/35] vfs: export vfs_dedupe_file_range_one() " Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 06/35] ovl: copy up times Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 07/35] ovl: copy up inode flags Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 08/35] Revert "Revert "ovl: get_write_access() in truncate"" Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 09/35] ovl: copy up file size as well Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 10/35] ovl: deal with overlay files in ovl_d_real() Miklos Szeredi
2018-05-07 13:17 ` Vivek Goyal
2018-05-07 8:37 ` [PATCH v2 11/35] ovl: stack file ops Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 12/35] ovl: add helper to return real file Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 13/35] ovl: add ovl_read_iter() Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 14/35] ovl: add ovl_write_iter() Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 15/35] ovl: add ovl_fsync() Miklos Szeredi
2018-05-08 5:14 ` Amir Goldstein
2018-05-08 14:57 ` Miklos Szeredi
2018-05-08 15:02 ` Amir Goldstein
2018-05-07 8:37 ` [PATCH v2 16/35] ovl: add ovl_mmap() Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 17/35] ovl: add ovl_fallocate() Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 18/35] ovl: add lsattr/chattr support Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 19/35] ovl: add ovl_fiemap() Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 20/35] ovl: add O_DIRECT support Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 21/35] ovl: add reflink/copyfile/dedup support Miklos Szeredi
2018-05-07 20:43 ` Darrick J. Wong
2018-05-08 14:13 ` Miklos Szeredi
2018-05-08 14:38 ` Darrick J. Wong
2018-05-07 8:37 ` [PATCH v2 22/35] vfs: don't open real Miklos Szeredi
2018-05-07 10:27 ` Amir Goldstein
2018-05-07 10:29 ` Miklos Szeredi
2018-05-11 18:54 ` Vivek Goyal
2018-05-11 19:42 ` Vivek Goyal [this message]
2018-05-14 13:58 ` Vivek Goyal
2018-05-15 20:42 ` Vivek Goyal
2018-05-14 14:03 ` Daniel Walsh
2018-05-07 8:37 ` [PATCH v2 23/35] ovl: copy-up on MAP_SHARED Miklos Szeredi
2018-05-07 19:28 ` Randy Dunlap
2018-05-08 15:03 ` Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 24/35] vfs: simplify dentry_open() Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 25/35] Revert "ovl: fix may_write_real() for overlayfs directories" Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 26/35] Revert "ovl: don't allow writing ioctl on lower layer" Miklos Szeredi
2018-05-07 8:37 ` [PATCH v2 27/35] vfs: fix freeze protection in mnt_want_write_file() for overlayfs Miklos Szeredi
2018-05-07 8:38 ` [PATCH v2 28/35] Revert "ovl: fix relatime for directories" Miklos Szeredi
2018-05-07 8:38 ` [PATCH v2 29/35] Revert "vfs: update ovl inode before relatime check" Miklos Szeredi
2018-05-07 8:38 ` [PATCH v2 30/35] Revert "vfs: add flags to d_real()" Miklos Szeredi
2018-05-07 8:38 ` [PATCH v2 31/35] Revert "vfs: do get_write_access() on upper layer of overlayfs" Miklos Szeredi
2018-05-07 8:38 ` [PATCH v2 32/35] Partially revert "locks: fix file locking on overlayfs" Miklos Szeredi
2018-05-08 15:15 ` Jeff Layton
2018-05-07 8:38 ` [PATCH v2 33/35] Revert "fsnotify: support overlayfs" Miklos Szeredi
2018-05-07 8:38 ` [PATCH v2 34/35] vfs: remove open_flags from d_real() Miklos Szeredi
2018-05-07 8:38 ` [PATCH v2 35/35] ovl: fix documentation of non-standard behavior Miklos Szeredi
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=20180511194248.GF6044@redhat.com \
--to=vgoyal@redhat.com \
--cc=dwalsh@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=linux-unionfs@vger.kernel.org \
--cc=mszeredi@redhat.com \
--cc=pmoore@redhat.com \
--cc=sds@tycho.nsa.gov \
--cc=viro@zeniv.linux.org.uk \
/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).