apparmor/kernel-patches/for-mainline/apparmor-dir-rename-fix.diff

Directory rename bugfix

Up until now, the profile permission checks for directory rename only
worked in most cases, and mostly be coincidence: when we did permission
checks, we ignored requests for MAY_WRITE and MAY_EXEC on directories
because else we would have to include x access on each directory that
should be accessed, and w access on each directory that files can be
created in or removed from.

We also ignored MAY_WRITE and MAY_EXEC for directory renames, and it was
only by sheer luck that most directory renames failed -- the only reason
why most they failed was because in most cases, the target file will
either not exist, or be a non-directory. If the target file exists and
is a directory, then it is sufficient to have r profile access in order
to rename directories, though.

Test case: create two empty directories d1 and d2 somewhere. Give a
process r access to that directory. Do a rename("d1", "d2") syscall, and
d2 will disappear. This will also fail with ENOTEMPTY if d2 is
non-empty. The expected result of course would be EPERM.

This issue may have been noticed before. Anyway, a fix is attached;
there are now also comments in the code that explain what's going on.

Caveat: there still is no way to to tell a directory traversal from the
access(2) system call, and so access("d1", W_OK) will still return 0
even if the profile does not allow w access to d1. This is mildly
annoying, but using access(2) for permission checking is insecure,
anyway.

