| 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: <b54ed3a9-4e21-50ea-51e3-00dbe7aece3d@linux.ibm.com>
Date: Thu, 9 Dec 2021 15:42:46 -0500
From: Stefan Berger <stefanb@...ux.ibm.com>
To: Christian Brauner <christian.brauner@...ntu.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 12/9/21 14:11, Christian Brauner wrote:
> 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?
I hadn't thought about inheritance or passing fds. But yes. I will adopt
your patch and extend all the files to tie them to the user namespace
they are opened in...
Thanks.
>
> 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);
>
Powered by blists - more mailing lists