linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/5] Fix overlayfs on EVM
@ 2019-02-11 16:53 Ignaz Forster
  2019-02-11 16:53 ` [PATCH 1/5] evm: instead of using the overlayfs i_ino, use the real i_ino Ignaz Forster
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Ignaz Forster @ 2019-02-11 16:53 UTC (permalink / raw)
  To: Mimi Zohar, linux-integrity, Miklos Szeredi, linux-unionfs
  Cc: Fabian Vogt, Ignaz Forster

From: Ignaz Forster <iforster@suse.de>

This patch series tries to solve several problems found when using
EVM on an overlay file system.

Especially patch 4 and 5 will need further discussion; patch 4 will
introduce follow up problems, patch 5 can be considered a workaround
at best.

Ignaz Forster (4):
  Rename ima_post_create_tmpfile
  Execute IMA post create hook in vfs_create
  Ignore IMA / EVM xattrs during copy_up
  Use __vfs_getxattr to get overlayfs xattrs

Mimi Zohar (1):
  evm: instead of using the overlayfs i_ino, use the real i_ino

 fs/namei.c                          |  6 ++++--
 fs/overlayfs/inode.c                |  3 ++-
 include/linux/ima.h                 |  4 ++--
 security/integrity/evm/evm_crypto.c |  3 +++
 security/integrity/evm/evm_main.c   | 23 +++++++++++++++++++++++
 security/integrity/ima/ima_main.c   | 10 +++++-----
 6 files changed, 39 insertions(+), 10 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/5] evm: instead of using the overlayfs i_ino, use the real i_ino
  2019-02-11 16:53 [RFC PATCH 0/5] Fix overlayfs on EVM Ignaz Forster
@ 2019-02-11 16:53 ` Ignaz Forster
  2019-02-11 16:53 ` [PATCH 2/5] Rename ima_post_create_tmpfile Ignaz Forster
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Ignaz Forster @ 2019-02-11 16:53 UTC (permalink / raw)
  To: Mimi Zohar, linux-integrity, Miklos Szeredi, linux-unionfs; +Cc: Fabian Vogt

From: Mimi Zohar <zohar@linux.ibm.com>

Using the overlayfs i_ino value in the HMAC calculation results in not
being able to validate the EVM HMAC.  This patch calculates the HMAC
using the real i_ino.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/evm/evm_crypto.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 43e2dc3a60d0..baddbbce6ac7 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -241,6 +241,9 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 		if (is_ima)
 			ima_present = true;
 	}
+
+	/* Use the real i_ino to calculate the HMAC */
+	inode = d_real_inode(dentry);
 	hmac_add_misc(desc, inode, type, data->digest);
 
 	/* Portable EVM signatures must include an IMA hash */
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/5] Rename ima_post_create_tmpfile
  2019-02-11 16:53 [RFC PATCH 0/5] Fix overlayfs on EVM Ignaz Forster
  2019-02-11 16:53 ` [PATCH 1/5] evm: instead of using the overlayfs i_ino, use the real i_ino Ignaz Forster
@ 2019-02-11 16:53 ` Ignaz Forster
  2019-02-11 16:53 ` [PATCH 3/5] Execute IMA post create hook in vfs_create Ignaz Forster
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Ignaz Forster @ 2019-02-11 16:53 UTC (permalink / raw)
  To: Mimi Zohar, linux-integrity, Miklos Szeredi, linux-unionfs
  Cc: Fabian Vogt, Ignaz Forster

From: Ignaz Forster <iforster@suse.de>

This function is not specific to tmpfiles, but can be used for
marking any file to be in policy. Change the name accordingly.

Signed-off-by: Ignaz Forster <iforster@suse.de>
---
 fs/namei.c                        |  2 +-
 include/linux/ima.h               |  4 ++--
 security/integrity/ima/ima_main.c | 10 +++++-----
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 373a7ec4b09d..744e89474cda 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3462,7 +3462,7 @@ struct dentry *vfs_tmpfile(struct dentry *dentry, umode_t mode, int open_flag)
 		inode->i_state |= I_LINKABLE;
 		spin_unlock(&inode->i_lock);
 	}
