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

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

[David Howells]

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.


The patches can be found here on top of some fix patches:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=overlayfs

David
---
David Howells (5):
      Security: Provide copy-up security hooks for unioned files
      Overlayfs: Use copy-up security hooks
      SELinux: Stub in copy-up handling
      SELinux: Handle opening of a unioned file
      SELinux: Check against union label for file operations


 fs/overlayfs/copy_up.c            |   12 ++++
 include/linux/lsm_hooks.h         |   23 ++++++++
 include/linux/security.h          |   14 +++++
 security/security.c               |   17 ++++++
 security/selinux/hooks.c          |  101 ++++++++++++++++++++++++++++++++++++-
 security/selinux/include/objsec.h |    1 
 6 files changed, 166 insertions(+), 2 deletions(-)

[PATCH 1/5] Security: Provide copy-up security hooks for unioned files

[David Howells]

Provide two new security hooks for use with security files that are used when
a file is copied up between layers:

 (1) security_inode_copy_up().  This is called so that the security label on
     the destination file can be set appropriately.

 (2) security_inode_copy_up_xattr().  This is called so that each xattr being
     copied up can be vetted - including modification and discard.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 include/linux/lsm_hooks.h |   23 +++++++++++++++++++++++
 include/linux/security.h  |   14 ++++++++++++++
 security/security.c       |   17 +++++++++++++++++
 3 files changed, 54 insertions(+)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index ec3a6bab29de..8c0c524dd232 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -401,6 +401,24 @@
  *	@inode contains a pointer to the inode.
  *	@secid contains a pointer to the location where result will be saved.
  *	In case of failure, @secid will be set to zero.
+ * @inode_copy_up:
+ *	Appropriately label the destination inode when a unioned file is copied
+ *	up from a lower layer to the union/overlay layer.
+ *	@src indicates the file that is being copied up.
+ *	@dst indicates the file that has being created by the copy up.
+ *	Returns 0 on success or a negative error code on error.
+ * @inode_copy_up_xattr:
+ *	Filter/modify the xattrs being copied up when a unioned file is copied
+ *	up from a lower layer to the union/overlay layer.
+ *	@src indicates the file that is being copied up.
+ *	@dst indicates the file that has being created by the copy up.
+ *	@name indicates the name of the xattr.
+ *	@value, *@size indicate the payload of the xattr.
+ *	Returns 0 to accept the xattr, 1 to discard the xattr or a negative
+ *	error code to abort the copy up.  The xattr buffer must be at least
+ *	XATTR_SIZE_MAX in capacity and the contents may be modified and *@size
+ *	changed appropriately.  Note that the caller is responsible for reading
+ *	and writing the xattrs as this hook is merely a filter.
  *
  * Security hooks for file operations
  *
