[v6,17/17] ima: Setup securityfs for IMA namespace

Commit Message

Stefan Berger Dec. 10, 2021, 7:47 p.m. UTC

Setup securityfs with symlinks, directories, and files for IMA
namespacing support. The same directory structure that IMA uses on the
host is also created for the namespacing case.

The securityfs file and directory ownerships cannot be set when the
IMA namespace is initialized. Therefore, delay the setup of the file
system to a later point when securityfs is in securityfs_fill_super.

This filesystem can now be mounted as follows:

mount -t securityfs /sys/kernel/security/ /sys/kernel/security/

The following directories, symlinks, and files are then available.

$ ls -l sys/kernel/security/
total 0
lr--r--r--. 1 root root 0 Dec  2 00:18 ima -> integrity/ima
drwxr-xr-x. 3 root root 0 Dec  2 00:18 integrity

$ ls -l sys/kernel/security/ima/
total 0
-r--r-----. 1 root root 0 Dec  2 00:18 ascii_runtime_measurements
-r--r-----. 1 root root 0 Dec  2 00:18 binary_runtime_measurements
-rw-------. 1 root root 0 Dec  2 00:18 policy
-r--r-----. 1 root root 0 Dec  2 00:18 runtime_measurements_count
-r--r-----. 1 root root 0 Dec  2 00:18 violations

Signed-off-by: Stefan Berger <[e-mail]>
Signed-off-by: James Bottomley <[e-mail]>
---
 include/linux/ima.h             | 15 ++++++++++++-
 security/inode.c                |  7 +++++-
 security/integrity/ima/ima_fs.c | 40 ++++++++++++++++++++++++---------
 3 files changed, 49 insertions(+), 13 deletions(-)

Comments

Mimi Zohar Dec. 15, 2021, 9:31 p.m. UTC | #1

Hi Stefan, James,

On Fri, 2021-12-10 at 14:47 -0500, Stefan Berger wrote:
> Setup securityfs with symlinks, directories, and files for IMA
> namespacing support. The same directory structure that IMA uses on the
> host is also created for the namespacing case.
> 
> The securityfs file and directory ownerships cannot be set when the
> IMA namespace is initialized. Therefore, delay the setup of the file
> system to a later point when securityfs is in securityfs_fill_super.
> 
> This filesystem can now be mounted as follows:
> 
> mount -t securityfs /sys/kernel/security/ /sys/kernel/security/
> 
> The following directories, symlinks, and files are then available.
> 
> $ ls -l sys/kernel/security/
> total 0
> lr--r--r--. 1 root root 0 Dec  2 00:18 ima -> integrity/ima
> drwxr-xr-x. 3 root root 0 Dec  2 00:18 integrity

The ima symlink was introduced for backwards compatibilty.  Refer to
commit 0c343af8065b ("integrity: Add an integrity directory in
securityfs").  The symlink shouldn't need to be supported in IMA
namespace.

thanks,

Mimi

> 
> $ ls -l sys/kernel/security/ima/
> total 0
> -r--r-----. 1 root root 0 Dec  2 00:18 ascii_runtime_measurements
> -r--r-----. 1 root root 0 Dec  2 00:18 binary_runtime_measurements
> -rw-------. 1 root root 0 Dec  2 00:18 policy
> -r--r-----. 1 root root 0 Dec  2 00:18 runtime_measurements_count
> -r--r-----. 1 root root 0 Dec  2 00:18 violations
> 
> Signed-off-by: Stefan Berger <[e-mail]>
> Signed-off-by: James Bottomley <[e-mail]>

Stefan Berger Dec. 16, 2021, 2:41 a.m. UTC | #2

On 12/15/21 16:31, Mimi Zohar wrote:
> Hi Stefan, James,
>
> On Fri, 2021-12-10 at 14:47 -0500, Stefan Berger wrote:
>> Setup securityfs with symlinks, directories, and files for IMA
>> namespacing support. The same directory structure that IMA uses on the
>> host is also created for the namespacing case.
>>
>> The securityfs file and directory ownerships cannot be set when the
>> IMA namespace is initialized. Therefore, delay the setup of the file
>> system to a later point when securityfs is in securityfs_fill_super.
>>
>> This filesystem can now be mounted as follows:
>>
>> mount -t securityfs /sys/kernel/security/ /sys/kernel/security/
>>
>> The following directories, symlinks, and files are then available.
>>
>> $ ls -l sys/kernel/security/
>> total 0
>> lr--r--r--. 1 root root 0 Dec  2 00:18 ima -> integrity/ima
>> drwxr-xr-x. 3 root root 0 Dec  2 00:18 integrity
> The ima symlink was introduced for backwards compatibilty.  Refer to
> commit 0c343af8065b ("integrity: Add an integrity directory in
> securityfs").  The symlink shouldn't need to be supported in IMA
> namespace.

That's backwards compatibility for applications and scripts. If we want 
to have these running unmodified inside IMA namespaces I think it's 
better to keep the symbolic link and not treat the IMA namespaces any 
different than the host.

    Stefan

>
> thanks,
>
> Mimi
>
>> $ ls -l sys/kernel/security/ima/
>> total 0
>> -r--r-----. 1 root root 0 Dec  2 00:18 ascii_runtime_measurements
>> -r--r-----. 1 root root 0 Dec  2 00:18 binary_runtime_measurements
>> -rw-------. 1 root root 0 Dec  2 00:18 policy
>> -r--r-----. 1 root root 0 Dec  2 00:18 runtime_measurements_count
>> -r--r-----. 1 root root 0 Dec  2 00:18 violations
>>
>> Signed-off-by: Stefan Berger <[e-mail]>
>> Signed-off-by: James Bottomley <[e-mail]>

Patch

diff --git a/include/linux/ima.h b/include/linux/ima.h
index cc950ccbf157..5524f439d9cf 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -40,6 +40,7 @@  extern int ima_measure_critical_data(const char *event_label,
 				     const char *event_name,
 				     const void *buf, size_t buf_len,
 				     bool hash, u8 *digest, size_t digest_len);
+extern int ima_fs_ns_init(struct user_namespace *user_ns, struct dentry *root);
 
 #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
 extern void ima_appraise_parse_cmdline(void);
@@ -275,6 +276,7 @@  struct ima_namespace {
 	int valid_policy;
 
 	struct dentry *policy_dentry;
+	bool policy_dentry_removed;
 };
 
 extern struct ima_namespace init_ima_ns;
@@ -284,12 +286,17 @@  extern struct list_head ima_default_rules;
 
 void free_ima_ns(struct user_namespace *ns);
 int create_ima_ns(struct user_namespace *user_ns);
-
 static inline struct ima_namespace *get_current_ns(void)
 {
 	return current_user_ns()->ima_ns;
 }
 
+static inline int ima_securityfs_init(struct user_namespace *user_ns,
+				      struct dentry *root)
+{
+	return ima_fs_ns_init(user_ns, root);
+}
+
 #else
 
 static inline void free_ima_ns(struct user_namespace *user_ns)
@@ -308,6 +315,12 @@  static inline struct ima_namespace *get_current_ns(void)
 {
 	return &init_ima_ns;
 }
+
+static inline int ima_securityfs_init(struct user_namespace *ns, struct dentry *root)
+{
+	return 0;
+}
+
 #endif /* CONFIG_IMA_NS */
 
 #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
diff --git a/security/inode.c b/security/inode.c
index c77ae8ecc464..c9327389ad80 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -16,6 +16,7 @@ 
 #include <linux/fs_context.h>
 #include <linux/mount.h>
 #include <linux/pagemap.h>
+#include <linux/ima.h>
 #include <linux/init.h>
 #include <linux/namei.h>
 #include <linux/security.h>
@@ -41,6 +42,7 @@  static const struct super_operations securityfs_super_operations = {
 static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc)
 {
 	static const struct tree_descr files[] = {{""}};
+	struct user_namespace *ns = fc->user_ns;
 	int error;
 
 	error = simple_fill_super(sb, SECURITYFS_MAGIC, files);
@@ -49,7 +51,10 @@  static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc)
 
 	sb->s_op = &securityfs_super_operations;
 
-	return 0;
+	if (ns != &init_user_ns)
+		error = ima_securityfs_init(ns, sb->s_root);
+
+	return error;
 }
 
 static int securityfs_get_tree(struct fs_context *fc)
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 84b3bf2f1454..f1c3cde25c80 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -479,6 +479,7 @@  static int ima_release_policy(struct inode *inode, struct file *file)
 #if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
 	securityfs_remove(ns->policy_dentry);
 	ns->policy_dentry = NULL;
+	ns->policy_dentry_removed = true;
 #elif defined(CONFIG_IMA_WRITE_POLICY)
 	clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags);
 #elif defined(CONFIG_IMA_READ_POLICY)