-	ima_post_create_tmpfile(inode);
+	ima_post_create_file(inode);
 	return child;
 
 out_err:
diff --git a/include/linux/ima.h b/include/linux/ima.h
index dc12fbcf484c..d47fe0a54efd 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -18,7 +18,7 @@ struct linux_binprm;
 #ifdef CONFIG_IMA
 extern int ima_bprm_check(struct linux_binprm *bprm);
 extern int ima_file_check(struct file *file, int mask);
-extern void ima_post_create_tmpfile(struct inode *inode);
+extern void ima_post_create_file(struct inode *inode);
 extern void ima_file_free(struct file *file);
 extern int ima_file_mmap(struct file *file, unsigned long prot);
 extern int ima_load_data(enum kernel_load_data_id id);
@@ -57,7 +57,7 @@ static inline int ima_file_check(struct file *file, int mask)
 	return 0;
 }
 
-static inline void ima_post_create_tmpfile(struct inode *inode)
+static inline void ima_post_create_file(struct inode *inode)
 {
 }
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 357edd140c09..629a2c538a7f 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -397,14 +397,14 @@ int ima_file_check(struct file *file, int mask)
 EXPORT_SYMBOL_GPL(ima_file_check);
 
 /**
- * ima_post_create_tmpfile - mark newly created tmpfile as new
- * @file : newly created tmpfile
+ * ima_post_create_file - mark newly created file as new
+ * @file : newly created file
  *
- * No measuring, appraising or auditing of newly created tmpfiles is needed.
+ * No measuring, appraising or auditing of newly created files is needed.
  * Skip calling process_measurement(), but indicate which newly, created
- * tmpfiles are in policy.
+ * files are in policy.
  */
-void ima_post_create_tmpfile(struct inode *inode)
+void ima_post_create_file(struct inode *inode)
 {
 	struct integrity_iint_cache *iint;
 	int must_appraise;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/5] Execute IMA post create hook in vfs_create
  2019-02-11 16:53 [RFC PATCH 0/5] Fix overlayfs on EVM Ignaz Forster
  2019-02-11 16:53 ` [PATCH 1/5] evm: instead of using the overlayfs i_ino, use the real i_ino Ignaz Forster
  2019-02-11 16:53 ` [PATCH 2/5] Rename ima_post_create_tmpfile Ignaz Forster
@ 2019-02-11 16:53 ` Ignaz Forster
  2019-02-11 16:53 ` [PATCH 4/5] Ignore IMA / EVM xattrs during copy_up Ignaz Forster
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Ignaz Forster @ 2019-02-11 16:53 UTC (permalink / raw)
  To: Mimi Zohar, linux-integrity, Miklos Szeredi, linux-unionfs
  Cc: Fabian Vogt, Ignaz Forster, Fabian Vogt

From: Ignaz Forster <iforster@suse.de>

When creating a new file on overlayfs, the file can not be accessed
due to missing security.ima / security.evm xattrs, as the creation of
the required hashes is never triggered.

Similarly to the existing handling of tmpfiles, trigger file hash
generation by calling ima_post_create_file.

Co-developed-by: Fabian Vogt <fvogt@suse.de>
Signed-off-by: Fabian Vogt <fvogt@suse.de>
Signed-off-by: Ignaz Forster <iforster@suse.de>
---
 fs/namei.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index 744e89474cda..3b4021e4fc32 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2910,8 +2910,10 @@ int vfs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
 	if (error)
 		return error;
 	error = dir->i_op->create(dir, dentry, mode, want_excl);
-	if (!error)
+	if (!error) {
 		fsnotify_create(dir, dentry);
+		ima_post_create_file(dentry->d_inode);
+	}
 	return error;
 }
 EXPORT_SYMBOL(vfs_create);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/5] Ignore IMA / EVM xattrs during copy_up
  2019-02-11 16:53 [RFC PATCH 0/5] Fix overlayfs on EVM Ignaz Forster
                   ` (2 preceding siblings ...)
  2019-02-11 16:53 ` [PATCH 3/5] Execute IMA post create hook in vfs_create Ignaz Forster
@ 2019-02-11 16:53 ` Ignaz Forster
  2019-02-12  2:55   ` Amir Goldstein
  2019-02-11 16:53 ` [PATCH 5/5] Use __vfs_getxattr to get overlayfs xattrs Ignaz Forster
  2019-02-12  3:29 ` [RFC PATCH 0/5] Fix overlayfs on EVM Amir Goldstein
  5 siblings, 1 reply; 13+ messages in thread
From: Ignaz Forster @ 2019-02-11 16:53 UTC (permalink / raw)
  To: Mimi Zohar, linux-integrity, Miklos Szeredi, linux-unionfs
  Cc: Fabian Vogt, Ignaz Forster, Fabian Vogt

From: Ignaz Forster <iforster@suse.de>

EVM tries to protect these attributes during copy_up, resulting in
the failure of the copy_up operation.

This patch will skip the attributes (similar to selinux) to allow
for later recalculation. This, however, introduces another problem:
As overlayfs does not check the file integrity on copy_up, files with
an invalid hash will suddenly become valid again after the copy_up
operation.

Co-developed-by: Fabian Vogt <fvogt@suse.de>
Signed-off-by: Fabian Vogt <fvogt@suse.de>
Signed-off-by: Ignaz Forster <iforster@suse.de>
---
 security/integrity/evm/evm_main.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 5ecaa3d6fe0b..e7667f1159c9 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -23,6 +23,7 @@
 #include <linux/integrity.h>
 #include <linux/evm.h>
 #include <linux/magic.h>
+#include <linux/lsm_hooks.h>
 
 #include <crypto/hash.h>
 #include <crypto/hash_info.h>
@@ -548,6 +549,22 @@ int evm_inode_init_security(struct inode *inode,
 }
 EXPORT_SYMBOL_GPL(evm_inode_init_security);
 
+static int evm_inode_copy_up_xattr(const char *name)
+{
+	/* The copy_up hook above sets the initial context on an inode, but we
+	 * don't then want to overwrite it by blindly copying all the lower
+	 * xattrs up.  Instead, we have to filter out SELinux-related xattrs.
+	 */
+	if (strcmp(name, XATTR_NAME_EVM) == 0 ||
+			strcmp(name, XATTR_NAME_IMA) == 0)
+		return 1; /* Discard */
+	/*
+	 * Any other attribute apart from SELINUX is not claimed, supported
+	 * by selinux.
+	 */
+	return -EOPNOTSUPP;
+}
+
 #ifdef CONFIG_EVM_LOAD_X509
 void __init evm_load_x509(void)
 {
@@ -559,6 +576,10 @@ void __init evm_load_x509(void)
 }
 #endif
 
+static struct security_hook_list evm_hooks[] __lsm_ro_after_init = {
+	LSM_HOOK_INIT(inode_copy_up_xattr, evm_inode_copy_up_xattr)
+};
+
 static int __init init_evm(void)
 {
 	int error;
@@ -567,6 +588,8 @@ static int __init init_evm(void)
 
 	evm_init_config();
 
+	security_add_hooks(evm_hooks, ARRAY_SIZE(evm_hooks), "evm");
+
 	error = integrity_init_keyring(INTEGRITY_KEYRING_EVM);
 	if (error)
 		goto error;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 5/5] Use __vfs_getxattr to get overlayfs xattrs
  2019-02-11 16:53 [RFC PATCH 0/5] Fix overlayfs on EVM Ignaz Forster
                   ` (3 preceding siblings ...)
  2019-02-11 16:53 ` [PATCH 4/5] Ignore IMA / EVM xattrs during copy_up Ignaz Forster
@ 2019-02-11 16:53 ` Ignaz Forster
  2019-02-12  3:29 ` [RFC PATCH 0/5] Fix overlayfs on EVM Amir Goldstein
  5 siblings, 0 replies; 13+ messages in thread
From: Ignaz Forster @ 2019-02-11 16:53 UTC (permalink / raw)
  To: Mimi Zohar, linux-integrity, Miklos Szeredi, linux-unionfs
  Cc: Fabian Vogt, Ignaz Forster, Fabian Vogt

From: Ignaz Forster <iforster@suse.de>

In vfs_getxattr a query of "security.selinux" will return
"unlabeled", while in __vfs_getxattr -ENODATA will be returned for
the same query. This causes a difference in the generated EVM hashes
for the file on the underlying file system and overlayfs.

Circumvent this by calling __vfs_getxattr directly.

Co-developed-by: Fabian Vogt <fvogt@suse.de>
Signed-off-by: Fabian Vogt <fvogt@suse.de>
Signed-off-by: Ignaz Forster <iforster@suse.de>
---
 fs/overlayfs/inode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 3b7ed5d2279c..e2c737936576 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -374,7 +374,8 @@ int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
 		ovl_i_dentry_upper(inode) ?: ovl_dentry_lower(dentry);
 
 	old_cred = ovl_override_creds(dentry->d_sb);
-	res = vfs_getxattr(realdentry, name, value, size);
+	res = __vfs_getxattr(realdentry, d_backing_inode(realdentry),
+			name, value, size);
 	revert_creds(old_cred);
 	return res;
 }
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/5] Ignore IMA / EVM xattrs during copy_up
  2019-02-11 16:53 ` [PATCH 4/5] Ignore IMA / EVM xattrs during copy_up Ignaz Forster
@ 2019-02-12  2:55   ` Amir Goldstein
  0 siblings, 0 replies; 13+ messages in thread
From: Amir Goldstein @ 2019-02-12  2:55 UTC (permalink / raw)
  To: Ignaz Forster
  Cc: Mimi Zohar, linux-integrity, Miklos Szeredi, overlayfs,
	Fabian Vogt, Ignaz Forster, Fabian Vogt

On Mon, Feb 11, 2019 at 10:27 PM Ignaz Forster <iforster@suse.com> wrote:
>
> From: Ignaz Forster <iforster@suse.de>
>
> EVM tries to protect these attributes during copy_up, resulting in
> the failure of the copy_up operation.
>
> This patch will skip the attributes (similar to selinux) to allow
> for later recalculation. This, however, introduces another problem:
> As overlayfs does not check the file integrity on copy_up, files with
> an invalid hash will suddenly become valid again after the copy_up
> operation.
>

"overlayfs doesn't check" means the check is to high in vfs and
needs to move to lower vfs layer?
Same as fixes in vfs_create and vfs_tmpfile.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 0/5] Fix overlayfs on EVM
  2019-02-11 16:53 [RFC PATCH 0/5] Fix overlayfs on EVM Ignaz Forster
                   ` (4 preceding siblings ...)
  2019-02-11 16:53 ` [PATCH 5/5] Use __vfs_getxattr to get overlayfs xattrs Ignaz Forster
@ 2019-02-12  3:29 ` Amir Goldstein
  2019-02-12 10:55   ` Fabian Vogt
  5 siblings, 1 reply; 13+ messages in thread
From: Amir Goldstein @ 2019-02-12  3:29 UTC (permalink / raw)
  To: Ignaz Forster
  Cc: Mimi Zohar, linux-integrity, Miklos Szeredi, overlayfs,
	Fabian Vogt, Ignaz Forster

On Mon, Feb 11, 2019 at 10:28 PM Ignaz Forster <iforster@suse.com> wrote:
>
> From: Ignaz Forster <iforster@suse.de>
>
> This patch series tries to solve several problems found when using
> EVM on an overlay file system.
>
> Especially patch 4 and 5 will need further discussion; patch 4 will
> introduce follow up problems, patch 5 can be considered a workaround
> at best.
>

I think maybe the entire series (short of vfs_create hook) is a workaround.

Let's stop and think. What is IMA/EVM meant to do?
Provide tamper proof protection for persistent storage. Right?

Overlayfs uses other filesystems to store persistent data/metadata,
so if IMA/EVM already protect those other filesystems, we got
tamper proof already don't we?