@@ -1421,6 +1439,9 @@ union security_list_options {
 	int (*inode_listsecurity)(struct inode *inode, char *buffer,
 					size_t buffer_size);
 	void (*inode_getsecid)(const struct inode *inode, u32 *secid);
+	int (*inode_copy_up) (struct dentry *src, struct dentry *dst);
+	int (*inode_copy_up_xattr) (struct dentry *src, struct dentry *dst,
+				    const char *name, void *value, size_t *size);
 
 	int (*file_permission)(struct file *file, int mask);
 	int (*file_alloc_security)(struct file *file);
@@ -1689,6 +1710,8 @@ struct security_hook_heads {
 	struct list_head inode_setsecurity;
 	struct list_head inode_listsecurity;
 	struct list_head inode_getsecid;
+	struct list_head inode_copy_up;
+	struct list_head inode_copy_up_xattr;
 	struct list_head file_permission;
 	struct list_head file_alloc_security;
 	struct list_head file_free_security;
diff --git a/include/linux/security.h b/include/linux/security.h
index 2f4c1f7aa7db..ec21144d8807 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -274,6 +274,10 @@ int security_inode_getsecurity(const struct inode *inode, const char *name, void
 int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags);
 int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
 void security_inode_getsecid(const struct inode *inode, u32 *secid);
+int security_inode_copy_up(struct dentry *src, struct dentry *dst);
+int security_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
+				 const char *name, void *value, size_t *size);
+
 int security_file_permission(struct file *file, int mask);
 int security_file_alloc(struct file *file);
 void security_file_free(struct file *file);
@@ -739,6 +743,16 @@ static inline void security_inode_getsecid(const struct inode *inode, u32 *secid
 	*secid = 0;
 }
 
+static inline int security_inode_copy_up(struct dentry *src, struct dentry *dst)
+{
+	return 0;
+}
+static inline int security_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
+					       const char *name, const void *value, size_t *size)
+{
+	return 0;
+}
+
 static inline int security_file_permission(struct file *file, int mask)
 {
 	return 0;
diff --git a/security/security.c b/security/security.c
index 46f405ce6b0f..e33c5d5bdc6b 100644
--- a/security/security.c
+++ b/security/security.c
@@ -726,6 +726,19 @@ void security_inode_getsecid(const struct inode *inode, u32 *secid)
 	call_void_hook(inode_getsecid, inode, secid);
 }
 
+int security_inode_copy_up(struct dentry *src, struct dentry *dst)
+{
+	return call_int_hook(inode_copy_up, 0, src, dst);
+}
+EXPORT_SYMBOL(security_inode_copy_up);
+
+int security_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
+				 const char *name, void *value, size_t *size)
+{
+	return call_int_hook(inode_copy_up_xattr, 0, src, dst, name, value, size);
+}
+EXPORT_SYMBOL(security_inode_copy_up_xattr);
+
 int security_file_permission(struct file *file, int mask)
 {
 	int ret;
@@ -1654,6 +1667,10 @@ struct security_hook_heads security_hook_heads = {
 		LIST_HEAD_INIT(security_hook_heads.inode_listsecurity),
 	.inode_getsecid =
 		LIST_HEAD_INIT(security_hook_heads.inode_getsecid),
+	.inode_copy_up =
+		LIST_HEAD_INIT(security_hook_heads.inode_copy_up),
+	.inode_copy_up_xattr =
+		LIST_HEAD_INIT(security_hook_heads.inode_copy_up_xattr),
 	.file_permission =
 		LIST_HEAD_INIT(security_hook_heads.file_permission),
 	.file_alloc_security =

[PATCH 2/5] Overlayfs: Use copy-up security hooks

[David Howells]

Use the copy-up security hooks previously provided to allow an LSM to adjust
the security on a newly created copy and to filter the xattrs copied to that
file copy.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/overlayfs/copy_up.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 871fcb67be97..865f80aa7e44 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -58,6 +58,14 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
 			error = size;
 			goto out_free_value;
 		}
+		error = security_inode_copy_up_xattr(old, new,
+						     name, value, &size);
+		if (error < 0)
+			goto out_free_value;
+		if (error == 1) {
+			error = 0;
+			continue; /* Discard */
+		}
 		error = vfs_setxattr(new, name, value, size, 0);
 		if (error)
 			goto out_free_value;
@@ -223,6 +231,10 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
 	if (err)
 		goto out2;
 
+	err = security_inode_copy_up(lowerpath->dentry, newdentry);
+	if (err < 0)
+		goto out_cleanup;
+
 	if (S_ISREG(stat->mode)) {
 		struct path upperpath;
 		ovl_path_upper(dentry, &upperpath);

[PATCH 3/5] SELinux: Stub in copy-up handling

[David Howells]

Provide stubs for union/overlay copy-up handling.  The xattr copy up stub
discards lower SELinux xattrs rather than letting them be copied up so that
the security label on the copy doesn't get corrupted.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/selinux/hooks.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e4369d86e588..7c1a44d9fac3 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3190,6 +3190,24 @@ static void selinux_inode_getsecid(const struct inode *inode, u32 *secid)
 	*secid = isec->sid;
 }
 
+static int selinux_inode_copy_up(struct dentry *src, struct dentry *dst)
+{
+	return 0;
+}
+
+static int selinux_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
+				       const char *name, void *value,
+				       size_t *size)
+{
+	/* The copy_up hook above sets the initial context on an inode, but we
+	 * don't then want to overwrite it by blindly copying all the lower
+	 * xattrs up.  Instead, we have to filter out SELinux-related xattrs.
+	 */
+	if (strcmp(name, XATTR_NAME_SELINUX) == 0)
+		return 1; /* Discard */
+	return 0;
+}
+
 /* file security operations */
 
 static int selinux_revalidate_file_permission(struct file *file, int mask)
