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

Stephen Smalley
2015-09-30T14:41:50Z

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).