The issue with overlayfs and security hooks is often credentials
because underlying filesystems are accessed with different
credentials (mounter credentials) than the overlay file access.
Is that an issue for IMA/EVM?
Or is it an issue that IMA policy is path based and may only
apply to the overlay path and not underlying path??

I had already suggested once to mark overlay inodes with
NOIMA flag:
https://marc.info/?l=linux-unionfs&m=154529013929472&w=2
and there was a similar suggestion for FUSE:
https://marc.info/?l=linux-fsdevel&m=151871326115716&w=2

If my assumptions so far are correct, then the effort for making
IMA/EVM work with overlayfs should focus around finding the
places where overlayfs uses lower level vfs interface (often
vfs_xxx helpers) and make sure that the IMA hooks are place
in those lower vfs interfaces, just like vfs_create() patch does
and like vfs_tmpfile() patch did before it.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 0/5] Fix overlayfs on EVM
  2019-02-12  3:29 ` [RFC PATCH 0/5] Fix overlayfs on EVM Amir Goldstein
@ 2019-02-12 10:55   ` Fabian Vogt
  2019-02-12 13:47     ` Amir Goldstein
  0 siblings, 1 reply; 13+ messages in thread
From: Fabian Vogt @ 2019-02-12 10:55 UTC (permalink / raw)
  To: Amir Goldstein, Mimi Zohar
  Cc: linux-integrity, Miklos Szeredi, overlayfs, Ignaz Forster

Hi,

Am Dienstag, 12. Februar 2019, 04:29:30 CET schrieb Amir Goldstein:
> On Mon, Feb 11, 2019 at 10:28 PM Ignaz Forster <iforster@suse.com> wrote:
> >
> > From: Ignaz Forster <iforster@suse.de>
> >
> > This patch series tries to solve several problems found when using
> > EVM on an overlay file system.
> >
> > Especially patch 4 and 5 will need further discussion; patch 4 will
> > introduce follow up problems, patch 5 can be considered a workaround
> > at best.
> >
> 
> I think maybe the entire series (short of vfs_create hook) is a workaround.

Correct.

> Let's stop and think. What is IMA/EVM meant to do?
> Provide tamper proof protection for persistent storage. Right?
> 
> Overlayfs uses other filesystems to store persistent data/metadata,
> so if IMA/EVM already protect those other filesystems, we got
> tamper proof already don't we?

Yup. There are two approaches overlayfs can interface with IMA/EVM:
a) (vfs) -> IMA/EVM -> overlayfs -> filesystem
b) (vfs) -> overlayfs -> IMA/EVM -> filesystem

Both are from my view equally valid, just with different issues.
Currently the first approach is used, as all IMA/EVM and security hooks
are invoked from the "high-level" vfs functions and overlayfs calls
the lower functions to interface with the underlying filesystems, skipping
all those checks/processes.

Doing b) has the disadvantage that overlayfs needs to become IMA/EVM-aware
(or use the high-level functions for everything) and special care needs to
be taken to a) do IMA/EVM work properly b) not do it twice.
It would avoid an entire class of IMA/EVM issues though, by only doing work
on the actual filesystem nodes and not on the virtual overlayfs ones.

> The issue with overlayfs and security hooks is often credentials
> because underlying filesystems are accessed with different
> credentials (mounter credentials) than the overlay file access.
> Is that an issue for IMA/EVM?

I'm not familiar enough with IMA/EVM internals to comment here, but I assume
that IMA/EVM is only invoked after a related operation succeeded, e.g.
security.ima is only updated after a successful write. So this shouldn't be
an issue (?). We could do some tests, but I'm not sure what we would need to
look out for.

> Or is it an issue that IMA policy is path based and may only
> apply to the overlay path and not underlying path??

I'm not quite sure what you mean by that - I've never seen any paths involved
in the IMA policy (/sys/kernel/security/ima/policy).

> I had already suggested once to mark overlay inodes with
> NOIMA flag:
> https://marc.info/?l=linux-unionfs&m=154529013929472&w=2
> and there was a similar suggestion for FUSE:
> https://marc.info/?l=linux-fsdevel&m=151871326115716&w=2

That explains why it was also broken with unionfs when I tried that briefly,
I expected that FUSE mounts would just ignore IMA and due to access from
userspace work properly with IMA/EVM OOTB as well - apparently not yet.

> If my assumptions so far are correct, then the effort for making
> IMA/EVM work with overlayfs should focus around finding the
> places where overlayfs uses lower level vfs interface (often
> vfs_xxx helpers) and make sure that the IMA hooks are place
> in those lower vfs interfaces, just like vfs_create() patch does
> and like vfs_tmpfile() patch did before it.

So basically turning on NOIMA for overlayfs while ensuring that integrity
checks and operations still perform as expected?

Thanks,
Fabian

> Thanks,
> Amir.



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 0/5] Fix overlayfs on EVM
  2019-02-12 10:55   ` Fabian Vogt
@ 2019-02-12 13:47     ` Amir Goldstein
  2019-02-12 22:51       ` Mimi Zohar
  0 siblings, 1 reply; 13+ messages in thread
From: Amir Goldstein @ 2019-02-12 13:47 UTC (permalink / raw)
  To: Fabian Vogt
  Cc: Mimi Zohar, linux-integrity, Miklos Szeredi, overlayfs, Ignaz Forster

On Tue, Feb 12, 2019 at 12:55 PM Fabian Vogt <fvogt@suse.de> wrote:
>
> Hi,
>
> Am Dienstag, 12. Februar 2019, 04:29:30 CET schrieb Amir Goldstein:
> > On Mon, Feb 11, 2019 at 10:28 PM Ignaz Forster <iforster@suse.com> wrote:
> > >
> > > From: Ignaz Forster <iforster@suse.de>
> > >
> > > This patch series tries to solve several problems found when using
> > > EVM on an overlay file system.
> > >
> > > Especially patch 4 and 5 will need further discussion; patch 4 will
> > > introduce follow up problems, patch 5 can be considered a workaround
> > > at best.
> > >
> >
> > I think maybe the entire series (short of vfs_create hook) is a workaround.
>
> Correct.
>
> > Let's stop and think. What is IMA/EVM meant to do?
> > Provide tamper proof protection for persistent storage. Right?
> >
> > Overlayfs uses other filesystems to store persistent data/metadata,
> > so if IMA/EVM already protect those other filesystems, we got
> > tamper proof already don't we?
>
> Yup. There are two approaches overlayfs can interface with IMA/EVM:
> a) (vfs) -> IMA/EVM -> overlayfs -> filesystem
> b) (vfs) -> overlayfs -> IMA/EVM -> filesystem
>
> Both are from my view equally valid, just with different issues.
> Currently the first approach is used, as all IMA/EVM and security hooks
> are invoked from the "high-level" vfs functions and overlayfs calls
> the lower functions to interface with the underlying filesystems, skipping
> all those checks/processes.
>
> Doing b) has the disadvantage that overlayfs needs to become IMA/EVM-aware
> (or use the high-level functions for everything) and special care needs to
> be taken to a) do IMA/EVM work properly b) not do it twice.
> It would avoid an entire class of IMA/EVM issues though, by only doing work
> on the actual filesystem nodes and not on the virtual overlayfs ones.
>

overlayfs doesn't need to become IMA aware.
It's mostly the other way around.
And BTW, overlay is not the only code calling vfs_xxx helpers
knfsd does that as well.

The fact that nfsd_open() has a ima_ hook means IMA hooks
are not low enough in vfs.

> > The issue with overlayfs and security hooks is often credentials
> > because underlying filesystems are accessed with different
> > credentials (mounter credentials) than the overlay file access.
> > Is that an issue for IMA/EVM?
>
> I'm not familiar enough with IMA/EVM internals to comment here, but I assume
> that IMA/EVM is only invoked after a related operation succeeded, e.g.
> security.ima is only updated after a successful write. So this shouldn't be
> an issue (?). We could do some tests, but I'm not sure what we would need to
> look out for.
>
> > Or is it an issue that IMA policy is path based and may only
> > apply to the overlay path and not underlying path??
>
> I'm not quite sure what you mean by that - I've never seen any paths involved
> in the IMA policy (/sys/kernel/security/ima/policy).
>

