On Tue, Jul 5, 2016 at 5:52 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> 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@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;
>> > +}
...
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
