Re: [PATCH v2 22/35] vfs: don't open real

[Vivek Goyal]

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@xxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html