* [PATCH 0/7] Security: Provide unioned file support
@ 2014-11-05 15:42 David Howells
2014-11-05 15:42 ` [PATCH 1/7] Security: Provide copy-up security hooks for unioned files David Howells
` (8 more replies)
0 siblings, 9 replies; 89+ messages in thread
From: David Howells @ 2014-11-05 15:42 UTC (permalink / raw)
To: linux-unionfs, selinux
Cc: dhowells, linux-fsdevel, linux-security-module, linux-kernel
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.
There are two problems that need addressing:
(1) The files that the user accesses through the overlayfs filesystem don't
really exist there. Overlayfs passes the accesses directly through to
the underlying upper or lower file by means of having the dentry_open
inode op redirect where file->f_path and file->f_inode point.
This means that SELinux (or similar) will see the security label on one
lower layer or the other - and not the label on the overlay. There are
three labels and all may be different.
(2) file->f_path *should* point at the overlay dentry and file->f_inode
should point at the lower layer inode.
I'm not addressing (2) in this series of patches, but will leave that to a
separate patch series.
After some discussion with docker people, the agreed theory of operation will
be:
(1) The docker source (ie. the lower layer) will all be under a single label.
(2) The docker root (ie. the overlay/union layer) will all be under a single,
but different label set on the overlay mount (and each docker root may be
under its own label).
(3) Inodes in the overlayfs upper layer will be given the overlay label.
(4) A security_copy_up() operation will be provided to set the label on the
upper inode when it is created.
(5) A security_copy_up_xattr() operation will be provided to vet (and maybe
modify) each xattr as it is copied up.
(6) An extra label slot will be placed in struct file_security_struct to hold
the overlay label.
(7) security_file_open() will need to be given both the overlay and lower
dentries.
For overlayfs, the way this probably should be done is file->f_path should
be set to point to the overlay dentry (thus getting /proc right) and
file->f_inode to the lower file and make use of d_fallthru in the overlay
dentry in common with unionmount.
(8) When the lower file is accessed, both the lower and overlay labels should
be checked and audited.
(9) When the upper file is accessed, only the overlay label needs to be
checked and audited.
I need someone to examine the SELinux bits and have a look to see whether I
need to make further checks.
The patches can be found here also:
http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=overlayfs
This is based on part of Al Viro's vfs/for-next branch.
David
---
David Howells (7):
Security: Provide copy-up security hooks for unioned files
Overlayfs: Use copy-up security hooks
SELinux: Stub in copy-up handling
Security: Pass the union-layer file path into security_file_open()
SELinux: Handle opening of a unioned file
SELinux: The copy-up operation must have read permission on the lower file
SELinux: Check against union and lower labels for file ops on lower files
fs/ceph/file.c | 3 +
fs/ceph/super.h | 1
fs/cifs/cifsfs.h | 1
fs/cifs/dir.c | 3 +
fs/namei.c | 11 +++--
fs/nfs/dir.c | 6 ++
fs/nfs/nfs4_fs.h | 2 -
fs/open.c | 31 ++++++++-----
fs/overlayfs/copy_up.c | 12 +++++
fs/overlayfs/inode.c | 8 ++-
include/linux/fs.h | 16 +++++--
include/linux/security.h | 43 +++++++++++++++++-
security/capability.c | 17 +++++++
security/security.c | 19 +++++++-
security/selinux/hooks.c | 89 +++++++++++++++++++++++++++++++++++--
security/selinux/include/objsec.h | 1
16 files changed, 228 insertions(+), 35 deletions(-)
^ permalink raw reply [flat|nested] 89+ messages in thread
* [PATCH 1/7] Security: Provide copy-up security hooks for unioned files
2014-11-05 15:42 [PATCH 0/7] Security: Provide unioned file support David Howells
@ 2014-11-05 15:42 ` David Howells
2014-11-06 17:46 ` Casey Schaufler
` (3 more replies)
2014-11-05 15:42 ` [PATCH 2/7] Overlayfs: Use copy-up security hooks David Howells
` (7 subsequent siblings)
8 siblings, 4 replies; 89+ messages in thread
From: David Howells @ 2014-11-05 15:42 UTC (permalink / raw)
To: linux-unionfs, selinux
Cc: dhowells, linux-fsdevel, linux-security-module, linux-kernel
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/security.h | 35 +++++++++++++++++++++++++++++++++++
security/capability.c | 13 +++++++++++++
security/security.c | 13 +++++++++++++
3 files changed, 61 insertions(+)
diff --git a/include/linux/security.h b/include/linux/security.h
index ba96471c11ba..637a24c75d46 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -562,6 +562,24 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
* @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:
+ * Generate the secid associated with 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.
+ *
*
* Security hooks for file operations
*
@@ -1543,6 +1561,9 @@ struct security_operations {
int (*inode_setsecurity) (struct inode *inode, const char *name, const void *value, size_t size, int flags);
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);
@@ -1823,6 +1844,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);
@@ -2264,6 +2289,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/capability.c b/security/capability.c
index d68c57a62bcf..6b21615d1500 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -245,6 +245,17 @@ static void cap_inode_getsecid(const struct inode *inode, u32 *secid)
*secid = 0;
}
+static int cap_inode_copy_up(struct dentry *src, struct dentry *dst)
+{
+ return 0;
+}
+
+static int cap_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
+ const char *name, void *value, size_t *size)
+{
+ return 0;
+}
+
#ifdef CONFIG_SECURITY_PATH
static int cap_path_mknod(struct path *dir, struct dentry *dentry, umode_t mode,
unsigned int dev)
@@ -986,6 +997,8 @@ void __init security_fixup_ops(struct security_operations *ops)
set_to_cap_if_null(ops, inode_setsecurity);
set_to_cap_if_null(ops, inode_listsecurity);
set_to_cap_if_null(ops, inode_getsecid);
+ set_to_cap_if_null(ops, inode_copy_up);
+ set_to_cap_if_null(ops, inode_copy_up_xattr);
#ifdef CONFIG_SECURITY_PATH
set_to_cap_if_null(ops, path_mknod);
set_to_cap_if_null(ops, path_mkdir);
diff --git a/security/security.c b/security/security.c
index 18b35c63fc0c..96e2f189ff1e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -683,6 +683,19 @@ void security_inode_getsecid(const struct inode *inode, u32 *secid)
security_ops->inode_getsecid(inode, secid);
}
+int security_inode_copy_up(struct dentry *src, struct dentry *dst)
+{
+ return security_ops->inode_copy_up(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 security_ops->inode_copy_up_xattr(src, dst, name, value, size);
+}
+EXPORT_SYMBOL(security_inode_copy_up_xattr);
+
int security_file_permission(struct file *file, int mask)
{
int ret;
^ permalink raw reply related [flat|nested] 89+ messages in thread
* [PATCH 2/7] Overlayfs: Use copy-up security hooks
2014-11-05 15:42 [PATCH 0/7] Security: Provide unioned file support David Howells
2014-11-05 15:42 ` [PATCH 1/7] Security: Provide copy-up security hooks for unioned files David Howells
@ 2014-11-05 15:42 ` David Howells
2014-11-07 21:39 ` Paul Moore
2014-11-07 22:05 ` David Howells
2014-11-05 15:42 ` [PATCH 3/7] SELinux: Stub in copy-up handling David Howells
` (6 subsequent siblings)
8 siblings, 2 replies; 89+ messages in thread
From: David Howells @ 2014-11-05 15:42 UTC (permalink / raw)
To: linux-unionfs, selinux
Cc: dhowells, linux-fsdevel, linux-security-module, linux-kernel
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 ea10a8719107..53a357d0a214 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;
@@ -224,6 +232,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);
^ permalink raw reply related [flat|nested] 89+ messages in thread
* [PATCH 3/7] SELinux: Stub in copy-up handling
2014-11-05 15:42 [PATCH 0/7] Security: Provide unioned file support David Howells
2014-11-05 15:42 ` [PATCH 1/7] Security: Provide copy-up security hooks for unioned files David Howells
2014-11-05 15:42 ` [PATCH 2/7] Overlayfs: Use copy-up security hooks David Howells
@ 2014-11-05 15:42 ` David Howells
2014-11-07 21:44 ` Paul Moore
2014-11-07 22:08 ` David Howells
2014-11-05 15:42 ` [PATCH 4/7] Security: Pass the union-layer file path into security_file_open() David Howells
` (5 subsequent siblings)
8 siblings, 2 replies; 89+ messages in thread
From: David Howells @ 2014-11-05 15:42 UTC (permalink / raw)
To: linux-unionfs, selinux
Cc: dhowells, linux-fsdevel, linux-security-module, linux-kernel
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 | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e66314138b38..f3fe7dbbf741 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3142,6 +3142,19 @@ 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)
+{
+ 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)
@@ -5868,6 +5881,8 @@ static struct security_operations selinux_ops = {
.inode_setsecurity = selinux_inode_setsecurity,
.inode_listsecurity = selinux_inode_listsecurity,
.inode_getsecid = selinux_inode_getsecid,
+ .inode_copy_up = selinux_inode_copy_up,
+ .inode_copy_up_xattr = selinux_inode_copy_up_xattr,
.file_permission = selinux_file_permission,
.file_alloc_security = selinux_file_alloc_security,
^ permalink raw reply related [flat|nested] 89+ messages in thread
* [PATCH 4/7] Security: Pass the union-layer file path into security_file_open()
2014-11-05 15:42 [PATCH 0/7] Security: Provide unioned file support David Howells
` (2 preceding siblings ...)
2014-11-05 15:42 ` [PATCH 3/7] SELinux: Stub in copy-up handling David Howells
@ 2014-11-05 15:42 ` David Howells
2014-11-05 15:43 ` [PATCH 5/7] SELinux: Handle opening of a unioned file David Howells
` (4 subsequent siblings)
8 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-05 15:42 UTC (permalink / raw)
To: linux-unionfs, selinux
Cc: dhowells, linux-fsdevel, linux-security-module, linux-kernel
Pass the path point representing the union-layer file into security_file_open()
so that the correct security state can be divined - otherwise for overlayfs,
only the security state for the lower filesystem can be accessed.
This is a stopgap and isn't really the correct solution: the correct solution
is to make file->f_path point at the union-layer path point and file->f_inode
point at the lower file inode - but this requires the union dentry to pin the
lower dentry.
Signed-off-by: David Howells <dhowells@redhat.com>
---
fs/ceph/file.c | 3 ++-
fs/ceph/super.h | 1 +
fs/cifs/cifsfs.h | 1 +
fs/cifs/dir.c | 3 ++-
fs/namei.c | 11 +++++++----
fs/nfs/dir.c | 6 ++++--
fs/nfs/nfs4_fs.h | 2 +-
fs/open.c | 31 ++++++++++++++++++++-----------
fs/overlayfs/inode.c | 8 +++++---
include/linux/fs.h | 16 +++++++++++++---
include/linux/security.h | 8 ++++++--
security/capability.c | 4 +++-
security/security.c | 6 ++++--
security/selinux/hooks.c | 3 ++-
14 files changed, 71 insertions(+), 32 deletions(-)
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index d7e0da8366e6..43abb473bf84 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -228,6 +228,7 @@ out:
* file or symlink, return 1 so the VFS can retry.
*/
int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
+ const struct path *union_path,
struct file *file, unsigned flags, umode_t mode,
int *opened)
{
@@ -302,7 +303,7 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
ceph_init_inode_acls(dentry->d_inode, &acls);
*opened |= FILE_CREATED;
}
- err = finish_open(file, dentry, ceph_open, opened);
+ err = finish_open(file, dentry, union_path, ceph_open, opened);
}
out_req:
if (!req->r_err && req->r_target_inode)
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index b82f507979b8..5120b1a04d33 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -849,6 +849,7 @@ extern const struct address_space_operations ceph_aops;
extern int ceph_open(struct inode *inode, struct file *file);
extern int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
+ const struct path *union_path,
struct file *file, unsigned flags, umode_t mode,
int *opened);
extern int ceph_release(struct inode *inode, struct file *filp);
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index 002e0c173939..0ff63fcbecba 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -59,6 +59,7 @@ extern struct inode *cifs_root_iget(struct super_block *);
extern int cifs_create(struct inode *, struct dentry *, umode_t,
bool excl);
extern int cifs_atomic_open(struct inode *, struct dentry *,
+ const struct path *,
struct file *, unsigned, umode_t,
int *);
extern struct dentry *cifs_lookup(struct inode *, struct dentry *,
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index b72bc29cba23..356bbd981415 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -414,6 +414,7 @@ out:
int
cifs_atomic_open(struct inode *inode, struct dentry *direntry,
+ const struct path *union_path,
struct file *file, unsigned oflags, umode_t mode,
int *opened)
{
@@ -489,7 +490,7 @@ cifs_atomic_open(struct inode *inode, struct dentry *direntry,
if ((oflags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))
*opened |= FILE_CREATED;
- rc = finish_open(file, direntry, generic_file_open, opened);
+ rc = finish_open(file, direntry, union_path, generic_file_open, opened);
if (rc) {
if (server->ops->close)
server->ops->close(xid, tcon, &fid);
diff --git a/fs/namei.c b/fs/namei.c
index 922f27068c4c..5d1e40c047f9 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2647,6 +2647,7 @@ static int atomic_open(struct nameidata *nd, struct dentry *dentry,
int *opened)
{
struct inode *dir = nd->path.dentry->d_inode;
+ struct path top;
unsigned open_flag = open_to_namei_flags(op->open_flag);
umode_t mode;
int error;
@@ -2712,10 +2713,12 @@ static int atomic_open(struct nameidata *nd, struct dentry *dentry,
if (nd->flags & LOOKUP_DIRECTORY)
open_flag |= O_DIRECTORY;
+ top.dentry = dentry;
+ top.mnt = nd->path.mnt;
file->f_path.dentry = DENTRY_NOT_SET;
file->f_path.mnt = nd->path.mnt;
- error = dir->i_op->atomic_open(dir, dentry, file, open_flag, mode,
- opened);
+ error = dir->i_op->atomic_open(dir, dentry, &top, file,
+ open_flag, mode, opened);
if (error < 0) {
if (create_error && error == -ENOENT)
error = create_error;
@@ -3062,7 +3065,7 @@ finish_open_created:
goto out;
BUG_ON(*opened & FILE_OPENED); /* once it's opened, it's opened */
- error = vfs_open(&nd->path, file, current_cred());
+ error = vfs_open(&nd->path, &nd->path, file, current_cred());
if (!error) {
*opened |= FILE_OPENED;
} else {
@@ -3158,7 +3161,7 @@ static int do_tmpfile(int dfd, struct filename *pathname,
if (error)
goto out2;
file->f_path.mnt = nd->path.mnt;
- error = finish_open(file, nd->path.dentry, NULL, opened);
+ error = finish_open(file, nd->path.dentry, &nd->path, NULL, opened);
if (error)
goto out2;
error = open_check_o_direct(file);
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 06e8cfcbb670..0fd37112dc87 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1446,6 +1446,7 @@ static int do_open(struct inode *inode, struct file *filp)
static int nfs_finish_open(struct nfs_open_context *ctx,
struct dentry *dentry,
+ const struct path *union_path,
struct file *file, unsigned open_flags,
int *opened)
{
@@ -1454,7 +1455,7 @@ static int nfs_finish_open(struct nfs_open_context *ctx,
if ((open_flags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))
*opened |= FILE_CREATED;
- err = finish_open(file, dentry, do_open, opened);
+ err = finish_open(file, dentry, union_path, do_open, opened);
if (err)
goto out;
nfs_file_set_open_context(file, ctx);
@@ -1464,6 +1465,7 @@ out:
}
int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
+ const struct path *union_path,
struct file *file, unsigned open_flags,
umode_t mode, int *opened)
{
@@ -1542,7 +1544,7 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
goto out;
}
- err = nfs_finish_open(ctx, ctx->dentry, file, open_flags, opened);
+ err = nfs_finish_open(ctx, ctx->dentry, union_path, file, open_flags, opened);
trace_nfs_atomic_open_exit(dir, ctx, open_flags, err);
put_nfs_open_context(ctx);
out:
diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index be6cac37ea10..2b68a876780d 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -212,7 +212,7 @@ struct nfs4_mig_recovery_ops {
extern const struct dentry_operations nfs4_dentry_operations;
/* dir.c */
-int nfs_atomic_open(struct inode *, struct dentry *, struct file *,
+int nfs_atomic_open(struct inode *, struct dentry *, const struct path *, struct file *,
unsigned, umode_t, int *);
/* super.c */
diff --git a/fs/open.c b/fs/open.c
index de92c13b58be..0cb66c96924a 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -665,6 +665,7 @@ int open_check_o_direct(struct file *f)
}
static int do_dentry_open(struct file *f,
+ const struct path *union_path,
int (*open)(struct inode *, struct file *),
const struct cred *cred)
{
@@ -707,7 +708,7 @@ static int do_dentry_open(struct file *f,
goto cleanup_all;
}
- error = security_file_open(f, cred);
+ error = security_file_open(f, union_path, cred);
if (error)
goto cleanup_all;
@@ -755,6 +756,7 @@ cleanup_file:
* finish_open - finish opening a file
* @file: file pointer
* @dentry: pointer to dentry
+ * @union_path: path userspace actually asked for
* @open: open callback
* @opened: state of open
*
@@ -772,7 +774,9 @@ cleanup_file:
*
* Returns zero on success or -errno if the open failed.
*/
-int finish_open(struct file *file, struct dentry *dentry,
+int finish_open(struct file *file,
+ struct dentry *dentry,
+ const struct path *union_path,
int (*open)(struct inode *, struct file *),
int *opened)
{
@@ -780,7 +784,7 @@ int finish_open(struct file *file, struct dentry *dentry,
BUG_ON(*opened & FILE_OPENED); /* once it's opened, it's opened */
file->f_path.dentry = dentry;
- error = do_dentry_open(file, open, current_cred());
+ error = do_dentry_open(file, union_path, open, current_cred());
if (!error)
*opened |= FILE_OPENED;
@@ -792,7 +796,7 @@ EXPORT_SYMBOL(finish_open);
* finish_no_open - finish ->atomic_open() without opening the file
*
* @file: file pointer
- * @dentry: dentry or NULL (as returned from ->lookup())
+ * @path: The path of the file actually opened (as returned from ->lookup())
*
* This can be used to set the result of a successful lookup in ->atomic_open().
*
@@ -809,8 +813,9 @@ int finish_no_open(struct file *file, struct dentry *dentry)
}
EXPORT_SYMBOL(finish_no_open);
-struct file *dentry_open(const struct path *path, int flags,
- const struct cred *cred)
+struct file *_dentry_open(const struct path *path,
+ const struct path *union_path, int flags,
+ const struct cred *cred)
{
int error;
struct file *f;
@@ -823,7 +828,7 @@ struct file *dentry_open(const struct path *path, int flags,
f = get_empty_filp();
if (!IS_ERR(f)) {
f->f_flags = flags;
- error = vfs_open(path, f, cred);
+ error = vfs_open(path, union_path, f, cred);
if (!error) {
/* from now on we need fput() to dispose of f */
error = open_check_o_direct(f);
@@ -838,24 +843,28 @@ struct file *dentry_open(const struct path *path, int flags,
}
return f;
}
-EXPORT_SYMBOL(dentry_open);
+EXPORT_SYMBOL(_dentry_open);
/**
* vfs_open - open the file at the given path
* @path: path to open
+ * @union_path: path userspace actually asked for
* @filp: newly allocated file with f_flag initialized
* @cred: credentials to use
*/
-int vfs_open(const struct path *path, struct file *filp,
+int vfs_open(const struct path *path,
+ const struct path *union_path,
+ struct file *filp,
const struct cred *cred)
{
struct inode *inode = path->dentry->d_inode;
if (inode->i_op->dentry_open)
- return inode->i_op->dentry_open(path->dentry, filp, cred);
+ return inode->i_op->dentry_open(path->dentry,
+ union_path, filp, cred);
else {
filp->f_path = *path;
- return do_dentry_open(filp, NULL, cred);
+ return do_dentry_open(filp, union_path, NULL, cred);
}
}
EXPORT_SYMBOL(vfs_open);
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index af2d18c9fcee..87316e93dbfa 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -324,8 +324,10 @@ static bool ovl_open_need_copy_up(int flags, enum ovl_path_type type,
return true;
}
-static int ovl_dentry_open(struct dentry *dentry, struct file *file,
- const struct cred *cred)
+static int ovl_dentry_open(struct dentry *dentry,
+ const struct path *union_path,
+ struct file *file,
+ const struct cred *cred)
{
int err;
struct path realpath;
@@ -349,7 +351,7 @@ static int ovl_dentry_open(struct dentry *dentry, struct file *file,
ovl_path_upper(dentry, &realpath);
}
- err = vfs_open(&realpath, file, cred);
+ err = vfs_open(&realpath, union_path, file, cred);
out_drop_write:
if (want_write)
ovl_drop_write(dentry);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 1c12c681803f..6f8768b3cad5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1549,13 +1549,15 @@ struct inode_operations {
u64 len);
int (*update_time)(struct inode *, struct timespec *, int);
int (*atomic_open)(struct inode *, struct dentry *,
+ const struct path *,
struct file *, unsigned open_flag,
umode_t create_mode, int *opened);
int (*tmpfile) (struct inode *, struct dentry *, umode_t);
int (*set_acl)(struct inode *, struct posix_acl *, int);
/* WARNING: probably going away soon, do not use! */
- int (*dentry_open)(struct dentry *, struct file *, const struct cred *);
+ int (*dentry_open)(struct dentry *, const struct path *,
+ struct file *, const struct cred *);
} ____cacheline_aligned;
ssize_t rw_copy_check_uvector(int type, const struct iovec __user * uvector,
@@ -2071,8 +2073,15 @@ extern struct file *file_open_name(struct filename *, int, umode_t);
extern struct file *filp_open(const char *, int, umode_t);
extern struct file *file_open_root(struct dentry *, struct vfsmount *,
const char *, int);
-extern int vfs_open(const struct path *, struct file *, const struct cred *);
-extern struct file * dentry_open(const struct path *, int, const struct cred *);
+extern int vfs_open(const struct path *, const struct path *,
+ struct file *, const struct cred *);
+extern struct file *_dentry_open(const struct path *, const struct path *,
+ int, const struct cred *);
+static inline struct file *dentry_open(const struct path *path,
+ int flags, const struct cred *cred)
+{
+ return _dentry_open(path, path, flags, cred);
+}
extern int filp_close(struct file *, fl_owner_t id);
extern struct filename *getname(const char __user *);
@@ -2083,6 +2092,7 @@ enum {
FILE_OPENED = 2
};
extern int finish_open(struct file *file, struct dentry *dentry,
+ const struct path *union_path,
int (*open)(struct inode *, struct file *),
int *opened);
extern int finish_no_open(struct file *file, struct dentry *dentry);
diff --git a/include/linux/security.h b/include/linux/security.h
index 637a24c75d46..78fa30f5d708 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1584,7 +1584,9 @@ struct security_operations {
int (*file_send_sigiotask) (struct task_struct *tsk,
struct fown_struct *fown, int sig);
int (*file_receive) (struct file *file);
- int (*file_open) (struct file *file, const struct cred *cred);
+ int (*file_open) (struct file *file,
+ const struct path *union_path,
+ const struct cred *cred);
int (*task_create) (unsigned long clone_flags);
void (*task_free) (struct task_struct *task);
@@ -1863,7 +1865,8 @@ void security_file_set_fowner(struct file *file);
int security_file_send_sigiotask(struct task_struct *tsk,
struct fown_struct *fown, int sig);
int security_file_receive(struct file *file);
-int security_file_open(struct file *file, const struct cred *cred);
+int security_file_open(struct file *file, const struct path *union_path,
+ const struct cred *cred);
int security_task_create(unsigned long clone_flags);
void security_task_free(struct task_struct *task);
int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
@@ -2365,6 +2368,7 @@ static inline int security_file_receive(struct file *file)
}
static inline int security_file_open(struct file *file,
+ const struct path *union_path,
const struct cred *cred)
{
return 0;
diff --git a/security/capability.c b/security/capability.c
index 6b21615d1500..10dacb48ff53 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -370,7 +370,9 @@ static int cap_file_receive(struct file *file)
return 0;
}
-static int cap_file_open(struct file *file, const struct cred *cred)
+static int cap_file_open(struct file *file,
+ const struct path *union_path,
+ const struct cred *cred)
{
return 0;
}
diff --git a/security/security.c b/security/security.c
index 96e2f189ff1e..44b889a88d18 100644
--- a/security/security.c
+++ b/security/security.c
@@ -804,11 +804,13 @@ int security_file_receive(struct file *file)
return security_ops->file_receive(file);
}
-int security_file_open(struct file *file, const struct cred *cred)
+int security_file_open(struct file *file,
+ const struct path *union_path,
+ const struct cred *cred)
{
int ret;
- ret = security_ops->file_open(file, cred);
+ ret = security_ops->file_open(file, union_path, cred);
if (ret)
return ret;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f3fe7dbbf741..6fd8090cc7a5 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3431,7 +3431,8 @@ static int selinux_file_receive(struct file *file)
return file_has_perm(cred, file, file_to_av(file));
}
-static int selinux_file_open(struct file *file, const struct cred *cred)
+static int selinux_file_open(struct file *file, const struct path *union_path,
+ const struct cred *cred)
{
struct file_security_struct *fsec;
struct inode_security_struct *isec;
^ permalink raw reply related [flat|nested] 89+ messages in thread
* [PATCH 5/7] SELinux: Handle opening of a unioned file
2014-11-05 15:42 [PATCH 0/7] Security: Provide unioned file support David Howells
` (3 preceding siblings ...)
2014-11-05 15:42 ` [PATCH 4/7] Security: Pass the union-layer file path into security_file_open() David Howells
@ 2014-11-05 15:43 ` David Howells
2014-11-05 16:35 ` Stephen Smalley
` (4 more replies)
2014-11-05 15:43 ` [PATCH 6/7] SELinux: The copy-up operation must have read permission on the lower file David Howells
` (3 subsequent siblings)
8 siblings, 5 replies; 89+ messages in thread
From: David Howells @ 2014-11-05 15:43 UTC (permalink / raw)
To: linux-unionfs, selinux
Cc: dhowells, linux-fsdevel, linux-security-module, linux-kernel
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 | 56 +++++++++++++++++++++++++++++++++++++
security/selinux/include/objsec.h | 1 +
2 files changed, 57 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6fd8090cc7a5..f43f07fdc028 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3431,6 +3431,58 @@ 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.
+ */
+static int selinux_file_open_union(struct file *file,
+ const struct path *union_path,
+ struct file_security_struct *fsec,
+ const struct cred *cred)
+{
+ const struct superblock_security_struct *sbsec;
+ const struct inode_security_struct *isec, *dsec;
+ const struct task_security_struct *tsec = current_security();
+ struct common_audit_data ad;
+ const struct inode *inode = union_path->dentry->d_inode;
+ struct dentry *dir;
+ int rc;
+
+ sbsec = union_path->dentry->d_sb->s_security;
+
+ if (inode) {
+ isec = 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_path->dentry);
+ dsec = dir->d_inode->i_security;
+
+ rc = security_transition_sid(
+ tsec->sid, dsec->sid,
+ inode_mode_to_security_class(file_inode(file)->i_mode),
+ &union_path->dentry->d_name,
+ &fsec->union_isid);
+ dput(dir);
+ if (rc) {
+ printk(KERN_WARNING "%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.
+ */
+ ad.type = LSM_AUDIT_DATA_PATH;
+ ad.u.path = *union_path;
+ return inode_has_perm(cred, file_inode(file), fsec->union_isid, &ad);
+}
+
static int selinux_file_open(struct file *file, const struct path *union_path,
const struct cred *cred)
{
@@ -3456,6 +3508,10 @@ static int selinux_file_open(struct file *file, const struct path *union_path,
* new inode label or new policy.
* This check is not redundant - do not remove.
*/
+
+ if (union_path->dentry != file->f_path.dentry)
+ selinux_file_open_union(file, union_path, fsec, cred);
+
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 */
};
^ permalink raw reply related [flat|nested] 89+ messages in thread
* [PATCH 6/7] SELinux: The copy-up operation must have read permission on the lower file
2014-11-05 15:42 [PATCH 0/7] Security: Provide unioned file support David Howells
` (4 preceding siblings ...)
2014-11-05 15:43 ` [PATCH 5/7] SELinux: Handle opening of a unioned file David Howells
@ 2014-11-05 15:43 ` David Howells
2014-11-05 16:43 ` Stephen Smalley
2014-11-05 15:43 ` [PATCH 7/7] SELinux: Check against union and lower labels for file ops on lower files David Howells
` (2 subsequent siblings)
8 siblings, 1 reply; 89+ messages in thread
From: David Howells @ 2014-11-05 15:43 UTC (permalink / raw)
To: linux-unionfs, selinux
Cc: dhowells, linux-fsdevel, linux-security-module, linux-kernel
The copy-up operation must have read permission on the lower file for the task
that caused the copy-up. This helps prevent overlayfs from being used to
access something it shouldn't.
Signed-off-by: David Howells <dhowells@redhat.com>
---
security/selinux/hooks.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f43f07fdc028..57f9c641779f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3144,7 +3144,8 @@ static void selinux_inode_getsecid(const struct inode *inode, u32 *secid)
static int selinux_inode_copy_up(struct dentry *src, struct dentry *dst)
{
- return 0;
+ const struct cred *cred = current_cred();
+ return dentry_has_perm(cred, src, FILE__OPEN | FILE__READ);
}
static int selinux_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
^ permalink raw reply related [flat|nested] 89+ messages in thread
* [PATCH 7/7] SELinux: Check against union and lower labels for file ops on lower files
2014-11-05 15:42 [PATCH 0/7] Security: Provide unioned file support David Howells
` (5 preceding siblings ...)
2014-11-05 15:43 ` [PATCH 6/7] SELinux: The copy-up operation must have read permission on the lower file David Howells
@ 2014-11-05 15:43 ` David Howells
2014-11-06 17:35 ` Casey Schaufler
2014-11-06 17:58 ` David Howells
8 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-05 15:43 UTC (permalink / raw)
To: linux-unionfs, selinux
Cc: dhowells, linux-fsdevel, linux-security-module, linux-kernel
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 as well as the lower-layer label.
Signed-off-by: David Howells <dhowells@redhat.com>
---
security/selinux/hooks.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 57f9c641779f..7084ccb1321c 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1673,6 +1673,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;
@@ -1681,7 +1682,7 @@ static int file_has_perm(const struct cred *cred,
ad.type = LSM_AUDIT_DATA_PATH;
ad.u.path = file->f_path;
-
+
if (sid != fsec->sid) {
rc = avc_has_perm(sid, fsec->sid,
SECCLASS_FD,
@@ -1693,8 +1694,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;
^ permalink raw reply related [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2014-11-05 15:43 ` [PATCH 5/7] SELinux: Handle opening of a unioned file David Howells
@ 2014-11-05 16:35 ` Stephen Smalley
2014-11-06 12:03 ` David Howells
` (3 subsequent siblings)
4 siblings, 0 replies; 89+ messages in thread
From: Stephen Smalley @ 2014-11-05 16:35 UTC (permalink / raw)
To: David Howells, linux-unionfs, selinux
Cc: linux-fsdevel, linux-security-module, linux-kernel
On 11/05/2014 10:43 AM, David Howells wrote:
> 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 | 56 +++++++++++++++++++++++++++++++++++++
> security/selinux/include/objsec.h | 1 +
> 2 files changed, 57 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 6fd8090cc7a5..f43f07fdc028 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3431,6 +3431,58 @@ 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.
> + */
> +static int selinux_file_open_union(struct file *file,
> + const struct path *union_path,
> + struct file_security_struct *fsec,
> + const struct cred *cred)
> +{
> + const struct superblock_security_struct *sbsec;
> + const struct inode_security_struct *isec, *dsec;
> + const struct task_security_struct *tsec = current_security();
> + struct common_audit_data ad;
> + const struct inode *inode = union_path->dentry->d_inode;
> + struct dentry *dir;
> + int rc;
> +
> + sbsec = union_path->dentry->d_sb->s_security;
> +
> + if (inode) {
> + isec = 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_path->dentry);
> + dsec = dir->d_inode->i_security;
> +
> + rc = security_transition_sid(
> + tsec->sid, dsec->sid,
> + inode_mode_to_security_class(file_inode(file)->i_mode),
> + &union_path->dentry->d_name,
> + &fsec->union_isid);
> + dput(dir);
> + if (rc) {
> + printk(KERN_WARNING "%s: "
> + "security_transition_sid failed, rc=%d (name=%pD)\n",
> + __func__, -rc, file);
> + return rc;
> + }
> + }
How do we know that this union_isid will bear any relation to the actual
SID assigned to the union inode when it is created? If the union inode
does not already exist, when/where does it get created?
Also, would be good to create a common helper for use here, by
selinux_dentry_init_security(), selinux_inode_init_security(), and
may_create(). Already some seeming potential for inconsistencies there.
> +
> + /* 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.
> + */
> + ad.type = LSM_AUDIT_DATA_PATH;
> + ad.u.path = *union_path;
> + return inode_has_perm(cred, file_inode(file), fsec->union_isid, &ad);
Something is seriously wrong here; you are passing fsec->union_isid
where we expect a permissions bitmap / access vector.
> +}
> +
> static int selinux_file_open(struct file *file, const struct path *union_path,
> const struct cred *cred)
> {
> @@ -3456,6 +3508,10 @@ static int selinux_file_open(struct file *file, const struct path *union_path,
> * new inode label or new policy.
> * This check is not redundant - do not remove.
> */
> +
> + if (union_path->dentry != file->f_path.dentry)
> + selinux_file_open_union(file, union_path, fsec, cred);
Ignored return value.
> +
> 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 */
> };
>
>
> _______________________________________________
> 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.
>
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 6/7] SELinux: The copy-up operation must have read permission on the lower file
2014-11-05 15:43 ` [PATCH 6/7] SELinux: The copy-up operation must have read permission on the lower file David Howells
@ 2014-11-05 16:43 ` Stephen Smalley
2014-11-05 17:54 ` Stephen Smalley
` (2 more replies)
0 siblings, 3 replies; 89+ messages in thread
From: Stephen Smalley @ 2014-11-05 16:43 UTC (permalink / raw)
To: David Howells, linux-unionfs, selinux
Cc: linux-fsdevel, linux-security-module, linux-kernel
On 11/05/2014 10:43 AM, David Howells wrote:
> The copy-up operation must have read permission on the lower file for the task
> that caused the copy-up. This helps prevent overlayfs from being used to
> access something it shouldn't.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
> security/selinux/hooks.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f43f07fdc028..57f9c641779f 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3144,7 +3144,8 @@ static void selinux_inode_getsecid(const struct inode *inode, u32 *secid)
>
> static int selinux_inode_copy_up(struct dentry *src, struct dentry *dst)
> {
> - return 0;
> + const struct cred *cred = current_cred();
> + return dentry_has_perm(cred, src, FILE__OPEN | FILE__READ);
> }
Won't this get checked anyway when overlayfs calls vfs helpers to open
the source and those vfs helpers call the security hooks and apply the
usual checks?
Or, if not, where do you check permissions for the destination?
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 6/7] SELinux: The copy-up operation must have read permission on the lower file
2014-11-05 16:43 ` Stephen Smalley
@ 2014-11-05 17:54 ` Stephen Smalley
2014-11-06 13:39 ` Stephen Smalley
2014-11-27 14:17 ` David Howells
2014-11-27 14:21 ` David Howells
2 siblings, 1 reply; 89+ messages in thread
From: Stephen Smalley @ 2014-11-05 17:54 UTC (permalink / raw)
To: David Howells, linux-unionfs, selinux
Cc: linux-fsdevel, linux-security-module, linux-kernel
On 11/05/2014 11:43 AM, Stephen Smalley wrote:
> On 11/05/2014 10:43 AM, David Howells wrote:
>> The copy-up operation must have read permission on the lower file for the task
>> that caused the copy-up. This helps prevent overlayfs from being used to
>> access something it shouldn't.
>>
>> Signed-off-by: David Howells <dhowells@redhat.com>
>> ---
>>
>> security/selinux/hooks.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index f43f07fdc028..57f9c641779f 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -3144,7 +3144,8 @@ static void selinux_inode_getsecid(const struct inode *inode, u32 *secid)
>>
>> static int selinux_inode_copy_up(struct dentry *src, struct dentry *dst)
>> {
>> - return 0;
>> + const struct cred *cred = current_cred();
>> + return dentry_has_perm(cred, src, FILE__OPEN | FILE__READ);
>> }
>
> Won't this get checked anyway when overlayfs calls vfs helpers to open
> the source and those vfs helpers call the security hooks and apply the
> usual checks?
>
> Or, if not, where do you check permissions for the destination?
BTW, a quick look at overlayfs suggests further problems with regard to
permission checking, e.g. ovl_copy_up_one() does:
/*
* CAP_SYS_ADMIN for copying up extended attributes
* CAP_DAC_OVERRIDE for create
* CAP_FOWNER for chmod, timestamp update
* CAP_FSETID for chmod
* CAP_CHOWN for chown
* CAP_MKNOD for mknod
*/
cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE);
cap_raise(override_cred->cap_effective, CAP_FOWNER);
cap_raise(override_cred->cap_effective, CAP_FSETID);
cap_raise(override_cred->cap_effective, CAP_CHOWN);
cap_raise(override_cred->cap_effective, CAP_MKNOD);
old_cred = override_creds(override_cred);
This means that it expects to trigger those capability checks as part of
its subsequent actions. Raising those capabilities temporarily in its
credentials will pass the capability module checks but won't address the
corresponding SELinux checks (both capability and file-based), so you'll
end up triggering an entire set of checks against the current process'
credentials. This same pattern is repeated elsewhere in overlayfs.
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2014-11-05 15:43 ` [PATCH 5/7] SELinux: Handle opening of a unioned file David Howells
@ 2014-11-06 12:03 ` David Howells
2014-11-06 12:03 ` David Howells
` (3 subsequent siblings)
4 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-06 12:03 UTC (permalink / raw)
To: Stephen Smalley
Cc: dhowells, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> How do we know that this union_isid will bear any relation to the actual
> SID assigned to the union inode when it is created?
Note that overlayfs *will* have a union inode at this point, but will just not
use it for non-directories - so in this case we just use the first branch of
the if-statement:
+ if (inode) {
+ isec = inode->i_security;
+ fsec->union_isid = isec->sid;
+ } ...
in which case, I think that we can be fairly sure that we will have the right
label.
The other two cases are in case there isn't an inode - unionmount, for
example. The second case is used (if I understand the flag correctly) if the
superblock imposes a single label over all its inodes - so no problem there:
+ } else if ((sbsec->flags & SE_SBINITIALIZED) &&
+ (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
+ fsec->union_isid = sbsec->mntpoint_sid;
+ } ...
The third case is the tricky one because we have to try and derive a label.
I've copied the code from the inode creation - so unless the policy changes or
the parent directory inode changes, I would've thought we'd be okay.
> If the union inode does not already exist, when/where does it get created?
For overlayfs, union inodes *have* to exist because it's a filesystem and are
created at the normal times and in the normal way. They need to exist because
otherwise the dentry at that point in the overlay fs would be negative and the
VFS wouldn't call into the filesystem.
> Also, would be good to create a common helper for use here, by
> selinux_dentry_init_security(), selinux_inode_init_security(), and
> may_create(). Already some seeming potential for inconsistencies there.
Okay, I'll have a look at that.
> > + return inode_has_perm(cred, file_inode(file), fsec->union_isid, &ad);
>
> Something is seriously wrong here; you are passing fsec->union_isid
> where we expect a permissions bitmap / access vector.
Good point. I need to call avc_has_perm() directly. I don't necessarily have
an sclass, though, hmmm...
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2014-11-06 12:03 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-06 12:03 UTC (permalink / raw)
To: Stephen Smalley
Cc: linux-unionfs, linux-kernel, dhowells, linux-security-module,
selinux, linux-fsdevel
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> How do we know that this union_isid will bear any relation to the actual
> SID assigned to the union inode when it is created?
Note that overlayfs *will* have a union inode at this point, but will just not
use it for non-directories - so in this case we just use the first branch of
the if-statement:
+ if (inode) {
+ isec = inode->i_security;
+ fsec->union_isid = isec->sid;
+ } ...
in which case, I think that we can be fairly sure that we will have the right
label.
The other two cases are in case there isn't an inode - unionmount, for
example. The second case is used (if I understand the flag correctly) if the
superblock imposes a single label over all its inodes - so no problem there:
+ } else if ((sbsec->flags & SE_SBINITIALIZED) &&
+ (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
+ fsec->union_isid = sbsec->mntpoint_sid;
+ } ...
The third case is the tricky one because we have to try and derive a label.
I've copied the code from the inode creation - so unless the policy changes or
the parent directory inode changes, I would've thought we'd be okay.
> If the union inode does not already exist, when/where does it get created?
For overlayfs, union inodes *have* to exist because it's a filesystem and are
created at the normal times and in the normal way. They need to exist because
otherwise the dentry at that point in the overlay fs would be negative and the
VFS wouldn't call into the filesystem.
> Also, would be good to create a common helper for use here, by
> selinux_dentry_init_security(), selinux_inode_init_security(), and
> may_create(). Already some seeming potential for inconsistencies there.
Okay, I'll have a look at that.
> > + return inode_has_perm(cred, file_inode(file), fsec->union_isid, &ad);
>
> Something is seriously wrong here; you are passing fsec->union_isid
> where we expect a permissions bitmap / access vector.
Good point. I need to call avc_has_perm() directly. I don't necessarily have
an sclass, though, hmmm...
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2014-11-05 15:43 ` [PATCH 5/7] SELinux: Handle opening of a unioned file David Howells
2014-11-05 16:35 ` Stephen Smalley
@ 2014-11-06 12:27 ` David Howells
2014-11-06 12:27 ` David Howells
` (2 subsequent siblings)
4 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-06 12:27 UTC (permalink / raw)
Cc: dhowells, Stephen Smalley, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel
David Howells <dhowells@redhat.com> wrote:
> Good point. I need to call avc_has_perm() directly. I don't necessarily have
> an sclass, though, hmmm...
I do have an sclass on the lower file, however, as pointed to by
file_inode(file).
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2014-11-06 12:27 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-06 12:27 UTC (permalink / raw)
Cc: linux-unionfs, linux-kernel, dhowells, linux-security-module,
selinux, linux-fsdevel, Stephen Smalley
David Howells <dhowells@redhat.com> wrote:
> Good point. I need to call avc_has_perm() directly. I don't necessarily have
> an sclass, though, hmmm...
I do have an sclass on the lower file, however, as pointed to by
file_inode(file).
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2014-11-06 12:27 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-06 12:27 UTC (permalink / raw)
Cc: dhowells, Stephen Smalley, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel
David Howells <dhowells@redhat.com> wrote:
> Good point. I need to call avc_has_perm() directly. I don't necessarily have
> an sclass, though, hmmm...
I do have an sclass on the lower file, however, as pointed to by
file_inode(file).
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2014-11-06 12:03 ` David Howells
@ 2014-11-06 13:13 ` Stephen Smalley
-1 siblings, 0 replies; 89+ messages in thread
From: Stephen Smalley @ 2014-11-06 13:13 UTC (permalink / raw)
To: David Howells
Cc: linux-unionfs, selinux, linux-fsdevel, linux-security-module,
linux-kernel
On 11/06/2014 07:03 AM, David Howells wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
>> How do we know that this union_isid will bear any relation to the actual
>> SID assigned to the union inode when it is created?
>
> Note that overlayfs *will* have a union inode at this point, but will just not
> use it for non-directories - so in this case we just use the first branch of
> the if-statement:
>
> + if (inode) {
> + isec = inode->i_security;
> + fsec->union_isid = isec->sid;
> + } ...
>
> in which case, I think that we can be fairly sure that we will have the right
> label.
Yes, that case is fine.
> The other two cases are in case there isn't an inode - unionmount, for
> example. The second case is used (if I understand the flag correctly) if the
> superblock imposes a single label over all its inodes - so no problem there:
>
> + } else if ((sbsec->flags & SE_SBINITIALIZED) &&
> + (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> + fsec->union_isid = sbsec->mntpoint_sid;
> + } ...
Likewise.
> The third case is the tricky one because we have to try and derive a label.
> I've copied the code from the inode creation - so unless the policy changes or
> the parent directory inode changes, I would've thought we'd be okay.
Only if the filesystem ultimately calls security_inode_init_security()
on the new inode. Originally we had the vfs call post_create() hooks to
do this, but that was racy and unsafe, so we had to take the
initialization down into the filesystem code and replicate it in each
filesystem that supports labeling. So whether or not that label will in
fact be assigned to the inode depends on the filesystem in question.
Currently called by btrfs, ext[234], f2fs, gfs2, hfsplus, jffs2, jfs,
ocsfs2, xfs. Not sure what filesystems you have in mind for the
unionmount scenario.
>> If the union inode does not already exist, when/where does it get created?
>
> For overlayfs, union inodes *have* to exist because it's a filesystem and are
> created at the normal times and in the normal way. They need to exist because
> otherwise the dentry at that point in the overlay fs would be negative and the
> VFS wouldn't call into the filesystem.
>
>> Also, would be good to create a common helper for use here, by
>> selinux_dentry_init_security(), selinux_inode_init_security(), and
>> may_create(). Already some seeming potential for inconsistencies there.
>
> Okay, I'll have a look at that.
Thanks.
>>> + return inode_has_perm(cred, file_inode(file), fsec->union_isid, &ad);
>>
>> Something is seriously wrong here; you are passing fsec->union_isid
>> where we expect a permissions bitmap / access vector.
>
> Good point. I need to call avc_has_perm() directly. I don't necessarily have
> an sclass, though, hmmm...
Looks like you can get it from the other inode, as long as they are
guaranteed to have the same class (which in turn will be true as long as
they have the same file type value from the mode).
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2014-11-06 13:13 ` Stephen Smalley
0 siblings, 0 replies; 89+ messages in thread
From: Stephen Smalley @ 2014-11-06 13:13 UTC (permalink / raw)
To: David Howells
Cc: linux-fsdevel, linux-security-module, linux-unionfs, selinux,
linux-kernel
On 11/06/2014 07:03 AM, David Howells wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
>> How do we know that this union_isid will bear any relation to the actual
>> SID assigned to the union inode when it is created?
>
> Note that overlayfs *will* have a union inode at this point, but will just not
> use it for non-directories - so in this case we just use the first branch of
> the if-statement:
>
> + if (inode) {
> + isec = inode->i_security;
> + fsec->union_isid = isec->sid;
> + } ...
>
> in which case, I think that we can be fairly sure that we will have the right
> label.
Yes, that case is fine.
> The other two cases are in case there isn't an inode - unionmount, for
> example. The second case is used (if I understand the flag correctly) if the
> superblock imposes a single label over all its inodes - so no problem there:
>
> + } else if ((sbsec->flags & SE_SBINITIALIZED) &&
> + (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> + fsec->union_isid = sbsec->mntpoint_sid;
> + } ...
Likewise.
> The third case is the tricky one because we have to try and derive a label.
> I've copied the code from the inode creation - so unless the policy changes or
> the parent directory inode changes, I would've thought we'd be okay.
Only if the filesystem ultimately calls security_inode_init_security()
on the new inode. Originally we had the vfs call post_create() hooks to
do this, but that was racy and unsafe, so we had to take the
initialization down into the filesystem code and replicate it in each
filesystem that supports labeling. So whether or not that label will in
fact be assigned to the inode depends on the filesystem in question.
Currently called by btrfs, ext[234], f2fs, gfs2, hfsplus, jffs2, jfs,
ocsfs2, xfs. Not sure what filesystems you have in mind for the
unionmount scenario.
>> If the union inode does not already exist, when/where does it get created?
>
> For overlayfs, union inodes *have* to exist because it's a filesystem and are
> created at the normal times and in the normal way. They need to exist because
> otherwise the dentry at that point in the overlay fs would be negative and the
> VFS wouldn't call into the filesystem.
>
>> Also, would be good to create a common helper for use here, by
>> selinux_dentry_init_security(), selinux_inode_init_security(), and
>> may_create(). Already some seeming potential for inconsistencies there.
>
> Okay, I'll have a look at that.
Thanks.
>>> + return inode_has_perm(cred, file_inode(file), fsec->union_isid, &ad);
>>
>> Something is seriously wrong here; you are passing fsec->union_isid
>> where we expect a permissions bitmap / access vector.
>
> Good point. I need to call avc_has_perm() directly. I don't necessarily have
> an sclass, though, hmmm...
Looks like you can get it from the other inode, as long as they are
guaranteed to have the same class (which in turn will be true as long as
they have the same file type value from the mode).
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2014-11-06 12:03 ` David Howells
@ 2014-11-06 13:34 ` David Howells
-1 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-06 13:34 UTC (permalink / raw)
To: Stephen Smalley
Cc: dhowells, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> Not sure what filesystems you have in mind for the unionmount scenario.
Currently any filesystem that can be used for a unionmount top layer has to be
modified to provide certain things (DT_WHITEOUT and DT_FALLTHRU directory
entry creation) - and the way this is done precludes a number of filesystems
from being used (it modifies the on-disk format).
Whiteout creation is being provided by some filesystems for overlayfs - but in
a different way to unionmount (0,0 chardevs rather than DT_WHITEOUT dirents).
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2014-11-06 13:34 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-06 13:34 UTC (permalink / raw)
To: Stephen Smalley
Cc: linux-unionfs, linux-kernel, dhowells, linux-security-module,
selinux, linux-fsdevel
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> Not sure what filesystems you have in mind for the unionmount scenario.
Currently any filesystem that can be used for a unionmount top layer has to be
modified to provide certain things (DT_WHITEOUT and DT_FALLTHRU directory
entry creation) - and the way this is done precludes a number of filesystems
from being used (it modifies the on-disk format).
Whiteout creation is being provided by some filesystems for overlayfs - but in
a different way to unionmount (0,0 chardevs rather than DT_WHITEOUT dirents).
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 6/7] SELinux: The copy-up operation must have read permission on the lower file
2014-11-05 17:54 ` Stephen Smalley
@ 2014-11-06 13:39 ` Stephen Smalley
0 siblings, 0 replies; 89+ messages in thread
From: Stephen Smalley @ 2014-11-06 13:39 UTC (permalink / raw)
To: David Howells, linux-unionfs, selinux
Cc: linux-fsdevel, linux-security-module, linux-kernel
On 11/05/2014 12:54 PM, Stephen Smalley wrote:
> On 11/05/2014 11:43 AM, Stephen Smalley wrote:
>> On 11/05/2014 10:43 AM, David Howells wrote:
>>> The copy-up operation must have read permission on the lower file for the task
>>> that caused the copy-up. This helps prevent overlayfs from being used to
>>> access something it shouldn't.
>>>
>>> Signed-off-by: David Howells <dhowells@redhat.com>
>>> ---
>>>
>>> security/selinux/hooks.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index f43f07fdc028..57f9c641779f 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -3144,7 +3144,8 @@ static void selinux_inode_getsecid(const struct inode *inode, u32 *secid)
>>>
>>> static int selinux_inode_copy_up(struct dentry *src, struct dentry *dst)
>>> {
>>> - return 0;
>>> + const struct cred *cred = current_cred();
>>> + return dentry_has_perm(cred, src, FILE__OPEN | FILE__READ);
>>> }
>>
>> Won't this get checked anyway when overlayfs calls vfs helpers to open
>> the source and those vfs helpers call the security hooks and apply the
>> usual checks?
>>
>> Or, if not, where do you check permissions for the destination?
>
> BTW, a quick look at overlayfs suggests further problems with regard to
> permission checking, e.g. ovl_copy_up_one() does:
> /*
> * CAP_SYS_ADMIN for copying up extended attributes
> * CAP_DAC_OVERRIDE for create
> * CAP_FOWNER for chmod, timestamp update
> * CAP_FSETID for chmod
> * CAP_CHOWN for chown
> * CAP_MKNOD for mknod
> */
> cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
> cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE);
> cap_raise(override_cred->cap_effective, CAP_FOWNER);
> cap_raise(override_cred->cap_effective, CAP_FSETID);
> cap_raise(override_cred->cap_effective, CAP_CHOWN);
> cap_raise(override_cred->cap_effective, CAP_MKNOD);
> old_cred = override_creds(override_cred);
>
> This means that it expects to trigger those capability checks as part of
> its subsequent actions. Raising those capabilities temporarily in its
> credentials will pass the capability module checks but won't address the
> corresponding SELinux checks (both capability and file-based), so you'll
> end up triggering an entire set of checks against the current process'
> credentials. This same pattern is repeated elsewhere in overlayfs.
So I guess the question is what, if any, permission checks performed by
the vfs helpers are appropriate and desired when called by overlayfs,
and which ones should be always-allowed/skipped. Normally in this kind
of situation we'd either switch to a special credential set up to allow
the desired set of permissions or we'd use vfs helpers that skip the
usual permission checks. For the former, we not only need to change
cap_effective but also the SELinux context/SID in order to both allow
the desired capabilities and to allow the desired file accesses without
needing to directly allow them to the current process' context/SID. At
which point you would need to replicate in overlayfs whichever checks
should be performed on the original credentials.
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 0/7] Security: Provide unioned file support
2014-11-05 15:42 [PATCH 0/7] Security: Provide unioned file support David Howells
@ 2014-11-06 17:35 ` Casey Schaufler
2014-11-05 15:42 ` [PATCH 2/7] Overlayfs: Use copy-up security hooks David Howells
` (7 subsequent siblings)
8 siblings, 0 replies; 89+ messages in thread
From: Casey Schaufler @ 2014-11-06 17:35 UTC (permalink / raw)
To: David Howells, linux-unionfs, selinux
Cc: linux-fsdevel, linux-security-module, linux-kernel,
Casey Schaufler
On 11/5/2014 7:42 AM, 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.
I am going to have to look at this very carefully for the
Smack implications. I cannot have a review done for several
days. I do have some immediate comments below.
>
> There are two problems that need addressing:
>
> (1) The files that the user accesses through the overlayfs filesystem don't
> really exist there. Overlayfs passes the accesses directly through to
> the underlying upper or lower file by means of having the dentry_open
> inode op redirect where file->f_path and file->f_inode point.
>
> This means that SELinux (or similar) will see the security label on one
> lower layer or the other - and not the label on the overlay. There are
> three labels and all may be different.
>
> (2) file->f_path *should* point at the overlay dentry and file->f_inode
> should point at the lower layer inode.
>
> I'm not addressing (2) in this series of patches, but will leave that to a
> separate patch series.
>
>
> After some discussion with docker people, the agreed theory of operation will
> be:
>
> (1) The docker source (ie. the lower layer) will all be under a single label.
What does this mean? Is the "lower layer" the "real" file, or something else?
What is a "docker source"? What "single label" are you talking about, and
where does it come from? What about LSMs that have multiple labels on a file?
> (2) The docker root (ie. the overlay/union layer) will all be under a single,
> but different label set on the overlay mount (and each docker root may be
> under its own label).
No. Sorry, but this is the one notion that doesn't work. A layer should
either be label transparent or restricted by some sort of range concept.
Giving it a label just makes it necessary to grant everyone access to objects
with that label.
>
> (3) Inodes in the overlayfs upper layer will be given the overlay label.
Could you explain your terminology? I have no idea what the "overlay label"
might be. Is it the real one on the real filesystem? What about attributes
other than the access label? Smack has execution labels and a transmute
attribute as well as the access label.
>
> (4) A security_copy_up() operation will be provided to set the label on the
> upper inode when it is created.
>
> (5) A security_copy_up_xattr() operation will be provided to vet (and maybe
> modify) each xattr as it is copied up.
>
> (6) An extra label slot will be placed in struct file_security_struct to hold
> the overlay label.
Are you assuming a single overlay layer?
>
> (7) security_file_open() will need to be given both the overlay and lower
> dentries.
>
> For overlayfs, the way this probably should be done is file->f_path should
> be set to point to the overlay dentry (thus getting /proc right) and
> file->f_inode to the lower file and make use of d_fallthru in the overlay
> dentry in common with unionmount.
>
> (8) When the lower file is accessed, both the lower and overlay labels should
> be checked and audited.
>
> (9) When the upper file is accessed, only the overlay label needs to be
> checked and audited.
>
> I need someone to examine the SELinux bits and have a look to see whether I
> need to make further checks.
The Smack code, too.
Oh, and I have been lead to believe that upcoming AppArmor enhancements
could be affected as well.
>
>
> The patches can be found here also:
>
> http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=overlayfs
>
> This is based on part of Al Viro's vfs/for-next branch.
>
> David
> ---
> David Howells (7):
> Security: Provide copy-up security hooks for unioned files
> Overlayfs: Use copy-up security hooks
> SELinux: Stub in copy-up handling
> Security: Pass the union-layer file path into security_file_open()
> SELinux: Handle opening of a unioned file
> SELinux: The copy-up operation must have read permission on the lower file
> SELinux: Check against union and lower labels for file ops on lower files
>
>
> fs/ceph/file.c | 3 +
> fs/ceph/super.h | 1
> fs/cifs/cifsfs.h | 1
> fs/cifs/dir.c | 3 +
> fs/namei.c | 11 +++--
> fs/nfs/dir.c | 6 ++
> fs/nfs/nfs4_fs.h | 2 -
> fs/open.c | 31 ++++++++-----
> fs/overlayfs/copy_up.c | 12 +++++
> fs/overlayfs/inode.c | 8 ++-
> include/linux/fs.h | 16 +++++--
> include/linux/security.h | 43 +++++++++++++++++-
> security/capability.c | 17 +++++++
> security/security.c | 19 +++++++-
> security/selinux/hooks.c | 89 +++++++++++++++++++++++++++++++++++--
> security/selinux/include/objsec.h | 1
> 16 files changed, 228 insertions(+), 35 deletions(-)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 0/7] Security: Provide unioned file support
@ 2014-11-06 17:35 ` Casey Schaufler
0 siblings, 0 replies; 89+ messages in thread
From: Casey Schaufler @ 2014-11-06 17:35 UTC (permalink / raw)
To: David Howells, linux-unionfs, selinux
Cc: linux-fsdevel, linux-security-module, linux-kernel
On 11/5/2014 7:42 AM, 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.
I am going to have to look at this very carefully for the
Smack implications. I cannot have a review done for several
days. I do have some immediate comments below.
>
> There are two problems that need addressing:
>
> (1) The files that the user accesses through the overlayfs filesystem don't
> really exist there. Overlayfs passes the accesses directly through to
> the underlying upper or lower file by means of having the dentry_open
> inode op redirect where file->f_path and file->f_inode point.
>
> This means that SELinux (or similar) will see the security label on one
> lower layer or the other - and not the label on the overlay. There are
> three labels and all may be different.
>
> (2) file->f_path *should* point at the overlay dentry and file->f_inode
> should point at the lower layer inode.
>
> I'm not addressing (2) in this series of patches, but will leave that to a
> separate patch series.
>
>
> After some discussion with docker people, the agreed theory of operation will
> be:
>
> (1) The docker source (ie. the lower layer) will all be under a single label.
What does this mean? Is the "lower layer" the "real" file, or something else?
What is a "docker source"? What "single label" are you talking about, and
where does it come from? What about LSMs that have multiple labels on a file?
> (2) The docker root (ie. the overlay/union layer) will all be under a single,
> but different label set on the overlay mount (and each docker root may be
> under its own label).
No. Sorry, but this is the one notion that doesn't work. A layer should
either be label transparent or restricted by some sort of range concept.
Giving it a label just makes it necessary to grant everyone access to objects
with that label.
>
> (3) Inodes in the overlayfs upper layer will be given the overlay label.
Could you explain your terminology? I have no idea what the "overlay label"
might be. Is it the real one on the real filesystem? What about attributes
other than the access label? Smack has execution labels and a transmute
attribute as well as the access label.
>
> (4) A security_copy_up() operation will be provided to set the label on the
> upper inode when it is created.
>
> (5) A security_copy_up_xattr() operation will be provided to vet (and maybe
> modify) each xattr as it is copied up.
>
> (6) An extra label slot will be placed in struct file_security_struct to hold
> the overlay label.
Are you assuming a single overlay layer?
>
> (7) security_file_open() will need to be given both the overlay and lower
> dentries.
>
> For overlayfs, the way this probably should be done is file->f_path should
> be set to point to the overlay dentry (thus getting /proc right) and
> file->f_inode to the lower file and make use of d_fallthru in the overlay
> dentry in common with unionmount.
>
> (8) When the lower file is accessed, both the lower and overlay labels should
> be checked and audited.
>
> (9) When the upper file is accessed, only the overlay label needs to be
> checked and audited.
>
> I need someone to examine the SELinux bits and have a look to see whether I
> need to make further checks.
The Smack code, too.
Oh, and I have been lead to believe that upcoming AppArmor enhancements
could be affected as well.
>
>
> The patches can be found here also:
>
> http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=overlayfs
>
> This is based on part of Al Viro's vfs/for-next branch.
>
> David
> ---
> David Howells (7):
> Security: Provide copy-up security hooks for unioned files
> Overlayfs: Use copy-up security hooks
> SELinux: Stub in copy-up handling
> Security: Pass the union-layer file path into security_file_open()
> SELinux: Handle opening of a unioned file
> SELinux: The copy-up operation must have read permission on the lower file
> SELinux: Check against union and lower labels for file ops on lower files
>
>
> fs/ceph/file.c | 3 +
> fs/ceph/super.h | 1
> fs/cifs/cifsfs.h | 1
> fs/cifs/dir.c | 3 +
> fs/namei.c | 11 +++--
> fs/nfs/dir.c | 6 ++
> fs/nfs/nfs4_fs.h | 2 -
> fs/open.c | 31 ++++++++-----
> fs/overlayfs/copy_up.c | 12 +++++
> fs/overlayfs/inode.c | 8 ++-
> include/linux/fs.h | 16 +++++--
> include/linux/security.h | 43 +++++++++++++++++-
> security/capability.c | 17 +++++++
> security/security.c | 19 +++++++-
> security/selinux/hooks.c | 89 +++++++++++++++++++++++++++++++++++--
> security/selinux/include/objsec.h | 1
> 16 files changed, 228 insertions(+), 35 deletions(-)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 1/7] Security: Provide copy-up security hooks for unioned files
2014-11-05 15:42 ` [PATCH 1/7] Security: Provide copy-up security hooks for unioned files David Howells
@ 2014-11-06 17:46 ` Casey Schaufler
2014-11-07 14:49 ` David Howells
` (2 subsequent siblings)
3 siblings, 0 replies; 89+ messages in thread
From: Casey Schaufler @ 2014-11-06 17:46 UTC (permalink / raw)
To: David Howells, linux-unionfs, selinux
Cc: linux-fsdevel, linux-security-module, linux-kernel
On 11/5/2014 7:42 AM, David Howells wrote:
> 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/security.h | 35 +++++++++++++++++++++++++++++++++++
> security/capability.c | 13 +++++++++++++
> security/security.c | 13 +++++++++++++
> 3 files changed, 61 insertions(+)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ba96471c11ba..637a24c75d46 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -562,6 +562,24 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
> * @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:
> + * Generate the secid associated with the destination inode when a unioned
NAK.
You can't just deal with the access label, nor can you assume that there
is exactly one. Don't use secids. Secids are a performance problem for
Smack and any LSM that uses blobs directly. I don't see anything in the
code here that involves secids. Why comment on them here?
> + * 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.
Who is going to call this? How are is the caller going to know all the xattr
names that matter?
> + *
> *
> * Security hooks for file operations
> *
> @@ -1543,6 +1561,9 @@ struct security_operations {
> int (*inode_setsecurity) (struct inode *inode, const char *name, const void *value, size_t size, int flags);
> 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);
> @@ -1823,6 +1844,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);
> @@ -2264,6 +2289,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/capability.c b/security/capability.c
> index d68c57a62bcf..6b21615d1500 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -245,6 +245,17 @@ static void cap_inode_getsecid(const struct inode *inode, u32 *secid)
> *secid = 0;
> }
>
> +static int cap_inode_copy_up(struct dentry *src, struct dentry *dst)
> +{
> + return 0;
> +}
> +
> +static int cap_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
> + const char *name, void *value, size_t *size)
> +{
> + return 0;
> +}
> +
Does this mean that without LSM help no xattrs ever get copied?
> #ifdef CONFIG_SECURITY_PATH
> static int cap_path_mknod(struct path *dir, struct dentry *dentry, umode_t mode,
> unsigned int dev)
> @@ -986,6 +997,8 @@ void __init security_fixup_ops(struct security_operations *ops)
> set_to_cap_if_null(ops, inode_setsecurity);
> set_to_cap_if_null(ops, inode_listsecurity);
> set_to_cap_if_null(ops, inode_getsecid);
> + set_to_cap_if_null(ops, inode_copy_up);
> + set_to_cap_if_null(ops, inode_copy_up_xattr);
> #ifdef CONFIG_SECURITY_PATH
> set_to_cap_if_null(ops, path_mknod);
> set_to_cap_if_null(ops, path_mkdir);
> diff --git a/security/security.c b/security/security.c
> index 18b35c63fc0c..96e2f189ff1e 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -683,6 +683,19 @@ void security_inode_getsecid(const struct inode *inode, u32 *secid)
> security_ops->inode_getsecid(inode, secid);
> }
>
> +int security_inode_copy_up(struct dentry *src, struct dentry *dst)
> +{
> + return security_ops->inode_copy_up(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 security_ops->inode_copy_up_xattr(src, dst, name, value, size);
> +}
> +EXPORT_SYMBOL(security_inode_copy_up_xattr);
> +
> int security_file_permission(struct file *file, int mask)
> {
> int ret;
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 0/7] Security: Provide unioned file support
2014-11-05 15:42 [PATCH 0/7] Security: Provide unioned file support David Howells
@ 2014-11-06 17:58 ` David Howells
2014-11-05 15:42 ` [PATCH 2/7] Overlayfs: Use copy-up security hooks David Howells
` (7 subsequent siblings)
8 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-06 17:58 UTC (permalink / raw)
To: Casey Schaufler
Cc: dhowells, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel
Casey Schaufler <casey@schaufler-ca.com> wrote:
> I am going to have to look at this very carefully for the
> Smack implications. I cannot have a review done for several
> days. I do have some immediate comments below.
Note that Smack and Tomoyo won't compile with the patches as they are because
some changes will need to be made. I haven't made them yet as I want to work
out the changes for SELinux first.
> > (1) The docker source (ie. the lower layer) will all be under a single
> > label.
>
> What does this mean? Is the "lower layer" the "real" file, or something else?
> What is a "docker source"? What "single label" are you talking about, and
> where does it come from? What about LSMs that have multiple labels on a file?
Firstly, docker: https://www.docker.com/
What we're dealing with is unioning two filesystems. The way it works is that
you start off with a "lower" filesystem that contains the basic files you want
to appear and you overlay on top of that another filesystem such that the
files and directories in the lower filesystem can be accessed through the
overlay (the overlay redirects queries to the lower fs).
So, say the lower fs is mounted on /foo and you mount an overlay on /bar that
points to /foo as its lower layer. If you access /bar/bin/ls, the overlay
will pass that down, giving you a file referring to /foo/bin/ls.
The clever bit comes when you try altering /bar/bin/ls. At that point
/foo/bin/ls is "copied up" to the overlay - to /bar/bin/ls becomes a real
file. The writes are then made to the file in the overlay - and these files
are persistent.
In theory, any further access to /bar/bin/ls will then serve up the contents
of /bar/bin/ls and ignore /foo/bin/ls. In practice, as things stand, if you
have a read-only fd already referring to /foo/bin/ls through /bar/bin/ls, this
will be unaffected by the copy up.
Docker uses containers and chroots to provide not-virtual-machines, using an
overlay to manage the root filesystem. Indeed, you can provide several
overlays using the same lower fs.
So to answer your questions:
> What does this mean?
It has been decided that for the purposes of docker, all files within the
docker root fs will have the same label. We'd have to provide policy
namespacing otherwise (I think).
> Is the "lower layer" the "real" file, or something else?
The lower layer is a filesystem, though it contains real files. In the case
of unionmount (an alternative to overlayfs), there may be multiple ordered
lower layers.
> What is a "docker source"?
The lower layer contains the "source" files for your docker image.
> What "single label" are you talking about, and where does it come from?
The label you set on the lower layer files.
> What about LSMs that have multiple labels on a file?
Setting policy is something I'll have to leave to the docker people and the
administrators of systems that use docker.
But all I'm proposing is a way to give the LSM access to both the file in the
overlay and the file in the lower fs that is leeching through into the
overlay.
> > (2) The docker root (ie. the overlay/union layer) will all be under a
> > single, but different label set on the overlay mount (and each docker
> > root may be under its own label).
>
> No. Sorry, but this is the one notion that doesn't work. A layer should
> either be label transparent or restricted by some sort of range concept.
> Giving it a label just makes it necessary to grant everyone access to objects
> with that label.
The problem is that we're not using a virtual machine. Labels on the docker
chroot and labels on the master root filesystem are under the same policy.
Further, we may have several docker instances that have separate overlays on
the same lower layer but should perhaps have separate labels.
> > (3) Inodes in the overlayfs upper layer will be given the overlay label.
>
> Could you explain your terminology? I have no idea what the "overlay label"
> might be. Is it the real one on the real filesystem? What about attributes
> other than the access label? Smack has execution labels and a transmute
> attribute as well as the access label.
The overlay label is the label on the inode in the overlay layer (overlayfs)
or that would be on the inode if it existed (unionmount).
> > (6) An extra label slot will be placed in struct file_security_struct to
> > hold the overlay label.
>
> Are you assuming a single overlay layer?
There can only be only overlay at the top. It may have lower layers that are
also overlays, but we really want the top label.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 0/7] Security: Provide unioned file support
@ 2014-11-06 17:58 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-06 17:58 UTC (permalink / raw)
To: Casey Schaufler
Cc: linux-unionfs, linux-kernel, dhowells, linux-security-module,
selinux, linux-fsdevel
Casey Schaufler <casey@schaufler-ca.com> wrote:
> I am going to have to look at this very carefully for the
> Smack implications. I cannot have a review done for several
> days. I do have some immediate comments below.
Note that Smack and Tomoyo won't compile with the patches as they are because
some changes will need to be made. I haven't made them yet as I want to work
out the changes for SELinux first.
> > (1) The docker source (ie. the lower layer) will all be under a single
> > label.
>
> What does this mean? Is the "lower layer" the "real" file, or something else?
> What is a "docker source"? What "single label" are you talking about, and
> where does it come from? What about LSMs that have multiple labels on a file?
Firstly, docker: https://www.docker.com/
What we're dealing with is unioning two filesystems. The way it works is that
you start off with a "lower" filesystem that contains the basic files you want
to appear and you overlay on top of that another filesystem such that the
files and directories in the lower filesystem can be accessed through the
overlay (the overlay redirects queries to the lower fs).
So, say the lower fs is mounted on /foo and you mount an overlay on /bar that
points to /foo as its lower layer. If you access /bar/bin/ls, the overlay
will pass that down, giving you a file referring to /foo/bin/ls.
The clever bit comes when you try altering /bar/bin/ls. At that point
/foo/bin/ls is "copied up" to the overlay - to /bar/bin/ls becomes a real
file. The writes are then made to the file in the overlay - and these files
are persistent.
In theory, any further access to /bar/bin/ls will then serve up the contents
of /bar/bin/ls and ignore /foo/bin/ls. In practice, as things stand, if you
have a read-only fd already referring to /foo/bin/ls through /bar/bin/ls, this
will be unaffected by the copy up.
Docker uses containers and chroots to provide not-virtual-machines, using an
overlay to manage the root filesystem. Indeed, you can provide several
overlays using the same lower fs.
So to answer your questions:
> What does this mean?
It has been decided that for the purposes of docker, all files within the
docker root fs will have the same label. We'd have to provide policy
namespacing otherwise (I think).
> Is the "lower layer" the "real" file, or something else?
The lower layer is a filesystem, though it contains real files. In the case
of unionmount (an alternative to overlayfs), there may be multiple ordered
lower layers.
> What is a "docker source"?
The lower layer contains the "source" files for your docker image.
> What "single label" are you talking about, and where does it come from?
The label you set on the lower layer files.
> What about LSMs that have multiple labels on a file?
Setting policy is something I'll have to leave to the docker people and the
administrators of systems that use docker.
But all I'm proposing is a way to give the LSM access to both the file in the
overlay and the file in the lower fs that is leeching through into the
overlay.
> > (2) The docker root (ie. the overlay/union layer) will all be under a
> > single, but different label set on the overlay mount (and each docker
> > root may be under its own label).
>
> No. Sorry, but this is the one notion that doesn't work. A layer should
> either be label transparent or restricted by some sort of range concept.
> Giving it a label just makes it necessary to grant everyone access to objects
> with that label.
The problem is that we're not using a virtual machine. Labels on the docker
chroot and labels on the master root filesystem are under the same policy.
Further, we may have several docker instances that have separate overlays on
the same lower layer but should perhaps have separate labels.
> > (3) Inodes in the overlayfs upper layer will be given the overlay label.
>
> Could you explain your terminology? I have no idea what the "overlay label"
> might be. Is it the real one on the real filesystem? What about attributes
> other than the access label? Smack has execution labels and a transmute
> attribute as well as the access label.
The overlay label is the label on the inode in the overlay layer (overlayfs)
or that would be on the inode if it existed (unionmount).
> > (6) An extra label slot will be placed in struct file_security_struct to
> > hold the overlay label.
>
> Are you assuming a single overlay layer?
There can only be only overlay at the top. It may have lower layers that are
also overlays, but we really want the top label.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 0/7] Security: Provide unioned file support
2014-11-06 17:58 ` David Howells
@ 2014-11-06 18:40 ` Casey Schaufler
-1 siblings, 0 replies; 89+ messages in thread
From: Casey Schaufler @ 2014-11-06 18:40 UTC (permalink / raw)
To: David Howells
Cc: linux-unionfs, selinux, linux-fsdevel, linux-security-module,
linux-kernel, Casey Schaufler
On 11/6/2014 9:58 AM, David Howells wrote:
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>> I am going to have to look at this very carefully for the
>> Smack implications. I cannot have a review done for several
>> days. I do have some immediate comments below.
> Note that Smack and Tomoyo won't compile with the patches as they are because
> some changes will need to be made. I haven't made them yet as I want to work
> out the changes for SELinux first.
There are going to be lots of implications. Please look at the
bigger picture.
>
>>> (1) The docker source (ie. the lower layer) will all be under a single
>>> label.
>> What does this mean? Is the "lower layer" the "real" file, or something else?
>> What is a "docker source"? What "single label" are you talking about, and
>> where does it come from? What about LSMs that have multiple labels on a file?
> Firstly, docker: https://www.docker.com/
>
> What we're dealing with is unioning two filesystems. The way it works is that
> you start off with a "lower" filesystem that contains the basic files you want
> to appear and you overlay on top of that another filesystem such that the
> files and directories in the lower filesystem can be accessed through the
> overlay (the overlay redirects queries to the lower fs).
>
> So, say the lower fs is mounted on /foo and you mount an overlay on /bar that
> points to /foo as its lower layer. If you access /bar/bin/ls, the overlay
> will pass that down, giving you a file referring to /foo/bin/ls.
>
> The clever bit comes when you try altering /bar/bin/ls. At that point
> /foo/bin/ls is "copied up" to the overlay - to /bar/bin/ls becomes a real
> file. The writes are then made to the file in the overlay - and these files
> are persistent.
>
> In theory, any further access to /bar/bin/ls will then serve up the contents
> of /bar/bin/ls and ignore /foo/bin/ls. In practice, as things stand, if you
> have a read-only fd already referring to /foo/bin/ls through /bar/bin/ls, this
> will be unaffected by the copy up.
>
> Docker uses containers and chroots to provide not-virtual-machines, using an
> overlay to manage the root filesystem. Indeed, you can provide several
> overlays using the same lower fs.
>
> So to answer your questions:
>
>> What does this mean?
> It has been decided that for the purposes of docker, all files within the
> docker root fs will have the same label. We'd have to provide policy
> namespacing otherwise (I think).
That's just insane. "It has been decided" by who? Obviously not people
who care about security policies. Maybe it's good enough for your
particular use case, but labeling of files has to be up to the security
module. That's what the modules are for.
>
>> Is the "lower layer" the "real" file, or something else?
> The lower layer is a filesystem, though it contains real files. In the case
> of unionmount (an alternative to overlayfs), there may be multiple ordered
> lower layers.
>
>> What is a "docker source"?
> The lower layer contains the "source" files for your docker image.
>
>> What "single label" are you talking about, and where does it come from?
> The label you set on the lower layer files.
>
>> What about LSMs that have multiple labels on a file?
> Setting policy is something I'll have to leave to the docker people and the
> administrators of systems that use docker.
What does that mean? If the underlying mechanism can't do the job,
how the dickens is the administrator supposed to make it happen?
>
> But all I'm proposing is a way to give the LSM access to both the file in the
> overlay and the file in the lower fs that is leeching through into the
> overlay.
But your mechanisms are simultaneously incomplete and over specified.
>
>>> (2) The docker root (ie. the overlay/union layer) will all be under a
>>> single, but different label set on the overlay mount (and each docker
>>> root may be under its own label).
>> No. Sorry, but this is the one notion that doesn't work. A layer should
>> either be label transparent or restricted by some sort of range concept.
>> Giving it a label just makes it necessary to grant everyone access to objects
>> with that label.
> The problem is that we're not using a virtual machine. Labels on the docker
> chroot and labels on the master root filesystem are under the same policy.
>
> Further, we may have several docker instances that have separate overlays on
> the same lower layer but should perhaps have separate labels.
Your proposal is a cop out. It may work in a limited set of cases.
It is not a general solution.
>
>>> (3) Inodes in the overlayfs upper layer will be given the overlay label.
>> Could you explain your terminology? I have no idea what the "overlay label"
>> might be. Is it the real one on the real filesystem? What about attributes
>> other than the access label? Smack has execution labels and a transmute
>> attribute as well as the access label.
> The overlay label is the label on the inode in the overlay layer (overlayfs)
> or that would be on the inode if it existed (unionmount).
>
>>> (6) An extra label slot will be placed in struct file_security_struct to
>>> hold the overlay label.
>> Are you assuming a single overlay layer?
> There can only be only overlay at the top. It may have lower layers that are
> also overlays, but we really want the top label.
>
> David
>
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 0/7] Security: Provide unioned file support
@ 2014-11-06 18:40 ` Casey Schaufler
0 siblings, 0 replies; 89+ messages in thread
From: Casey Schaufler @ 2014-11-06 18:40 UTC (permalink / raw)
To: David Howells
Cc: linux-unionfs, linux-kernel, linux-security-module, selinux,
linux-fsdevel
On 11/6/2014 9:58 AM, David Howells wrote:
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>> I am going to have to look at this very carefully for the
>> Smack implications. I cannot have a review done for several
>> days. I do have some immediate comments below.
> Note that Smack and Tomoyo won't compile with the patches as they are because
> some changes will need to be made. I haven't made them yet as I want to work
> out the changes for SELinux first.
There are going to be lots of implications. Please look at the
bigger picture.
>
>>> (1) The docker source (ie. the lower layer) will all be under a single
>>> label.
>> What does this mean? Is the "lower layer" the "real" file, or something else?
>> What is a "docker source"? What "single label" are you talking about, and
>> where does it come from? What about LSMs that have multiple labels on a file?
> Firstly, docker: https://www.docker.com/
>
> What we're dealing with is unioning two filesystems. The way it works is that
> you start off with a "lower" filesystem that contains the basic files you want
> to appear and you overlay on top of that another filesystem such that the
> files and directories in the lower filesystem can be accessed through the
> overlay (the overlay redirects queries to the lower fs).
>
> So, say the lower fs is mounted on /foo and you mount an overlay on /bar that
> points to /foo as its lower layer. If you access /bar/bin/ls, the overlay
> will pass that down, giving you a file referring to /foo/bin/ls.
>
> The clever bit comes when you try altering /bar/bin/ls. At that point
> /foo/bin/ls is "copied up" to the overlay - to /bar/bin/ls becomes a real
> file. The writes are then made to the file in the overlay - and these files
> are persistent.
>
> In theory, any further access to /bar/bin/ls will then serve up the contents
> of /bar/bin/ls and ignore /foo/bin/ls. In practice, as things stand, if you
> have a read-only fd already referring to /foo/bin/ls through /bar/bin/ls, this
> will be unaffected by the copy up.
>
> Docker uses containers and chroots to provide not-virtual-machines, using an
> overlay to manage the root filesystem. Indeed, you can provide several
> overlays using the same lower fs.
>
> So to answer your questions:
>
>> What does this mean?
> It has been decided that for the purposes of docker, all files within the
> docker root fs will have the same label. We'd have to provide policy
> namespacing otherwise (I think).
That's just insane. "It has been decided" by who? Obviously not people
who care about security policies. Maybe it's good enough for your
particular use case, but labeling of files has to be up to the security
module. That's what the modules are for.
>
>> Is the "lower layer" the "real" file, or something else?
> The lower layer is a filesystem, though it contains real files. In the case
> of unionmount (an alternative to overlayfs), there may be multiple ordered
> lower layers.
>
>> What is a "docker source"?
> The lower layer contains the "source" files for your docker image.
>
>> What "single label" are you talking about, and where does it come from?
> The label you set on the lower layer files.
>
>> What about LSMs that have multiple labels on a file?
> Setting policy is something I'll have to leave to the docker people and the
> administrators of systems that use docker.
What does that mean? If the underlying mechanism can't do the job,
how the dickens is the administrator supposed to make it happen?
>
> But all I'm proposing is a way to give the LSM access to both the file in the
> overlay and the file in the lower fs that is leeching through into the
> overlay.
But your mechanisms are simultaneously incomplete and over specified.
>
>>> (2) The docker root (ie. the overlay/union layer) will all be under a
>>> single, but different label set on the overlay mount (and each docker
>>> root may be under its own label).
>> No. Sorry, but this is the one notion that doesn't work. A layer should
>> either be label transparent or restricted by some sort of range concept.
>> Giving it a label just makes it necessary to grant everyone access to objects
>> with that label.
> The problem is that we're not using a virtual machine. Labels on the docker
> chroot and labels on the master root filesystem are under the same policy.
>
> Further, we may have several docker instances that have separate overlays on
> the same lower layer but should perhaps have separate labels.
Your proposal is a cop out. It may work in a limited set of cases.
It is not a general solution.
>
>>> (3) Inodes in the overlayfs upper layer will be given the overlay label.
>> Could you explain your terminology? I have no idea what the "overlay label"
>> might be. Is it the real one on the real filesystem? What about attributes
>> other than the access label? Smack has execution labels and a transmute
>> attribute as well as the access label.
> The overlay label is the label on the inode in the overlay layer (overlayfs)
> or that would be on the inode if it existed (unionmount).
>
>>> (6) An extra label slot will be placed in struct file_security_struct to
>>> hold the overlay label.
>> Are you assuming a single overlay layer?
> There can only be only overlay at the top. It may have lower layers that are
> also overlays, but we really want the top label.
>
> David
>
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 1/7] Security: Provide copy-up security hooks for unioned files
2014-11-05 15:42 ` [PATCH 1/7] Security: Provide copy-up security hooks for unioned files David Howells
@ 2014-11-07 14:49 ` David Howells
2014-11-07 14:49 ` David Howells
` (2 subsequent siblings)
3 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-07 14:49 UTC (permalink / raw)
To: Casey Schaufler
Cc: dhowells, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel
Casey Schaufler <casey@schaufler-ca.com> wrote:
> > + * @inode_copy_up:
> > + * Generate the secid associated with the destination inode when a unioned
>
> NAK.
>
> You can't just deal with the access label, nor can you assume that there
> is exactly one. Don't use secids. ...
Okay, I've changed the documentation to:
* @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.
>
> Who is going to call this?
Anyone that copies up a file from the lower layer in a union to an upper
layer. Overlayfs for example. Unionmount for another.
> How are is the caller going to know all the xattr names that matter?
The caller has no idea. That's why it presents them all here.
> > +static int cap_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
> > + const char *name, void *value, size_t *size)
> > +{
> > + return 0;
> > +}
> > +
>
> Does this mean that without LSM help no xattrs ever get copied?
No. This is merely a filter. The caller reads the xattr, presents them to
this hook and then, if not asked to discard, writes the xattr. I've added a
bit to the end of the comment to this effect.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 1/7] Security: Provide copy-up security hooks for unioned files
@ 2014-11-07 14:49 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-07 14:49 UTC (permalink / raw)
To: Casey Schaufler
Cc: linux-unionfs, linux-kernel, dhowells, linux-security-module,
selinux, linux-fsdevel
Casey Schaufler <casey@schaufler-ca.com> wrote:
> > + * @inode_copy_up:
> > + * Generate the secid associated with the destination inode when a unioned
>
> NAK.
>
> You can't just deal with the access label, nor can you assume that there
> is exactly one. Don't use secids. ...
Okay, I've changed the documentation to:
* @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.
>
> Who is going to call this?
Anyone that copies up a file from the lower layer in a union to an upper
layer. Overlayfs for example. Unionmount for another.
> How are is the caller going to know all the xattr names that matter?
The caller has no idea. That's why it presents them all here.
> > +static int cap_inode_copy_up_xattr(struct dentry *src, struct dentry *dst,
> > + const char *name, void *value, size_t *size)
> > +{
> > + return 0;
> > +}
> > +
>
> Does this mean that without LSM help no xattrs ever get copied?
No. This is merely a filter. The caller reads the xattr, presents them to
this hook and then, if not asked to discard, writes the xattr. I've added a
bit to the end of the comment to this effect.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 0/7] Security: Provide unioned file support
2014-11-06 17:58 ` David Howells
@ 2014-11-07 15:21 ` David Howells
-1 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-07 15:21 UTC (permalink / raw)
To: Casey Schaufler
Cc: dhowells, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel
Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> What does this mean?
> > It has been decided that for the purposes of docker, all files within the
> > docker root fs will have the same label. We'd have to provide policy
> > namespacing otherwise (I think).
>
> That's just insane. "It has been decided" by who? Obviously not people
> who care about security policies. Maybe it's good enough for your
> particular use case, but labeling of files has to be up to the security
> module. That's what the modules are for.
It has been decided by the docker people that I've dealt with. I was
expecting there to be different labels throughout a docker image, but
apparently not...
> >> What about LSMs that have multiple labels on a file?
> > Setting policy is something I'll have to leave to the docker people and the
> > administrators of systems that use docker.
>
> What does that mean? If the underlying mechanism can't do the job,
> how the dickens is the administrator supposed to make it happen?
I'm trying to make the kernel able to support a policy on this at all - not
actually write the policy itself. The policy may well vary depending on the
installation anyway.
> > But all I'm proposing is a way to give the LSM access to both the file in
> > the overlay and the file in the lower fs that is leeching through into the
> > overlay.
>
> But your mechanisms are simultaneously incomplete and over specified.
Well then, specify better ones! I'm fairly certain it is incomplete and I'm
*trying* to get input on how it may be improved. I should've marked the
patches [RFC] but it didn't occur to me until just after I'd sent them (of
course).
Anyway, there are a number of things one has to take account of:
(1) There may be multiple 'views' or instances of a file in a union - and
each may be labelled differently.
(2) The lower file may have xattrs attached to it that represent the security
policy.
(3) The union file may have xattrs attached to it that represent the security
policy.
(4) When copying the lower file up, the xattrs representing the security
attributes of the lower file must not be written as they may incorrectly
overwrite the security attributes of the union file.
(5) There needs to be a way to set the security attributes on the union file.
(6) When setting the attributes on the union file, the LSM might need to take
account of the attributes of the lower file in their derivation.
(7) There may be no inode in the union layer on which to hang the attributes
for the upper file.
(8) struct file::f_security should be used to contain the union layer
labellage on open files that point to a lower layer.
(9) Ideally, file->f_path would point to the union layer and file->f_inode to
the lower layer when there is no upper file. However, this is not the
case at the moment. The file struct *only* points to the lower file or
the upper file and never both.
(10) Overlayfs has an extra complication in that there are potentially three
files involved in any union. The lower file that is the source, the
upper file that where the copy-up goes and the virtual union file in the
overlay fs that redirects to one or the other.
I've taken the approach that we assume that the upper file (if it exists)
has the same labellage as the union file.
> Your proposal is a cop out. It may work in a limited set of cases.
> It is not a general solution.
Actually, I think the actual code interface I've proposed is pretty close to
being a general solution. I've given the LSM the information I have, it can
implement a fabulous maze of specially crafted labels if it wants to - and
even have multiple labels per inode or per file. The VFS does not care.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 0/7] Security: Provide unioned file support
@ 2014-11-07 15:21 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-07 15:21 UTC (permalink / raw)
To: Casey Schaufler
Cc: linux-unionfs, linux-kernel, dhowells, linux-security-module,
selinux, linux-fsdevel
Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> What does this mean?
> > It has been decided that for the purposes of docker, all files within the
> > docker root fs will have the same label. We'd have to provide policy
> > namespacing otherwise (I think).
>
> That's just insane. "It has been decided" by who? Obviously not people
> who care about security policies. Maybe it's good enough for your
> particular use case, but labeling of files has to be up to the security
> module. That's what the modules are for.
It has been decided by the docker people that I've dealt with. I was
expecting there to be different labels throughout a docker image, but
apparently not...
> >> What about LSMs that have multiple labels on a file?
> > Setting policy is something I'll have to leave to the docker people and the
> > administrators of systems that use docker.
>
> What does that mean? If the underlying mechanism can't do the job,
> how the dickens is the administrator supposed to make it happen?
I'm trying to make the kernel able to support a policy on this at all - not
actually write the policy itself. The policy may well vary depending on the
installation anyway.
> > But all I'm proposing is a way to give the LSM access to both the file in
> > the overlay and the file in the lower fs that is leeching through into the
> > overlay.
>
> But your mechanisms are simultaneously incomplete and over specified.
Well then, specify better ones! I'm fairly certain it is incomplete and I'm
*trying* to get input on how it may be improved. I should've marked the
patches [RFC] but it didn't occur to me until just after I'd sent them (of
course).
Anyway, there are a number of things one has to take account of:
(1) There may be multiple 'views' or instances of a file in a union - and
each may be labelled differently.
(2) The lower file may have xattrs attached to it that represent the security
policy.
(3) The union file may have xattrs attached to it that represent the security
policy.
(4) When copying the lower file up, the xattrs representing the security
attributes of the lower file must not be written as they may incorrectly
overwrite the security attributes of the union file.
(5) There needs to be a way to set the security attributes on the union file.
(6) When setting the attributes on the union file, the LSM might need to take
account of the attributes of the lower file in their derivation.
(7) There may be no inode in the union layer on which to hang the attributes
for the upper file.
(8) struct file::f_security should be used to contain the union layer
labellage on open files that point to a lower layer.
(9) Ideally, file->f_path would point to the union layer and file->f_inode to
the lower layer when there is no upper file. However, this is not the
case at the moment. The file struct *only* points to the lower file or
the upper file and never both.
(10) Overlayfs has an extra complication in that there are potentially three
files involved in any union. The lower file that is the source, the
upper file that where the copy-up goes and the virtual union file in the
overlay fs that redirects to one or the other.
I've taken the approach that we assume that the upper file (if it exists)
has the same labellage as the union file.
> Your proposal is a cop out. It may work in a limited set of cases.
> It is not a general solution.
Actually, I think the actual code interface I've proposed is pretty close to
being a general solution. I've given the LSM the information I have, it can
implement a fabulous maze of specially crafted labels if it wants to - and
even have multiple labels per inode or per file. The VFS does not care.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 0/7] Security: Provide unioned file support
2014-11-07 15:21 ` David Howells
@ 2014-11-07 18:54 ` Daniel J Walsh
-1 siblings, 0 replies; 89+ messages in thread
From: Daniel J Walsh @ 2014-11-07 18:54 UTC (permalink / raw)
To: David Howells, Casey Schaufler
Cc: linux-unionfs, linux-kernel, linux-security-module, selinux,
linux-fsdevel
On 11/07/2014 10:21 AM, David Howells wrote:
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>>>> What does this mean?
>>> It has been decided that for the purposes of docker, all files within the
>>> docker root fs will have the same label. We'd have to provide policy
>>> namespacing otherwise (I think).
>> That's just insane. "It has been decided" by who? Obviously not people
>> who care about security policies. Maybe it's good enough for your
>> particular use case, but labeling of files has to be up to the security
>> module. That's what the modules are for.
> It has been decided by the docker people that I've dealt with. I was
> expecting there to be different labels throughout a docker image, but
> apparently not...
We want the equivalent of the mount option
context="system_u:object_r:svirt_sandbox_file_t:s0:c1,c2"
Which would label all of the content in the OVERLAYFS with this label.
If we have to have a readonly label for the lower
level and a RW Label for the context mount then that is fine with us.
The lower level can actually just reveal the label of the INODE.
We can take care of labeling it with a single label.
If the context="LABEL" (Or its equivalent is not passed), it should get
some kind of label based on the parent directory I would guess
or transition rules.
With docker we want to treat all contents of a container with a single
label and then separate containers based on MCS(Svirt) separation.
>>>> What about LSMs that have multiple labels on a file?
>>> Setting policy is something I'll have to leave to the docker people and the
>>> administrators of systems that use docker.
>> What does that mean? If the underlying mechanism can't do the job,
>> how the dickens is the administrator supposed to make it happen?
> I'm trying to make the kernel able to support a policy on this at all - not
> actually write the policy itself. The policy may well vary depending on the
> installation anyway.
>
>>> But all I'm proposing is a way to give the LSM access to both the file in
>>> the overlay and the file in the lower fs that is leeching through into the
>>> overlay.
>> But your mechanisms are simultaneously incomplete and over specified.
> Well then, specify better ones! I'm fairly certain it is incomplete and I'm
> *trying* to get input on how it may be improved. I should've marked the
> patches [RFC] but it didn't occur to me until just after I'd sent them (of
> course).
>
> Anyway, there are a number of things one has to take account of:
>
> (1) There may be multiple 'views' or instances of a file in a union - and
> each may be labelled differently.
>
> (2) The lower file may have xattrs attached to it that represent the security
> policy.
>
> (3) The union file may have xattrs attached to it that represent the security
> policy.
>
> (4) When copying the lower file up, the xattrs representing the security
> attributes of the lower file must not be written as they may incorrectly
> overwrite the security attributes of the union file.
>
> (5) There needs to be a way to set the security attributes on the union file.
>
> (6) When setting the attributes on the union file, the LSM might need to take
> account of the attributes of the lower file in their derivation.
>
> (7) There may be no inode in the union layer on which to hang the attributes
> for the upper file.
>
> (8) struct file::f_security should be used to contain the union layer
> labellage on open files that point to a lower layer.
>
> (9) Ideally, file->f_path would point to the union layer and file->f_inode to
> the lower layer when there is no upper file. However, this is not the
> case at the moment. The file struct *only* points to the lower file or
> the upper file and never both.
>
> (10) Overlayfs has an extra complication in that there are potentially three
> files involved in any union. The lower file that is the source, the
> upper file that where the copy-up goes and the virtual union file in the
> overlay fs that redirects to one or the other.
>
> I've taken the approach that we assume that the upper file (if it exists)
> has the same labellage as the union file.
>
>> Your proposal is a cop out. It may work in a limited set of cases.
>> It is not a general solution.
> Actually, I think the actual code interface I've proposed is pretty close to
> being a general solution. I've given the LSM the information I have, it can
> implement a fabulous maze of specially crafted labels if it wants to - and
> even have multiple labels per inode or per file. The VFS does not care.
>
> David
>
> _______________________________________________
> 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.
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 0/7] Security: Provide unioned file support
@ 2014-11-07 18:54 ` Daniel J Walsh
0 siblings, 0 replies; 89+ messages in thread
From: Daniel J Walsh @ 2014-11-07 18:54 UTC (permalink / raw)
To: David Howells, Casey Schaufler
Cc: linux-fsdevel, linux-security-module, linux-unionfs, selinux,
linux-kernel
On 11/07/2014 10:21 AM, David Howells wrote:
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>>>> What does this mean?
>>> It has been decided that for the purposes of docker, all files within the
>>> docker root fs will have the same label. We'd have to provide policy
>>> namespacing otherwise (I think).
>> That's just insane. "It has been decided" by who? Obviously not people
>> who care about security policies. Maybe it's good enough for your
>> particular use case, but labeling of files has to be up to the security
>> module. That's what the modules are for.
> It has been decided by the docker people that I've dealt with. I was
> expecting there to be different labels throughout a docker image, but
> apparently not...
We want the equivalent of the mount option
context="system_u:object_r:svirt_sandbox_file_t:s0:c1,c2"
Which would label all of the content in the OVERLAYFS with this label.
If we have to have a readonly label for the lower
level and a RW Label for the context mount then that is fine with us.
The lower level can actually just reveal the label of the INODE.
We can take care of labeling it with a single label.
If the context="LABEL" (Or its equivalent is not passed), it should get
some kind of label based on the parent directory I would guess
or transition rules.
With docker we want to treat all contents of a container with a single
label and then separate containers based on MCS(Svirt) separation.
>>>> What about LSMs that have multiple labels on a file?
>>> Setting policy is something I'll have to leave to the docker people and the
>>> administrators of systems that use docker.
>> What does that mean? If the underlying mechanism can't do the job,
>> how the dickens is the administrator supposed to make it happen?
> I'm trying to make the kernel able to support a policy on this at all - not
> actually write the policy itself. The policy may well vary depending on the
> installation anyway.
>
>>> But all I'm proposing is a way to give the LSM access to both the file in
>>> the overlay and the file in the lower fs that is leeching through into the
>>> overlay.
>> But your mechanisms are simultaneously incomplete and over specified.
> Well then, specify better ones! I'm fairly certain it is incomplete and I'm
> *trying* to get input on how it may be improved. I should've marked the
> patches [RFC] but it didn't occur to me until just after I'd sent them (of
> course).
>
> Anyway, there are a number of things one has to take account of:
>
> (1) There may be multiple 'views' or instances of a file in a union - and
> each may be labelled differently.
>
> (2) The lower file may have xattrs attached to it that represent the security
> policy.
>
> (3) The union file may have xattrs attached to it that represent the security
> policy.
>
> (4) When copying the lower file up, the xattrs representing the security
> attributes of the lower file must not be written as they may incorrectly
> overwrite the security attributes of the union file.
>
> (5) There needs to be a way to set the security attributes on the union file.
>
> (6) When setting the attributes on the union file, the LSM might need to take
> account of the attributes of the lower file in their derivation.
>
> (7) There may be no inode in the union layer on which to hang the attributes
> for the upper file.
>
> (8) struct file::f_security should be used to contain the union layer
> labellage on open files that point to a lower layer.
>
> (9) Ideally, file->f_path would point to the union layer and file->f_inode to
> the lower layer when there is no upper file. However, this is not the
> case at the moment. The file struct *only* points to the lower file or
> the upper file and never both.
>
> (10) Overlayfs has an extra complication in that there are potentially three
> files involved in any union. The lower file that is the source, the
> upper file that where the copy-up goes and the virtual union file in the
> overlay fs that redirects to one or the other.
>
> I've taken the approach that we assume that the upper file (if it exists)
> has the same labellage as the union file.
>
>> Your proposal is a cop out. It may work in a limited set of cases.
>> It is not a general solution.
> Actually, I think the actual code interface I've proposed is pretty close to
> being a general solution. I've given the LSM the information I have, it can
> implement a fabulous maze of specially crafted labels if it wants to - and
> even have multiple labels per inode or per file. The VFS does not care.
>
> David
>
> _______________________________________________
> 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.
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 1/7] Security: Provide copy-up security hooks for unioned files
2014-11-05 15:42 ` [PATCH 1/7] Security: Provide copy-up security hooks for unioned files David Howells
@ 2014-11-07 21:22 ` Paul Moore
2014-11-07 14:49 ` David Howells
` (2 subsequent siblings)
3 siblings, 0 replies; 89+ messages in thread
From: Paul Moore @ 2014-11-07 21:22 UTC (permalink / raw)
To: David Howells
Cc: linux-unionfs, selinux, linux-fsdevel, linux-security-module,
linux-kernel
On Wednesday, November 05, 2014 03:42:28 PM David Howells wrote:
> 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.
This didn't occur to me earlier, but we may want to pick a different phrase to
use instead of "copy_up" as that has a special meaning for some security/MLS
folks (although strangely enough, I suspect most of these copy-on-write
operations will be "copy up" in the MLS sense of the word).
How about "security_inode_copy_overlay" or something like that?
> + * @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.
Just so I'm clean, if the LSM wanted to modify the xattr it would modify
@value/@size and return 0?
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 1/7] Security: Provide copy-up security hooks for unioned files
@ 2014-11-07 21:22 ` Paul Moore
0 siblings, 0 replies; 89+ messages in thread
From: Paul Moore @ 2014-11-07 21:22 UTC (permalink / raw)
To: David Howells
Cc: linux-fsdevel, linux-security-module, linux-unionfs, selinux,
linux-kernel
On Wednesday, November 05, 2014 03:42:28 PM David Howells wrote:
> 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.
This didn't occur to me earlier, but we may want to pick a different phrase to
use instead of "copy_up" as that has a special meaning for some security/MLS
folks (although strangely enough, I suspect most of these copy-on-write
operations will be "copy up" in the MLS sense of the word).
How about "security_inode_copy_overlay" or something like that?
> + * @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.
Just so I'm clean, if the LSM wanted to modify the xattr it would modify
@value/@size and return 0?
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 2/7] Overlayfs: Use copy-up security hooks
2014-11-05 15:42 ` [PATCH 2/7] Overlayfs: Use copy-up security hooks David Howells
@ 2014-11-07 21:39 ` Paul Moore
2014-11-07 22:05 ` David Howells
1 sibling, 0 replies; 89+ messages in thread
From: Paul Moore @ 2014-11-07 21:39 UTC (permalink / raw)
To: David Howells
Cc: linux-unionfs, selinux, linux-fsdevel, linux-security-module,
linux-kernel
On Wednesday, November 05, 2014 03:42:38 PM David Howells wrote:
> 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 ea10a8719107..53a357d0a214 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);
So the LSM must modify the xattr in place? I suppose that since the @value is
allocated to the max size it shouldn't be a problem. Just checking ...
> + 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;
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 2/7] Overlayfs: Use copy-up security hooks
@ 2014-11-07 21:39 ` Paul Moore
0 siblings, 0 replies; 89+ messages in thread
From: Paul Moore @ 2014-11-07 21:39 UTC (permalink / raw)
To: David Howells
Cc: linux-fsdevel, linux-security-module, linux-unionfs, selinux,
linux-kernel
On Wednesday, November 05, 2014 03:42:38 PM David Howells wrote:
> 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 ea10a8719107..53a357d0a214 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);
So the LSM must modify the xattr in place? I suppose that since the @value is
allocated to the max size it shouldn't be a problem. Just checking ...
> + 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;
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 3/7] SELinux: Stub in copy-up handling
2014-11-05 15:42 ` [PATCH 3/7] SELinux: Stub in copy-up handling David Howells
@ 2014-11-07 21:44 ` Paul Moore
2014-11-07 22:08 ` David Howells
1 sibling, 0 replies; 89+ messages in thread
From: Paul Moore @ 2014-11-07 21:44 UTC (permalink / raw)
To: David Howells
Cc: linux-unionfs, selinux, linux-fsdevel, linux-security-module,
linux-kernel
On Wednesday, November 05, 2014 03:42:48 PM David Howells wrote:
> 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 | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e66314138b38..f3fe7dbbf741 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3142,6 +3142,19 @@ 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)
> +{
> + if (strcmp(name, XATTR_NAME_SELINUX) == 0)
> + return 1; /* Discard */
I understand that this is just a stub, but I we need to discuss this at some
point and I figure better in this patch than elsewhere where it might get
lost.
For the docker, context= mount use case (similar to SELinux/sVirt) dropping
the SELinux xattr is probably an okay behavior. However, I would expect that
ultimately this is something we would want to control by policy and/or the
presence of a context= label.
> + return 0;
> +}
> +
> /* file security operations */
>
> static int selinux_revalidate_file_permission(struct file *file, int mask)
> @@ -5868,6 +5881,8 @@ static struct security_operations selinux_ops = {
> .inode_setsecurity = selinux_inode_setsecurity,
> .inode_listsecurity = selinux_inode_listsecurity,
> .inode_getsecid = selinux_inode_getsecid,
> + .inode_copy_up = selinux_inode_copy_up,
> + .inode_copy_up_xattr = selinux_inode_copy_up_xattr,
>
> .file_permission = selinux_file_permission,
> .file_alloc_security = selinux_file_alloc_security,
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-security-module" in the body of a message to
> majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 3/7] SELinux: Stub in copy-up handling
@ 2014-11-07 21:44 ` Paul Moore
0 siblings, 0 replies; 89+ messages in thread
From: Paul Moore @ 2014-11-07 21:44 UTC (permalink / raw)
To: David Howells
Cc: linux-fsdevel, linux-security-module, linux-unionfs, selinux,
linux-kernel
On Wednesday, November 05, 2014 03:42:48 PM David Howells wrote:
> 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 | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e66314138b38..f3fe7dbbf741 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3142,6 +3142,19 @@ 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)
> +{
> + if (strcmp(name, XATTR_NAME_SELINUX) == 0)
> + return 1; /* Discard */
I understand that this is just a stub, but I we need to discuss this at some
point and I figure better in this patch than elsewhere where it might get
lost.
For the docker, context= mount use case (similar to SELinux/sVirt) dropping
the SELinux xattr is probably an okay behavior. However, I would expect that
ultimately this is something we would want to control by policy and/or the
presence of a context= label.
> + return 0;
> +}
> +
> /* file security operations */
>
> static int selinux_revalidate_file_permission(struct file *file, int mask)
> @@ -5868,6 +5881,8 @@ static struct security_operations selinux_ops = {
> .inode_setsecurity = selinux_inode_setsecurity,
> .inode_listsecurity = selinux_inode_listsecurity,
> .inode_getsecid = selinux_inode_getsecid,
> + .inode_copy_up = selinux_inode_copy_up,
> + .inode_copy_up_xattr = selinux_inode_copy_up_xattr,
>
> .file_permission = selinux_file_permission,
> .file_alloc_security = selinux_file_alloc_security,
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-security-module" in the body of a message to
> majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 2/7] Overlayfs: Use copy-up security hooks
2014-11-05 15:42 ` [PATCH 2/7] Overlayfs: Use copy-up security hooks David Howells
@ 2014-11-07 22:05 ` David Howells
2014-11-07 22:05 ` David Howells
1 sibling, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-07 22:05 UTC (permalink / raw)
To: Paul Moore
Cc: dhowells, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel
Paul Moore <paul@paul-moore.com> wrote:
> So the LSM must modify the xattr in place? I suppose that since the @value
> is allocated to the max size it shouldn't be a problem. Just checking ...
... And the caller must provide a maximally sized buffer (which it likely has
to allocate anyway).
I'm not sure I really need to provide the modification thing. I suspect a
binary keep or discard decision is sufficient.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 2/7] Overlayfs: Use copy-up security hooks
@ 2014-11-07 22:05 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-07 22:05 UTC (permalink / raw)
To: Paul Moore
Cc: linux-unionfs, linux-kernel, dhowells, linux-security-module,
selinux, linux-fsdevel
Paul Moore <paul@paul-moore.com> wrote:
> So the LSM must modify the xattr in place? I suppose that since the @value
> is allocated to the max size it shouldn't be a problem. Just checking ...
... And the caller must provide a maximally sized buffer (which it likely has
to allocate anyway).
I'm not sure I really need to provide the modification thing. I suspect a
binary keep or discard decision is sufficient.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 3/7] SELinux: Stub in copy-up handling
2014-11-05 15:42 ` [PATCH 3/7] SELinux: Stub in copy-up handling David Howells
@ 2014-11-07 22:08 ` David Howells
2014-11-07 22:08 ` David Howells
1 sibling, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-07 22:08 UTC (permalink / raw)
To: Paul Moore
Cc: dhowells, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel
Paul Moore <paul@paul-moore.com> wrote:
> > +static int selinux_inode_copy_up_xattr(struct dentry *src, struct dentry
> > ...*dst,
> > + const char *name, void *value, size_t *size)
> > +{
> > + if (strcmp(name, XATTR_NAME_SELINUX) == 0)
> > + return 1; /* Discard */
>
> I understand that this is just a stub, but I we need to discuss this at some
> point and I figure better in this patch than elsewhere where it might get
> lost.
>
> For the docker, context= mount use case (similar to SELinux/sVirt) dropping
> the SELinux xattr is probably an okay behavior. However, I would expect that
> ultimately this is something we would want to control by policy and/or the
> presence of a context= label.
The problem is that the label you actually want has already been set and you
have to be careful not to overwrite it.
The other hook (->inode_copy_up) is called first to label the destination
inode - and that has access to the source file also so it can label the
destination with consideration of the attributes on the source. Normally, the
attributes on the source will be in memory attached to the source inode, I
would imagine.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 3/7] SELinux: Stub in copy-up handling
@ 2014-11-07 22:08 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-07 22:08 UTC (permalink / raw)
To: Paul Moore
Cc: linux-unionfs, linux-kernel, dhowells, linux-security-module,
selinux, linux-fsdevel
Paul Moore <paul@paul-moore.com> wrote:
> > +static int selinux_inode_copy_up_xattr(struct dentry *src, struct dentry
> > ...*dst,
> > + const char *name, void *value, size_t *size)
> > +{
> > + if (strcmp(name, XATTR_NAME_SELINUX) == 0)
> > + return 1; /* Discard */
>
> I understand that this is just a stub, but I we need to discuss this at some
> point and I figure better in this patch than elsewhere where it might get
> lost.
>
> For the docker, context= mount use case (similar to SELinux/sVirt) dropping
> the SELinux xattr is probably an okay behavior. However, I would expect that
> ultimately this is something we would want to control by policy and/or the
> presence of a context= label.
The problem is that the label you actually want has already been set and you
have to be careful not to overwrite it.
The other hook (->inode_copy_up) is called first to label the destination
inode - and that has access to the source file also so it can label the
destination with consideration of the attributes on the source. Normally, the
attributes on the source will be in memory attached to the source inode, I
would imagine.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 1/7] Security: Provide copy-up security hooks for unioned files
2014-11-05 15:42 ` [PATCH 1/7] Security: Provide copy-up security hooks for unioned files David Howells
@ 2014-11-07 22:10 ` David Howells
2014-11-07 14:49 ` David Howells
` (2 subsequent siblings)
3 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-07 22:10 UTC (permalink / raw)
To: Paul Moore
Cc: dhowells, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel
Paul Moore <paul@paul-moore.com> wrote:
> This didn't occur to me earlier, but we may want to pick a different phrase
> to use instead of "copy_up" as that has a special meaning for some
> security/MLS folks...
It does?
security_inode_make_union()?
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 1/7] Security: Provide copy-up security hooks for unioned files
@ 2014-11-07 22:10 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-07 22:10 UTC (permalink / raw)
To: Paul Moore
Cc: linux-unionfs, linux-kernel, dhowells, linux-security-module,
selinux, linux-fsdevel
Paul Moore <paul@paul-moore.com> wrote:
> This didn't occur to me earlier, but we may want to pick a different phrase
> to use instead of "copy_up" as that has a special meaning for some
> security/MLS folks...
It does?
security_inode_make_union()?
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 0/7] Security: Provide unioned file support
2014-11-07 18:54 ` Daniel J Walsh
@ 2014-11-09 1:31 ` Casey Schaufler
-1 siblings, 0 replies; 89+ messages in thread
From: Casey Schaufler @ 2014-11-09 1:31 UTC (permalink / raw)
To: Daniel J Walsh, David Howells
Cc: linux-unionfs, linux-kernel, linux-security-module, selinux,
linux-fsdevel, Casey Schaufler
On 11/7/2014 10:54 AM, Daniel J Walsh wrote:
> On 11/07/2014 10:21 AM, David Howells wrote:
>> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>
>>>>> What does this mean?
>>>> It has been decided that for the purposes of docker, all files within the
>>>> docker root fs will have the same label. We'd have to provide policy
>>>> namespacing otherwise (I think).
>>> That's just insane. "It has been decided" by who? Obviously not people
>>> who care about security policies. Maybe it's good enough for your
>>> particular use case, but labeling of files has to be up to the security
>>> module. That's what the modules are for.
>> It has been decided by the docker people that I've dealt with. I was
>> expecting there to be different labels throughout a docker image, but
>> apparently not...
> We want the equivalent of the mount option
> context="system_u:object_r:svirt_sandbox_file_t:s0:c1,c2"
Which just means that the policy used in the container is going
to have to grant access to everything running in that container
to that context. That's fine if you want to emasculate your module
within the container. But it requires that you do so, and that is
wrong.
> Which would label all of the content in the OVERLAYFS with this label.
> If we have to have a readonly label for the lower
> level and a RW Label for the context mount then that is fine with us.
> The lower level can actually just reveal the label of the INODE.
> We can take care of labeling it with a single label.
>
> If the context="LABEL" (Or its equivalent is not passed), it should get
> some kind of label based on the parent directory I would guess
> or transition rules.
>
> With docker we want to treat all contents of a container with a single
> label and then separate containers based on MCS(Svirt) separation.
That seems like a limited and rather pointless use case to me.
>>>>> What about LSMs that have multiple labels on a file?
>>>> Setting policy is something I'll have to leave to the docker people and the
>>>> administrators of systems that use docker.
>>> What does that mean? If the underlying mechanism can't do the job,
>>> how the dickens is the administrator supposed to make it happen?
>> I'm trying to make the kernel able to support a policy on this at all - not
>> actually write the policy itself. The policy may well vary depending on the
>> installation anyway.
>>
>>>> But all I'm proposing is a way to give the LSM access to both the file in
>>>> the overlay and the file in the lower fs that is leeching through into the
>>>> overlay.
>>> But your mechanisms are simultaneously incomplete and over specified.
>> Well then, specify better ones! I'm fairly certain it is incomplete and I'm
>> *trying* to get input on how it may be improved. I should've marked the
>> patches [RFC] but it didn't occur to me until just after I'd sent them (of
>> course).
I'd love to do so, but I'm frying too many kittens right now
to put the effort required into making this right. I do have
faith that this can be worked out. Sorry that I missed the implicit
[RFC]. It helps to know that feedback is still open.
>>
>> Anyway, there are a number of things one has to take account of:
>>
>> (1) There may be multiple 'views' or instances of a file in a union - and
>> each may be labelled differently.
>>
>> (2) The lower file may have xattrs attached to it that represent the security
>> policy.
>>
>> (3) The union file may have xattrs attached to it that represent the security
>> policy.
>>
>> (4) When copying the lower file up, the xattrs representing the security
>> attributes of the lower file must not be written as they may incorrectly
>> overwrite the security attributes of the union file.
>>
>> (5) There needs to be a way to set the security attributes on the union file.
>>
>> (6) When setting the attributes on the union file, the LSM might need to take
>> account of the attributes of the lower file in their derivation.
>>
>> (7) There may be no inode in the union layer on which to hang the attributes
>> for the upper file.
>>
>> (8) struct file::f_security should be used to contain the union layer
>> labellage on open files that point to a lower layer.
>>
>> (9) Ideally, file->f_path would point to the union layer and file->f_inode to
>> the lower layer when there is no upper file. However, this is not the
>> case at the moment. The file struct *only* points to the lower file or
>> the upper file and never both.
>>
>> (10) Overlayfs has an extra complication in that there are potentially three
>> files involved in any union. The lower file that is the source, the
>> upper file that where the copy-up goes and the virtual union file in the
>> overlay fs that redirects to one or the other.
>>
>> I've taken the approach that we assume that the upper file (if it exists)
>> has the same labellage as the union file.
>>
>>> Your proposal is a cop out. It may work in a limited set of cases.
>>> It is not a general solution.
>> Actually, I think the actual code interface I've proposed is pretty close to
>> being a general solution. I've given the LSM the information I have, it can
>> implement a fabulous maze of specially crafted labels if it wants to - and
>> even have multiple labels per inode or per file. The VFS does not care.
>>
>> David
>>
>> _______________________________________________
>> 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.
>
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 0/7] Security: Provide unioned file support
@ 2014-11-09 1:31 ` Casey Schaufler
0 siblings, 0 replies; 89+ messages in thread
From: Casey Schaufler @ 2014-11-09 1:31 UTC (permalink / raw)
To: Daniel J Walsh, David Howells
Cc: linux-unionfs, linux-kernel, linux-security-module, selinux,
linux-fsdevel
On 11/7/2014 10:54 AM, Daniel J Walsh wrote:
> On 11/07/2014 10:21 AM, David Howells wrote:
>> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>
>>>>> What does this mean?
>>>> It has been decided that for the purposes of docker, all files within the
>>>> docker root fs will have the same label. We'd have to provide policy
>>>> namespacing otherwise (I think).
>>> That's just insane. "It has been decided" by who? Obviously not people
>>> who care about security policies. Maybe it's good enough for your
>>> particular use case, but labeling of files has to be up to the security
>>> module. That's what the modules are for.
>> It has been decided by the docker people that I've dealt with. I was
>> expecting there to be different labels throughout a docker image, but
>> apparently not...
> We want the equivalent of the mount option
> context="system_u:object_r:svirt_sandbox_file_t:s0:c1,c2"
Which just means that the policy used in the container is going
to have to grant access to everything running in that container
to that context. That's fine if you want to emasculate your module
within the container. But it requires that you do so, and that is
wrong.
> Which would label all of the content in the OVERLAYFS with this label.
> If we have to have a readonly label for the lower
> level and a RW Label for the context mount then that is fine with us.
> The lower level can actually just reveal the label of the INODE.
> We can take care of labeling it with a single label.
>
> If the context="LABEL" (Or its equivalent is not passed), it should get
> some kind of label based on the parent directory I would guess
> or transition rules.
>
> With docker we want to treat all contents of a container with a single
> label and then separate containers based on MCS(Svirt) separation.
That seems like a limited and rather pointless use case to me.
>>>>> What about LSMs that have multiple labels on a file?
>>>> Setting policy is something I'll have to leave to the docker people and the
>>>> administrators of systems that use docker.
>>> What does that mean? If the underlying mechanism can't do the job,
>>> how the dickens is the administrator supposed to make it happen?
>> I'm trying to make the kernel able to support a policy on this at all - not
>> actually write the policy itself. The policy may well vary depending on the
>> installation anyway.
>>
>>>> But all I'm proposing is a way to give the LSM access to both the file in
>>>> the overlay and the file in the lower fs that is leeching through into the
>>>> overlay.
>>> But your mechanisms are simultaneously incomplete and over specified.
>> Well then, specify better ones! I'm fairly certain it is incomplete and I'm
>> *trying* to get input on how it may be improved. I should've marked the
>> patches [RFC] but it didn't occur to me until just after I'd sent them (of
>> course).
I'd love to do so, but I'm frying too many kittens right now
to put the effort required into making this right. I do have
faith that this can be worked out. Sorry that I missed the implicit
[RFC]. It helps to know that feedback is still open.
>>
>> Anyway, there are a number of things one has to take account of:
>>
>> (1) There may be multiple 'views' or instances of a file in a union - and
>> each may be labelled differently.
>>
>> (2) The lower file may have xattrs attached to it that represent the security
>> policy.
>>
>> (3) The union file may have xattrs attached to it that represent the security
>> policy.
>>
>> (4) When copying the lower file up, the xattrs representing the security
>> attributes of the lower file must not be written as they may incorrectly
>> overwrite the security attributes of the union file.
>>
>> (5) There needs to be a way to set the security attributes on the union file.
>>
>> (6) When setting the attributes on the union file, the LSM might need to take
>> account of the attributes of the lower file in their derivation.
>>
>> (7) There may be no inode in the union layer on which to hang the attributes
>> for the upper file.
>>
>> (8) struct file::f_security should be used to contain the union layer
>> labellage on open files that point to a lower layer.
>>
>> (9) Ideally, file->f_path would point to the union layer and file->f_inode to
>> the lower layer when there is no upper file. However, this is not the
>> case at the moment. The file struct *only* points to the lower file or
>> the upper file and never both.
>>
>> (10) Overlayfs has an extra complication in that there are potentially three
>> files involved in any union. The lower file that is the source, the
>> upper file that where the copy-up goes and the virtual union file in the
>> overlay fs that redirects to one or the other.
>>
>> I've taken the approach that we assume that the upper file (if it exists)
>> has the same labellage as the union file.
>>
>>> Your proposal is a cop out. It may work in a limited set of cases.
>>> It is not a general solution.
>> Actually, I think the actual code interface I've proposed is pretty close to
>> being a general solution. I've given the LSM the information I have, it can
>> implement a fabulous maze of specially crafted labels if it wants to - and
>> even have multiple labels per inode or per file. The VFS does not care.
>>
>> David
>>
>> _______________________________________________
>> 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.
>
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 0/7] Security: Provide unioned file support
2014-11-09 1:31 ` Casey Schaufler
@ 2014-11-10 13:59 ` Daniel J Walsh
-1 siblings, 0 replies; 89+ messages in thread
From: Daniel J Walsh @ 2014-11-10 13:59 UTC (permalink / raw)
To: Casey Schaufler, David Howells
Cc: linux-unionfs, linux-kernel, linux-security-module, selinux,
linux-fsdevel
On 11/08/2014 08:31 PM, Casey Schaufler wrote:
> On 11/7/2014 10:54 AM, Daniel J Walsh wrote:
>> On 11/07/2014 10:21 AM, David Howells wrote:
>>> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>
>>>>>> What does this mean?
>>>>> It has been decided that for the purposes of docker, all files within the
>>>>> docker root fs will have the same label. We'd have to provide policy
>>>>> namespacing otherwise (I think).
>>>> That's just insane. "It has been decided" by who? Obviously not people
>>>> who care about security policies. Maybe it's good enough for your
>>>> particular use case, but labeling of files has to be up to the security
>>>> module. That's what the modules are for.
>>> It has been decided by the docker people that I've dealt with. I was
>>> expecting there to be different labels throughout a docker image, but
>>> apparently not...
>> We want the equivalent of the mount option
>> context="system_u:object_r:svirt_sandbox_file_t:s0:c1,c2"
> Which just means that the policy used in the container is going
> to have to grant access to everything running in that container
> to that context. That's fine if you want to emasculate your module
> within the container. But it requires that you do so, and that is
> wrong.
Huh? I am asking for this to be supported not enforced. If you don't
do the command context="" or its equivalent, then you can run with ever
labels you want and see the underlying XATTRS on inodes. I just need a way
to support sVirt controls as defined for docker containers, matching
what we
are able to do with Device Mapper back end.
>> Which would label all of the content in the OVERLAYFS with this label.
>> If we have to have a readonly label for the lower
>> level and a RW Label for the context mount then that is fine with us.
>> The lower level can actually just reveal the label of the INODE.
>> We can take care of labeling it with a single label.
>>
>> If the context="LABEL" (Or its equivalent is not passed), it should get
>> some kind of label based on the parent directory I would guess
>> or transition rules.
>>
>> With docker we want to treat all contents of a container with a single
>> label and then separate containers based on MCS(Svirt) separation.
> That seems like a limited and rather pointless use case to me.
>>>>>> What about LSMs that have multiple labels on a file?
>>>>> Setting policy is something I'll have to leave to the docker people and the
>>>>> administrators of systems that use docker.
>>>> What does that mean? If the underlying mechanism can't do the job,
>>>> how the dickens is the administrator supposed to make it happen?
>>> I'm trying to make the kernel able to support a policy on this at all - not
>>> actually write the policy itself. The policy may well vary depending on the
>>> installation anyway.
>>>
>>>>> But all I'm proposing is a way to give the LSM access to both the file in
>>>>> the overlay and the file in the lower fs that is leeching through into the
>>>>> overlay.
>>>> But your mechanisms are simultaneously incomplete and over specified.
>>> Well then, specify better ones! I'm fairly certain it is incomplete and I'm
>>> *trying* to get input on how it may be improved. I should've marked the
>>> patches [RFC] but it didn't occur to me until just after I'd sent them (of
>>> course).
> I'd love to do so, but I'm frying too many kittens right now
> to put the effort required into making this right. I do have
> faith that this can be worked out. Sorry that I missed the implicit
> [RFC]. It helps to know that feedback is still open.
>
>>> Anyway, there are a number of things one has to take account of:
>>>
>>> (1) There may be multiple 'views' or instances of a file in a union - and
>>> each may be labelled differently.
>>>
>>> (2) The lower file may have xattrs attached to it that represent the security
>>> policy.
>>>
>>> (3) The union file may have xattrs attached to it that represent the security
>>> policy.
>>>
>>> (4) When copying the lower file up, the xattrs representing the security
>>> attributes of the lower file must not be written as they may incorrectly
>>> overwrite the security attributes of the union file.
>>>
>>> (5) There needs to be a way to set the security attributes on the union file.
>>>
>>> (6) When setting the attributes on the union file, the LSM might need to take
>>> account of the attributes of the lower file in their derivation.
>>>
>>> (7) There may be no inode in the union layer on which to hang the attributes
>>> for the upper file.
>>>
>>> (8) struct file::f_security should be used to contain the union layer
>>> labellage on open files that point to a lower layer.
>>>
>>> (9) Ideally, file->f_path would point to the union layer and file->f_inode to
>>> the lower layer when there is no upper file. However, this is not the
>>> case at the moment. The file struct *only* points to the lower file or
>>> the upper file and never both.
>>>
>>> (10) Overlayfs has an extra complication in that there are potentially three
>>> files involved in any union. The lower file that is the source, the
>>> upper file that where the copy-up goes and the virtual union file in the
>>> overlay fs that redirects to one or the other.
>>>
>>> I've taken the approach that we assume that the upper file (if it exists)
>>> has the same labellage as the union file.
>>>
>>>> Your proposal is a cop out. It may work in a limited set of cases.
>>>> It is not a general solution.
>>> Actually, I think the actual code interface I've proposed is pretty close to
>>> being a general solution. I've given the LSM the information I have, it can
>>> implement a fabulous maze of specially crafted labels if it wants to - and
>>> even have multiple labels per inode or per file. The VFS does not care.
>>>
>>> David
>>>
>>> _______________________________________________
>>> 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.
> _______________________________________________
> 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.
>
>
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 0/7] Security: Provide unioned file support
@ 2014-11-10 13:59 ` Daniel J Walsh
0 siblings, 0 replies; 89+ messages in thread
From: Daniel J Walsh @ 2014-11-10 13:59 UTC (permalink / raw)
To: Casey Schaufler, David Howells
Cc: linux-fsdevel, linux-security-module, linux-unionfs, selinux,
linux-kernel
On 11/08/2014 08:31 PM, Casey Schaufler wrote:
> On 11/7/2014 10:54 AM, Daniel J Walsh wrote:
>> On 11/07/2014 10:21 AM, David Howells wrote:
>>> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>
>>>>>> What does this mean?
>>>>> It has been decided that for the purposes of docker, all files within the
>>>>> docker root fs will have the same label. We'd have to provide policy
>>>>> namespacing otherwise (I think).
>>>> That's just insane. "It has been decided" by who? Obviously not people
>>>> who care about security policies. Maybe it's good enough for your
>>>> particular use case, but labeling of files has to be up to the security
>>>> module. That's what the modules are for.
>>> It has been decided by the docker people that I've dealt with. I was
>>> expecting there to be different labels throughout a docker image, but
>>> apparently not...
>> We want the equivalent of the mount option
>> context="system_u:object_r:svirt_sandbox_file_t:s0:c1,c2"
> Which just means that the policy used in the container is going
> to have to grant access to everything running in that container
> to that context. That's fine if you want to emasculate your module
> within the container. But it requires that you do so, and that is
> wrong.
Huh? I am asking for this to be supported not enforced. If you don't
do the command context="" or its equivalent, then you can run with ever
labels you want and see the underlying XATTRS on inodes. I just need a way
to support sVirt controls as defined for docker containers, matching
what we
are able to do with Device Mapper back end.
>> Which would label all of the content in the OVERLAYFS with this label.
>> If we have to have a readonly label for the lower
>> level and a RW Label for the context mount then that is fine with us.
>> The lower level can actually just reveal the label of the INODE.
>> We can take care of labeling it with a single label.
>>
>> If the context="LABEL" (Or its equivalent is not passed), it should get
>> some kind of label based on the parent directory I would guess
>> or transition rules.
>>
>> With docker we want to treat all contents of a container with a single
>> label and then separate containers based on MCS(Svirt) separation.
> That seems like a limited and rather pointless use case to me.
>>>>>> What about LSMs that have multiple labels on a file?
>>>>> Setting policy is something I'll have to leave to the docker people and the
>>>>> administrators of systems that use docker.
>>>> What does that mean? If the underlying mechanism can't do the job,
>>>> how the dickens is the administrator supposed to make it happen?
>>> I'm trying to make the kernel able to support a policy on this at all - not
>>> actually write the policy itself. The policy may well vary depending on the
>>> installation anyway.
>>>
>>>>> But all I'm proposing is a way to give the LSM access to both the file in
>>>>> the overlay and the file in the lower fs that is leeching through into the
>>>>> overlay.
>>>> But your mechanisms are simultaneously incomplete and over specified.
>>> Well then, specify better ones! I'm fairly certain it is incomplete and I'm
>>> *trying* to get input on how it may be improved. I should've marked the
>>> patches [RFC] but it didn't occur to me until just after I'd sent them (of
>>> course).
> I'd love to do so, but I'm frying too many kittens right now
> to put the effort required into making this right. I do have
> faith that this can be worked out. Sorry that I missed the implicit
> [RFC]. It helps to know that feedback is still open.
>
>>> Anyway, there are a number of things one has to take account of:
>>>
>>> (1) There may be multiple 'views' or instances of a file in a union - and
>>> each may be labelled differently.
>>>
>>> (2) The lower file may have xattrs attached to it that represent the security
>>> policy.
>>>
>>> (3) The union file may have xattrs attached to it that represent the security
>>> policy.
>>>
>>> (4) When copying the lower file up, the xattrs representing the security
>>> attributes of the lower file must not be written as they may incorrectly
>>> overwrite the security attributes of the union file.
>>>
>>> (5) There needs to be a way to set the security attributes on the union file.
>>>
>>> (6) When setting the attributes on the union file, the LSM might need to take
>>> account of the attributes of the lower file in their derivation.
>>>
>>> (7) There may be no inode in the union layer on which to hang the attributes
>>> for the upper file.
>>>
>>> (8) struct file::f_security should be used to contain the union layer
>>> labellage on open files that point to a lower layer.
>>>
>>> (9) Ideally, file->f_path would point to the union layer and file->f_inode to
>>> the lower layer when there is no upper file. However, this is not the
>>> case at the moment. The file struct *only* points to the lower file or
>>> the upper file and never both.
>>>
>>> (10) Overlayfs has an extra complication in that there are potentially three
>>> files involved in any union. The lower file that is the source, the
>>> upper file that where the copy-up goes and the virtual union file in the
>>> overlay fs that redirects to one or the other.
>>>
>>> I've taken the approach that we assume that the upper file (if it exists)
>>> has the same labellage as the union file.
>>>
>>>> Your proposal is a cop out. It may work in a limited set of cases.
>>>> It is not a general solution.
>>> Actually, I think the actual code interface I've proposed is pretty close to
>>> being a general solution. I've given the LSM the information I have, it can
>>> implement a fabulous maze of specially crafted labels if it wants to - and
>>> even have multiple labels per inode or per file. The VFS does not care.
>>>
>>> David
>>>
>>> _______________________________________________
>>> 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.
> _______________________________________________
> 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.
>
>
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 1/7] Security: Provide copy-up security hooks for unioned files
2014-11-07 22:10 ` David Howells
@ 2014-11-10 15:28 ` Paul Moore
-1 siblings, 0 replies; 89+ messages in thread
From: Paul Moore @ 2014-11-10 15:28 UTC (permalink / raw)
To: David Howells
Cc: linux-unionfs, selinux, linux-fsdevel, linux-security-module,
linux-kernel
On Friday, November 07, 2014 10:10:34 PM David Howells wrote:
> Paul Moore <paul@paul-moore.com> wrote:
> > This didn't occur to me earlier, but we may want to pick a different
> > phrase to use instead of "copy_up" as that has a special meaning for some
> > security/MLS folks...
>
> It does?
Yep, think upgrading the sensitivity level, e.g. relabeling a "secret" file up
to a "top secret".
> security_inode_make_union()?
Actually, after thinking on this some more, forget I mentioned anything. The
existing "copy_up" makes the most sense for the LSM hook (it matches the
naming of the caller in overlayfs, which is a nice thing) and the number of
times any MLS will look that closely at the kernel code is going to be very
few.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 1/7] Security: Provide copy-up security hooks for unioned files
@ 2014-11-10 15:28 ` Paul Moore
0 siblings, 0 replies; 89+ messages in thread
From: Paul Moore @ 2014-11-10 15:28 UTC (permalink / raw)
To: David Howells
Cc: linux-fsdevel, linux-security-module, linux-unionfs, selinux,
linux-kernel
On Friday, November 07, 2014 10:10:34 PM David Howells wrote:
> Paul Moore <paul@paul-moore.com> wrote:
> > This didn't occur to me earlier, but we may want to pick a different
> > phrase to use instead of "copy_up" as that has a special meaning for some
> > security/MLS folks...
>
> It does?
Yep, think upgrading the sensitivity level, e.g. relabeling a "secret" file up
to a "top secret".
> security_inode_make_union()?
Actually, after thinking on this some more, forget I mentioned anything. The
existing "copy_up" makes the most sense for the LSM hook (it matches the
naming of the caller in overlayfs, which is a nice thing) and the number of
times any MLS will look that closely at the kernel code is going to be very
few.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 2/7] Overlayfs: Use copy-up security hooks
2014-11-07 22:05 ` David Howells
@ 2014-11-10 15:45 ` Paul Moore
-1 siblings, 0 replies; 89+ messages in thread
From: Paul Moore @ 2014-11-10 15:45 UTC (permalink / raw)
To: David Howells
Cc: linux-unionfs, selinux, linux-fsdevel, linux-security-module,
linux-kernel
On Friday, November 07, 2014 10:05:40 PM David Howells wrote:
> Paul Moore <paul@paul-moore.com> wrote:
> > So the LSM must modify the xattr in place? I suppose that since the
> > @value is allocated to the max size it shouldn't be a problem. Just
> > checking ...
>
> ... And the caller must provide a maximally sized buffer (which it likely
> has to allocate anyway).
>
> I'm not sure I really need to provide the modification thing. I suspect a
> binary keep or discard decision is sufficient.
The docker use case we've been talking about in this thread doesn't really
care about the on-disk file labels (xattrs) because the docker folks want to
use context= mounts; however if someone did care about on-disk file labels for
the upper layer in the overlayfs then they might want to modify the xattr.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 2/7] Overlayfs: Use copy-up security hooks
@ 2014-11-10 15:45 ` Paul Moore
0 siblings, 0 replies; 89+ messages in thread
From: Paul Moore @ 2014-11-10 15:45 UTC (permalink / raw)
To: David Howells
Cc: linux-fsdevel, linux-security-module, linux-unionfs, selinux,
linux-kernel
On Friday, November 07, 2014 10:05:40 PM David Howells wrote:
> Paul Moore <paul@paul-moore.com> wrote:
> > So the LSM must modify the xattr in place? I suppose that since the
> > @value is allocated to the max size it shouldn't be a problem. Just
> > checking ...
>
> ... And the caller must provide a maximally sized buffer (which it likely
> has to allocate anyway).
>
> I'm not sure I really need to provide the modification thing. I suspect a
> binary keep or discard decision is sufficient.
The docker use case we've been talking about in this thread doesn't really
care about the on-disk file labels (xattrs) because the docker folks want to
use context= mounts; however if someone did care about on-disk file labels for
the upper layer in the overlayfs then they might want to modify the xattr.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 3/7] SELinux: Stub in copy-up handling
2014-11-07 22:08 ` David Howells
@ 2014-11-10 15:47 ` Paul Moore
-1 siblings, 0 replies; 89+ messages in thread
From: Paul Moore @ 2014-11-10 15:47 UTC (permalink / raw)
To: David Howells
Cc: linux-unionfs, selinux, linux-fsdevel, linux-security-module,
linux-kernel
On Friday, November 07, 2014 10:08:48 PM David Howells wrote:
> Paul Moore <paul@paul-moore.com> wrote:
> > > +static int selinux_inode_copy_up_xattr(struct dentry *src, struct
> > > dentry
> > > ...*dst,
> > > + const char *name, void *value, size_t *size)
> > > +{
> > > + if (strcmp(name, XATTR_NAME_SELINUX) == 0)
> > > + return 1; /* Discard */
> >
> > I understand that this is just a stub, but I we need to discuss this at
> > some point and I figure better in this patch than elsewhere where it
> > might get lost.
> >
> > For the docker, context= mount use case (similar to SELinux/sVirt)
> > dropping
> > the SELinux xattr is probably an okay behavior. However, I would expect
> > that ultimately this is something we would want to control by policy
> > and/or the presence of a context= label.
>
> The problem is that the label you actually want has already been set and you
> have to be careful not to overwrite it.
>
> The other hook (->inode_copy_up) is called first to label the destination
> inode - and that has access to the source file also so it can label the
> destination with consideration of the attributes on the source. Normally,
> the attributes on the source will be in memory attached to the source
> inode, I would imagine.
Hmmm, I guess I need to take a closer look at how overlayfs creates files
during a copy on write.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 3/7] SELinux: Stub in copy-up handling
@ 2014-11-10 15:47 ` Paul Moore
0 siblings, 0 replies; 89+ messages in thread
From: Paul Moore @ 2014-11-10 15:47 UTC (permalink / raw)
To: David Howells
Cc: linux-fsdevel, linux-security-module, linux-unionfs, selinux,
linux-kernel
On Friday, November 07, 2014 10:08:48 PM David Howells wrote:
> Paul Moore <paul@paul-moore.com> wrote:
> > > +static int selinux_inode_copy_up_xattr(struct dentry *src, struct
> > > dentry
> > > ...*dst,
> > > + const char *name, void *value, size_t *size)
> > > +{
> > > + if (strcmp(name, XATTR_NAME_SELINUX) == 0)
> > > + return 1; /* Discard */
> >
> > I understand that this is just a stub, but I we need to discuss this at
> > some point and I figure better in this patch than elsewhere where it
> > might get lost.
> >
> > For the docker, context= mount use case (similar to SELinux/sVirt)
> > dropping
> > the SELinux xattr is probably an okay behavior. However, I would expect
> > that ultimately this is something we would want to control by policy
> > and/or the presence of a context= label.
>
> The problem is that the label you actually want has already been set and you
> have to be careful not to overwrite it.
>
> The other hook (->inode_copy_up) is called first to label the destination
> inode - and that has access to the source file also so it can label the
> destination with consideration of the attributes on the source. Normally,
> the attributes on the source will be in memory attached to the source
> inode, I would imagine.
Hmmm, I guess I need to take a closer look at how overlayfs creates files
during a copy on write.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2014-11-06 12:03 ` David Howells
@ 2014-11-27 14:15 ` David Howells
-1 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-27 14:15 UTC (permalink / raw)
To: Stephen Smalley
Cc: dhowells, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > The third case is the tricky one because we have to try and derive a
> > label. I've copied the code from the inode creation - so unless the
> > policy changes or the parent directory inode changes, I would've thought
> > we'd be okay.
>
> Only if the filesystem ultimately calls security_inode_init_security()
> on the new inode.
I'm not sure what you mean? What new inode?
The isec label derived by the code goes in the file struct, not the inode
struct because we can only get here if there isn't an inode struct.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2014-11-27 14:15 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-27 14:15 UTC (permalink / raw)
To: Stephen Smalley
Cc: linux-unionfs, linux-kernel, dhowells, linux-security-module,
selinux, linux-fsdevel
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > The third case is the tricky one because we have to try and derive a
> > label. I've copied the code from the inode creation - so unless the
> > policy changes or the parent directory inode changes, I would've thought
> > we'd be okay.
>
> Only if the filesystem ultimately calls security_inode_init_security()
> on the new inode.
I'm not sure what you mean? What new inode?
The isec label derived by the code goes in the file struct, not the inode
struct because we can only get here if there isn't an inode struct.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 6/7] SELinux: The copy-up operation must have read permission on the lower file
2014-11-05 16:43 ` Stephen Smalley
@ 2014-11-27 14:17 ` David Howells
2014-11-27 14:17 ` David Howells
2014-11-27 14:21 ` David Howells
2 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-27 14:17 UTC (permalink / raw)
To: Stephen Smalley
Cc: dhowells, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> This means that it expects to trigger those capability checks as part of
> its subsequent actions. Raising those capabilities temporarily in its
> credentials will pass the capability module checks but won't address the
> corresponding SELinux checks (both capability and file-based), so you'll
> end up triggering an entire set of checks against the current process'
> credentials. This same pattern is repeated elsewhere in overlayfs.
Hmmm... Yes. I need to check whether the lower file can be read *before*
overriding the creds.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 6/7] SELinux: The copy-up operation must have read permission on the lower file
@ 2014-11-27 14:17 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-27 14:17 UTC (permalink / raw)
To: Stephen Smalley
Cc: linux-unionfs, linux-kernel, dhowells, linux-security-module,
selinux, linux-fsdevel
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> This means that it expects to trigger those capability checks as part of
> its subsequent actions. Raising those capabilities temporarily in its
> credentials will pass the capability module checks but won't address the
> corresponding SELinux checks (both capability and file-based), so you'll
> end up triggering an entire set of checks against the current process'
> credentials. This same pattern is repeated elsewhere in overlayfs.
Hmmm... Yes. I need to check whether the lower file can be read *before*
overriding the creds.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 6/7] SELinux: The copy-up operation must have read permission on the lower file
2014-11-05 16:43 ` Stephen Smalley
2014-11-05 17:54 ` Stephen Smalley
@ 2014-11-27 14:21 ` David Howells
2014-11-27 14:21 ` David Howells
2 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-27 14:21 UTC (permalink / raw)
Cc: dhowells, Stephen Smalley, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel
David Howells <dhowells@redhat.com> wrote:
> > This means that it expects to trigger those capability checks as part of
> > its subsequent actions. Raising those capabilities temporarily in its
> > credentials will pass the capability module checks but won't address the
> > corresponding SELinux checks (both capability and file-based), so you'll
> > end up triggering an entire set of checks against the current process'
> > credentials. This same pattern is repeated elsewhere in overlayfs.
>
> Hmmm... Yes. I need to check whether the lower file can be read *before*
> overriding the creds.
Actually, I think ovl_permission() does sufficient checks on the lower inode
by calling __inode_permission() upon it.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 6/7] SELinux: The copy-up operation must have read permission on the lower file
@ 2014-11-27 14:21 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-27 14:21 UTC (permalink / raw)
Cc: linux-unionfs, linux-kernel, dhowells, linux-security-module,
selinux, linux-fsdevel, Stephen Smalley
David Howells <dhowells@redhat.com> wrote:
> > This means that it expects to trigger those capability checks as part of
> > its subsequent actions. Raising those capabilities temporarily in its
> > credentials will pass the capability module checks but won't address the
> > corresponding SELinux checks (both capability and file-based), so you'll
> > end up triggering an entire set of checks against the current process'
> > credentials. This same pattern is repeated elsewhere in overlayfs.
>
> Hmmm... Yes. I need to check whether the lower file can be read *before*
> overriding the creds.
Actually, I think ovl_permission() does sufficient checks on the lower inode
by calling __inode_permission() upon it.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 6/7] SELinux: The copy-up operation must have read permission on the lower file
@ 2014-11-27 14:21 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-27 14:21 UTC (permalink / raw)
Cc: dhowells, Stephen Smalley, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel
David Howells <dhowells@redhat.com> wrote:
> > This means that it expects to trigger those capability checks as part of
> > its subsequent actions. Raising those capabilities temporarily in its
> > credentials will pass the capability module checks but won't address the
> > corresponding SELinux checks (both capability and file-based), so you'll
> > end up triggering an entire set of checks against the current process'
> > credentials. This same pattern is repeated elsewhere in overlayfs.
>
> Hmmm... Yes. I need to check whether the lower file can be read *before*
> overriding the creds.
Actually, I think ovl_permission() does sufficient checks on the lower inode
by calling __inode_permission() upon it.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2014-11-05 15:43 ` [PATCH 5/7] SELinux: Handle opening of a unioned file David Howells
@ 2014-11-27 17:25 ` David Howells
2014-11-06 12:03 ` David Howells
` (3 subsequent siblings)
4 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-27 17:25 UTC (permalink / raw)
To: Stephen Smalley
Cc: dhowells, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> Also, would be good to create a common helper for use here, by
> selinux_dentry_init_security(), selinux_inode_init_security(), and
> may_create(). Already some seeming potential for inconsistencies there.
selinux_dentry_init_security() and selinux_inode_init_security() do something
different depending on SECURITY_FS_USE_MNTPOINT. Is the dentry variant wrong?
Shouldn't it be using the mountpoint label if that flag _is_ set?
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2014-11-27 17:25 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2014-11-27 17:25 UTC (permalink / raw)
To: Stephen Smalley
Cc: linux-unionfs, linux-kernel, dhowells, linux-security-module,
selinux, linux-fsdevel
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> Also, would be good to create a common helper for use here, by
> selinux_dentry_init_security(), selinux_inode_init_security(), and
> may_create(). Already some seeming potential for inconsistencies there.
selinux_dentry_init_security() and selinux_inode_init_security() do something
different depending on SECURITY_FS_USE_MNTPOINT. Is the dentry variant wrong?
Shouldn't it be using the mountpoint label if that flag _is_ set?
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2014-11-05 15:43 ` [PATCH 5/7] SELinux: Handle opening of a unioned file David Howells
@ 2015-06-12 15:30 ` David Howells
2014-11-06 12:03 ` David Howells
` (3 subsequent siblings)
4 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2015-06-12 15:30 UTC (permalink / raw)
To: Stephen Smalley
Cc: dhowells, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel
Hi Stephen,
David Howells <dhowells@redhat.com> wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> > Also, would be good to create a common helper for use here, by
> > selinux_dentry_init_security(), selinux_inode_init_security(), and
> > may_create(). Already some seeming potential for inconsistencies there.
>
> selinux_dentry_init_security() and selinux_inode_init_security() do
> something different depending on SECURITY_FS_USE_MNTPOINT. Is the dentry
> variant wrong? Shouldn't it be using the mountpoint label if that flag _is_
> set?
Any answer to that?
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2015-06-12 15:30 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2015-06-12 15:30 UTC (permalink / raw)
To: Stephen Smalley
Cc: linux-unionfs, linux-kernel, dhowells, linux-security-module,
selinux, linux-fsdevel
Hi Stephen,
David Howells <dhowells@redhat.com> wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> > Also, would be good to create a common helper for use here, by
> > selinux_dentry_init_security(), selinux_inode_init_security(), and
> > may_create(). Already some seeming potential for inconsistencies there.
>
> selinux_dentry_init_security() and selinux_inode_init_security() do
> something different depending on SECURITY_FS_USE_MNTPOINT. Is the dentry
> variant wrong? Shouldn't it be using the mountpoint label if that flag _is_
> set?
Any answer to that?
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2015-06-12 15:30 ` David Howells
@ 2015-06-15 12:57 ` Stephen Smalley
-1 siblings, 0 replies; 89+ messages in thread
From: Stephen Smalley @ 2015-06-15 12:57 UTC (permalink / raw)
To: David Howells
Cc: linux-unionfs, selinux, linux-fsdevel, linux-security-module,
linux-kernel, drquigl, Eric Paris
On 06/12/2015 11:30 AM, David Howells wrote:
> Hi Stephen,
>
> David Howells <dhowells@redhat.com> wrote:
>
>> Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>
>>> Also, would be good to create a common helper for use here, by
>>> selinux_dentry_init_security(), selinux_inode_init_security(), and
>>> may_create(). Already some seeming potential for inconsistencies there.
>>
>> selinux_dentry_init_security() and selinux_inode_init_security() do
>> something different depending on SECURITY_FS_USE_MNTPOINT. Is the dentry
>> variant wrong? Shouldn't it be using the mountpoint label if that flag _is_
>> set?
>
> Any answer to that?
It looks like commit 415103f9932d45f7927f4b17e3a9a13834cdb9a1 changed
selinux_inode_init_security()'s handling of SECURITY_FS_USE_MNTPOINT,
and this change was never propagated to selinux_dentry_init_security().
However, that commit also did not update
security/selinux/hooks.c:may_create()'s logic for computing the new file
label when checking CREATE permission, and therefore introduced a
potential inconsistency between the label used for the permission check
and the label assigned to the inode.
That's why I suggested that we need a common helper for all three to
ensure consistency there.
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2015-06-15 12:57 ` Stephen Smalley
0 siblings, 0 replies; 89+ messages in thread
From: Stephen Smalley @ 2015-06-15 12:57 UTC (permalink / raw)
To: David Howells
Cc: linux-unionfs, linux-kernel, drquigl, linux-security-module,
selinux, linux-fsdevel
On 06/12/2015 11:30 AM, David Howells wrote:
> Hi Stephen,
>
> David Howells <dhowells@redhat.com> wrote:
>
>> Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>
>>> Also, would be good to create a common helper for use here, by
>>> selinux_dentry_init_security(), selinux_inode_init_security(), and
>>> may_create(). Already some seeming potential for inconsistencies there.
>>
>> selinux_dentry_init_security() and selinux_inode_init_security() do
>> something different depending on SECURITY_FS_USE_MNTPOINT. Is the dentry
>> variant wrong? Shouldn't it be using the mountpoint label if that flag _is_
>> set?
>
> Any answer to that?
It looks like commit 415103f9932d45f7927f4b17e3a9a13834cdb9a1 changed
selinux_inode_init_security()'s handling of SECURITY_FS_USE_MNTPOINT,
and this change was never propagated to selinux_dentry_init_security().
However, that commit also did not update
security/selinux/hooks.c:may_create()'s logic for computing the new file
label when checking CREATE permission, and therefore introduced a
potential inconsistency between the label used for the permission check
and the label assigned to the inode.
That's why I suggested that we need a common helper for all three to
ensure consistency there.
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2015-06-12 15:30 ` David Howells
@ 2015-06-16 9:41 ` David Howells
-1 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2015-06-16 9:41 UTC (permalink / raw)
To: Stephen Smalley
Cc: dhowells, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel, drquigl, Eric Paris
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> It looks like commit 415103f9932d45f7927f4b17e3a9a13834cdb9a1 changed
> selinux_inode_init_security()'s handling of SECURITY_FS_USE_MNTPOINT,
> and this change was never propagated to selinux_dentry_init_security().
> However, that commit also did not update
> security/selinux/hooks.c:may_create()'s logic for computing the new file
> label when checking CREATE permission, and therefore introduced a
> potential inconsistency between the label used for the permission check
> and the label assigned to the inode.
>
> That's why I suggested that we need a common helper for all three to
> ensure consistency there.
Ah, okay. I didn't realise selinux_dentry_init_security() wasn't supposed to
be so different.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2015-06-16 9:41 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2015-06-16 9:41 UTC (permalink / raw)
To: Stephen Smalley
Cc: linux-unionfs, linux-kernel, dhowells, drquigl,
linux-security-module, selinux, linux-fsdevel
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> It looks like commit 415103f9932d45f7927f4b17e3a9a13834cdb9a1 changed
> selinux_inode_init_security()'s handling of SECURITY_FS_USE_MNTPOINT,
> and this change was never propagated to selinux_dentry_init_security().
> However, that commit also did not update
> security/selinux/hooks.c:may_create()'s logic for computing the new file
> label when checking CREATE permission, and therefore introduced a
> potential inconsistency between the label used for the permission check
> and the label assigned to the inode.
>
> That's why I suggested that we need a common helper for all three to
> ensure consistency there.
Ah, okay. I didn't realise selinux_dentry_init_security() wasn't supposed to
be so different.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2015-06-12 15:30 ` David Howells
@ 2015-06-16 16:49 ` David Howells
-1 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2015-06-16 16:49 UTC (permalink / raw)
To: Stephen Smalley
Cc: dhowells, linux-unionfs, selinux, linux-fsdevel,
linux-security-module, linux-kernel, drquigl, Eric Paris
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> It looks like commit 415103f9932d45f7927f4b17e3a9a13834cdb9a1 changed
> selinux_inode_init_security()'s handling of SECURITY_FS_USE_MNTPOINT,
> and this change was never propagated to selinux_dentry_init_security().
> However, that commit also did not update
> security/selinux/hooks.c:may_create()'s logic for computing the new file
> label when checking CREATE permission, and therefore introduced a
> potential inconsistency between the label used for the permission check
> and the label assigned to the inode.
>
> That's why I suggested that we need a common helper for all three to
> ensure consistency there.
I think a common helper is harder than it seems. We need the parent dir in
one of the cases the helper has to consider, but finding it is done in three
different ways, depending on the caller:
(1) dentry_init can just use ->d_parent as there's a lock held that prevents
it changing (I think). This could use (2) instead, however.
(2) file_open has to use dget_parent().
(3) inode_init doesn't have any dentries, but rather has the object and
parent inodes.
If we don't mind file_open() always calling dget_parent(), then the common
helper can take the dir inode.
Also, thinking ahead to the possibility of bringing unionmount into the kernel
at some point: union non-dir dentries that are not yet copied up have no inode
attached, but rather fall through to the underlying lower inode in the VFS.
This, however, gives us nowhere to hang the inode label. How expensive is the
security_transition_sid() call?
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2015-06-16 16:49 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2015-06-16 16:49 UTC (permalink / raw)
To: Stephen Smalley
Cc: linux-unionfs, linux-kernel, dhowells, drquigl,
linux-security-module, selinux, linux-fsdevel
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> It looks like commit 415103f9932d45f7927f4b17e3a9a13834cdb9a1 changed
> selinux_inode_init_security()'s handling of SECURITY_FS_USE_MNTPOINT,
> and this change was never propagated to selinux_dentry_init_security().
> However, that commit also did not update
> security/selinux/hooks.c:may_create()'s logic for computing the new file
> label when checking CREATE permission, and therefore introduced a
> potential inconsistency between the label used for the permission check
> and the label assigned to the inode.
>
> That's why I suggested that we need a common helper for all three to
> ensure consistency there.
I think a common helper is harder than it seems. We need the parent dir in
one of the cases the helper has to consider, but finding it is done in three
different ways, depending on the caller:
(1) dentry_init can just use ->d_parent as there's a lock held that prevents
it changing (I think). This could use (2) instead, however.
(2) file_open has to use dget_parent().
(3) inode_init doesn't have any dentries, but rather has the object and
parent inodes.
If we don't mind file_open() always calling dget_parent(), then the common
helper can take the dir inode.
Also, thinking ahead to the possibility of bringing unionmount into the kernel
at some point: union non-dir dentries that are not yet copied up have no inode
attached, but rather fall through to the underlying lower inode in the VFS.
This, however, gives us nowhere to hang the inode label. How expensive is the
security_transition_sid() call?
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2015-06-16 16:49 ` David Howells
@ 2015-06-16 17:20 ` Stephen Smalley
-1 siblings, 0 replies; 89+ messages in thread
From: Stephen Smalley @ 2015-06-16 17:20 UTC (permalink / raw)
To: David Howells
Cc: linux-unionfs, linux-kernel, drquigl, linux-security-module,
selinux, linux-fsdevel
On 06/16/2015 12:49 PM, David Howells wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
>> It looks like commit 415103f9932d45f7927f4b17e3a9a13834cdb9a1 changed
>> selinux_inode_init_security()'s handling of SECURITY_FS_USE_MNTPOINT,
>> and this change was never propagated to selinux_dentry_init_security().
>> However, that commit also did not update
>> security/selinux/hooks.c:may_create()'s logic for computing the new file
>> label when checking CREATE permission, and therefore introduced a
>> potential inconsistency between the label used for the permission check
>> and the label assigned to the inode.
>>
>> That's why I suggested that we need a common helper for all three to
>> ensure consistency there.
>
> I think a common helper is harder than it seems. We need the parent dir in
> one of the cases the helper has to consider, but finding it is done in three
> different ways, depending on the caller:
>
> (1) dentry_init can just use ->d_parent as there's a lock held that prevents
> it changing (I think). This could use (2) instead, however.
>
> (2) file_open has to use dget_parent().
>
> (3) inode_init doesn't have any dentries, but rather has the object and
> parent inodes.
>
> If we don't mind file_open() always calling dget_parent(), then the common
> helper can take the dir inode.
>
> Also, thinking ahead to the possibility of bringing unionmount into the kernel
> at some point: union non-dir dentries that are not yet copied up have no inode
> attached, but rather fall through to the underlying lower inode in the VFS.
> This, however, gives us nowhere to hang the inode label. How expensive is the
> security_transition_sid() call?
Why are you talking about file_open()? It is may_create() that has the
duplicated logic, which has the parent dir passed to it by its callers
(selinux_inode_create, selinux_inode_symlink, selinux_inode_mkdir,
selinux_mknod). selinux_inode_init_security() also gets passed the
parent dir directly. Only selinux_dentry_init_security() has to use
d_parent, which as you say is safe, so it can just pass the resulting
dir to the helper.
Until a process writes to the file, we just want to use the lower inode
label, right? At the point a process writes to the file and a copy-up
is produced, we could perform a one-time computation, and then once the
upper inode is created, it should get set accordingly. So I wouldn't
think we would need to call security_transition_sid() frequently. If
so, we might want to do what was previously done in the userspace AVC in
libselinux, and start caching security_transition_sid() results in the
AVC itself and add an avc_transition_sid() interface (in the userspace
AVC, this is avc_compute_create()).
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2015-06-16 17:20 ` Stephen Smalley
0 siblings, 0 replies; 89+ messages in thread
From: Stephen Smalley @ 2015-06-16 17:20 UTC (permalink / raw)
To: David Howells
Cc: linux-unionfs, linux-kernel, linux-fsdevel, linux-security-module,
selinux, drquigl
On 06/16/2015 12:49 PM, David Howells wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
>> It looks like commit 415103f9932d45f7927f4b17e3a9a13834cdb9a1 changed
>> selinux_inode_init_security()'s handling of SECURITY_FS_USE_MNTPOINT,
>> and this change was never propagated to selinux_dentry_init_security().
>> However, that commit also did not update
>> security/selinux/hooks.c:may_create()'s logic for computing the new file
>> label when checking CREATE permission, and therefore introduced a
>> potential inconsistency between the label used for the permission check
>> and the label assigned to the inode.
>>
>> That's why I suggested that we need a common helper for all three to
>> ensure consistency there.
>
> I think a common helper is harder than it seems. We need the parent dir in
> one of the cases the helper has to consider, but finding it is done in three
> different ways, depending on the caller:
>
> (1) dentry_init can just use ->d_parent as there's a lock held that prevents
> it changing (I think). This could use (2) instead, however.
>
> (2) file_open has to use dget_parent().
>
> (3) inode_init doesn't have any dentries, but rather has the object and
> parent inodes.
>
> If we don't mind file_open() always calling dget_parent(), then the common
> helper can take the dir inode.
>
> Also, thinking ahead to the possibility of bringing unionmount into the kernel
> at some point: union non-dir dentries that are not yet copied up have no inode
> attached, but rather fall through to the underlying lower inode in the VFS.
> This, however, gives us nowhere to hang the inode label. How expensive is the
> security_transition_sid() call?
Why are you talking about file_open()? It is may_create() that has the
duplicated logic, which has the parent dir passed to it by its callers
(selinux_inode_create, selinux_inode_symlink, selinux_inode_mkdir,
selinux_mknod). selinux_inode_init_security() also gets passed the
parent dir directly. Only selinux_dentry_init_security() has to use
d_parent, which as you say is safe, so it can just pass the resulting
dir to the helper.
Until a process writes to the file, we just want to use the lower inode
label, right? At the point a process writes to the file and a copy-up
is produced, we could perform a one-time computation, and then once the
upper inode is created, it should get set accordingly. So I wouldn't
think we would need to call security_transition_sid() frequently. If
so, we might want to do what was previously done in the userspace AVC in
libselinux, and start caching security_transition_sid() results in the
AVC itself and add an avc_transition_sid() interface (in the userspace
AVC, this is avc_compute_create()).
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2015-06-16 16:49 ` David Howells
@ 2015-06-16 21:34 ` David Howells
-1 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2015-06-16 21:34 UTC (permalink / raw)
To: Stephen Smalley
Cc: dhowells, linux-unionfs, linux-kernel, drquigl,
linux-security-module, selinux, linux-fsdevel
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> Why are you talking about file_open()?
Because that's the focus of the patch 5/7 that this comment chain is in
response to. You said that it should have a common helper with the dentry and
inode init functions.
Also, would be good to create a common helper for use here, by
selinux_dentry_init_security(), selinux_inode_init_security(), and
may_create(). Already some seeming potential for inconsistencies
there.
Okay, I missed that you'd said may_create() too. I further assumed that you
meant that selinux_file_open_union() should use the common helper too.
> Until a process writes to the file, we just want to use the lower inode
> label, right?
No.
There are two issues:
(1) Non-fd accesses to an overlayfs file use the security label on the
overlay inode, not the lower inode, even before copy up because they go
through the inode ops of the overlayfs file first.
(2) I'm told that we want the ability to have a different label on the upper
file to that on the lower file. This is trivial in overlayfs since you
always have an overlay inode off which to hang the security label, but
tricky with unionmount since you may only have a dentry.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2015-06-16 21:34 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2015-06-16 21:34 UTC (permalink / raw)
To: Stephen Smalley
Cc: linux-unionfs, linux-kernel, dhowells, linux-fsdevel,
linux-security-module, selinux, drquigl
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> Why are you talking about file_open()?
Because that's the focus of the patch 5/7 that this comment chain is in
response to. You said that it should have a common helper with the dentry and
inode init functions.
Also, would be good to create a common helper for use here, by
selinux_dentry_init_security(), selinux_inode_init_security(), and
may_create(). Already some seeming potential for inconsistencies
there.
Okay, I missed that you'd said may_create() too. I further assumed that you
meant that selinux_file_open_union() should use the common helper too.
> Until a process writes to the file, we just want to use the lower inode
> label, right?
No.
There are two issues:
(1) Non-fd accesses to an overlayfs file use the security label on the
overlay inode, not the lower inode, even before copy up because they go
through the inode ops of the overlayfs file first.
(2) I'm told that we want the ability to have a different label on the upper
file to that on the lower file. This is trivial in overlayfs since you
always have an overlay inode off which to hang the security label, but
tricky with unionmount since you may only have a dentry.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2015-06-16 21:34 ` David Howells
@ 2015-06-17 14:44 ` Stephen Smalley
-1 siblings, 0 replies; 89+ messages in thread
From: Stephen Smalley @ 2015-06-17 14:44 UTC (permalink / raw)
To: David Howells
Cc: linux-unionfs, linux-kernel, drquigl, linux-security-module,
selinux, linux-fsdevel
On 06/16/2015 05:34 PM, David Howells wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
>> Why are you talking about file_open()?
>
> Because that's the focus of the patch 5/7 that this comment chain is in
> response to. You said that it should have a common helper with the dentry and
> inode init functions.
Ah, thanks - I had lost the context (the patch and prior discussion was
sufficiently long ago to drop out of my cache).
>
> Also, would be good to create a common helper for use here, by
> selinux_dentry_init_security(), selinux_inode_init_security(), and
> may_create(). Already some seeming potential for inconsistencies
> there.
>
> Okay, I missed that you'd said may_create() too. I further assumed that you
> meant that selinux_file_open_union() should use the common helper too.
If it also needs to compute the context of a newly created file. That's
what the logic in may_create, inode_init_security, and
dentry_init_security is doing.
>> Until a process writes to the file, we just want to use the lower inode
>> label, right?
>
> No.
>
> There are two issues:
>
> (1) Non-fd accesses to an overlayfs file use the security label on the
> overlay inode, not the lower inode, even before copy up because they go
> through the inode ops of the overlayfs file first.
>
> (2) I'm told that we want the ability to have a different label on the upper
> file to that on the lower file. This is trivial in overlayfs since you
> always have an overlay inode off which to hang the security label, but
> tricky with unionmount since you may only have a dentry.
I recall that being controversial. I agree that if a process attempts
to write to the file and a copy-up is triggered, then we want to be able
to label the copy of the file differently. But until that happens, why
wouldn't we simply treat the file as having the lower file label for
access control purposes on read operations?
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2015-06-17 14:44 ` Stephen Smalley
0 siblings, 0 replies; 89+ messages in thread
From: Stephen Smalley @ 2015-06-17 14:44 UTC (permalink / raw)
To: David Howells
Cc: linux-unionfs, linux-kernel, linux-fsdevel, linux-security-module,
selinux, drquigl
On 06/16/2015 05:34 PM, David Howells wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
>> Why are you talking about file_open()?
>
> Because that's the focus of the patch 5/7 that this comment chain is in
> response to. You said that it should have a common helper with the dentry and
> inode init functions.
Ah, thanks - I had lost the context (the patch and prior discussion was
sufficiently long ago to drop out of my cache).
>
> Also, would be good to create a common helper for use here, by
> selinux_dentry_init_security(), selinux_inode_init_security(), and
> may_create(). Already some seeming potential for inconsistencies
> there.
>
> Okay, I missed that you'd said may_create() too. I further assumed that you
> meant that selinux_file_open_union() should use the common helper too.
If it also needs to compute the context of a newly created file. That's
what the logic in may_create, inode_init_security, and
dentry_init_security is doing.
>> Until a process writes to the file, we just want to use the lower inode
>> label, right?
>
> No.
>
> There are two issues:
>
> (1) Non-fd accesses to an overlayfs file use the security label on the
> overlay inode, not the lower inode, even before copy up because they go
> through the inode ops of the overlayfs file first.
>
> (2) I'm told that we want the ability to have a different label on the upper
> file to that on the lower file. This is trivial in overlayfs since you
> always have an overlay inode off which to hang the security label, but
> tricky with unionmount since you may only have a dentry.
I recall that being controversial. I agree that if a process attempts
to write to the file and a copy-up is triggered, then we want to be able
to label the copy of the file differently. But until that happens, why
wouldn't we simply treat the file as having the lower file label for
access control purposes on read operations?
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2015-06-16 21:34 ` David Howells
@ 2015-06-18 10:15 ` David Howells
-1 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2015-06-18 10:15 UTC (permalink / raw)
To: Stephen Smalley, eparis
Cc: dhowells, linux-unionfs, linux-kernel, drquigl,
linux-security-module, selinux, linux-fsdevel
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >> Until a process writes to the file, we just want to use the lower inode
> >> label, right?
> >
> > No.
> >
> > There are two issues:
> >
> > (1) Non-fd accesses to an overlayfs file use the security label on the
> > overlay inode, not the lower inode, even before copy up because they
> > go through the inode ops of the overlayfs file first.
> >
> > (2) I'm told that we want the ability to have a different label on the
> > upper file to that on the lower file. This is trivial in overlayfs
> > since you always have an overlay inode off which to hang the security
> > label, but tricky with unionmount since you may only have a dentry.
>
> I recall that being controversial. I agree that if a process attempts
> to write to the file and a copy-up is triggered, then we want to be able
> to label the copy of the file differently. But until that happens, why
> wouldn't we simply treat the file as having the lower file label for
> access control purposes on read operations?
Actually, for overlayfs, I've made it such that the label off of the overlay
inode is used for the open file. This label gets determined when the overlay
inode is set up and so is cached for the lifetime of the inode struct. Like
this in the patch I posted:
+ if (inode) {
+ isec = inode->i_security;
+ fsec->union_isid = isec->sid;
+ } ...
where 'inode' is the overlay inode. The subsequent bits:
+ } else if ((sbsec->flags & SE_SBINITIALIZED) &&
+ (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
+ fsec->union_isid = sbsec->mntpoint_sid;
+ } else {
+ dir = dget_parent(union_path->dentry);
+ dsec = dir->d_inode->i_security;
+
+ rc = security_transition_sid(
+ tsec->sid, dsec->sid,
+ inode_mode_to_security_class(file_inode(file)->i_mode),
+ &union_path->dentry->d_name,
+ &fsec->union_isid);
+ dput(dir);
+ if (rc) {
+ printk(KERN_WARNING "%s: "
+ "security_transition_sid failed, rc=%d (name=%pD)\n",
+ __func__, -rc, file);
+ return rc;
+ }
+ }
are for future unionmount support where there isn't an inode in the union
layer but only an inode in a lower layer.
In patch 7/7, if fsec->union_isid is non-zero in file_has_perm() then that is
used directly with avc_has_perm() in preference to calling inode_has_perm() on
the overlay or lower inodes.
Btw, is it correct that file operations must follow the label currently on the
inode rather than caching it in file_security_struct?
> > (2) I'm told that we want the ability to have a different label on the
> > upper file to that on the lower file. This is trivial in overlayfs
> > since you always have an overlay inode off which to hang the security
> > label, but tricky with unionmount since you may only have a dentry.
>
> I recall that being controversial. I agree that if a process attempts
> to write to the file and a copy-up is triggered, then we want to be able
> to label the copy of the file differently. But until that happens, why
> wouldn't we simply treat the file as having the lower file label for
> access control purposes on read operations?
I'm hoping Eric Paris might address this. As I understand it, we don't want
to give the container access to the lower layer labels, but would rather
consistently use the overlay labels.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2015-06-18 10:15 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2015-06-18 10:15 UTC (permalink / raw)
To: Stephen Smalley, eparis
Cc: linux-unionfs, linux-kernel, dhowells, linux-fsdevel,
linux-security-module, selinux, drquigl
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >> Until a process writes to the file, we just want to use the lower inode
> >> label, right?
> >
> > No.
> >
> > There are two issues:
> >
> > (1) Non-fd accesses to an overlayfs file use the security label on the
> > overlay inode, not the lower inode, even before copy up because they
> > go through the inode ops of the overlayfs file first.
> >
> > (2) I'm told that we want the ability to have a different label on the
> > upper file to that on the lower file. This is trivial in overlayfs
> > since you always have an overlay inode off which to hang the security
> > label, but tricky with unionmount since you may only have a dentry.
>
> I recall that being controversial. I agree that if a process attempts
> to write to the file and a copy-up is triggered, then we want to be able
> to label the copy of the file differently. But until that happens, why
> wouldn't we simply treat the file as having the lower file label for
> access control purposes on read operations?
Actually, for overlayfs, I've made it such that the label off of the overlay
inode is used for the open file. This label gets determined when the overlay
inode is set up and so is cached for the lifetime of the inode struct. Like
this in the patch I posted:
+ if (inode) {
+ isec = inode->i_security;
+ fsec->union_isid = isec->sid;
+ } ...
where 'inode' is the overlay inode. The subsequent bits:
+ } else if ((sbsec->flags & SE_SBINITIALIZED) &&
+ (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
+ fsec->union_isid = sbsec->mntpoint_sid;
+ } else {
+ dir = dget_parent(union_path->dentry);
+ dsec = dir->d_inode->i_security;
+
+ rc = security_transition_sid(
+ tsec->sid, dsec->sid,
+ inode_mode_to_security_class(file_inode(file)->i_mode),
+ &union_path->dentry->d_name,
+ &fsec->union_isid);
+ dput(dir);
+ if (rc) {
+ printk(KERN_WARNING "%s: "
+ "security_transition_sid failed, rc=%d (name=%pD)\n",
+ __func__, -rc, file);
+ return rc;
+ }
+ }
are for future unionmount support where there isn't an inode in the union
layer but only an inode in a lower layer.
In patch 7/7, if fsec->union_isid is non-zero in file_has_perm() then that is
used directly with avc_has_perm() in preference to calling inode_has_perm() on
the overlay or lower inodes.
Btw, is it correct that file operations must follow the label currently on the
inode rather than caching it in file_security_struct?
> > (2) I'm told that we want the ability to have a different label on the
> > upper file to that on the lower file. This is trivial in overlayfs
> > since you always have an overlay inode off which to hang the security
> > label, but tricky with unionmount since you may only have a dentry.
>
> I recall that being controversial. I agree that if a process attempts
> to write to the file and a copy-up is triggered, then we want to be able
> to label the copy of the file differently. But until that happens, why
> wouldn't we simply treat the file as having the lower file label for
> access control purposes on read operations?
I'm hoping Eric Paris might address this. As I understand it, we don't want
to give the container access to the lower layer labels, but would rather
consistently use the overlay labels.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2015-06-16 16:49 ` David Howells
@ 2015-06-18 10:32 ` David Howells
-1 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2015-06-18 10:32 UTC (permalink / raw)
To: Stephen Smalley
Cc: dhowells, linux-unionfs, linux-kernel, drquigl,
linux-security-module, selinux, linux-fsdevel
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> It is may_create() that has the duplicated logic
Not precisely. may_create() doesn't use SECURITY_FS_USE_MNTPOINT and
sbsec->mntpoint_sid. Should it?
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2015-06-18 10:32 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2015-06-18 10:32 UTC (permalink / raw)
To: Stephen Smalley
Cc: linux-unionfs, linux-kernel, dhowells, linux-fsdevel,
linux-security-module, selinux, drquigl
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> It is may_create() that has the duplicated logic
Not precisely. may_create() doesn't use SECURITY_FS_USE_MNTPOINT and
sbsec->mntpoint_sid. Should it?
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2015-06-18 10:32 ` David Howells
@ 2015-06-18 12:16 ` Stephen Smalley
-1 siblings, 0 replies; 89+ messages in thread
From: Stephen Smalley @ 2015-06-18 12:16 UTC (permalink / raw)
To: David Howells
Cc: linux-unionfs, linux-kernel, drquigl, linux-security-module,
selinux, linux-fsdevel
On 06/18/2015 06:32 AM, David Howells wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
>> It is may_create() that has the duplicated logic
>
> Not precisely. may_create() doesn't use SECURITY_FS_USE_MNTPOINT and
> sbsec->mntpoint_sid. Should it?
Yes, they should all use exactly the same logic.
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2015-06-18 12:16 ` Stephen Smalley
0 siblings, 0 replies; 89+ messages in thread
From: Stephen Smalley @ 2015-06-18 12:16 UTC (permalink / raw)
To: David Howells
Cc: linux-unionfs, linux-kernel, linux-fsdevel, linux-security-module,
selinux, drquigl
On 06/18/2015 06:32 AM, David Howells wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
>> It is may_create() that has the duplicated logic
>
> Not precisely. may_create() doesn't use SECURITY_FS_USE_MNTPOINT and
> sbsec->mntpoint_sid. Should it?
Yes, they should all use exactly the same logic.
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2015-06-18 10:15 ` David Howells
@ 2015-06-18 12:48 ` Stephen Smalley
-1 siblings, 0 replies; 89+ messages in thread
From: Stephen Smalley @ 2015-06-18 12:48 UTC (permalink / raw)
To: David Howells, eparis
Cc: linux-unionfs, linux-kernel, drquigl, linux-security-module,
selinux, linux-fsdevel, Paul Moore
On 06/18/2015 06:15 AM, David Howells wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
>>>> Until a process writes to the file, we just want to use the lower inode
>>>> label, right?
>>>
>>> No.
>>>
>>> There are two issues:
>>>
>>> (1) Non-fd accesses to an overlayfs file use the security label on the
>>> overlay inode, not the lower inode, even before copy up because they
>>> go through the inode ops of the overlayfs file first.
>>>
>>> (2) I'm told that we want the ability to have a different label on the
>>> upper file to that on the lower file. This is trivial in overlayfs
>>> since you always have an overlay inode off which to hang the security
>>> label, but tricky with unionmount since you may only have a dentry.
>>
>> I recall that being controversial. I agree that if a process attempts
>> to write to the file and a copy-up is triggered, then we want to be able
>> to label the copy of the file differently. But until that happens, why
>> wouldn't we simply treat the file as having the lower file label for
>> access control purposes on read operations?
>
> Actually, for overlayfs, I've made it such that the label off of the overlay
> inode is used for the open file. This label gets determined when the overlay
> inode is set up and so is cached for the lifetime of the inode struct. Like
> this in the patch I posted:
>
> + if (inode) {
> + isec = inode->i_security;
> + fsec->union_isid = isec->sid;
> + } ...
>
> where 'inode' is the overlay inode. The subsequent bits:
>
> + } else if ((sbsec->flags & SE_SBINITIALIZED) &&
> + (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> + fsec->union_isid = sbsec->mntpoint_sid;
> + } else {
> + dir = dget_parent(union_path->dentry);
> + dsec = dir->d_inode->i_security;
> +
> + rc = security_transition_sid(
> + tsec->sid, dsec->sid,
> + inode_mode_to_security_class(file_inode(file)->i_mode),
> + &union_path->dentry->d_name,
> + &fsec->union_isid);
> + dput(dir);
> + if (rc) {
> + printk(KERN_WARNING "%s: "
> + "security_transition_sid failed, rc=%d (name=%pD)\n",
> + __func__, -rc, file);
> + return rc;
> + }
> + }
>
> are for future unionmount support where there isn't an inode in the union
> layer but only an inode in a lower layer.
>
> In patch 7/7, if fsec->union_isid is non-zero in file_has_perm() then that is
> used directly with avc_has_perm() in preference to calling inode_has_perm() on
> the overlay or lower inodes.
>
> Btw, is it correct that file operations must follow the label currently on the
> inode rather than caching it in file_security_struct?
Yes. The inode label is cached (in the fsec->isid field) at open time
by selinux_file_open() so that selinux_file_permission() can skip
rechecking access if there has been no change to the opening task SID
(cached in fsec->sid), the inode SID (cached in fsec->isid), or the
policy (policy sequence number cached in fsec->pseqno). The cached
inode SID is never used for permission checking however.
>>> (2) I'm told that we want the ability to have a different label on the
>>> upper file to that on the lower file. This is trivial in overlayfs
>>> since you always have an overlay inode off which to hang the security
>>> label, but tricky with unionmount since you may only have a dentry.
>>
>> I recall that being controversial. I agree that if a process attempts
>> to write to the file and a copy-up is triggered, then we want to be able
>> to label the copy of the file differently. But until that happens, why
>> wouldn't we simply treat the file as having the lower file label for
>> access control purposes on read operations?
>
> I'm hoping Eric Paris might address this. As I understand it, we don't want
> to give the container access to the lower layer labels, but would rather
> consistently use the overlay labels.
I've seen issues with that approach for e.g. ecryptfs in the past, which
similarly has a notion of layers or stacking. There are currently two
different ways of handling ecryptfs:
1. Assign the ecryptfs inodes a fixed label via genfscon in policy or
context= mount, with no connection to the underlying inode labels.
Potential advantage: Policy could potentially distinguish access to the
plaintext vs encrypted inodes as they will have different labels (except
that, in practice, I don't believe this is true because ecryptfs calls
vfs helpers that trigger the permission checks on the lower inodes with
the current process' context, so this potential advantage is not being
realized today). Disadvantage: We lose per-file granularity on the
ecryptfs inodes, so policy cannot distinguish one file from another.
And we can't tell from policy alone how a given file is being protected,
as the file can be effectively accessed under two labels.
2. Have encryptfs pass through the underlying inode labels (i.e. use
fs_use_xattr for ecryptfs in policy). Advantage: We retain per-file
granularity and access control to both representations of the file
content is consistent. Disadvantage: No possibility of distinguishing
access to the encrypted vs plaintext versions of the file in policy,
even if we fixed ecryptfs internals to avoid the permission checks when
it internally accesses the lower files.
Almost want to be able to compute a transition label for the ecryptfs
inodes from the lower inode label so that it can be derived from but
potentially different from the lower inode label. That way policy could
maintain per-file distinctions within an ecryptfs mount and distinguish
between access to the encrypted vs plaintext representations.
Let's make sure that we provide suitable flexibility for overlayfs and
don't lock ourselves into the same problems as with ecryptfs.
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2015-06-18 12:48 ` Stephen Smalley
0 siblings, 0 replies; 89+ messages in thread
From: Stephen Smalley @ 2015-06-18 12:48 UTC (permalink / raw)
To: David Howells, eparis
Cc: linux-unionfs, linux-kernel, linux-fsdevel, linux-security-module,
selinux, drquigl
On 06/18/2015 06:15 AM, David Howells wrote:
> Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
>>>> Until a process writes to the file, we just want to use the lower inode
>>>> label, right?
>>>
>>> No.
>>>
>>> There are two issues:
>>>
>>> (1) Non-fd accesses to an overlayfs file use the security label on the
>>> overlay inode, not the lower inode, even before copy up because they
>>> go through the inode ops of the overlayfs file first.
>>>
>>> (2) I'm told that we want the ability to have a different label on the
>>> upper file to that on the lower file. This is trivial in overlayfs
>>> since you always have an overlay inode off which to hang the security
>>> label, but tricky with unionmount since you may only have a dentry.
>>
>> I recall that being controversial. I agree that if a process attempts
>> to write to the file and a copy-up is triggered, then we want to be able
>> to label the copy of the file differently. But until that happens, why
>> wouldn't we simply treat the file as having the lower file label for
>> access control purposes on read operations?
>
> Actually, for overlayfs, I've made it such that the label off of the overlay
> inode is used for the open file. This label gets determined when the overlay
> inode is set up and so is cached for the lifetime of the inode struct. Like
> this in the patch I posted:
>
> + if (inode) {
> + isec = inode->i_security;
> + fsec->union_isid = isec->sid;
> + } ...
>
> where 'inode' is the overlay inode. The subsequent bits:
>
> + } else if ((sbsec->flags & SE_SBINITIALIZED) &&
> + (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> + fsec->union_isid = sbsec->mntpoint_sid;
> + } else {
> + dir = dget_parent(union_path->dentry);
> + dsec = dir->d_inode->i_security;
> +
> + rc = security_transition_sid(
> + tsec->sid, dsec->sid,
> + inode_mode_to_security_class(file_inode(file)->i_mode),
> + &union_path->dentry->d_name,
> + &fsec->union_isid);
> + dput(dir);
> + if (rc) {
> + printk(KERN_WARNING "%s: "
> + "security_transition_sid failed, rc=%d (name=%pD)\n",
> + __func__, -rc, file);
> + return rc;
> + }
> + }
>
> are for future unionmount support where there isn't an inode in the union
> layer but only an inode in a lower layer.
>
> In patch 7/7, if fsec->union_isid is non-zero in file_has_perm() then that is
> used directly with avc_has_perm() in preference to calling inode_has_perm() on
> the overlay or lower inodes.
>
> Btw, is it correct that file operations must follow the label currently on the
> inode rather than caching it in file_security_struct?
Yes. The inode label is cached (in the fsec->isid field) at open time
by selinux_file_open() so that selinux_file_permission() can skip
rechecking access if there has been no change to the opening task SID
(cached in fsec->sid), the inode SID (cached in fsec->isid), or the
policy (policy sequence number cached in fsec->pseqno). The cached
inode SID is never used for permission checking however.
>>> (2) I'm told that we want the ability to have a different label on the
>>> upper file to that on the lower file. This is trivial in overlayfs
>>> since you always have an overlay inode off which to hang the security
>>> label, but tricky with unionmount since you may only have a dentry.
>>
>> I recall that being controversial. I agree that if a process attempts
>> to write to the file and a copy-up is triggered, then we want to be able
>> to label the copy of the file differently. But until that happens, why
>> wouldn't we simply treat the file as having the lower file label for
>> access control purposes on read operations?
>
> I'm hoping Eric Paris might address this. As I understand it, we don't want
> to give the container access to the lower layer labels, but would rather
> consistently use the overlay labels.
I've seen issues with that approach for e.g. ecryptfs in the past, which
similarly has a notion of layers or stacking. There are currently two
different ways of handling ecryptfs:
1. Assign the ecryptfs inodes a fixed label via genfscon in policy or
context= mount, with no connection to the underlying inode labels.
Potential advantage: Policy could potentially distinguish access to the
plaintext vs encrypted inodes as they will have different labels (except
that, in practice, I don't believe this is true because ecryptfs calls
vfs helpers that trigger the permission checks on the lower inodes with
the current process' context, so this potential advantage is not being
realized today). Disadvantage: We lose per-file granularity on the
ecryptfs inodes, so policy cannot distinguish one file from another.
And we can't tell from policy alone how a given file is being protected,
as the file can be effectively accessed under two labels.
2. Have encryptfs pass through the underlying inode labels (i.e. use
fs_use_xattr for ecryptfs in policy). Advantage: We retain per-file
granularity and access control to both representations of the file
content is consistent. Disadvantage: No possibility of distinguishing
access to the encrypted vs plaintext versions of the file in policy,
even if we fixed ecryptfs internals to avoid the permission checks when
it internally accesses the lower files.
Almost want to be able to compute a transition label for the ecryptfs
inodes from the lower inode label so that it can be derived from but
potentially different from the lower inode label. That way policy could
maintain per-file distinctions within an ecryptfs mount and distinguish
between access to the encrypted vs plaintext representations.
Let's make sure that we provide suitable flexibility for overlayfs and
don't lock ourselves into the same problems as with ecryptfs.
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
2015-06-18 10:15 ` David Howells
@ 2015-06-18 15:26 ` David Howells
-1 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2015-06-18 15:26 UTC (permalink / raw)
To: Stephen Smalley
Cc: dhowells, eparis, linux-unionfs, linux-kernel, drquigl,
linux-security-module, selinux, linux-fsdevel, Paul Moore
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> Almost want to be able to compute a transition label for the ecryptfs
> inodes from the lower inode label so that it can be derived from but
> potentially different from the lower inode label. That way policy could
> maintain per-file distinctions within an ecryptfs mount and distinguish
> between access to the encrypted vs plaintext representations.
Yeah. What we want is something like:
lower-inode-label + proposed-upper-label + subject-label -> inode-label
By proposed-upper-label I mean the default label for a new inode at that the
point in the directory tree at which the copy up will take place.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
@ 2015-06-18 15:26 ` David Howells
0 siblings, 0 replies; 89+ messages in thread
From: David Howells @ 2015-06-18 15:26 UTC (permalink / raw)
To: Stephen Smalley
Cc: linux-unionfs, eparis, linux-kernel, dhowells, linux-fsdevel,
linux-security-module, selinux, drquigl
Stephen Smalley <sds@tycho.nsa.gov> wrote:
> Almost want to be able to compute a transition label for the ecryptfs
> inodes from the lower inode label so that it can be derived from but
> potentially different from the lower inode label. That way policy could
> maintain per-file distinctions within an ecryptfs mount and distinguish
> between access to the encrypted vs plaintext representations.
Yeah. What we want is something like:
lower-inode-label + proposed-upper-label + subject-label -> inode-label
By proposed-upper-label I mean the default label for a new inode at that the
point in the directory tree at which the copy up will take place.
David
^ permalink raw reply [flat|nested] 89+ messages in thread
end of thread, other threads:[~2015-06-18 15:27 UTC | newest]
Thread overview: 89+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-05 15:42 [PATCH 0/7] Security: Provide unioned file support David Howells
2014-11-05 15:42 ` [PATCH 1/7] Security: Provide copy-up security hooks for unioned files David Howells
2014-11-06 17:46 ` Casey Schaufler
2014-11-07 14:49 ` David Howells
2014-11-07 14:49 ` David Howells
2014-11-07 21:22 ` Paul Moore
2014-11-07 21:22 ` Paul Moore
2014-11-07 22:10 ` David Howells
2014-11-07 22:10 ` David Howells
2014-11-10 15:28 ` Paul Moore
2014-11-10 15:28 ` Paul Moore
2014-11-05 15:42 ` [PATCH 2/7] Overlayfs: Use copy-up security hooks David Howells
2014-11-07 21:39 ` Paul Moore
2014-11-07 21:39 ` Paul Moore
2014-11-07 22:05 ` David Howells
2014-11-07 22:05 ` David Howells
2014-11-10 15:45 ` Paul Moore
2014-11-10 15:45 ` Paul Moore
2014-11-05 15:42 ` [PATCH 3/7] SELinux: Stub in copy-up handling David Howells
2014-11-07 21:44 ` Paul Moore
2014-11-07 21:44 ` Paul Moore
2014-11-07 22:08 ` David Howells
2014-11-07 22:08 ` David Howells
2014-11-10 15:47 ` Paul Moore
2014-11-10 15:47 ` Paul Moore
2014-11-05 15:42 ` [PATCH 4/7] Security: Pass the union-layer file path into security_file_open() David Howells
2014-11-05 15:43 ` [PATCH 5/7] SELinux: Handle opening of a unioned file David Howells
2014-11-05 16:35 ` Stephen Smalley
2014-11-06 12:03 ` David Howells
2014-11-06 12:03 ` David Howells
2014-11-06 13:13 ` Stephen Smalley
2014-11-06 13:13 ` Stephen Smalley
2014-11-06 13:34 ` David Howells
2014-11-06 13:34 ` David Howells
2014-11-27 14:15 ` David Howells
2014-11-27 14:15 ` David Howells
2014-11-06 12:27 ` David Howells
2014-11-06 12:27 ` David Howells
2014-11-06 12:27 ` David Howells
2014-11-27 17:25 ` David Howells
2014-11-27 17:25 ` David Howells
2015-06-12 15:30 ` David Howells
2015-06-12 15:30 ` David Howells
2015-06-15 12:57 ` Stephen Smalley
2015-06-15 12:57 ` Stephen Smalley
2015-06-16 9:41 ` David Howells
2015-06-16 9:41 ` David Howells
2015-06-16 16:49 ` David Howells
2015-06-16 16:49 ` David Howells
2015-06-16 17:20 ` Stephen Smalley
2015-06-16 17:20 ` Stephen Smalley
2015-06-16 21:34 ` David Howells
2015-06-16 21:34 ` David Howells
2015-06-17 14:44 ` Stephen Smalley
2015-06-17 14:44 ` Stephen Smalley
2015-06-18 10:15 ` David Howells
2015-06-18 10:15 ` David Howells
2015-06-18 12:48 ` Stephen Smalley
2015-06-18 12:48 ` Stephen Smalley
2015-06-18 15:26 ` David Howells
2015-06-18 15:26 ` David Howells
2015-06-18 10:32 ` David Howells
2015-06-18 10:32 ` David Howells
2015-06-18 12:16 ` Stephen Smalley
2015-06-18 12:16 ` Stephen Smalley
2014-11-05 15:43 ` [PATCH 6/7] SELinux: The copy-up operation must have read permission on the lower file David Howells
2014-11-05 16:43 ` Stephen Smalley
2014-11-05 17:54 ` Stephen Smalley
2014-11-06 13:39 ` Stephen Smalley
2014-11-27 14:17 ` David Howells
2014-11-27 14:17 ` David Howells
2014-11-27 14:21 ` David Howells
2014-11-27 14:21 ` David Howells
2014-11-27 14:21 ` David Howells
2014-11-05 15:43 ` [PATCH 7/7] SELinux: Check against union and lower labels for file ops on lower files David Howells
2014-11-06 17:35 ` [PATCH 0/7] Security: Provide unioned file support Casey Schaufler
2014-11-06 17:35 ` Casey Schaufler
2014-11-06 17:58 ` David Howells
2014-11-06 17:58 ` David Howells
2014-11-06 18:40 ` Casey Schaufler
2014-11-06 18:40 ` Casey Schaufler
2014-11-07 15:21 ` David Howells
2014-11-07 15:21 ` David Howells
2014-11-07 18:54 ` Daniel J Walsh
2014-11-07 18:54 ` Daniel J Walsh
2014-11-09 1:31 ` Casey Schaufler
2014-11-09 1:31 ` Casey Schaufler
2014-11-10 13:59 ` Daniel J Walsh
2014-11-10 13:59 ` Daniel J Walsh
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.