I've never seen an IMA policy, but I read that IMA policy can be per file,
so the underlying filesystem files must be enforced by IMA policy.

> > I had already suggested once to mark overlay inodes with
> > NOIMA flag:
> > https://marc.info/?l=linux-unionfs&m=154529013929472&w=2
> > and there was a similar suggestion for FUSE:
> > https://marc.info/?l=linux-fsdevel&m=151871326115716&w=2
>
> That explains why it was also broken with unionfs when I tried that briefly,
> I expected that FUSE mounts would just ignore IMA and due to access from
> userspace work properly with IMA/EVM OOTB as well - apparently not yet.
>
> > If my assumptions so far are correct, then the effort for making
> > IMA/EVM work with overlayfs should focus around finding the
> > places where overlayfs uses lower level vfs interface (often
> > vfs_xxx helpers) and make sure that the IMA hooks are place
> > in those lower vfs interfaces, just like vfs_create() patch does
> > and like vfs_tmpfile() patch did before it.
>
> So basically turning on NOIMA for overlayfs while ensuring that integrity
> checks and operations still perform as expected?
>

Yes.
As far as IMA is concerned, Overlayfs is like a filesystem user from kernel.
Very similar to knfsd in that respect.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 0/5] Fix overlayfs on EVM
  2019-02-12 13:47     ` Amir Goldstein
@ 2019-02-12 22:51       ` Mimi Zohar
  2019-02-13  8:05         ` Fabian Vogt
  0 siblings, 1 reply; 13+ messages in thread
From: Mimi Zohar @ 2019-02-12 22:51 UTC (permalink / raw)
  To: Amir Goldstein, Fabian Vogt
  Cc: linux-integrity, Miklos Szeredi, overlayfs, Ignaz Forster


> > > If my assumptions so far are correct, then the effort for making
> > > IMA/EVM work with overlayfs should focus around finding the
> > > places where overlayfs uses lower level vfs interface (often
> > > vfs_xxx helpers) and make sure that the IMA hooks are place
> > > in those lower vfs interfaces, just like vfs_create() patch does
> > > and like vfs_tmpfile() patch did before it.
> >
> > So basically turning on NOIMA for overlayfs while ensuring that integrity
> > checks and operations still perform as expected?
> >
> 
> Yes.
> As far as IMA is concerned, Overlayfs is like a filesystem user from kernel.
> Very similar to knfsd in that respect.

Fabian, if you're thinking of disabling IMA-appraisal on overlay filesystems, 
have you tried defining an appraise policy rule based on the overlayfs
magic number (eg. dont_appraise fsmagic=0x794c7630)?

Mimi


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 0/5] Fix overlayfs on EVM
  2019-02-12 22:51       ` Mimi Zohar
@ 2019-02-13  8:05         ` Fabian Vogt
  2019-02-13  9:13           ` Amir Goldstein
  0 siblings, 1 reply; 13+ messages in thread
From: Fabian Vogt @ 2019-02-13  8:05 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Amir Goldstein, linux-integrity, Miklos Szeredi, overlayfs,
	Ignaz Forster

Hi,

Am Dienstag, 12. Februar 2019, 23:51:37 CET schrieb Mimi Zohar:
> 
> > > > If my assumptions so far are correct, then the effort for making
> > > > IMA/EVM work with overlayfs should focus around finding the
> > > > places where overlayfs uses lower level vfs interface (often
> > > > vfs_xxx helpers) and make sure that the IMA hooks are place
> > > > in those lower vfs interfaces, just like vfs_create() patch does
> > > > and like vfs_tmpfile() patch did before it.
> > >
> > > So basically turning on NOIMA for overlayfs while ensuring that integrity
> > > checks and operations still perform as expected?
> > 
> > Yes.
> > As far as IMA is concerned, Overlayfs is like a filesystem user from kernel.
> > Very similar to knfsd in that respect.
> 
> Fabian, if you're thinking of disabling IMA-appraisal on overlay filesystems, 
> have you tried defining an appraise policy rule based on the overlayfs
> magic number (eg. dont_appraise fsmagic=0x794c7630)?

