Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files

[Paul Moore]

On Tue, Jul 5, 2016 at 11:50 AM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> Provide a security hook to label new file correctly when a file is copied
> up from lower layer to upper layer of a overlay/union mount.
>
> This hook can prepare and switch to a new set of creds which are suitable
> for new file creation during copy up. Caller should revert to old creds
> after file creation.
>
> In SELinux, newly copied up file gets same label as lower file for
> non-context mounts. But it gets label specified in mount option context=
> for context mounts.
>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> ---
>  fs/overlayfs/copy_up.c    |  8 ++++++++
>  include/linux/lsm_hooks.h | 13 +++++++++++++
>  include/linux/security.h  |  6 ++++++
>  security/security.c       |  8 ++++++++
>  security/selinux/hooks.c  | 27 +++++++++++++++++++++++++++
>  5 files changed, 62 insertions(+)

..

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index a86d537..1b1a1e5 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3270,6 +3270,32 @@ static void selinux_inode_getsecid(struct inode *inode, u32 *secid)
>         *secid = isec->sid;
>  }
>
> +static int selinux_inode_copy_up(struct dentry *src, const struct cred **old)
> +{
> +       u32 sid;
> +       struct cred *new_creds;
> +       struct task_security_struct *tsec;
> +
> +       new_creds = prepare_creds();
> +       if (!new_creds)
> +               return -ENOMEM;
> +
> +       /* Get label from overlay inode and set it in create_sid */
> +       selinux_inode_getsecid(d_inode(src), &sid);
> +       tsec = new_creds->security;
> +       tsec->create_sid = sid;
> +       *old = override_creds(new_creds);
> +
> +       /*
> +        * At this point of time we have 2 refs on new_creds. One by
> +        * prepare_creds and other by override_creds. Drop one reference
> +        * so that as soon as caller calls revert_creds(old), this cred
> +        * will be freed.
> +        */
> +       put_cred(new_creds);
> +       return 0;
> +}

One quick point of clarification: in addition to the SELinux specific
comments in lsm_hooks.h, I think it would be a good idea if the
SELinux hook implementation, e.g. security/selinux/hooks.c, was in its
own patch and not part of the hook definition.

Beyond that, I'm not overly excited about reusing create_sid for this
purpose.  I understand why you did it, but what if the process had
explicitly set create_sid?  I think I would prefer the creation of a
new field (create_up_sid?) to track this new label and then add an
additional check to selinux_inode_init_security() to prefer the
existing create_sid to this new field when both are set.

Sound reasonable?

-- 
paul moore
security @ redhat