Re: [PATCH 00/11] cross rename v3

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Miklos Szeredi wrote:
> On Mon, Jan 13, 2014 at 11:03 PM, Tetsuo Handa
> <penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote:
> > Miklos Szeredi wrote:
> >> Cross rename (A, B) is equivalent to plain rename(A, B) + plain rename
> >> (B, A) done as a single atomic operation.  If security module allows
> >> both then cross rename is allowed.  If at least one is denied then the
> >> cross rename is denied.
> >
> > Yes, the functionality itself is fine. The problem is how LSM users check
> > their permissions for the functionality.
> >
> >>
> >> This is prepared for in "[PATCH 06/11] security: add flags to rename
> >> hooks" and actually done in "[PATCH 07/11] vfs: add cross-rename".
> >>
> >> Security people are free to implement a explicit security check for
> >> cross rename, but I don't think that is in the scope of this patchset.
> >>
> > I don't know how their permissions are checked, but I think that
> > swapping /A/B and /C/D should check not only
> >
> >   Remove a name from directory A
> >   Add a name to directory C
> >
> > but also
> >
> >   Add a name to directory A
> >   Remove a name from directory C
> >
> > using their security labels.
> >
> > Without making changes to security/*/ directory, SELinux/SMACK/TOMOYO/AppArmor
> > might fail to check the latter permissions.
> 
> Those permissions will be checked.   Please see security/security.c in
> patch 07/11 of the series.
> 
Oh, I see. But I think that 07/11 is wasteful for security_path_rename() users.
Why bother to re-calculate /A/B and /C/D using d_absolute_path()?

I prefer flags argument passed to tomoyo_path_rename(). Untested patch follows.
John, what about AppArmor?
----------
>From 4344f31e40b908ab1a6dba9121018d7f37130393 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
Date: Tue, 14 Jan 2014 21:55:48 +0900
Subject: [PATCH] LSM: Pass flags argument to security_path_rename hook users.

Passing flags argument can save TOMOYO from recalculating pathnames
when cross rename operation is requested.

Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
---
 include/linux/security.h |    4 +++-
 security/apparmor/lsm.c  |   17 ++++++++++++++---
 security/capability.c    |    3 ++-
 security/security.c      |    9 +--------
 security/tomoyo/common.c |    1 +
 security/tomoyo/common.h |    5 ++++-
 security/tomoyo/file.c   |   10 +++++++++-
 security/tomoyo/tomoyo.c |    8 ++++++--
 security/tomoyo/util.c   |    1 +
 9 files changed, 41 insertions(+), 17 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index 95cfccc..ba8ee7a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -453,6 +453,7 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	@old_dentry contains the dentry structure of the old link.
  *	@new_dir contains the path structure for parent of the new link.
  *	@new_dentry contains the dentry structure of the new link.
+ *	@flags contains rename flags.
  *	Return 0 if permission is granted.
  * @path_chmod:
  *	Check for permission to change DAC's permission of a file or directory.
@@ -1491,7 +1492,8 @@ struct security_operations {
 	int (*path_link) (struct dentry *old_dentry, struct path *new_dir,
 			  struct dentry *new_dentry);
 	int (*path_rename) (struct path *old_dir, struct dentry *old_dentry,
-			    struct path *new_dir, struct dentry *new_dentry);
+			    struct path *new_dir, struct dentry *new_dentry,
+			    unsigned int flags);
 	int (*path_chmod) (struct path *path, umode_t mode);
 	int (*path_chown) (struct path *path, kuid_t uid, kgid_t gid);
 	int (*path_chroot) (struct path *path);
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 4257b7e..f5d4704 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -315,7 +315,8 @@ static int apparmor_path_link(struct dentry *old_dentry, struct path *new_dir,
 }
 
 static int apparmor_path_rename(struct path *old_dir, struct dentry *old_dentry,
-				struct path *new_dir, struct dentry *new_dentry)
+				struct path *new_dir, struct dentry *new_dentry,
+				unsigned int flags)
 {
 	struct aa_profile *profile;
 	int error = 0;
@@ -330,7 +331,7 @@ static int apparmor_path_rename(struct path *old_dir, struct dentry *old_dentry,
 		struct path_cond cond = { old_dentry->d_inode->i_uid,
 					  old_dentry->d_inode->i_mode
 		};
-
+retry:
 		error = aa_path_perm(OP_RENAME_SRC, profile, &old_path, 0,
 				     MAY_READ | AA_MAY_META_READ | MAY_WRITE |
 				     AA_MAY_META_WRITE | AA_MAY_DELETE,
@@ -339,7 +340,17 @@ static int apparmor_path_rename(struct path *old_dir, struct dentry *old_dentry,
 			error = aa_path_perm(OP_RENAME_DEST, profile, &new_path,
 					     0, MAY_WRITE | AA_MAY_META_WRITE |
 					     AA_MAY_CREATE, &cond);
-
+		if (!error && (flags & RENAME_EXCHANGE)) {
+			/* Cross rename requires both inodes to exist. */
+			old_path.mnt = new_dir->mnt;
+			old_path.dentry = new_dentry;
+			new_path.mnt = old_dir->mnt;
+			new_path.dentry = old_dentry;
+			cond.uid = new_dentry->d_inode->i_uid;
+			cond.mode = new_dentry->d_inode->i_mode;
+			flags = 0;
+			goto retry;
+		}
 	}
 	return error;
 }