@@ -5919,6 +5937,8 @@ static struct security_hook_list selinux_hooks[] = {
 	LSM_HOOK_INIT(inode_setsecurity, selinux_inode_setsecurity),
 	LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity),
 	LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
+	LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
+	LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
 
 	LSM_HOOK_INIT(file_permission, selinux_file_permission),
 	LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),

[PATCH 4/5] SELinux: Handle opening of a unioned file

[David Howells]

Handle the opening of a unioned file by trying to derive the label that would
be attached to the union-layer inode if it doesn't exist.

If the union-layer inode does exist (as it necessarily does in overlayfs, but
not in unionmount), we assume that it has the right label and use that.
Otherwise we try to get it from the superblock.

If the superblock has a globally-applied label, we use that, otherwise we try
to transition to an appropriate label.  This union label is then stored in the
file_security_struct.

We then perform an additional check to make sure that the calling task is
granted permission by the union-layer inode label to open the file in addition
to a check to make sure that the task is granted permission to open the lower
file with the lower inode label.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/selinux/hooks.c          |   69 +++++++++++++++++++++++++++++++++++++
 security/selinux/include/objsec.h |    1 +
 2 files changed, 70 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 7c1a44d9fac3..522b070d9e2b 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3520,10 +3520,72 @@ static int selinux_file_receive(struct file *file)
 	return file_has_perm(cred, file, file_to_av(file));
 }
 
