2007-06-26 23:22:34

by John Johansen

[permalink] [raw]
Subject: [AppArmor 32/44] Enable LSM hooks to distinguish operations on file descriptors from operations on pathnames

Struct iattr already contains ia_file since commit cc4e69de from
Miklos (which is related to commit befc649c). Use this to pass
struct file down the setattr hooks. This allows LSMs to distinguish
operations on file descriptors from operations on paths.

Signed-off-by: Andreas Gruenbacher <[email protected]>
Signed-off-by: John Johansen <[email protected]>
Cc: Miklos Szeredi <[email protected]>

---
fs/nfsd/vfs.c | 12 +++++++-----
fs/open.c | 16 +++++++++++-----
include/linux/fs.h | 3 +++
3 files changed, 21 insertions(+), 10 deletions(-)

--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -383,7 +383,7 @@ static ssize_t nfsd_getxattr(struct dent
{
ssize_t buflen;

- buflen = vfs_getxattr(dentry, mnt, key, NULL, 0);
+ buflen = vfs_getxattr(dentry, mnt, key, NULL, 0, NULL);
if (buflen <= 0)
return buflen;

@@ -391,7 +391,7 @@ static ssize_t nfsd_getxattr(struct dent
if (!*buf)
return -ENOMEM;

- return vfs_getxattr(dentry, mnt, key, *buf, buflen);
+ return vfs_getxattr(dentry, mnt, key, *buf, buflen, NULL);
}
#endif

@@ -417,7 +417,7 @@ set_nfsv4_acl_one(struct dentry *dentry,
goto out;
}

- error = vfs_setxattr(dentry, mnt, key, buf, len, 0);
+ error = vfs_setxattr(dentry, mnt, key, buf, len, 0, NULL);
out:
kfree(buf);
return error;
@@ -1992,12 +1992,14 @@ nfsd_set_posix_acl(struct svc_fh *fhp, i

mnt = fhp->fh_export->ex_mnt;
if (size)
- error = vfs_setxattr(fhp->fh_dentry, mnt, name, value, size,0);
+ error = vfs_setxattr(fhp->fh_dentry, mnt, name, value, size, 0,
+ NULL);
else {
if (!S_ISDIR(inode->i_mode) && type == ACL_TYPE_DEFAULT)
error = 0;
else {
- error = vfs_removexattr(fhp->fh_dentry, mnt, name);
+ error = vfs_removexattr(fhp->fh_dentry, mnt, name,
+ NULL);
if (error == -ENODATA)
error = 0;
}
--- a/fs/open.c
+++ b/fs/open.c
@@ -522,6 +522,8 @@ asmlinkage long sys_fchmod(unsigned int
mode = inode->i_mode;
newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
newattrs.ia_valid = ATTR_MODE | ATTR_CTIME;
+ newattrs.ia_valid |= ATTR_FILE;
+ newattrs.ia_file = file;
err = notify_change(dentry, file->f_path.mnt, &newattrs);
mutex_unlock(&inode->i_mutex);

@@ -572,7 +574,7 @@ asmlinkage long sys_chmod(const char __u
}

static int chown_common(struct dentry * dentry, struct vfsmount *mnt,
- uid_t user, gid_t group)
+ uid_t user, gid_t group, struct file *file)
{
struct inode * inode;
int error;
@@ -600,6 +602,10 @@ static int chown_common(struct dentry *
}
if (!S_ISDIR(inode->i_mode))
newattrs.ia_valid |= ATTR_KILL_SUID|ATTR_KILL_SGID;
+ if (file) {
+ newattrs.ia_file = file;
+ newattrs.ia_valid |= ATTR_FILE;
+ }
mutex_lock(&inode->i_mutex);
error = notify_change(dentry, mnt, &newattrs);
mutex_unlock(&inode->i_mutex);
@@ -615,7 +621,7 @@ asmlinkage long sys_chown(const char __u
error = user_path_walk(filename, &nd);
if (error)
goto out;
- error = chown_common(nd.dentry, nd.mnt, user, group);
+ error = chown_common(nd.dentry, nd.mnt, user, group, NULL);
path_release(&nd);
out:
return error;
@@ -635,7 +641,7 @@ asmlinkage long sys_fchownat(int dfd, co
error = __user_walk_fd(dfd, filename, follow, &nd);
if (error)
goto out;
- error = chown_common(nd.dentry, nd.mnt, user, group);
+ error = chown_common(nd.dentry, nd.mnt, user, group, NULL);
path_release(&nd);
out:
return error;
@@ -649,7 +655,7 @@ asmlinkage long sys_lchown(const char __
error = user_path_walk_link(filename, &nd);
if (error)
goto out;
- error = chown_common(nd.dentry, nd.mnt, user, group);
+ error = chown_common(nd.dentry, nd.mnt, user, group, NULL);
path_release(&nd);
out:
return error;
@@ -668,7 +674,7 @@ asmlinkage long sys_fchown(unsigned int

dentry = file->f_path.dentry;
audit_inode(NULL, dentry->d_inode);
- error = chown_common(dentry, file->f_path.mnt, user, group);
+ error = chown_common(dentry, file->f_path.mnt, user, group, file);
fput(file);
out:
return error;
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -351,6 +351,9 @@ struct iattr {
* Not an attribute, but an auxilary info for filesystems wanting to
* implement an ftruncate() like method. NOTE: filesystem should
* check for (ia_valid & ATTR_FILE), and not for (ia_file != NULL).
+ *
+ * The LSM hooks also use this to distinguish operations on a file
+ * descriptors from operations on pathnames.
*/
struct file *ia_file;
};

--


2007-06-28 16:12:25

by James Morris

[permalink] [raw]
Subject: Re: [AppArmor 32/44] Enable LSM hooks to distinguish operations on file descriptors from operations on pathnames

On Tue, 26 Jun 2007, [email protected] wrote:

> Struct iattr already contains ia_file since commit cc4e69de from
> Miklos (which is related to commit befc649c). Use this to pass
> struct file down the setattr hooks. This allows LSMs to distinguish
> operations on file descriptors from operations on paths.

I'm not quite sure I understand this.

Why would you distinguish operations based on whether you have a pathname
or an inode for an object ?

Are you trying to cater for the case where you're holding an open fd for a
file which has been deleted, and thus has no pathname?




- James
--
James Morris
<[email protected]>

2007-06-28 18:16:10

by Andreas Gruenbacher

[permalink] [raw]
Subject: Re: [AppArmor 32/44] Enable LSM hooks to distinguish operations on file descriptors from operations on pathnames

On Thursday 28 June 2007 18:12, James Morris wrote:
> Are you trying to cater for the case where you're holding an open fd for a
> file which has been deleted, and thus has no pathname?

Yes, see the AA_CHECK_FD flag in security/apparmor/main.c:aa_perm_dentry(). We
want to distinguish between the following two cases:

- process performs an operation on an open file descriptor,

- process performs an operation on a pathname, and between the dentry
lookup and the LSM permission check, the file gets deleted.

In the former case, we obviously want to continue giving the process access to
his fd (the classical pattern: open temporary file; delete it so that it will
self-recycle, continue using the open file descriptor).

In the latter case, The file still existed at the time of the lookup but not
anymore at the time of the permission check. The file obviously doesn't have
a filename anymore, so we cannot check permissions. If we granted access in
that case, processes could bypass their profile permissions in that race
window. We close the race by returning -ENOENT in that case, the same result
as if the file had already been deleted before the lookup.

Andreas