diff --git a/security/capability.c b/security/capability.c
index 8b4f24a..cb67fe2 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -280,7 +280,8 @@ static int cap_path_link(struct dentry *old_dentry, struct path *new_dir,
 }
 
 static int cap_path_rename(struct path *old_path, struct dentry *old_dentry,
-			   struct path *new_path, struct dentry *new_dentry)
+			   struct path *new_path, struct dentry *new_dentry,
+			   unsigned int flags)
 {
 	return 0;
 }
diff --git a/security/security.c b/security/security.c
index 3dd2258..b14574e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -440,15 +440,8 @@ int security_path_rename(struct path *old_dir, struct dentry *old_dentry,
 		     (new_dentry->d_inode && IS_PRIVATE(new_dentry->d_inode))))
 		return 0;
 
-	if (flags & RENAME_EXCHANGE) {
-		int err = security_ops->path_rename(new_dir, new_dentry,
-						    old_dir, old_dentry);
-		if (err)
-			return err;
-	}
-
 	return security_ops->path_rename(old_dir, old_dentry, new_dir,
-					 new_dentry);
+					 new_dentry, flags);
 }
 EXPORT_SYMBOL(security_path_rename);
 
diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
index 283862a..86747a7 100644
--- a/security/tomoyo/common.c
+++ b/security/tomoyo/common.c
@@ -36,6 +36,7 @@ const char * const tomoyo_mac_keywords[TOMOYO_MAX_MAC_INDEX
 	[TOMOYO_MAC_FILE_MKCHAR]     = "mkchar",
 	[TOMOYO_MAC_FILE_LINK]       = "link",
 	[TOMOYO_MAC_FILE_RENAME]     = "rename",
+	[TOMOYO_MAC_FILE_SWAPNAME]   = "swapname",
 	[TOMOYO_MAC_FILE_CHMOD]      = "chmod",
 	[TOMOYO_MAC_FILE_CHOWN]      = "chown",
 	[TOMOYO_MAC_FILE_CHGRP]      = "chgrp",
diff --git a/security/tomoyo/common.h b/security/tomoyo/common.h
index b897d48..0349ae9 100644
--- a/security/tomoyo/common.h
+++ b/security/tomoyo/common.h
@@ -276,6 +276,7 @@ enum tomoyo_network_acl_index {
 enum tomoyo_path2_acl_index {
 	TOMOYO_TYPE_LINK,
 	TOMOYO_TYPE_RENAME,
+	TOMOYO_TYPE_SWAPNAME,
 	TOMOYO_TYPE_PIVOT_ROOT,
 	TOMOYO_MAX_PATH2_OPERATION
 };
@@ -335,6 +336,7 @@ enum tomoyo_mac_index {
 	TOMOYO_MAC_FILE_MKCHAR,
 	TOMOYO_MAC_FILE_LINK,
 	TOMOYO_MAC_FILE_RENAME,
+	TOMOYO_MAC_FILE_SWAPNAME,
 	TOMOYO_MAC_FILE_CHMOD,
 	TOMOYO_MAC_FILE_CHOWN,
 	TOMOYO_MAC_FILE_CHGRP,
@@ -730,7 +732,8 @@ struct tomoyo_mkdev_acl {
 };
 
 /*
- * Structure for "file rename", "file link" and "file pivot_root" directive.
+ * Structure for "file rename", "file swapname", "file link" and
+ * "file pivot_root" directive.
  */
 struct tomoyo_path2_acl {
 	struct tomoyo_acl_info head; /* type = TOMOYO_TYPE_PATH2_ACL */
diff --git a/security/tomoyo/file.c b/security/tomoyo/file.c
index 4003907..c7d9546 100644
--- a/security/tomoyo/file.c
+++ b/security/tomoyo/file.c
@@ -38,6 +38,7 @@ const u8 tomoyo_pnnn2mac[TOMOYO_MAX_MKDEV_OPERATION] = {
 const u8 tomoyo_pp2mac[TOMOYO_MAX_PATH2_OPERATION] = {
 	[TOMOYO_TYPE_LINK]       = TOMOYO_MAC_FILE_LINK,
 	[TOMOYO_TYPE_RENAME]     = TOMOYO_MAC_FILE_RENAME,
+	[TOMOYO_TYPE_SWAPNAME]   = TOMOYO_MAC_FILE_SWAPNAME,
 	[TOMOYO_TYPE_PIVOT_ROOT] = TOMOYO_MAC_FILE_PIVOT_ROOT,
 };
 
@@ -874,7 +875,7 @@ int tomoyo_mkdev_perm(const u8 operation, struct path *path,
 }
 
 /**
- * tomoyo_path2_perm - Check permission for "rename", "link" and "pivot_root".
+ * tomoyo_path2_perm - Check permission for "rename", "swapname", "link" and "pivot_root".
  *
  * @operation: Type of operation.
  * @path1:      Pointer to "struct path".
@@ -916,6 +917,13 @@ int tomoyo_path2_perm(const u8 operation, struct path *path1,
 		tomoyo_add_slash(&buf1);
 		tomoyo_add_slash(&buf2);
 		break;
+	case TOMOYO_TYPE_SWAPNAME:
+		/* Cross rename requires both inodes to exist. */
+		if (S_ISDIR(path1->dentry->d_inode->i_mode))
+			tomoyo_add_slash(&buf1);
+		if (S_ISDIR(path2->dentry->d_inode->i_mode))
+			tomoyo_add_slash(&buf2);
+		break;
 	}
 	r.obj = &obj;
 	r.param_type = TOMOYO_TYPE_PATH2_ACL;
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index f0b756e..8e9fb4a 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -287,17 +287,21 @@ static int tomoyo_path_link(struct dentry *old_dentry, struct path *new_dir,
  * @old_dentry: Pointer to "struct dentry".
  * @new_parent: Pointer to "struct path".
  * @new_dentry: Pointer to "struct dentry".
+ * @flags:      Rename flags.
  *
  * Returns 0 on success, negative value otherwise.
  */
 static int tomoyo_path_rename(struct path *old_parent,
 			      struct dentry *old_dentry,
 			      struct path *new_parent,
-			      struct dentry *new_dentry)
+			      struct dentry *new_dentry,
+			      unsigned int flags)
 {
 	struct path path1 = { old_parent->mnt, old_dentry };
 	struct path path2 = { new_parent->mnt, new_dentry };
-	return tomoyo_path2_perm(TOMOYO_TYPE_RENAME, &path1, &path2);
+	return tomoyo_path2_perm((flags & RENAME_EXCHANGE) ?
+				 TOMOYO_TYPE_SWAPNAME : TOMOYO_TYPE_RENAME,
+				 &path1, &path2);
 }
 
 /**
diff --git a/security/tomoyo/util.c b/security/tomoyo/util.c
index 2952ba5..f0ac0be 100644
--- a/security/tomoyo/util.c
+++ b/security/tomoyo/util.c
@@ -34,6 +34,7 @@ const u8 tomoyo_index2category[TOMOYO_MAX_MAC_INDEX] = {
 	[TOMOYO_MAC_FILE_MKCHAR]     = TOMOYO_MAC_CATEGORY_FILE,
 	[TOMOYO_MAC_FILE_LINK]       = TOMOYO_MAC_CATEGORY_FILE,
 	[TOMOYO_MAC_FILE_RENAME]     = TOMOYO_MAC_CATEGORY_FILE,
+	[TOMOYO_MAC_FILE_SWAPNAME]   = TOMOYO_MAC_CATEGORY_FILE,
 	[TOMOYO_MAC_FILE_CHMOD]      = TOMOYO_MAC_CATEGORY_FILE,
 	[TOMOYO_MAC_FILE_CHOWN]      = TOMOYO_MAC_CATEGORY_FILE,
 	[TOMOYO_MAC_FILE_CHGRP]      = TOMOYO_MAC_CATEGORY_FILE,
-- 
1.7.1
--
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




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux