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

Daniel J Walsh
2015-09-30T15:19:44Z

On 09/30/2015 10:41 AM, Stephen Smalley wrote:
> 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).
>
Yes lower_t should be read only and we want to transition the
directories to upper_t, or more specifically upper_t:MCS label.

lower_t:s0 -> upper_t:s0:c1,c2

For Container images.