Yes, that was one of the first approaches we tested - basically switching from
a) to b) using configuration. It didn't work: Then IMA was completely
circumvented and neither were hashes updated for changed files nor were they
checked on access. That was a few months ago though, so it might have changed.

Cheers,
Fabian

> Mimi




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 0/5] Fix overlayfs on EVM
  2019-02-13  8:05         ` Fabian Vogt
@ 2019-02-13  9:13           ` Amir Goldstein
  0 siblings, 0 replies; 13+ messages in thread
From: Amir Goldstein @ 2019-02-13  9:13 UTC (permalink / raw)
  To: Fabian Vogt
  Cc: Mimi Zohar, linux-integrity, Miklos Szeredi, overlayfs, Ignaz Forster

On Wed, Feb 13, 2019 at 10:05 AM Fabian Vogt <fvogt@suse.de> wrote:
>
> Hi,
>
> Am Dienstag, 12. Februar 2019, 23:51:37 CET schrieb Mimi Zohar:
> >
> > > > > If my assumptions so far are correct, then the effort for making
> > > > > IMA/EVM work with overlayfs should focus around finding the
> > > > > places where overlayfs uses lower level vfs interface (often
> > > > > vfs_xxx helpers) and make sure that the IMA hooks are place
> > > > > in those lower vfs interfaces, just like vfs_create() patch does
> > > > > and like vfs_tmpfile() patch did before it.
> > > >
> > > > So basically turning on NOIMA for overlayfs while ensuring that integrity
> > > > checks and operations still perform as expected?
> > >
> > > Yes.
> > > As far as IMA is concerned, Overlayfs is like a filesystem user from kernel.
> > > Very similar to knfsd in that respect.
> >
> > Fabian, if you're thinking of disabling IMA-appraisal on overlay filesystems,
> > have you tried defining an appraise policy rule based on the overlayfs
> > magic number (eg. dont_appraise fsmagic=0x794c7630)?
>
> Yes, that was one of the first approaches we tested - basically switching from
> a) to b) using configuration. It didn't work: Then IMA was completely
> circumvented and neither were hashes updated for changed files nor were they
> checked on access. That was a few months ago though, so it might have changed.
>

Not very surprising considering the missing IMA hooks in level of
vfs_create() and vfs_tmpfile().

Here is a partial list of vfs interfaces used by overlayfs for you to audit:

For copy up:
dentry_open()
do_clone_file_range()
do_splice_direct()
notify_change()

For file open (since kernel v4.19):
open_with_fake_path()

All around:
git grep "vfs_" fs/overlayfs/

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-02-13  9:14 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-11 16:53 [RFC PATCH 0/5] Fix overlayfs on EVM Ignaz Forster
2019-02-11 16:53 ` [PATCH 1/5] evm: instead of using the overlayfs i_ino, use the real i_ino Ignaz Forster
2019-02-11 16:53 ` [PATCH 2/5] Rename ima_post_create_tmpfile Ignaz Forster
2019-02-11 16:53 ` [PATCH 3/5] Execute IMA post create hook in vfs_create Ignaz Forster
2019-02-11 16:53 ` [PATCH 4/5] Ignore IMA / EVM xattrs during copy_up Ignaz Forster
2019-02-12  2:55   ` Amir Goldstein
2019-02-11 16:53 ` [PATCH 5/5] Use __vfs_getxattr to get overlayfs xattrs Ignaz Forster
2019-02-12  3:29 ` [RFC PATCH 0/5] Fix overlayfs on EVM Amir Goldstein
2019-02-12 10:55   ` Fabian Vogt
2019-02-12 13:47     ` Amir Goldstein
2019-02-12 22:51       ` Mimi Zohar
2019-02-13  8:05         ` Fabian Vogt
2019-02-13  9:13           ` Amir Goldstein

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).