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

[Jan Kara <jack@suse.cz>]

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).

								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