Re: [PATCH 00/11] cross rename v3
From: Tetsuo Handa
Date: Tue Jan 14 2014 - 08:04:57 EST
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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/