Index: b/security/apparmor/lsm.c
===================================================================
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -278,7 +278,7 @@ out:
 }
 
 static int aa_permission(struct inode *inode, struct dentry *dentry,
-			 struct vfsmount *mnt, int mask)
+			 struct vfsmount *mnt, int mask, int leaf)
 {
 	int error = 0;
 
@@ -286,7 +286,7 @@ static int aa_permission(struct inode *i
 		struct aa_profile *active = get_active_aa_profile();
 
 		if (active)
-			error = aa_perm(active, dentry, mnt, mask);
+			error = aa_perm(active, dentry, mnt, mask, leaf);
 		put_aa_profile(active);
 	}
 	return error;
@@ -295,7 +295,7 @@ static int aa_permission(struct inode *i
 static int apparmor_inode_create(struct inode *dir, struct dentry *dentry,
 				 struct vfsmount *mnt, int mask)
 {
-	return aa_permission(dir, dentry, mnt, MAY_WRITE);
+	return aa_permission(dir, dentry, mnt, MAY_WRITE, 1);
 }
 
 static int apparmor_inode_link(struct dentry *old_dentry,
@@ -325,19 +325,19 @@ static int apparmor_inode_unlink(struct 
 				 struct dentry *dentry,
 				 struct vfsmount *mnt)
 {
-	return aa_permission(dir, dentry, mnt, MAY_WRITE);
+	return aa_permission(dir, dentry, mnt, MAY_WRITE, 1);
 }
 
 static int apparmor_inode_symlink(struct inode *dir, struct dentry *dentry,
 				  struct vfsmount *mnt, const char *old_name)
 {
-	return aa_permission(dir, dentry, mnt, MAY_WRITE);
+	return aa_permission(dir, dentry, mnt, MAY_WRITE, 1);
 }
 
 static int apparmor_inode_mknod(struct inode *dir, struct dentry *dentry,
 				struct vfsmount *mnt, int mode, dev_t dev)
 {
-	return aa_permission(dir, dentry, mnt, MAY_WRITE);
+	return aa_permission(dir, dentry, mnt, MAY_WRITE, 1);
 }
 
 static int apparmor_inode_rename(struct inode *old_dir,
@@ -358,11 +358,11 @@ static int apparmor_inode_rename(struct 
 	if (active) {
 		if (old_mnt)
 			error = aa_perm(active, old_dentry, old_mnt,
-					MAY_READ|MAY_WRITE);
+					MAY_READ | MAY_WRITE, 1);
 
 		if (!error && new_mnt)
 			error = aa_perm(active, new_dentry, new_mnt,
-					MAY_WRITE);
+					MAY_WRITE, 1);
 	}
 
 	put_aa_profile(active);
@@ -376,8 +376,22 @@ static int apparmor_inode_permission(str
 {
 	if (!nd)
 		return 0;
+	/*
+	 * Assume we are /not/ checking a leaf directory.  We do not
+	 * require profile access to non-leaf directories in order to
+	 * traverse them, create or remove files in them.  We do require
+	 * MAY_WRITE profile access on the actual file or directory
+	 * being created or removed, though, which makes the model safe
+	 * again.
+	 *
+	 * For the access(2) system call this means that when used on a
+	 * leaf directory, we may return 0 even though the requesting
+	 * process doesn't actually have MAY_WRITE nor MAY_EXEC access
+	 * in the profile.  This is nothing but an inconvenience; using
+	 * access(2) for permission checking is invalid, anyways.
+	 */
 	return aa_permission(inode, nd->dentry, nd->mnt,
-			     mask & (MAY_READ | MAY_WRITE | MAY_EXEC));
+			     mask & (MAY_READ | MAY_WRITE | MAY_EXEC), 0);
 }
 
 static int apparmor_inode_setattr(struct dentry *dentry, struct vfsmount *mnt,
@@ -454,6 +468,7 @@ static int apparmor_inode_removexattr(st
 static inline int aa_mmap(struct file *file, unsigned long prot,
 			  unsigned long flags)
 {
+	struct dentry *dentry;
 	int mask = 0;
 
 	if (!file)
@@ -468,8 +483,8 @@ static inline int aa_mmap(struct file *f
 	if (prot & PROT_EXEC)
 		mask |= AA_EXEC_MMAP;
 
-	return aa_permission(file->f_dentry->d_inode, file->f_dentry,
-			     file->f_vfsmnt, mask);
+	dentry = file->f_dentry;
+	return aa_permission(dentry->d_inode, dentry, file->f_vfsmnt, mask, 1);
 }
 
 static int apparmor_file_mmap(struct file *file, unsigned long reqprot,
Index: b/security/apparmor/main.c
===================================================================
--- a/security/apparmor/main.c
+++ b/security/apparmor/main.c
@@ -80,32 +80,6 @@ out:
 	return perms;
 }
 
-/**
- * aa_filter_mask
- * @mask: requested mask
- * @inode: potential directory inode
- *
- * This fn performs pre-verification of the requested mask
- * We ignore append. Previously we required 'w' on a dir to add a file.
- * No longer. Now we require 'w' on just the file itself. Traversal 'x' is
- * also ignored for directories.
- *
- * Returned value of %0 indicates no need to perform a perm check.
- */
-static inline int aa_filter_mask(int mask, struct inode *inode)
-{
-	if (mask) {
-		int elim = MAY_APPEND;
-
-		if (inode && S_ISDIR(inode->i_mode))
-			elim |= (MAY_EXEC | MAY_WRITE);
-
-		mask &= ~elim;
-	}
-
-	return mask;
-}
-
 static inline void aa_permerror2result(int perm_result, struct aa_audit *sa)
 {
 	if (perm_result == 0) {	/* success */
@@ -599,17 +573,30 @@ int aa_perm_xattr(struct aa_profile *act
  * @dentry: dentry
  * @mnt: mountpoint
  * @mask: access mode requested
+ * @leaf: are we checking a leaf node?
  *
  * Determine if access (mask) for dentry is authorized by active
  * profile.  Result, %0 (success), -ve (error)
  */
 int aa_perm(struct aa_profile *active, struct dentry *dentry,
-	    struct vfsmount *mnt, int mask)
+	    struct vfsmount *mnt, int mask, int leaf)
 {
-	int error = 0;
+	struct inode *inode = dentry->d_inode;
 	struct aa_audit sa;
+	int error = 0;
 
-	if ((mask = aa_filter_mask(mask, dentry->d_inode)) == 0)
+	if (!leaf && inode && S_ISDIR(inode->i_mode)) {
+		/*
+		 * If checking a non-leaf directory, allow traverse and
+		 * write access: we do not require profile access to
+		 * non-leaf directories in order to traverse them,
+		 * create or remove files in them. We do require
+		 * MAY_WRITE profile access on the actual file or
+		 * directory being created or removed, though.
+		 */
+		mask &= ~(MAY_EXEC | MAY_WRITE);
+	}
+	if (mask == 0)
 		goto out;
 
 	sa.type = AA_AUDITTYPE_FILE;
Index: b/security/apparmor/apparmor.h
===================================================================
--- a/security/apparmor/apparmor.h
+++ b/security/apparmor/apparmor.h
@@ -209,7 +209,7 @@ extern int aa_perm_xattr(struct aa_profi
 			 const char *xattr_xattr, int mask);
 extern int aa_capability(struct aa_profile *active, int cap);
 extern int aa_perm(struct aa_profile *active, struct dentry *dentry,
-		   struct vfsmount *mnt, int mask);
+		   struct vfsmount *mnt, int mask, int leaf);
 extern int aa_perm_dir(struct aa_profile *active, struct dentry *dentry,
 		       struct vfsmount *mnt, const char *operation, int mask);
 extern int aa_link(struct aa_profile *active,