Message-ID: <560BF47D.5020506@tycho.nsa.gov>
Date:	Wed, 30 Sep 2015 10:41:01 -0400
From:	Stephen Smalley <sds@...ho.nsa.gov>
To:	David Howells <dhowells@...hat.com>, linux-unionfs@...r.kernel.org,
	selinux@...ho.nsa.gov
Cc:	linux-fsdevel@...r.kernel.org, mjg59@...f.ucam.org,
	linux-security-module@...r.kernel.org,
	linux-kernel@...r.kernel.org, eparis@...hat.com,
	Daniel J Walsh <dwalsh@...hat.com>
Subject: Re: [PATCH 0/5] Security: Provide unioned file support

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists