From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932644AbcGHPeZ (ORCPT ); Fri, 8 Jul 2016 11:34:25 -0400 Received: from nm28.bullet.mail.bf1.yahoo.com ([98.139.212.187]:54975 "EHLO nm28.bullet.mail.bf1.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932566AbcGHPeW (ORCPT ); Fri, 8 Jul 2016 11:34:22 -0400 X-Yahoo-Newman-Id: 326125.38400.bm@smtp207.mail.bf1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: vZY0AksVM1ntLP.yjjE2ajDnLJA7bo5rPtcgRXslpZWVniy gnuBgQ6Z5WduUWCdn7PGJVtHGrq1ysVgK8foyVtpftPXGR3jDTkuZ0jJvwPx p8kYG0e6i9MfK6hH4A12TWzX3jqto5m3Y2t455S54J9CNuHa_LXTd_2NTtMs BHU8ZwBK711e4dW2WuPQHl8ccBwq1Gq3.csgSoTNOuYM4oTS28g2ThJnyx.6 jMr0b8vdPGuWjt_YjcEl2jRf6w3DIuKG3_la80DhWF6r8GtM8xNK6URfLAe7 MKuRtRoEsa7gsqhWKujEoJk8Ly7qWxWoUiz3P6Y9dCxos0JB_4nCFedOr9DB BM0TS61OpArZ3Y4OksRgi1arxnTCMLBKZFsbofOKQQw5DgkP__ejozfUMbT7 BqFSCrHo_r5KRYXU9MO1G37KcG_fQHvP5AC1SKI_8_.BdWxam4Cite6bGZIa uY_m9tbud_4s9T2Zi2Bty65eF0qHT70cQm8HO3NkhARX2dhZLx2_4SXMssaJ Gv.EGhVNJiiu48PH1U00KFRxMNjASy39zTK8j5GrOy13cW.mt X-Yahoo-SMTP: OIJXglSswBDfgLtXluJ6wiAYv6_cnw-- Subject: Re: [PATCH 1/5] security, overlayfs: provide copy up security hook for unioned files To: Vivek Goyal , Miklos Szeredi References: <1467733854-6314-1-git-send-email-vgoyal@redhat.com> <1467733854-6314-2-git-send-email-vgoyal@redhat.com> <63906f0e-25cf-266b-6df9-18317f1e7c59@schaufler-ca.com> <20160707203329.GD11036@redhat.com> <2e7462e4-c84b-a6cf-74d3-35bf68048a61@schaufler-ca.com> <20160708124534.GA7732@redhat.com> <20160708134257.GB7732@redhat.com> Cc: Stephen Smalley , linux-kernel@vger.kernel.org, "linux-unionfs@vger.kernel.org" , LSM , Daniel J Walsh , David Howells , pmoore@redhat.com, Al Viro , linux-fsdevel@vger.kernel.org From: Casey Schaufler Message-ID: <2f2d480d-5fab-82aa-c77f-c3b3f5bb7d0e@schaufler-ca.com> Date: Fri, 8 Jul 2016 08:34:17 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <20160708134257.GB7732@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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), >