| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <535ACA76.4030106@schaufler-ca.com>
Date: Fri, 25 Apr 2014 13:49:58 -0700
From: Casey Schaufler <casey@...aufler-ca.com>
To: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
linux-security-module@...r.kernel.org
CC: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
miklos@...redi.hu
Subject: Re: [PATCH (for 3.15) 1/5] LSM: Pass the rename flags to each LSM
module.
On 4/24/2014 4:22 AM, Tetsuo Handa wrote:
> >From 207346b4153e2b441e0c45d933043e6ba7abb419 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> Date: Thu, 24 Apr 2014 20:04:57 +0900
> Subject: [PATCH (for 3.15) 1/5] LSM: Pass the rename flags to each LSM module.
>
> Commit da1ce067 "vfs: add cross-rename" changed security_inode_rename() and
> security_path_rename() to call LSM module with reversed arguments if the rename
> flags contain RENAME_EXCHANGE. But that commit introduced a race window for
> TOMOYO and AppArmor (users of security_path_rename) because the pathnames may
> have been changed between the first call and the second call.
>
> To fix this race condition, the rename flags should be passed to LSM module
> in order to allow LSM module to avoid re-calculation of pathnames.
> Passing the rename flags allows SMACK (one of users of security_inode_rename)
> to avoid needlessly checking the same permission twice.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Acked-by: Casey Schaufler <casey@...aufler-ca.com>
At least for the Smack changes.
> ---
> include/linux/security.h | 8 ++++++--
> security/apparmor/lsm.c | 3 ++-
> security/capability.c | 6 ++++--
> security/security.c | 10 ++++++----
> security/selinux/hooks.c | 3 ++-
> security/smack/smack_lsm.c | 4 +++-
> security/tomoyo/tomoyo.c | 4 +++-
> 7 files changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 9c6b972..12770bb 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -446,6 +446,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 inode 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_rename:
> * Check for permission to rename a file or directory.
> @@ -453,6 +454,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.
> @@ -1492,7 +1494,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);
> @@ -1515,7 +1518,8 @@ struct security_operations {
> int (*inode_mknod) (struct inode *dir, struct dentry *dentry,
> umode_t mode, dev_t dev);
> int (*inode_rename) (struct inode *old_dir, struct dentry *old_dentry,
> - struct inode *new_dir, struct dentry *new_dentry);
> + struct inode *new_dir, struct dentry *new_dentry,
> + unsigned int flags);
> int (*inode_readlink) (struct dentry *dentry);
> int (*inode_follow_link) (struct dentry *dentry, struct nameidata *nd);
> int (*inode_permission) (struct inode *inode, int mask);
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 9981000..c0b4366 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;
> diff --git a/security/capability.c b/security/capability.c
> index e76373d..b3690cc 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -176,7 +176,8 @@ static int cap_inode_mknod(struct inode *inode, struct dentry *dentry,
> }
>
> static int cap_inode_rename(struct inode *old_inode, struct dentry *old_dentry,
> - struct inode *new_inode, struct dentry *new_dentry)
> + struct inode *new_inode, struct dentry *new_dentry,
> + unsigned int flags)
> {
> return 0;
> }
> @@ -280,7 +281,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 31614e9..65ceef3 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -442,13 +442,14 @@ int security_path_rename(struct path *old_dir, struct dentry *old_dentry,
>
> if (flags & RENAME_EXCHANGE) {
> int err = security_ops->path_rename(new_dir, new_dentry,
> - old_dir, old_dentry);
> + old_dir, old_dentry,
> + flags);
> 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);
>
> @@ -542,13 +543,14 @@ int security_inode_rename(struct inode *old_dir, struct dentry *old_dentry,
>
> if (flags & RENAME_EXCHANGE) {
> int err = security_ops->inode_rename(new_dir, new_dentry,
> - old_dir, old_dentry);
> + old_dir, old_dentry,
> + flags);
> if (err)
> return err;
> }
>
> return security_ops->inode_rename(old_dir, old_dentry,
> - new_dir, new_dentry);
> + new_dir, new_dentry, flags);
> }
>
> int security_inode_readlink(struct dentry *dentry)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 6befc92..d4913d1 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2749,7 +2749,8 @@ static int selinux_inode_mknod(struct inode *dir, struct dentry *dentry, umode_t
> }
>
> static int selinux_inode_rename(struct inode *old_inode, struct dentry *old_dentry,
> - struct inode *new_inode, struct dentry *new_dentry)
> + struct inode *new_inode, struct dentry *new_dentry,
> + unsigned int flags)
> {
> return may_rename(old_inode, old_dentry, new_inode, new_dentry);
> }
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 8177e7d..41bd059 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -697,6 +697,7 @@ static int smack_inode_rmdir(struct inode *dir, struct dentry *dentry)
> * @old_dentry: unused
> * @new_inode: the new directory
> * @new_dentry: unused
> + * @flags: unused
> *
> * Read and write access is required on both the old and
> * new directories.
> @@ -706,7 +707,8 @@ static int smack_inode_rmdir(struct inode *dir, struct dentry *dentry)
> static int smack_inode_rename(struct inode *old_inode,
> struct dentry *old_dentry,
> struct inode *new_inode,
> - struct dentry *new_dentry)
> + struct dentry *new_dentry,
> + unsigned int flags)
> {
> int rc;
> char *isp;
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index f0b756e..ac7dd97 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -287,13 +287,15 @@ 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 };
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists