On 7/8/2016 6:42 AM, Vivek Goyal wrote:
> On Fri, Jul 08, 2016 at 08:45:34AM -0400, Vivek Goyal wrote:
>
> [..]
>>>>>> I don't much care for the way part of the credential manipulation
>>>>>> is done in the caller and part is done the the security module.
>>>>>> If the caller is going to restore the old state, the caller should
>>>>>> save the old state.
>>> Conversely if the SM is setting the state it should restore it.
>>> This needs yet another hook, but that's fine, I think.
>>>
>>>>> One advantage of current patches is that we switch to new creds only if
>>>>> it is needed. For example, if there are no LSMs loaded,
>>>> Point.
>>>>
>>>>> then there is
>>>>> no need to modify creds and make a switch to new creds.
>>>> I'm not a fan of cred flipping. There are too many ways for it to go
>>>> wrong. Consider interrupts. I assume you've ruled that out as a possibility
>>>> in the caller, but I still think the practice is dangerous.
>>>>
>>>> I greatly prefer "create and set attributes" to "change cred, create and
>>>> reset cred". I know that has it's own set of problems, including races
>>>> and faking privilege.
>>> Yeah, we've talked about this. The races can be eliminated by always
>>> doing the create in a the temporary "workdir" area and atomically
>>> renaming to the final destination after everything has been set up.
>>> OTOH that has a performance impact that the cred flipping eliminates.
>>>
>>>>> But if I start allocating new creds and save old state in caller, then
>>>>> caller always has to do it (irrespective of the fact whether any LSM
>>>>> modified the creds or not).
>>>> It starts getting messy when I have two modules that want to
>>>> change change the credential. Each module will have to check to
>>>> see if a module called before it has allocated a new cred.
>>> Doesn't seem to me too difficult: check if *credp == NULL and allocate
>>> if so. Can even invent a heper for this if needed.
>> Right. I like this approach. So cred allocation happens in LSM and
>> switching to new creds and freeing of new creds is done by caller.
>>
>> That way, if no new creds are allocated, then caller does not have to
>> switch creds. Also all LSMs can work on single copy of newly allocated
>> cred and modify it. Also all LSMs can check if creds have already been
>> allocated otherwise allocate new one.
> How about something like this. This should allow stacking modules nicely
> at the same time if no LSMs are loaded or LSM decide not to allocate new
> creds,there is no need to switch creds.
>
>
> Subject: security, overlayfs: provide copy up security hook for unioned files
>
> 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 a new set of creds which are suitable for new file
> creation during copy up. Caller will use new creds to create file and then
> revert back to old creds and release new creds.
>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
I like this better.
> ---
>
> fs/overlayfs/copy_up.c | 18 ++++++++++++++++++
> include/linux/lsm_hooks.h | 11 +++++++++++
> include/linux/security.h | 6 ++++++
> security/security.c | 8 ++++++++
> security/selinux/hooks.c | 21 +++++++++++++++++++++
> 5 files changed, 64 insertions(+)
>
> Index: rhvgoyal-linux/include/linux/lsm_hooks.h
> ===================================================================
> --- rhvgoyal-linux.orig/include/linux/lsm_hooks.h 2016-07-07 15:43:57.885498096 -0400
> +++ rhvgoyal-linux/include/linux/lsm_hooks.h 2016-07-08 09:39:22.052676141 -0400
> @@ -401,6 +401,15 @@
> * @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:
> + * A file is about to be copied up from lower layer to upper layer of
> + * overlay filesystem. Security module can prepare a set of new creds
> + * and modify as need be and return new creds. Caller will switch to
> + * new creds temporarily to create new file and release newly allocated
> + * creds.
> + * @src indicates the union dentry of file that is being copied up.
> + * @new pointer to pointer to return newly allocated creds.
> + * Returns 0 on success or a negative error code on error.
> *
> * Security hooks for file operations
> *
> @@ -1425,6 +1434,7 @@ union security_list_options {
> int (*inode_listsecurity)(struct inode *inode, char *buffer,
> size_t buffer_size);
> void (*inode_getsecid)(struct inode *inode, u32 *secid);
> + int (*inode_copy_up) (struct dentry *src, struct cred **new);
>
> int (*file_permission)(struct file *file, int mask);
> int (*file_alloc_security)(struct file *file);
> @@ -1696,6 +1706,7 @@ struct security_hook_heads {
> struct list_head inode_setsecurity;
> struct list_head inode_listsecurity;
> struct list_head inode_getsecid;
> + struct list_head inode_copy_up;
> struct list_head file_permission;
> struct list_head file_alloc_security;
> struct list_head file_free_security;
> Index: rhvgoyal-linux/include/linux/security.h
> ===================================================================
> --- rhvgoyal-linux.orig/include/linux/security.h 2016-07-07 15:43:57.885498096 -0400
> +++ rhvgoyal-linux/include/linux/security.h 2016-07-08 09:39:22.052676141 -0400
> @@ -282,6 +282,7 @@ int security_inode_getsecurity(struct in
> 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(struct inode *inode, u32 *secid);
> +int security_inode_copy_up(struct dentry *src, struct cred **new);
> int security_file_permission(struct file *file, int mask);
> int security_file_alloc(struct file *file);
> void security_file_free(struct file *file);
> @@ -758,6 +759,11 @@ static inline void security_inode_getsec
> *secid = 0;
> }
>
> +static inline int security_inode_copy_up(struct dentry *src, struct cred **new)
> +{
> + return 0;
> +}
> +
> static inline int security_file_permission(struct file *file, int mask)
> {
> return 0;
> Index: rhvgoyal-linux/security/security.c
> ===================================================================
> --- rhvgoyal-linux.orig/security/security.c 2016-07-07 15:43:57.885498096 -0400
> +++ rhvgoyal-linux/security/security.c 2016-07-08 09:39:22.052676141 -0400
> @@ -727,6 +727,12 @@ void security_inode_getsecid(struct inod
> call_void_hook(inode_getsecid, inode, secid);
> }
>
> +int security_inode_copy_up(struct dentry *src, struct cred **new)
> +{
> + return call_int_hook(inode_copy_up, 0, src, new);
> +}
> +EXPORT_SYMBOL(security_inode_copy_up);
> +
> int security_file_permission(struct file *file, int mask)
> {
> int ret;
> @@ -1663,6 +1669,8 @@ struct security_hook_heads security_hook
> LIST_HEAD_INIT(security_hook_heads.inode_listsecurity),
> .inode_getsecid =
> LIST_HEAD_INIT(security_hook_heads.inode_getsecid),
> + .inode_copy_up =
> + LIST_HEAD_INIT(security_hook_heads.inode_copy_up),
> .file_permission =
> LIST_HEAD_INIT(security_hook_heads.file_permission),
> .file_alloc_security =
> Index: rhvgoyal-linux/fs/overlayfs/copy_up.c
> ===================================================================
> --- rhvgoyal-linux.orig/fs/overlayfs/copy_up.c 2016-07-07 15:43:57.885498096 -0400
> +++ rhvgoyal-linux/fs/overlayfs/copy_up.c 2016-07-08 09:39:22.052676141 -0400
> @@ -246,6 +246,8 @@ static int ovl_copy_up_locked(struct den
> struct dentry *upper = NULL;
> umode_t mode = stat->mode;
> int err;
> + const struct cred *old_creds = NULL;
> + struct cred *new_creds = NULL;
>
> newdentry = ovl_lookup_temp(workdir, dentry);
> err = PTR_ERR(newdentry);
> @@ -258,10 +260,26 @@ static int ovl_copy_up_locked(struct den
> if (IS_ERR(upper))
> goto out1;
>
> + err = security_inode_copy_up(dentry, &new_creds);
> + if (err < 0) {
> + if (new_creds)
> + put_cred(new_creds);
> + goto out2;
> + }
> +
> + if (new_creds)
> + old_creds = override_creds(new_creds);
> +
> /* Can't properly set mode on creation because of the umask */
> stat->mode &= S_IFMT;
> err = ovl_create_real(wdir, newdentry, stat, link, NULL, true);
> stat->mode = mode;
> +
> + if (new_creds) {
> + revert_creds(old_creds);
> + put_cred(new_creds);
> + }
> +
> if (err)
> goto out2;
>
> Index: rhvgoyal-linux/security/selinux/hooks.c
> ===================================================================
> --- rhvgoyal-linux.orig/security/selinux/hooks.c 2016-07-08 09:39:24.261676141 -0400
> +++ rhvgoyal-linux/security/selinux/hooks.c 2016-07-08 09:39:32.651676141 -0400
> @@ -3270,6 +3270,26 @@ static void selinux_inode_getsecid(struc
> *secid = isec->sid;
> }
>
> +static int selinux_inode_copy_up(struct dentry *src, struct cred **new)
> +{
> + u32 sid;
> + struct task_security_struct *tsec;
> + struct cred *new_creds = *new;
> +
> + if (new_creds == NULL) {
> + new_creds = prepare_creds();
> + if (!new_creds)
> + return -ENOMEM;
> + }
> +
> + tsec = new_creds->security;
> + /* Get label from overlay inode and set it in create_sid */
> + selinux_inode_getsecid(d_inode(src), &sid);
> + tsec->create_sid = sid;
> + *new = new_creds;
> + return 0;
> +}
> +
> /* file security operations */
>
> static int selinux_revalidate_file_permission(struct file *file, int mask)
> @@ -6056,6 +6076,7 @@ static struct security_hook_list selinux
> LSM_HOOK_INIT(inode_setsecurity, selinux_inode_setsecurity),
> LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity),
> LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
> + LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
>
> LSM_HOOK_INIT(file_permission, selinux_file_permission),
> LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),
>
--
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