+/*
+ * We have a file opened on a unioned file system that falls through to a file
+ * on a lower layer.  If there is a union inode, we try to get the label from
+ * that, otherwise we need to get it from the superblock.
+ *
+ * file->f_path points to the union layer and file->f_inode points to the lower
+ * layer.
+ */
+static int selinux_file_open_union(struct file *file,
+				   struct file_security_struct *fsec,
+				   const struct cred *cred)
+{
+	const struct superblock_security_struct *sbsec;
+	const struct inode_security_struct *isec, *dsec, *fisec;
+	const struct task_security_struct *tsec = current_security();
+	struct common_audit_data ad;
+	struct dentry *union_dentry = file->f_path.dentry;
+	const struct inode *union_inode = d_inode(union_dentry);
+	const struct inode *lower_inode = file_inode(file);
+	struct dentry *dir;
+	int rc;
+
+	sbsec = union_dentry->d_sb->s_security;
+
+	if (union_inode) {
+		isec = union_inode->i_security;
+		fsec->union_isid = isec->sid;
+	} else if ((sbsec->flags & SE_SBINITIALIZED) &&
+		   (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
+		fsec->union_isid = sbsec->mntpoint_sid;
+	} else {
+		dir = dget_parent(union_dentry);
+		dsec = d_inode(dir)->i_security;
+
+		rc = security_transition_sid(
+			tsec->sid, dsec->sid,
+			inode_mode_to_security_class(lower_inode->i_mode),
+			&union_dentry->d_name,
+			&fsec->union_isid);
+		dput(dir);
+		if (rc) {
+			pr_warn("%s:  security_transition_sid failed, rc=%d (name=%pD)\n",
+				__func__, -rc, file);
+			return rc;
+		}
+	}
+
+	/* We need to check that the union file is allowed to be opened as well
+	 * as checking that the lower file is allowed to be opened.
+	 */
+	if (unlikely(IS_PRIVATE(lower_inode)))
+		return 0;
+
+	ad.type = LSM_AUDIT_DATA_PATH;
+	ad.u.path = file->f_path;
+
+	fisec = lower_inode->i_security;
+	return avc_has_perm(cred_sid(cred), fsec->union_isid, fisec->sclass,
+			    open_file_to_av(file), &ad);
+}
+
 static int selinux_file_open(struct file *file, const struct cred *cred)
 {
 	struct file_security_struct *fsec;
 	struct inode_security_struct *isec;
+	int rc;
 
 	fsec = file->f_security;
 	isec = file_inode(file)->i_security;
@@ -3544,6 +3606,13 @@ static int selinux_file_open(struct file *file, const struct cred *cred)
 	 * new inode label or new policy.
 	 * This check is not redundant - do not remove.
 	 */
+
+	if (d_inode(file->f_path.dentry) != file->f_inode) {
+		rc = selinux_file_open_union(file, fsec, cred);
+		if (rc < 0)
+			return rc;
+	}
+
 	return file_path_has_perm(cred, file, open_file_to_av(file));
 }
 
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index 81fa718d5cb3..f088c080aa9e 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -54,6 +54,7 @@ struct file_security_struct {
 	u32 sid;		/* SID of open file description */
 	u32 fown_sid;		/* SID of file owner (for SIGIO) */
 	u32 isid;		/* SID of inode at the time of file open */
+	u32 union_isid;		/* SID of would-be inodes in union top (or 0) */
 	u32 pseqno;		/* Policy seqno at the time of file open */
 };
 

[PATCH 5/5] SELinux: Check against union label for file operations

[David Howells]

File operations (eg. read, write) issued against a file that is attached to
the lower layer of a union file needs to be checked against the union-layer
label not the lower layer label.

The union label is stored in the file_security_struct rather than being
retrieved from one of the inodes.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/selinux/hooks.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 522b070d9e2b..ecc883b6d463 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1682,6 +1682,7 @@ static int file_has_perm(const struct cred *cred,
 			 struct file *file,
 			 u32 av)
 {
+	struct inode_security_struct *isec;
 	struct file_security_struct *fsec = file->f_security;
 	struct inode *inode = file_inode(file);
 	struct common_audit_data ad;
@@ -1702,8 +1703,15 @@ static int file_has_perm(const struct cred *cred,
 
 	/* av is zero if only checking access to the descriptor. */
 	rc = 0;
-	if (av)
-		rc = inode_has_perm(cred, inode, av, &ad);
+	if (av && likely(!IS_PRIVATE(inode))) {
+		if (fsec->union_isid) {
+			isec = inode->i_security;
+			rc = avc_has_perm(sid, fsec->union_isid, isec->sclass,
+					  av, &ad);
+		}
+		if (!rc)
+			rc = inode_has_perm(cred, inode, av, &ad);
+	}
 
 out:
 	return rc;

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

[Stephen Smalley]

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.

>
>
> The patches can be found here on top of some fix patches:
>
> 	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=overlayfs
>
> David
> ---
> David Howells (5):
>        Security: Provide copy-up security hooks for unioned files
>        Overlayfs: Use copy-up security hooks
>        SELinux: Stub in copy-up handling
>        SELinux: Handle opening of a unioned file
>        SELinux: Check against union label for file operations
>
>
>   fs/overlayfs/copy_up.c            |   12 ++++
>   include/linux/lsm_hooks.h         |   23 ++++++++
>   include/linux/security.h          |   14 +++++
>   security/security.c               |   17 ++++++
>   security/selinux/hooks.c          |  101 ++++++++++++++++++++++++++++++++++++-
>   security/selinux/include/objsec.h |    1
>   6 files changed, 166 insertions(+), 2 deletions(-)
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>

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

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

[Daniel J Walsh]

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.