@@ -495,21 +496,31 @@  static const struct file_operations ima_measure_policy_ops = {
 	.llseek = generic_file_llseek,
 };
 
-static int __init ima_fs_ns_init(struct user_namespace *user_ns)
+int ima_fs_ns_init(struct user_namespace *user_ns, struct dentry *root)
 {
 	struct ima_namespace *ns = user_ns->ima_ns;
-	struct dentry *ima_dir;
+	struct dentry *int_dir;
+	struct dentry *ima_dir = NULL;
 	struct dentry *ima_symlink = NULL;
 	struct dentry *binary_runtime_measurements = NULL;
 	struct dentry *ascii_runtime_measurements = NULL;
 	struct dentry *runtime_measurements_count = NULL;
 	struct dentry *violations = NULL;
 
-	ima_dir = securityfs_create_dir("ima", integrity_dir);
+	/* FIXME: update when evm and integrity are namespaced */
+	if (user_ns != &init_user_ns) {
+		int_dir =
+			securityfs_create_dir("integrity", root);
+		if (IS_ERR(int_dir))
+			return -1;
+	} else
+		int_dir = integrity_dir;
+
+	ima_dir = securityfs_create_dir("ima", int_dir);
 	if (IS_ERR(ima_dir))
-		return -1;
+		goto out;
 
-	ima_symlink = securityfs_create_symlink("ima", NULL, "integrity/ima",
+	ima_symlink = securityfs_create_symlink("ima", root, "integrity/ima",
 						NULL);
 	if (IS_ERR(ima_symlink))
 		goto out;
@@ -541,11 +552,15 @@  static int __init ima_fs_ns_init(struct user_namespace *user_ns)
 	if (IS_ERR(violations))
 		goto out;
 
-	ns->policy_dentry = securityfs_create_file("policy", POLICY_FILE_FLAGS,
-					    ima_dir, NULL,
-					    &ima_measure_policy_ops);
-	if (IS_ERR(ns->policy_dentry))
-		goto out;
+
+	if (!ns->policy_dentry_removed) {
+		ns->policy_dentry =
+		    securityfs_create_file("policy", POLICY_FILE_FLAGS,
+					   ima_dir, NULL,
+					   &ima_measure_policy_ops);
+		if (IS_ERR(ns->policy_dentry))
+			goto out;
+	}
 
 	return 0;
 out:
@@ -556,10 +571,13 @@  static int __init ima_fs_ns_init(struct user_namespace *user_ns)
 	securityfs_remove(binary_runtime_measurements);
 	securityfs_remove(ima_symlink);
 	securityfs_remove(ima_dir);
+	if (user_ns != &init_user_ns)
+		securityfs_remove(integrity_dir);
+
 	return -1;
 }
 
 int __init ima_fs_init(void)
 {
-	return ima_fs_ns_init(&init_user_ns);
+	return ima_fs_ns_init(&init_user_ns, NULL);
 }