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@xxxxxxx>
> >>> ---
> >>> 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@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
