Re: [PATCH 0/5] Security: Provide unioned file support

[Stephen Smalley]

On 09/29/2015 05:03 PM, Stephen Smalley wrote:
> On 09/28/2015 04:00 PM, David Howells wrote:
> > The attached patches provide security support for unioned files where the
> > security involves an object-label-based LSM (such as SELinux) rather
> > than a
> > path-based LSM.
> >
> > [Note that a number of the bits that were in the original patch set
> > are now
> > upstream and I've rebased on Casey's changes to the security hook system]
> >
> > The patches can be broken down into two sets:
> >
> >   (1) A patch to add LSM hooks to handle copy up of a file, including
> > label
> >       determination/setting and xattr filtration and a patch to have
> >       overlayfs call the hooks during the copy-up procedure.
> >
> >   (2) My SELinux implementations of these hooks.  I do three things:
> >
> >       (a) Don't copy up SELinux xattrs from the lower file to the upper
> >            file.  It is assumed that the upper file will be created
> > with the
> >            attrs we want or the selinux_inode_copy_up() hook will set it
> >            appropriately.
> >
> >      The reason there are two separate hooks here is that
> >      selinux_inode_copy_up_xattr() might not ever be called if there
> >      aren't actually any xattrs on the lower inode.
> >
> >       (b) I try to derive a label to be used by file operations by, in
> > order
> >            of preference: using the label on the union inode if there
> > is one
> >            (the normal overlayfs case); using the override label set
> > on the
> >            superblock, if provided; or trying to derive a new label by
> > sid
> >            transition operation.
> >
> >       (c) Using the label obtained in (b) in file_has_perm() rather than
> >            using the label on the lower inode.
> >
> > Now the steps I have outlined in (b) and (c) seem to be at odds with what
> > Dan Walsh and Stephen Smalley want - but I'm not sure I follow what that
> > is, let alone how to do it:
> >
> >     Wanted to bring back the original proposal.  Stephen suggested that
> >     we could change back to the MLS way of handling labels.
> >
> >     In MCS we base the MCS label of content created by a process on the
> >     containing directory.  Which means that if a process running as
> >     s0:c1,c2 creates content in a directory labeled s0, it will get
> >     created as s0.
> >
> >     In MLS if a process running as s0:c1,c2 creates content in a
> >     directory it labels it s0:c1,c2.  No matter what the label of the
> >     directory is.  (Well actually if the directory is not ranged the
> >     process will not be able to create the content.)
> >
> >     We changed the default for MCS in Rawhide for about a week, when I
> >     realized this was a huge problem for containers sharing content.
> >     Currently if you want two containers to share the same volume
> >     mount, we label the content as svirt_sandbox_file_t:s0 If one
> >     process running as s0:c1,c2 creates content it gets created as s0,
> >     if the second container process is running as s0:c3,c4, it can
> >     still read/write the content.  If we changed the default the object
> >     would get created as s0:c1,c2 and process runing as s0:c3,c4 would
> >     be blocked.
> >
> >     So I had it reverted back to the standard MCS Mode.
> >
> >     If we could get the default to be MLS mode on COW file systems and
> >     MCS on Volumes, we would get the best of both worlds.
>
> How are you testing this?
> I tried as follows:
>
> # Make sure we have a policy that is using xattrs to label overlay inodes.
> $ seinfo --fs_use | grep overlay
>     fs_use_xattr overlay system_u:object_r:fs_t:s0
>
> # Define some types for the different directories involved.
> $ cat overlay.te
> policy_module(overlay, 1.0)
>
> type lower_t;
> files_type(lower_t)
>
> type upper_t;
> files_type(upper_t)
>
> type work_t;
> files_type(work_t)
>
> type merged_t;
> files_type(merged_t)
>
> $ make -f /usr/share/selinux/devel/Makefile overlay.pp
> $ sudo semodule -i overlay.pp
>
> # Create and label the different directories involved.
> $ mkdir lower upper work merged
> $ chcon -t lower_t lower
> $ chcon -t upper_t upper
> $ chcon -t work_t work
> $ chcon -t merged_t merged
>
> # Populate lower
> $ echo "lower" > lower/a
> $ mkdir lower/b
>
> # Mount overlay
> $ sudo mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work
> merged
>
> # Look at the merged dir and labels.
> $ ls -Z merged
> unconfined_u:object_r:lower_t:s0 a
> unconfined_u:object_r:lower_t:s0 b
>
> # Write/create some files/directories.
> $ echo "foo" >> merged/a
> $ mkdir merged/b/c
> $ mkdir merged/c
>
> # Look again.
> $ ls -ZR merged
> merged:
> unconfined_u:object_r:lower_t:s0 a  unconfined_u:object_r:upper_t:s0 c
> unconfined_u:object_r:lower_t:s0 b
>
> merged/b:
> unconfined_u:object_r:work_t:s0 c
>
> merged/b/c:
>
> $ ls -ZR upper
> upper:
>   unconfined_u:object_r:work_t:s0 a  unconfined_u:object_r:upper_t:s0 c
>   unconfined_u:object_r:work_t:s0 b
>
> upper/b:
> unconfined_u:object_r:work_t:s0 c
>
> upper/b/c:
>
> Note that the copied-up file (a) and directory (b) are labeled lower_t
> in the overlay, but work_t in the upper dir, and neither of those is
> really what we want IIUC.
>
> And that's just the labeling question.  Then there is the question of
> what permission checks were applied during those copy-up operations and
> whether the current process ends up needing write permissions to them all.

Also, the labels on the overlay inodes change if you umount and then 
mount it again:

$ sudo umount merged
$ sudo mount -t overlay overlay -o 
lowerdir=lower,upperdir=upper,workdir=work merged
$ ls -ZR merged
merged:
 unconfined_u:object_r:work_t:s0 a  unconfined_u:object_r:upper_t:s0 c
 unconfined_u:object_r:work_t:s0 b

merged/b:
unconfined_u:object_r:work_t:s0 c

merged/b/c:

merged/c:

It seems to me that either the copied-up files should be labeled upper_t 
(i.e. from upperdir) or merged_t (i.e. from the overlay).  But certainly 
not lower_t (which is supposed to be read-only to the container) or 
work_t (which isn't supposed to be directly accessed by processes in the 
first place).

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/