| 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: <20211209191109.o3x7nynnm52zhygz@wittgenstein>
Date: Thu, 9 Dec 2021 20:11:09 +0100
From: Christian Brauner <christian.brauner@...ntu.com>
To: Stefan Berger <stefanb@...ux.ibm.com>
Cc: linux-integrity@...r.kernel.org, zohar@...ux.ibm.com,
serge@...lyn.com, containers@...ts.linux.dev,
dmitry.kasatkin@...il.com, ebiederm@...ssion.com,
krzysztof.struczynski@...wei.com, roberto.sassu@...wei.com,
mpeters@...hat.com, lhinds@...hat.com, lsturman@...hat.com,
puiterwi@...hat.com, jejb@...ux.ibm.com, jamjoom@...ibm.com,
linux-kernel@...r.kernel.org, paul@...l-moore.com, rgb@...hat.com,
linux-security-module@...r.kernel.org, jmorris@...ei.org
Subject: Re: [PATCH v5 13/16] ima: Move some IMA policy and filesystem
related variables into ima_namespace
On Wed, Dec 08, 2021 at 05:18:15PM -0500, Stefan Berger wrote:
> Move the ima_write_mutex, ima_fs_flag, and valid_policy variables into
> ima_namespace. This way each IMA namespace can set those variables
> independently.
>
> Signed-off-by: Stefan Berger <stefanb@...ux.ibm.com>
> ---
> include/linux/ima.h | 5 ++++
> security/integrity/ima/ima_fs.c | 32 +++++++++++-------------
> security/integrity/ima/ima_init_ima_ns.c | 4 +++
> 3 files changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 2ce801bfc449..3aaf6e806db4 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -261,6 +261,11 @@ struct ima_namespace {
> struct ima_h_table ima_htable;
> struct list_head ima_measurements;
> unsigned long binary_runtime_size;
> +
> + /* IMA's filesystem */
> + struct mutex ima_write_mutex;
> + unsigned long ima_fs_flags;
> + int valid_policy;
> };
>
> extern struct ima_namespace init_ima_ns;
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index 38b1c26479b3..0e582ceecc7f 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -25,8 +25,6 @@
>
> #include "ima.h"
>
> -static DEFINE_MUTEX(ima_write_mutex);
> -
> bool ima_canonical_fmt;
> static int __init default_canonical_fmt_setup(char *str)
> {
> @@ -37,8 +35,6 @@ static int __init default_canonical_fmt_setup(char *str)
> }
> __setup("ima_canonical_fmt", default_canonical_fmt_setup);
>
> -static int valid_policy = 1;
> -
> static ssize_t ima_show_htable_value(char __user *buf, size_t count,
> loff_t *ppos, atomic_long_t *val)
> {
> @@ -339,7 +335,7 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
> goto out;
> }
>
> - result = mutex_lock_interruptible(&ima_write_mutex);
> + result = mutex_lock_interruptible(&ns->ima_write_mutex);
> if (result < 0)
> goto out_free;
>
> @@ -354,12 +350,12 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
> } else {
> result = ima_parse_add_rule(ns, data);
> }
> - mutex_unlock(&ima_write_mutex);
> + mutex_unlock(&ns->ima_write_mutex);
> out_free:
> kfree(data);
> out:
> if (result < 0)
> - valid_policy = 0;
> + ns->valid_policy = 0;
>
> return result;
> }
> @@ -376,8 +372,6 @@ enum ima_fs_flags {
> IMA_FS_BUSY,
> };
>
> -static unsigned long ima_fs_flags;
> -
> #ifdef CONFIG_IMA_READ_POLICY
> static const struct seq_operations ima_policy_seqops = {
> .start = ima_policy_start,
> @@ -392,6 +386,8 @@ static const struct seq_operations ima_policy_seqops = {
> */
> static int ima_open_policy(struct inode *inode, struct file *filp)
> {
> + struct ima_namespace *ns = get_current_ns();
> +
I'm a bit confused here. In all those callbacks:
.open = ima_open_policy,
.write = ima_write_policy,
.release = ima_release_policy,
you're calling get_current_ns() at the top of it. What guarantees that
the same ima_namespace is returned here? What if the fd is sent to
someone who is in a different user namespace and the write to that
file?
Maybe I'm just confused but wouldn't you want something like this?
>From 1f03dc427c583d5e9ebc9ebe9de77c3c535bbebe Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner@...ntu.com>
Date: Thu, 9 Dec 2021 20:07:02 +0100
Subject: [PATCH] !!!! HERE BE DRAGONS - UNTESTED !!!!
---
security/integrity/ima/ima_fs.c | 43 +++++++++++++++++++++++++++++----
1 file changed, 38 insertions(+), 5 deletions(-)
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 583462b29cb5..d5b302b925b8 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -317,10 +317,14 @@ static ssize_t ima_read_policy(char *path)
static ssize_t ima_write_policy(struct file *file, const char __user *buf,
size_t datalen, loff_t *ppos)
{
- struct ima_namespace *ns = get_current_ns();
+ struct ima_namespace *ns;
+ struct user_namespace *user_ns;
char *data;
ssize_t result;
+ user_ns = ima_filp_private(filp);
+ ns = user_ns->ima_ns
+
if (datalen >= PAGE_SIZE)
datalen = PAGE_SIZE - 1;
@@ -373,26 +377,51 @@ static const struct seq_operations ima_policy_seqops = {
};
#endif
+static struct user_namespace *ima_filp_private(struct file *filp)
+{
+ if (!(filp->f_flags & O_WRONLY)) {
+#ifdef CONFIG_IMA_READ_POLICY
+ struct seq_file *seq;
+
+ seq = filp->private_data;
+ return seq->private;
+#endif
+ }
+ return filp->private_data;
+}
+
/*
* ima_open_policy: sequentialize access to the policy file
*/
static int ima_open_policy(struct inode *inode, struct file *filp)
{
- struct ima_namespace *ns = get_current_ns();
+ struct user_namespace *user_ns = current_user_ns();
+ struct ima_namespace *ns = user_ns->ima_ns;
if (!(filp->f_flags & O_WRONLY)) {
#ifndef CONFIG_IMA_READ_POLICY
return -EACCES;
#else
+ int err;
+ struct seq_file *seq;
+
if ((filp->f_flags & O_ACCMODE) != O_RDONLY)
return -EACCES;
- if (!mac_admin_ns_capable(ima_user_ns(ns)))
+ if (!mac_admin_ns_capable(user_ns))
return -EPERM;
- return seq_open(filp, &ima_policy_seqops);
+ err = seq_open(filp, &ima_policy_seqops);
+ if (err)
+ return err;
+
+ seq = filp->private_data;
+ seq->private = user_ns;
+ return 0;
#endif
}
if (test_and_set_bit(IMA_FS_BUSY, &ns->ima_fs_flags))
return -EBUSY;
+
+ filp->private_data = user_ns;
return 0;
}
@@ -405,9 +434,13 @@ static int ima_open_policy(struct inode *inode, struct file *filp)
*/
static int ima_release_policy(struct inode *inode, struct file *file)
{
- struct ima_namespace *ns = get_current_ns();
+ struct ima_namespace *ns;
+ struct user_namespace *user_ns;
const char *cause = ns->valid_policy ? "completed" : "failed";
+ user_ns = ima_filp_private(filp);
+ ns = user_ns->ima_ns
+
if ((file->f_flags & O_ACCMODE) == O_RDONLY)
return seq_release(inode, file);
--
2.30.2
Powered by blists - more mailing lists