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

From: Casey Schaufler
Date: Thu Nov 06 2014 - 12:45:47 EST


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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/