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

[Paul Moore <pmoore@redhat.com>]

On Tue, Jul 5, 2016 at 5:52 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Jul 05, 2016 at 05:35:22PM -0400, Paul Moore wrote:
>> On Tue, Jul 5, 2016 at 11:50 AM, Vivek Goyal <vgoyal@redhat.com> 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@redhat.com>
>> > ---
>> >  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;
>> > +}

...

>> 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?
>
> When a file is copied up, either we retain the label of lower file or
> set the new label from context=. If any create_sid is set in task, that's
> ignored.
>
> And as we are setting create_sid in a new set of credentials, task will
> get to retain its create_sid for future operations.
>
> As task does not know we are creating a new file, create_sid of task
> should not matter at all. Task does not know if file is on upper or
> file is being copied up. For task this file already exists, so task
> should not expect create_sid label to be present.
>
> Am I missing something.

I forgot that you are manufacturing a new set of credentials; I must
have lost track of that when I was walking through some of the VFS
code, my mistake.  I'm still rather uneasy about this, but at least
you aren't overwriting a previously stored create_sid value.

-- 
paul moore
security @ redhat