Re: [PATCH 4/5] fs: Remove security attributes on truncate

[Jan Kara <jack@suse.cz>]

On Wed 10-12-14 12:11:23, Jan Kara wrote:
> On Tue 09-12-14 10:59:29, Casey Schaufler wrote:
> > On 12/9/2014 10:27 AM, Jan Kara wrote:
> > > On Fri 05-12-14 08:06:55, Casey Schaufler wrote:
> > >> On 12/4/2014 5:27 AM, Jan Kara wrote:
> > >>> Similarly as we remove suid bit on truncate, we also want to remove
> > >>> security extended attributes.
> > >> NAK
> > >>
> > >> Are you out of your mind?
> > >>
> > >> In Smack and SELinux the security attributes are associated with the
> > >> container, not the data.
> > >   Is there some doc for this? It just seems strange to me that when a file
> > > is written we clear the attributes
> > 
> > This is not true for the LSM based attributes.
> > 
> > >  but when the file is truncated we don't.
> > 
> > Have I miss-interpreted what you meant by "security extended attributes"?
> > Do you mean filesystem xattrs beginning with "security.", such as
> > "security.selinux" or "security.SMACK64", or something else?
>   Sorry, I'm not a security guy so I may be using wrong terminology. I
> meant attributes that are removed when you call security_inode_killpriv().
> There's a comment in security.h like:
>  * @inode_killpriv:
>  *      The setuid bit is being removed.  Remove similar security labels.
>  *      Called with the dentry->d_inode->i_mutex held.
>  *      @dentry is the dentry being changed.
>  *      Return 0 on success.  If error is returned, then the operation
>  *      causing setuid bit removal is failed.
> 
> So from that I'd think that security_inode_killpriv() should be called if
> we are removing SUID bit (i.e. also during truncate).
  Casey, so are you OK which this change?

								Honza

> > > What's the rationale behind this? To me both operations modify content of
> > > the file and thus I'd expect them to behave identically with respect to
> > > security attributes...
> > >
> > > 								Honza
> > >
> > >>> After this patch there's only one user of should_remove_suid() - ocfs2 -
> > >>> and indeed it's buggy because it doesn't clear security attributes on
> > >>> write. However fixing it is difficult because of special locking
> > >>> constraints.
> > >>>
> > >>> Signed-off-by: Jan Kara <jack@suse.cz>
> > >>> ---
> > >>>  fs/inode.c         | 5 ++---
> > >>>  fs/open.c          | 6 ++++--
> > >>>  include/linux/fs.h | 6 +++++-
> > >>>  3 files changed, 11 insertions(+), 6 deletions(-)
> > >>>
> > >>> diff --git a/fs/inode.c b/fs/inode.c
> > >>> index 6807a2707828..8595c7b8841c 100644
> > >>> --- a/fs/inode.c
> > >>> +++ b/fs/inode.c
> > >>> @@ -1603,9 +1603,8 @@ EXPORT_SYMBOL(should_remove_suid);
> > >>>   * response to write or truncate. Return 0 if nothing has to be changed.
> > >>>   * Negative value on error (change should be denied).
> > >>>   */
> > >>> -int file_needs_remove_privs(struct file *file)
> > >>> +int dentry_needs_remove_privs(struct dentry *dentry)
> > >>>  {
> > >>> -	struct dentry *dentry = file->f_path.dentry;
> > >>>  	struct inode *inode = dentry->d_inode;
> > >>>  	int mask = 0;
> > >>>  	int ret;
> > >>> @@ -1621,7 +1620,7 @@ int file_needs_remove_privs(struct file *file)
> > >>>  		mask |= ATTR_KILL_PRIV;
> > >>>  	return mask;
> > >>>  }
> > >>> -EXPORT_SYMBOL(file_needs_remove_privs);
> > >>> +EXPORT_SYMBOL(dentry_needs_remove_privs);
> > >>>  
> > >>>  static int __remove_privs(struct dentry *dentry, int kill)
> > >>>  {
> > >>> diff --git a/fs/open.c b/fs/open.c
> > >>> index de92c13b58be..e4e0863855d0 100644
> > >>> --- a/fs/open.c
> > >>> +++ b/fs/open.c
> > >>> @@ -51,8 +51,10 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
> > >>>  		newattrs.ia_valid |= ATTR_FILE;
> > >>>  	}
> > >>>  
> > >>> -	/* Remove suid/sgid on truncate too */
> > >>> -	ret = should_remove_suid(dentry);
> > >>> +	/* Remove suid/sgid and security markings on truncate too */
> > >>> +	ret = dentry_needs_remove_privs(dentry);
> > >>> +	if (ret < 0)
> > >>> +		return ret;
> > >>>  	if (ret)
> > >>>  		newattrs.ia_valid |= ret | ATTR_FORCE;
> > >>>  
> > >>> diff --git a/include/linux/fs.h b/include/linux/fs.h
> > >>> index aac707cced66..c5ccc311e8fb 100644
> > >>> --- a/include/linux/fs.h
> > >>> +++ b/include/linux/fs.h
> > >>> @@ -2429,7 +2429,11 @@ extern struct inode *new_inode(struct super_block *sb);
> > >>>  extern void free_inode_nonrcu(struct inode *inode);
> > >>>  extern int should_remove_suid(struct dentry *);
> > >>>  extern int file_remove_privs(struct file *);
> > >>> -extern int file_needs_remove_privs(struct file *file);
> > >>> +extern int dentry_needs_remove_privs(struct dentry *dentry);
> > >>> +static inline int file_needs_remove_privs(struct file *file)
> > >>> +{
> > >>> +	return dentry_needs_remove_privs(file->f_path.dentry);
> > >>> +}
> > >>>  
> > >>>  extern void __insert_inode_hash(struct inode *, unsigned long hashval);
> > >>>  static inline void insert_inode_hash(struct inode *inode)
> > 
> -- 
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs