[RFC PATCH v2 0/2] ima: Fix detection of read/write violations on stacked filesystems

[RFC PATCH v2 0/2] ima: Fix detection of read/write violations on stacked filesystems

[Stefan Berger]

This series fixes the detection of read/write violations on stacked
filesystems. To be able to access the relevant dentries necessary to
detect files opened for writing on a stacked filesystem a new d_real_type
D_REAL_FILEDATA is introduced that allows callers to access all relevant
files involved in a stacked filesystem while traversing the layers.

  Stefan

v2:
 - Simplified 2nd patch
 - Improvements on patch description on 1st patch

Stefan Berger (2):
  ovl: Define D_REAL_FILEDATA for d_real to return dentry with data
  ima: Fix detection of read/write violations on stacked filesystems

 fs/overlayfs/super.c              |  6 ++++++
 include/linux/dcache.h            |  1 +
 security/integrity/ima/ima_main.c | 21 ++++++++++++++++-----
 3 files changed, 23 insertions(+), 5 deletions(-)

-- 
2.43.0

[RFC PATCH v2 1/2] ovl: Define D_REAL_FILEDATA for d_real to return dentry with data

[Stefan Berger]

Define D_REAL_FILEDATA which is to be used as a parameter for d_real()
to return the dentry that is holding the file data, which is either the
upper or the lower denry. The caller is expected to call d_real() again
on the returned dentry to get to lower layers of a stacked filesystem,
if available. This allows a caller to get all dentries involved in hold
a file's data and iterate through the layers.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 fs/overlayfs/super.c   | 6 ++++++
 include/linux/dcache.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 06a231970cb5..f466ad89b005 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -36,6 +36,7 @@ static struct dentry *ovl_d_real(struct dentry *dentry, enum d_real_type type)
 	switch (type) {
 	case D_REAL_DATA:
 	case D_REAL_METADATA:
+	case D_REAL_FILEDATA:
 		break;
 	default:
 		goto bug;
@@ -47,6 +48,11 @@ static struct dentry *ovl_d_real(struct dentry *dentry, enum d_real_type type)
 	}
 
 	upper = ovl_dentry_upper(dentry);
+	if (type == D_REAL_FILEDATA) {
+		if (ovl_has_upperdata(d_inode(dentry)))
+			return upper;
+		return ovl_dentry_lower(dentry);
+	}
 	if (upper && (type == D_REAL_METADATA ||
 		      ovl_has_upperdata(d_inode(dentry))))
 		return upper;
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index bf53e3894aae..e4e54fb2cf4e 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -128,6 +128,7 @@ enum dentry_d_lock_class
 enum d_real_type {
 	D_REAL_DATA,
 	D_REAL_METADATA,
+	D_REAL_FILEDATA,
 };
 
 struct dentry_operations {
-- 
2.43.0

Re: [RFC PATCH v2 1/2] ovl: Define D_REAL_FILEDATA for d_real to return dentry with data

[Amir Goldstein]

On Mon, Apr 22, 2024 at 6:07 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
> Define D_REAL_FILEDATA which is to be used as a parameter for d_real()
> to return the dentry that is holding the file data, which is either the
> upper or the lower denry. The caller is expected to call d_real() again
> on the returned dentry to get to lower layers of a stacked filesystem,
> if available. This allows a caller to get all dentries involved in hold
> a file's data and iterate through the layers.
>

Did you miss my comments on v1?
https://lore.kernel.org/linux-unionfs/CAOQ4uxgNRYi-mYo=LZ5yiWch2zwDeTG2q9ZYD0ysEN6XaJkVhw@mail.gmail.com/

Did you not understand them?
Or did you just decide to ignore them?

Thanks,
Amir.

> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  fs/overlayfs/super.c   | 6 ++++++
>  include/linux/dcache.h | 1 +
>  2 files changed, 7 insertions(+)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 06a231970cb5..f466ad89b005 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -36,6 +36,7 @@ static struct dentry *ovl_d_real(struct dentry *dentry, enum d_real_type type)
>         switch (type) {
>         case D_REAL_DATA:
>         case D_REAL_METADATA:
> +       case D_REAL_FILEDATA:
>                 break;
>         default:
>                 goto bug;
> @@ -47,6 +48,11 @@ static struct dentry *ovl_d_real(struct dentry *dentry, enum d_real_type type)
>         }
>
>         upper = ovl_dentry_upper(dentry);
> +       if (type == D_REAL_FILEDATA) {
> +               if (ovl_has_upperdata(d_inode(dentry)))
> +                       return upper;
> +               return ovl_dentry_lower(dentry);
> +       }
>         if (upper && (type == D_REAL_METADATA ||
>                       ovl_has_upperdata(d_inode(dentry))))
>                 return upper;
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index bf53e3894aae..e4e54fb2cf4e 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -128,6 +128,7 @@ enum dentry_d_lock_class
>  enum d_real_type {
>         D_REAL_DATA,
>         D_REAL_METADATA,
> +       D_REAL_FILEDATA,
>  };
>
>  struct dentry_operations {
> --
> 2.43.0
>

[RFC PATCH v2 2/2] ima: Fix detection of read/write violations on stacked filesystems

[Stefan Berger]

On a stacked filesystem, when one process opens the file holding a file's
data (e.g., on upper or lower layer on overlayfs) then issue a violation
when another process opens the file for reading on the top layer (overlay
layer on overlayfs). This then provides similar behavior to the existing
case where a violation is generated when one process opens a file for
writing and another one opens the same file for reading.

Convert the current code so that it can handle the normal case as well
as the stacked filesystem case. Therefore, use d_real with parameter
D_REAL_FILEDATA to get the next dentry holding the file data. On a normal
filesystem this would be the dentry of the file and on a stacked filesystem
this could be an upper or lower dentry. Check the dentry's inode for
writes and if it has any issue the violation. Otherwise continue onto the
next dentry given the current dentry by again calling d_real. On a normal
filesystem this would return the same dentry as before and on a stacked
filesystem it would return the next-level dentry, so either the upper
or lower dentry of the next lower layer.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 security/integrity/ima/ima_main.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index f04f43af651c..7d727c448dc7 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -122,7 +122,9 @@ static void ima_rdwr_violation_check(struct file *file,
 				     char *filename)
 {
 	struct inode *inode = file_inode(file);
+	struct dentry *fd_dentry, *d;
 	fmode_t mode = file->f_mode;
+	struct inode *fd_inode;
 	bool send_tomtou = false, send_writers = false;
 
 	if (mode & FMODE_WRITE) {
@@ -134,11 +136,20 @@ static void ima_rdwr_violation_check(struct file *file,
 						&iint->atomic_flags))
 				send_tomtou = true;
 		}
-	} else {
-		if (must_measure)
-			set_bit(IMA_MUST_MEASURE, &iint->atomic_flags);
-		if (inode_is_open_for_write(inode) && must_measure)
-			send_writers = true;
+	} else if (must_measure) {
+		set_bit(IMA_MUST_MEASURE, &iint->atomic_flags);
+
+		d = d_real(file_dentry(file), D_REAL_FILEDATA);
+		do {
+			fd_dentry = d;
+			fd_inode = d_inode(fd_dentry);
+			if (inode_is_open_for_write(fd_inode)) {
+				send_writers = true;
+				break;
+			}
+			/* next layer of stacked fs */
+			d = d_real(fd_dentry, D_REAL_FILEDATA);
+		} while (d != fd_dentry);
 	}
 
 	if (!send_tomtou && !send_writers)
-- 
2.43.0

Re: [RFC PATCH v2 2/2] ima: Fix detection of read/write violations on stacked filesystems

[Roberto Sassu]

On Mon, 2024-04-22 at 11:06 -0400, Stefan Berger wrote:
> On a stacked filesystem, when one process opens the file holding a file's
> data (e.g., on upper or lower layer on overlayfs) then issue a violation
> when another process opens the file for reading on the top layer (overlay
> layer on overlayfs). This then provides similar behavior to the existing
> case where a violation is generated when one process opens a file for
> writing and another one opens the same file for reading.
> 
> Convert the current code so that it can handle the normal case as well
> as the stacked filesystem case. Therefore, use d_real with parameter
> D_REAL_FILEDATA to get the next dentry holding the file data. On a normal
> filesystem this would be the dentry of the file and on a stacked filesystem
> this could be an upper or lower dentry. Check the dentry's inode for
> writes and if it has any issue the violation. Otherwise continue onto the
> next dentry given the current dentry by again calling d_real. On a normal
> filesystem this would return the same dentry as before and on a stacked
> filesystem it would return the next-level dentry, so either the upper
> or lower dentry of the next lower layer.

I have a question. What happens for the opposite, when you open an
overlayfs file for write, and it was already opened and measured in the
lower layers?

If IMA was invoked for the lower layers, the ToMToU detection would be
detected. But it does not seem the case:

struct file *backing_file_open(const struct path *user_path, int flags,
			       const struct path *real_path,
			       const struct cred *cred)
{

[...]

	error = vfs_open(real_path, f);
	if (error) {
		fput(f);
		f = ERR_PTR(error);
	}

	return f;

security_file_post_open(), which invokes IMA, is called after
vfs_open() and only in do_open().

Also, consider that ima_iint_get() and ima_rdwr_violation_check() are
called with the inode lock held. If you access different inodes, the
locking scheme changes.

Thanks

Roberto

> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  security/integrity/ima/ima_main.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index f04f43af651c..7d727c448dc7 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -122,7 +122,9 @@ static void ima_rdwr_violation_check(struct file *file,
>  				     char *filename)
>  {
>  	struct inode *inode = file_inode(file);
> +	struct dentry *fd_dentry, *d;
>  	fmode_t mode = file->f_mode;
> +	struct inode *fd_inode;
>  	bool send_tomtou = false, send_writers = false;
>  
>  	if (mode & FMODE_WRITE) {
> @@ -134,11 +136,20 @@ static void ima_rdwr_violation_check(struct file *file,
>  						&iint->atomic_flags))
>  				send_tomtou = true;
>  		}
> -	} else {
> -		if (must_measure)
> -			set_bit(IMA_MUST_MEASURE, &iint->atomic_flags);
> -		if (inode_is_open_for_write(inode) && must_measure)
> -			send_writers = true;
> +	} else if (must_measure) {
> +		set_bit(IMA_MUST_MEASURE, &iint->atomic_flags);
> +
> +		d = d_real(file_dentry(file), D_REAL_FILEDATA);
> +		do {
> +			fd_dentry = d;
> +			fd_inode = d_inode(fd_dentry);
> +			if (inode_is_open_for_write(fd_inode)) {
> +				send_writers = true;
> +				break;
> +			}
> +			/* next layer of stacked fs */
> +			d = d_real(fd_dentry, D_REAL_FILEDATA);
> +		} while (d != fd_dentry);
>  	}
>  
>  	if (!send_tomtou && !send_writers)

Re: [RFC PATCH v2 0/2] ima: Fix detection of read/write violations on stacked filesystems

[Amir Goldstein]

On Mon, Apr 22, 2024 at 6:07 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
> This series fixes the detection of read/write violations on stacked
> filesystems. To be able to access the relevant dentries necessary to
> detect files opened for writing on a stacked filesystem a new d_real_type
> D_REAL_FILEDATA is introduced that allows callers to access all relevant
> files involved in a stacked filesystem while traversing the layers.
>

Stefan,

Both Miklos and myself objected to this solution:
https://lore.kernel.org/linux-unionfs/CAJfpeguctirEYECoigcAsJwpGPCX2NyfMZ8H8GHGW-0UyKfjgg@mail.gmail.com/

Not sure what you are hoping to achieve from re-posting the same solution.

I stopped counting how many times I already argued that *all* IMA/EVM
assertions,
including rw-ro violations should be enforced only on the real inode.
I know this does not work - so you should find out why it does not work and fix
the problem.

Enforcing IMA/EVM on the overlayfs inode layer is just the wrong way IMO.
Not once have I heard an argument from IMA/EVM developers why it is really
needed to enforce IMA/EVM on the overlayfs inode layer and not on the
real inode.
I am sorry that we are failing to communicate on this matter, but I am not
sure how else I can help.

Thanks,
Amir.

Re: [RFC PATCH v2 0/2] ima: Fix detection of read/write violations on stacked filesystems

[Stefan Berger]

On 4/23/24 02:02, Amir Goldstein wrote:
> On Mon, Apr 22, 2024 at 6:07 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>> This series fixes the detection of read/write violations on stacked
>> filesystems. To be able to access the relevant dentries necessary to
>> detect files opened for writing on a stacked filesystem a new d_real_type
>> D_REAL_FILEDATA is introduced that allows callers to access all relevant
>> files involved in a stacked filesystem while traversing the layers.
>>
> 
> Stefan,
> 
> Both Miklos and myself objected to this solution:
> https://lore.kernel.org/linux-unionfs/CAJfpeguctirEYECoigcAsJwpGPCX2NyfMZ8H8GHGW-0UyKfjgg@mail.gmail.com/

Thanks, the RFC has achieved its objective now.

    Stefan

Re: [RFC PATCH v2 0/2] ima: Fix detection of read/write violations on stacked filesystems

[Roberto Sassu]

On Tue, 2024-04-23 at 09:02 +0300, Amir Goldstein wrote:
> On Mon, Apr 22, 2024 at 6:07 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > 
> > This series fixes the detection of read/write violations on stacked
> > filesystems. To be able to access the relevant dentries necessary to
> > detect files opened for writing on a stacked filesystem a new d_real_type
> > D_REAL_FILEDATA is introduced that allows callers to access all relevant
> > files involved in a stacked filesystem while traversing the layers.
> > 
> 
> Stefan,
> 
> Both Miklos and myself objected to this solution:
> https://lore.kernel.org/linux-unionfs/CAJfpeguctirEYECoigcAsJwpGPCX2NyfMZ8H8GHGW-0UyKfjgg@mail.gmail.com/
> 
> Not sure what you are hoping to achieve from re-posting the same solution.
> 
> I stopped counting how many times I already argued that *all* IMA/EVM
> assertions,
> including rw-ro violations should be enforced only on the real inode.

I have hopefully a better idea. We should detect violations at each
level of the stack independently. And IMA should be invoked each time
overlayfs uses an underlying layer.

That is currently not easy, from the IMA policy perspective, because
there are filesystem-specific rules, such as fsname= or fsuuid=. At the
moment, I'm not planning to solve this, but I'm thinking to use for
example FMODE_BACKING to ignore the filesystem-specific keywords and
match the rule anyway.

For now, I'm only addressing the call to underlying layers. To make
sure that IMA evaluates every layer, I added a rule that checks the
inode UID:

measure fowner=2000 mask=MAY_READ


I just investigated a bit, and I made some changes (for now, I'm just
making it work, and you tell me what you think).

diff --git a/fs/backing-file.c b/fs/backing-file.c
index 740185198db3..8016f62cf770 100644
--- a/fs/backing-file.c
+++ b/fs/backing-file.c
@@ -12,6 +12,7 @@
 #include <linux/backing-file.h>
 #include <linux/splice.h>
 #include <linux/mm.h>
+#include <linux/security.h>
 
 #include "internal.h"
 
@@ -40,12 +41,16 @@ struct file *backing_file_open(const struct path
*user_path, int flags,
        if (IS_ERR(f))
                return f;
 
+       f->f_mode |= OPEN_FMODE(flags);
+
        path_get(user_path);
        *backing_file_user_path(f) = *user_path;
        error = vfs_open(real_path, f);
        if (error) {
                fput(f);
                f = ERR_PTR(error);
+       } else {
+               security_file_post_open(f, ACC_MODE(flags));
        }
 
        return f;


Setup:

# mount -t overlay -olowerdir=a,upperdir=b,workdir=c overlay d

open is a tool with the following syntax:

open <path> <perm>

It performs the open, and waits for user input before closing the file.



ToMToU (Time of Measurement - Time of Use):

Same fs (overlayfs)

# /root/open /root/test-dir/d/test-file r (terminal 1)
# /root/open /root/test-dir/d/test-file w (terminal 2)

This works:

10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
10 cef529d5d1032ffb6d3e2154664c83ba18cf2576 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 test-file
10 694277487b9753db78446192231b59b7be7c03ad ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/d/test-file
10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 test-file
10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 /root/test-dir/d/test-file

This is the result of calling IMA at both layers, and the violation of
course happens twice.

This is also confirmed in the logs:

Apr 23 14:52:45 fedora audit[994]: INTEGRITY_PCR pid=994 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=ToMToU comm="open" name="test-file" dev="sda3" ino=995512 res=1 errno=0
Apr 23 14:52:45 fedora audit[994]: INTEGRITY_PCR pid=994 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=ToMToU comm="open" name="/root/test-dir/d/test-file" dev="overlay" ino=995512 res=1 errno=0


Different fs (overlayfs, btrfs)

# /root/open /root/test-dir/d/test-file r (terminal 1)
# /root/open /root/test-dir/b/test-file w (terminal 2)

Again, this works despite the read is in overlayfs, and the write is in
btrfs:

10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
10 cef529d5d1032ffb6d3e2154664c83ba18cf2576 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 test-file
10 694277487b9753db78446192231b59b7be7c03ad ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/d/test-file
10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 /root/test-dir/b/test-file

The difference from the previous example is that now there is only one
violation, which is detected only in the upper layer. The logs have:

Apr 23 15:01:15 fedora audit[985]: INTEGRITY_PCR pid=985 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=ToMToU comm="open" name="/root/test-dir/b/test-file" dev="sda3" ino=995512 res=1 errno=0


Different fs (btrfs, overlayfs)

# /root/open /root/test-dir/b/test-file r (terminal 2)
# /root/open /root/test-dir/d/test-file w (terminal 1)

10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
10 d7a692e19158820d2755542a8d31b49ac7ac2729 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/b/test-file
10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 test-file

Works too. There is only one measurement, since that is done only for
the upper layer.

Apr 23 15:05:40 fedora audit[982]: INTEGRITY_PCR pid=982 uid=0 auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=ToMToU comm="open" name="test-file" dev="sda3" ino=995512 res=1 errno=0



Open writers

Same fs (overlayfs)

# /root/open /root/test-dir/d/test-file w (terminal 1)
# /root/open /root/test-dir/d/test-file r (terminal 2)

10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 test-file
10 cef529d5d1032ffb6d3e2154664c83ba18cf2576 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 test-file
10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 /root/test-dir/d/test-file
10 694277487b9753db78446192231b59b7be7c03ad ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/d/test-file

Apr 23 15:10:46 fedora audit[983]: INTEGRITY_PCR pid=983 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=open_writers comm="open" name="test-file" dev="sda3" ino=995512 res=1 errno=0
Apr 23 15:10:46 fedora audit[983]: INTEGRITY_PCR pid=983 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=open_writers comm="open" name="/root/test-dir/d/test-file" dev="overlay" ino=995512 res=1 errno=0


Different fs (overlayfs, btrfs)

# /root/open /root/test-dir/d/test-file w (terminal 1)
# /root/open /root/test-dir/b/test-file r (terminal 2)

10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 /root/test-dir/b/test-file
10 d7a692e19158820d2755542a8d31b49ac7ac2729 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/b/test-file

Apr 23 15:12:58 fedora audit[984]: INTEGRITY_PCR pid=984 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=open_writers comm="open" name="/root/test-dir/b/test-file" dev="sda3" ino=995512 res=1 errno=0


Different fs (btrfs, overlayfs)

# /root/open /root/test-dir/b/test-file w (terminal 1)
# /root/open /root/test-dir/d/test-file r (terminal 2)

10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 test-file
10 cef529d5d1032ffb6d3e2154664c83ba18cf2576 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 test-file
10 694277487b9753db78446192231b59b7be7c03ad ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/d/test-file

Apr 23 15:16:37 fedora audit[983]: INTEGRITY_PCR pid=983 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=open_writers comm="open" name="test-file" dev="sda3" ino=995512 res=1 errno=0

Roberto

> I know this does not work - so you should find out why it does not work and fix
> the problem.
> 
> Enforcing IMA/EVM on the overlayfs inode layer is just the wrong way IMO.
> Not once have I heard an argument from IMA/EVM developers why it is really
> needed to enforce IMA/EVM on the overlayfs inode layer and not on the
> real inode.
> I am sorry that we are failing to communicate on this matter, but I am not
> sure how else I can help.
> 
> Thanks,
> Amir.

Re: [RFC PATCH v2 0/2] ima: Fix detection of read/write violations on stacked filesystems

[Amir Goldstein]

On Tue, Apr 23, 2024 at 4:21 PM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> On Tue, 2024-04-23 at 09:02 +0300, Amir Goldstein wrote:
> > On Mon, Apr 22, 2024 at 6:07 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > >
> > > This series fixes the detection of read/write violations on stacked
> > > filesystems. To be able to access the relevant dentries necessary to
> > > detect files opened for writing on a stacked filesystem a new d_real_type
> > > D_REAL_FILEDATA is introduced that allows callers to access all relevant
> > > files involved in a stacked filesystem while traversing the layers.
> > >
> >
> > Stefan,
> >
> > Both Miklos and myself objected to this solution:
> > https://lore.kernel.org/linux-unionfs/CAJfpeguctirEYECoigcAsJwpGPCX2NyfMZ8H8GHGW-0UyKfjgg@mail.gmail.com/
> >
> > Not sure what you are hoping to achieve from re-posting the same solution.
> >
> > I stopped counting how many times I already argued that *all* IMA/EVM
> > assertions,
> > including rw-ro violations should be enforced only on the real inode.
>
> I have hopefully a better idea. We should detect violations at each
> level of the stack independently. And IMA should be invoked each time
> overlayfs uses an underlying layer.
>
> That is currently not easy, from the IMA policy perspective, because
> there are filesystem-specific rules, such as fsname= or fsuuid=. At the
> moment, I'm not planning to solve this, but I'm thinking to use for
> example FMODE_BACKING to ignore the filesystem-specific keywords and
> match the rule anyway.
>
> For now, I'm only addressing the call to underlying layers. To make
> sure that IMA evaluates every layer, I added a rule that checks the
> inode UID:
>
> measure fowner=2000 mask=MAY_READ
>
>
> I just investigated a bit, and I made some changes (for now, I'm just
> making it work, and you tell me what you think).

I did not examine this up close, but this seems like a change in the right
direction.
Will need Christian's approval that this does not break any assumptions
made on backing files.

Thanks,
Amir.

>
> diff --git a/fs/backing-file.c b/fs/backing-file.c
> index 740185198db3..8016f62cf770 100644
> --- a/fs/backing-file.c
> +++ b/fs/backing-file.c
> @@ -12,6 +12,7 @@
>  #include <linux/backing-file.h>
>  #include <linux/splice.h>
>  #include <linux/mm.h>
> +#include <linux/security.h>
>
>  #include "internal.h"
>
> @@ -40,12 +41,16 @@ struct file *backing_file_open(const struct path
> *user_path, int flags,
>         if (IS_ERR(f))
>                 return f;
>
> +       f->f_mode |= OPEN_FMODE(flags);
> +
>         path_get(user_path);
>         *backing_file_user_path(f) = *user_path;
>         error = vfs_open(real_path, f);
>         if (error) {
>                 fput(f);
>                 f = ERR_PTR(error);
> +       } else {
> +               security_file_post_open(f, ACC_MODE(flags));
>         }
>
>         return f;
>
>
> Setup:
>
> # mount -t overlay -olowerdir=a,upperdir=b,workdir=c overlay d
>
> open is a tool with the following syntax:
>
> open <path> <perm>
>
> It performs the open, and waits for user input before closing the file.
>
>
>
> ToMToU (Time of Measurement - Time of Use):
>
> Same fs (overlayfs)
>
> # /root/open /root/test-dir/d/test-file r (terminal 1)
> # /root/open /root/test-dir/d/test-file w (terminal 2)
>
> This works:
>
> 10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
> 10 cef529d5d1032ffb6d3e2154664c83ba18cf2576 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 test-file
> 10 694277487b9753db78446192231b59b7be7c03ad ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/d/test-file
> 10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 test-file
> 10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 /root/test-dir/d/test-file
>
> This is the result of calling IMA at both layers, and the violation of
> course happens twice.
>
> This is also confirmed in the logs:
>
> Apr 23 14:52:45 fedora audit[994]: INTEGRITY_PCR pid=994 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=ToMToU comm="open" name="test-file" dev="sda3" ino=995512 res=1 errno=0
> Apr 23 14:52:45 fedora audit[994]: INTEGRITY_PCR pid=994 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=ToMToU comm="open" name="/root/test-dir/d/test-file" dev="overlay" ino=995512 res=1 errno=0
>
>
> Different fs (overlayfs, btrfs)
>
> # /root/open /root/test-dir/d/test-file r (terminal 1)
> # /root/open /root/test-dir/b/test-file w (terminal 2)
>
> Again, this works despite the read is in overlayfs, and the write is in
> btrfs:
>
> 10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
> 10 cef529d5d1032ffb6d3e2154664c83ba18cf2576 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 test-file
> 10 694277487b9753db78446192231b59b7be7c03ad ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/d/test-file
> 10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 /root/test-dir/b/test-file
>
> The difference from the previous example is that now there is only one
> violation, which is detected only in the upper layer. The logs have:
>
> Apr 23 15:01:15 fedora audit[985]: INTEGRITY_PCR pid=985 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=ToMToU comm="open" name="/root/test-dir/b/test-file" dev="sda3" ino=995512 res=1 errno=0
>
>
> Different fs (btrfs, overlayfs)
>
> # /root/open /root/test-dir/b/test-file r (terminal 2)
> # /root/open /root/test-dir/d/test-file w (terminal 1)
>
> 10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
> 10 d7a692e19158820d2755542a8d31b49ac7ac2729 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/b/test-file
> 10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 test-file
>
> Works too. There is only one measurement, since that is done only for
> the upper layer.
>
> Apr 23 15:05:40 fedora audit[982]: INTEGRITY_PCR pid=982 uid=0 auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=ToMToU comm="open" name="test-file" dev="sda3" ino=995512 res=1 errno=0
>
>
>
> Open writers
>
> Same fs (overlayfs)
>
> # /root/open /root/test-dir/d/test-file w (terminal 1)
> # /root/open /root/test-dir/d/test-file r (terminal 2)
>
> 10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
> 10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 test-file
> 10 cef529d5d1032ffb6d3e2154664c83ba18cf2576 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 test-file
> 10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 /root/test-dir/d/test-file
> 10 694277487b9753db78446192231b59b7be7c03ad ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/d/test-file
>
> Apr 23 15:10:46 fedora audit[983]: INTEGRITY_PCR pid=983 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=open_writers comm="open" name="test-file" dev="sda3" ino=995512 res=1 errno=0
> Apr 23 15:10:46 fedora audit[983]: INTEGRITY_PCR pid=983 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=open_writers comm="open" name="/root/test-dir/d/test-file" dev="overlay" ino=995512 res=1 errno=0
>
>
> Different fs (overlayfs, btrfs)
>
> # /root/open /root/test-dir/d/test-file w (terminal 1)
> # /root/open /root/test-dir/b/test-file r (terminal 2)
>
> 10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
> 10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 /root/test-dir/b/test-file
> 10 d7a692e19158820d2755542a8d31b49ac7ac2729 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/b/test-file
>
> Apr 23 15:12:58 fedora audit[984]: INTEGRITY_PCR pid=984 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=open_writers comm="open" name="/root/test-dir/b/test-file" dev="sda3" ino=995512 res=1 errno=0
>
>
> Different fs (btrfs, overlayfs)
>
> # /root/open /root/test-dir/b/test-file w (terminal 1)
> # /root/open /root/test-dir/d/test-file r (terminal 2)
>
> 10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
> 10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 test-file
> 10 cef529d5d1032ffb6d3e2154664c83ba18cf2576 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 test-file
> 10 694277487b9753db78446192231b59b7be7c03ad ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/d/test-file
>
> Apr 23 15:16:37 fedora audit[983]: INTEGRITY_PCR pid=983 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=open_writers comm="open" name="test-file" dev="sda3" ino=995512 res=1 errno=0
>
> Roberto
>
> > I know this does not work - so you should find out why it does not work and fix
> > the problem.
> >
> > Enforcing IMA/EVM on the overlayfs inode layer is just the wrong way IMO.
> > Not once have I heard an argument from IMA/EVM developers why it is really
> > needed to enforce IMA/EVM on the overlayfs inode layer and not on the
> > real inode.
> > I am sorry that we are failing to communicate on this matter, but I am not
> > sure how else I can help.
> >
> > Thanks,
> > Amir.
>

Re: [RFC PATCH v2 0/2] ima: Fix detection of read/write violations on stacked filesystems

[Christian Brauner]

On Tue, Apr 23, 2024 at 05:30:52PM +0300, Amir Goldstein wrote:
> On Tue, Apr 23, 2024 at 4:21 PM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> >
> > On Tue, 2024-04-23 at 09:02 +0300, Amir Goldstein wrote:
> > > On Mon, Apr 22, 2024 at 6:07 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > >
> > > > This series fixes the detection of read/write violations on stacked
> > > > filesystems. To be able to access the relevant dentries necessary to
> > > > detect files opened for writing on a stacked filesystem a new d_real_type
> > > > D_REAL_FILEDATA is introduced that allows callers to access all relevant
> > > > files involved in a stacked filesystem while traversing the layers.
> > > >
> > >
> > > Stefan,
> > >
> > > Both Miklos and myself objected to this solution:
> > > https://lore.kernel.org/linux-unionfs/CAJfpeguctirEYECoigcAsJwpGPCX2NyfMZ8H8GHGW-0UyKfjgg@mail.gmail.com/
> > >
> > > Not sure what you are hoping to achieve from re-posting the same solution.
> > >
> > > I stopped counting how many times I already argued that *all* IMA/EVM
> > > assertions,
> > > including rw-ro violations should be enforced only on the real inode.
> >
> > I have hopefully a better idea. We should detect violations at each
> > level of the stack independently. And IMA should be invoked each time
> > overlayfs uses an underlying layer.
> >
> > That is currently not easy, from the IMA policy perspective, because
> > there are filesystem-specific rules, such as fsname= or fsuuid=. At the
> > moment, I'm not planning to solve this, but I'm thinking to use for
> > example FMODE_BACKING to ignore the filesystem-specific keywords and
> > match the rule anyway.
> >
> > For now, I'm only addressing the call to underlying layers. To make
> > sure that IMA evaluates every layer, I added a rule that checks the
> > inode UID:
> >
> > measure fowner=2000 mask=MAY_READ
> >
> >
> > I just investigated a bit, and I made some changes (for now, I'm just
> > making it work, and you tell me what you think).
> 
> I did not examine this up close, but this seems like a change in the right
> direction.
> Will need Christian's approval that this does not break any assumptions
> made on backing files.

In principle I don't care if IMA wants to call yet another security hook
in the backing file layer. I suspect it will impact performace if IMA is
enabled. So that's something to keep in mind. But it's certainly better
than blatantly abusing the dcache to achieve this.

> > diff --git a/fs/backing-file.c b/fs/backing-file.c
> > index 740185198db3..8016f62cf770 100644
> > --- a/fs/backing-file.c
> > +++ b/fs/backing-file.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/backing-file.h>
> >  #include <linux/splice.h>
> >  #include <linux/mm.h>
> > +#include <linux/security.h>
> >
> >  #include "internal.h"
> >
> > @@ -40,12 +41,16 @@ struct file *backing_file_open(const struct path
> > *user_path, int flags,
> >         if (IS_ERR(f))
> >                 return f;
> >
> > +       f->f_mode |= OPEN_FMODE(flags);
> > +
> >         path_get(user_path);
> >         *backing_file_user_path(f) = *user_path;
> >         error = vfs_open(real_path, f);
> >         if (error) {
> >                 fput(f);
> >                 f = ERR_PTR(error);
> > +       } else {
> > +               security_file_post_open(f, ACC_MODE(flags));
> >         }
> >
> >         return f;
> >
> >
> > Setup:
> >
> > # mount -t overlay -olowerdir=a,upperdir=b,workdir=c overlay d
> >
> > open is a tool with the following syntax:
> >
> > open <path> <perm>
> >
> > It performs the open, and waits for user input before closing the file.
> >
> >
> >
> > ToMToU (Time of Measurement - Time of Use):
> >
> > Same fs (overlayfs)
> >
> > # /root/open /root/test-dir/d/test-file r (terminal 1)
> > # /root/open /root/test-dir/d/test-file w (terminal 2)
> >
> > This works:
> >
> > 10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
> > 10 cef529d5d1032ffb6d3e2154664c83ba18cf2576 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 test-file
> > 10 694277487b9753db78446192231b59b7be7c03ad ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/d/test-file
> > 10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 test-file
> > 10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 /root/test-dir/d/test-file
> >
> > This is the result of calling IMA at both layers, and the violation of
> > course happens twice.
> >
> > This is also confirmed in the logs:
> >
> > Apr 23 14:52:45 fedora audit[994]: INTEGRITY_PCR pid=994 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=ToMToU comm="open" name="test-file" dev="sda3" ino=995512 res=1 errno=0
> > Apr 23 14:52:45 fedora audit[994]: INTEGRITY_PCR pid=994 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=ToMToU comm="open" name="/root/test-dir/d/test-file" dev="overlay" ino=995512 res=1 errno=0
> >
> >
> > Different fs (overlayfs, btrfs)
> >
> > # /root/open /root/test-dir/d/test-file r (terminal 1)
> > # /root/open /root/test-dir/b/test-file w (terminal 2)
> >
> > Again, this works despite the read is in overlayfs, and the write is in
> > btrfs:
> >
> > 10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
> > 10 cef529d5d1032ffb6d3e2154664c83ba18cf2576 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 test-file
> > 10 694277487b9753db78446192231b59b7be7c03ad ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/d/test-file
> > 10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 /root/test-dir/b/test-file
> >
> > The difference from the previous example is that now there is only one
> > violation, which is detected only in the upper layer. The logs have:
> >
> > Apr 23 15:01:15 fedora audit[985]: INTEGRITY_PCR pid=985 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=ToMToU comm="open" name="/root/test-dir/b/test-file" dev="sda3" ino=995512 res=1 errno=0
> >
> >
> > Different fs (btrfs, overlayfs)
> >
> > # /root/open /root/test-dir/b/test-file r (terminal 2)
> > # /root/open /root/test-dir/d/test-file w (terminal 1)
> >
> > 10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
> > 10 d7a692e19158820d2755542a8d31b49ac7ac2729 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/b/test-file
> > 10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 test-file
> >
> > Works too. There is only one measurement, since that is done only for
> > the upper layer.
> >
> > Apr 23 15:05:40 fedora audit[982]: INTEGRITY_PCR pid=982 uid=0 auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=ToMToU comm="open" name="test-file" dev="sda3" ino=995512 res=1 errno=0
> >
> >
> >
> > Open writers
> >
> > Same fs (overlayfs)
> >
> > # /root/open /root/test-dir/d/test-file w (terminal 1)
> > # /root/open /root/test-dir/d/test-file r (terminal 2)
> >
> > 10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
> > 10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 test-file
> > 10 cef529d5d1032ffb6d3e2154664c83ba18cf2576 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 test-file
> > 10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 /root/test-dir/d/test-file
> > 10 694277487b9753db78446192231b59b7be7c03ad ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/d/test-file
> >
> > Apr 23 15:10:46 fedora audit[983]: INTEGRITY_PCR pid=983 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=open_writers comm="open" name="test-file" dev="sda3" ino=995512 res=1 errno=0
> > Apr 23 15:10:46 fedora audit[983]: INTEGRITY_PCR pid=983 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=open_writers comm="open" name="/root/test-dir/d/test-file" dev="overlay" ino=995512 res=1 errno=0
> >
> >
> > Different fs (overlayfs, btrfs)
> >
> > # /root/open /root/test-dir/d/test-file w (terminal 1)
> > # /root/open /root/test-dir/b/test-file r (terminal 2)
> >
> > 10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
> > 10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 /root/test-dir/b/test-file
> > 10 d7a692e19158820d2755542a8d31b49ac7ac2729 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/b/test-file
> >
> > Apr 23 15:12:58 fedora audit[984]: INTEGRITY_PCR pid=984 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=open_writers comm="open" name="/root/test-dir/b/test-file" dev="sda3" ino=995512 res=1 errno=0
> >
> >
> > Different fs (btrfs, overlayfs)
> >
> > # /root/open /root/test-dir/b/test-file w (terminal 1)
> > # /root/open /root/test-dir/d/test-file r (terminal 2)
> >
> > 10 35435d0858d895b90097306171a2e5fcc7f5da9e ima-ng sha256:0e4acf326a82c6bded9d86f48d272d7a036b6490081bb6466ecc2a0e416b244a boot_aggregate
> > 10 0000000000000000000000000000000000000000 ima-ng sha256:0000000000000000000000000000000000000000000000000000000000000000 test-file
> > 10 cef529d5d1032ffb6d3e2154664c83ba18cf2576 ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 test-file
> > 10 694277487b9753db78446192231b59b7be7c03ad ima-ng sha256:f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 /root/test-dir/d/test-file
> >
> > Apr 23 15:16:37 fedora audit[983]: INTEGRITY_PCR pid=983 uid=0 auid=0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op=invalid_pcr cause=open_writers comm="open" name="test-file" dev="sda3" ino=995512 res=1 errno=0
> >
> > Roberto
> >
> > > I know this does not work - so you should find out why it does not work and fix
> > > the problem.
> > >
> > > Enforcing IMA/EVM on the overlayfs inode layer is just the wrong way IMO.
> > > Not once have I heard an argument from IMA/EVM developers why it is really
> > > needed to enforce IMA/EVM on the overlayfs inode layer and not on the
> > > real inode.
> > > I am sorry that we are failing to communicate on this matter, but I am not
> > > sure how else I can help.
> > >
> > > Thanks,
> > > Amir.
> >

Re: [RFC PATCH v2 0/2] ima: Fix detection of read/write violations on stacked filesystems

[Roberto Sassu]

On Tue, 2024-04-23 at 09:02 +0300, Amir Goldstein wrote:
> On Mon, Apr 22, 2024 at 6:07 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > 
> > This series fixes the detection of read/write violations on stacked
> > filesystems. To be able to access the relevant dentries necessary to
> > detect files opened for writing on a stacked filesystem a new d_real_type
> > D_REAL_FILEDATA is introduced that allows callers to access all relevant
> > files involved in a stacked filesystem while traversing the layers.
> > 
> 
> Stefan,
> 
> Both Miklos and myself objected to this solution:
> https://lore.kernel.org/linux-unionfs/CAJfpeguctirEYECoigcAsJwpGPCX2NyfMZ8H8GHGW-0UyKfjgg@mail.gmail.com/
> 
> Not sure what you are hoping to achieve from re-posting the same solution.
> 
> I stopped counting how many times I already argued that *all* IMA/EVM
> assertions,
> including rw-ro violations should be enforced only on the real inode.
> I know this does not work - so you should find out why it does not work and fix
> the problem.
> 
> Enforcing IMA/EVM on the overlayfs inode layer is just the wrong way IMO.
> Not once have I heard an argument from IMA/EVM developers why it is really
> needed to enforce IMA/EVM on the overlayfs inode layer and not on the
> real inode.

Ok, I try to provide an example regarding this, and we see if it makes
sense.

# echo test > test-file
# chown 2000 d/test-file
# ls -l a/test-file
-rw-r--r--. 1 2000 root 25 Apr 25 10:50 a/test-file

Initially there is a file in the lower layer with UID 2000.


# mount -t overlay -olowerdir=a,upperdir=b,workdir=c,metacopy=on overlay d
# chown 3000 d/test-file
# ls -l d/test-file
-rw-r--r--. 1 3000 root 25 Apr 25 10:50 d/test-file
# ls -l a/test-file
-rw-r--r--. 1 2000 root 25 Apr 25 10:50 a/test-file
# ls -l b/test-file
-rw-r--r--. 1 3000 root 25 Apr 25 10:50 b/test-file

If I have a policy like this:

# echo "measure fsname=overlay fowner=3000" > /sys/kernel/security/ima/policy

there won't be any match on the real file which still has UID 2000. But
what is observable by the processes through overlayfs is UID 3000.

Roberto

> I am sorry that we are failing to communicate on this matter, but I am not
> sure how else I can help.
> 
> Thanks,
> Amir.

Re: [RFC PATCH v2 0/2] ima: Fix detection of read/write violations on stacked filesystems

[Amir Goldstein]

On Thu, Apr 25, 2024 at 2:30 PM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> On Tue, 2024-04-23 at 09:02 +0300, Amir Goldstein wrote:
> > On Mon, Apr 22, 2024 at 6:07 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > >
> > > This series fixes the detection of read/write violations on stacked
> > > filesystems. To be able to access the relevant dentries necessary to
> > > detect files opened for writing on a stacked filesystem a new d_real_type
> > > D_REAL_FILEDATA is introduced that allows callers to access all relevant
> > > files involved in a stacked filesystem while traversing the layers.
> > >
> >
> > Stefan,
> >
> > Both Miklos and myself objected to this solution:
> > https://lore.kernel.org/linux-unionfs/CAJfpeguctirEYECoigcAsJwpGPCX2NyfMZ8H8GHGW-0UyKfjgg@mail.gmail.com/
> >
> > Not sure what you are hoping to achieve from re-posting the same solution.
> >
> > I stopped counting how many times I already argued that *all* IMA/EVM
> > assertions,
> > including rw-ro violations should be enforced only on the real inode.
> > I know this does not work - so you should find out why it does not work and fix
> > the problem.
> >
> > Enforcing IMA/EVM on the overlayfs inode layer is just the wrong way IMO.
> > Not once have I heard an argument from IMA/EVM developers why it is really
> > needed to enforce IMA/EVM on the overlayfs inode layer and not on the
> > real inode.
>
> Ok, I try to provide an example regarding this, and we see if it makes
> sense.
>
> # echo test > test-file
> # chown 2000 d/test-file
> # ls -l a/test-file
> -rw-r--r--. 1 2000 root 25 Apr 25 10:50 a/test-file
>
> Initially there is a file in the lower layer with UID 2000.
>
>
> # mount -t overlay -olowerdir=a,upperdir=b,workdir=c,metacopy=on overlay d
> # chown 3000 d/test-file
> # ls -l d/test-file
> -rw-r--r--. 1 3000 root 25 Apr 25 10:50 d/test-file
> # ls -l a/test-file
> -rw-r--r--. 1 2000 root 25 Apr 25 10:50 a/test-file
> # ls -l b/test-file
> -rw-r--r--. 1 3000 root 25 Apr 25 10:50 b/test-file
>
> If I have a policy like this:
>
> # echo "measure fsname=overlay fowner=3000" > /sys/kernel/security/ima/policy
>
> there won't be any match on the real file which still has UID 2000. But
> what is observable by the processes through overlayfs is UID 3000.
>

ok, it is simple to write an ima policy that is not respected by overlayfs.

My question is: under what circumstances is a policy like the above
useful in the real world?

Correct me if I am wrong, but AFAIK, the purpose of IMA/EVM is to
mitigate attack vectors of tampering with files offline or after the
file's data/metadata were measured. Is that a correct description?

AFAIK, IMA/EVM policy is system-wide and not namespace aware
so the policy has to be set on the container's host and not inside
containers. Is that correct?

If those statements are true then please try to explain to me what is
the thread model for tampering with overlayfs files, without tampering
with the real upper and/or lower files.

My thesis is that if an IMA/EVM policy is good enough to prevent
tampering with the real lower/upper files, then no extra measures
are needed to protect the virtual overlayfs files against tampering.

Is my thesis flawed?
I'm pretty sure that it is, but I never got a satisfying answer why.

Thanks,
Amir.

Re: [RFC PATCH v2 0/2] ima: Fix detection of read/write violations on stacked filesystems

[Roberto Sassu]

On Thu, 2024-04-25 at 15:37 +0300, Amir Goldstein wrote:
> > On Thu, Apr 25, 2024 at 2:30 PM Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > > > 
> > > > On Tue, 2024-04-23 at 09:02 +0300, Amir Goldstein wrote:
> > > > > > On Mon, Apr 22, 2024 at 6:07 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > > > > > > 
> > > > > > > > This series fixes the detection of read/write violations on stacked
> > > > > > > > filesystems. To be able to access the relevant dentries necessary to
> > > > > > > > detect files opened for writing on a stacked filesystem a new d_real_type
> > > > > > > > D_REAL_FILEDATA is introduced that allows callers to access all relevant
> > > > > > > > files involved in a stacked filesystem while traversing the layers.
> > > > > > > > 
> > > > > > 
> > > > > > Stefan,
> > > > > > 
> > > > > > Both Miklos and myself objected to this solution:
> > > > > > https://lore.kernel.org/linux-unionfs/CAJfpeguctirEYECoigcAsJwpGPCX2NyfMZ8H8GHGW-0UyKfjgg@mail.gmail.com/
> > > > > > 
> > > > > > Not sure what you are hoping to achieve from re-posting the same solution.
> > > > > > 
> > > > > > I stopped counting how many times I already argued that *all* IMA/EVM
> > > > > > assertions,
> > > > > > including rw-ro violations should be enforced only on the real inode.
> > > > > > I know this does not work - so you should find out why it does not work and fix
> > > > > > the problem.
> > > > > > 
> > > > > > Enforcing IMA/EVM on the overlayfs inode layer is just the wrong way IMO.
> > > > > > Not once have I heard an argument from IMA/EVM developers why it is really
> > > > > > needed to enforce IMA/EVM on the overlayfs inode layer and not on the
> > > > > > real inode.
> > > > 
> > > > Ok, I try to provide an example regarding this, and we see if it makes
> > > > sense.
> > > > 
> > > > # echo test > test-file
> > > > # chown 2000 d/test-file
> > > > # ls -l a/test-file
> > > > -rw-r--r--. 1 2000 root 25 Apr 25 10:50 a/test-file
> > > > 
> > > > Initially there is a file in the lower layer with UID 2000.
> > > > 
> > > > 
> > > > # mount -t overlay -olowerdir=a,upperdir=b,workdir=c,metacopy=on overlay d
> > > > # chown 3000 d/test-file
> > > > # ls -l d/test-file
> > > > -rw-r--r--. 1 3000 root 25 Apr 25 10:50 d/test-file
> > > > # ls -l a/test-file
> > > > -rw-r--r--. 1 2000 root 25 Apr 25 10:50 a/test-file
> > > > # ls -l b/test-file
> > > > -rw-r--r--. 1 3000 root 25 Apr 25 10:50 b/test-file
> > > > 
> > > > If I have a policy like this:
> > > > 
> > > > # echo "measure fsname=overlay fowner=3000" > /sys/kernel/security/ima/policy
> > > > 
> > > > there won't be any match on the real file which still has UID 2000. But
> > > > what is observable by the processes through overlayfs is UID 3000.
> > > > 
> > 
> > ok, it is simple to write an ima policy that is not respected by overlayfs.
> > 
> > My question is: under what circumstances is a policy like the above
> > useful in the real world?
> > 
> > Correct me if I am wrong, but AFAIK, the purpose of IMA/EVM is to
> > mitigate attack vectors of tampering with files offline or after the
> > file's data/metadata were measured. Is that a correct description?

(For now I would talk about IMA, EVM can be considered separately).

The main purpose of IMA is to evaluate files being accessed, and record
the access together with a file digest in a measurement list,
allow/deny access to the file (appraisal), or add a new event to audit
logs.

How files are selected depends on the IMA policy. A rule can be
subject-based or object-based, depending on whether respectively
process or file attributes are matched. It can also be both.

A subject-based rule means that you identify a process/set of
processes, and you evaluate everything it/they read.

An object-based rule means that you identify a file/set of files, and
you evaluate any process accessing them.

Since processes normally would access the top most layer (overlayfs),
the IMA policy should be written in terms of metadata seen in that
layer (but not necessarily).

This is just for identifying the set of files to
measure/appraise/audit, not which file is going to be evaluated, which
will be always the persistent one.

I have to admit, things are not very clear also to me.

Suppose you have a file in the lower filesystem with SELinux label
user_t, and then on overlayfs with metadata copy, you change the label
of this file to unconfined_t.

What will happen exactly? On the overlayfs layer, you will have a
permission request with the new label unconfined_t, but when overlayfs
calls vfs_open(), there will be another permission request with the old
label.

It is kind of the same challenge we are facing with IMA, we can observe
the file operations at different layers. That is why I think having
stacked IMA calls is a good idea (other than really fixing the
violations).

The current problem, that is very difficult to solve, is that the
policy should cover all layers, or some events will be missed. Now we
have overlayfs-specific code to detect changes in the backing inode,
while with stacked IMA calls, we can detect the change at the layer
where it happened (and we can remove the overlayfs-specific code).

Ideally, what I would do to cover all layers is that if there is a
match at one layer, the lower layers should automatically match too,
but it is not that easy since after the vfs_open() recursive calls we
start calling IMA in the botton most layer first.

(What I did with the stacked IMA calls is just an experiment to see how
far we can go, but still we didn't make any decision with Mimi).

> > AFAIK, IMA/EVM policy is system-wide and not namespace aware
> > so the policy has to be set on the container's host and not inside
> > containers. Is that correct?

I know that overlayfs is primarily aiming at containers, but I would
suggest to not add that complexity yet and just consider the host.

> > If those statements are true then please try to explain to me what is
> > the thread model for tampering with overlayfs files, without tampering
> > with the real upper and/or lower files.

I hope at this point is clear that what we care about is that, or the
process is reading the content of the file whose digest is recorded in
the measurement list, or we must signal to remote verifiers concurrent
accesses that make the statement above false.

> > My thesis is that if an IMA/EVM policy is good enough to prevent
> > tampering with the real lower/upper files, then no extra measures
> > are needed to protect the virtual overlayfs files against tampering.

What you say is correct, but the way you identify files to
measure/appraise/audit can be different.

Thanks

Roberto

> > Is my thesis flawed?
> > I'm pretty sure that it is, but I never got a satisfying answer why.
> > 
> > Thanks,
> > Amir.

Re: [RFC PATCH v2 0/2] ima: Fix detection of read/write violations on stacked filesystems

[Amir Goldstein]

On Fri, Apr 26, 2024 at 10:34 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> On Thu, 2024-04-25 at 15:37 +0300, Amir Goldstein wrote:
> > > On Thu, Apr 25, 2024 at 2:30 PM Roberto Sassu
> > > <roberto.sassu@huaweicloud.com> wrote:
> > > > >
> > > > > On Tue, 2024-04-23 at 09:02 +0300, Amir Goldstein wrote:
> > > > > > > On Mon, Apr 22, 2024 at 6:07 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > > > > > > >
> > > > > > > > > This series fixes the detection of read/write violations on stacked
> > > > > > > > > filesystems. To be able to access the relevant dentries necessary to
> > > > > > > > > detect files opened for writing on a stacked filesystem a new d_real_type
> > > > > > > > > D_REAL_FILEDATA is introduced that allows callers to access all relevant
> > > > > > > > > files involved in a stacked filesystem while traversing the layers.
> > > > > > > > >
> > > > > > >
> > > > > > > Stefan,
> > > > > > >
> > > > > > > Both Miklos and myself objected to this solution:
> > > > > > > https://lore.kernel.org/linux-unionfs/CAJfpeguctirEYECoigcAsJwpGPCX2NyfMZ8H8GHGW-0UyKfjgg@mail.gmail.com/
> > > > > > >
> > > > > > > Not sure what you are hoping to achieve from re-posting the same solution.
> > > > > > >
> > > > > > > I stopped counting how many times I already argued that *all* IMA/EVM
> > > > > > > assertions,
> > > > > > > including rw-ro violations should be enforced only on the real inode.
> > > > > > > I know this does not work - so you should find out why it does not work and fix
> > > > > > > the problem.
> > > > > > >
> > > > > > > Enforcing IMA/EVM on the overlayfs inode layer is just the wrong way IMO.
> > > > > > > Not once have I heard an argument from IMA/EVM developers why it is really
> > > > > > > needed to enforce IMA/EVM on the overlayfs inode layer and not on the
> > > > > > > real inode.
> > > > >
> > > > > Ok, I try to provide an example regarding this, and we see if it makes
> > > > > sense.
> > > > >
> > > > > # echo test > test-file
> > > > > # chown 2000 d/test-file
> > > > > # ls -l a/test-file
> > > > > -rw-r--r--. 1 2000 root 25 Apr 25 10:50 a/test-file
> > > > >
> > > > > Initially there is a file in the lower layer with UID 2000.
> > > > >
> > > > >
> > > > > # mount -t overlay -olowerdir=a,upperdir=b,workdir=c,metacopy=on overlay d
> > > > > # chown 3000 d/test-file
> > > > > # ls -l d/test-file
> > > > > -rw-r--r--. 1 3000 root 25 Apr 25 10:50 d/test-file
> > > > > # ls -l a/test-file
> > > > > -rw-r--r--. 1 2000 root 25 Apr 25 10:50 a/test-file
> > > > > # ls -l b/test-file
> > > > > -rw-r--r--. 1 3000 root 25 Apr 25 10:50 b/test-file
> > > > >
> > > > > If I have a policy like this:
> > > > >
> > > > > # echo "measure fsname=overlay fowner=3000" > /sys/kernel/security/ima/policy
> > > > >
> > > > > there won't be any match on the real file which still has UID 2000. But
> > > > > what is observable by the processes through overlayfs is UID 3000.
> > > > >
> > >
> > > ok, it is simple to write an ima policy that is not respected by overlayfs.
> > >
> > > My question is: under what circumstances is a policy like the above
> > > useful in the real world?
> > >
> > > Correct me if I am wrong, but AFAIK, the purpose of IMA/EVM is to
> > > mitigate attack vectors of tampering with files offline or after the
> > > file's data/metadata were measured. Is that a correct description?
>
> (For now I would talk about IMA, EVM can be considered separately).
>
> The main purpose of IMA is to evaluate files being accessed, and record
> the access together with a file digest in a measurement list,
> allow/deny access to the file (appraisal), or add a new event to audit
> logs.
>
> How files are selected depends on the IMA policy. A rule can be
> subject-based or object-based, depending on whether respectively
> process or file attributes are matched. It can also be both.
>
> A subject-based rule means that you identify a process/set of
> processes, and you evaluate everything it/they read.
>
> An object-based rule means that you identify a file/set of files, and
> you evaluate any process accessing them.
>
> Since processes normally would access the top most layer (overlayfs),
> the IMA policy should be written in terms of metadata seen in that
> layer (but not necessarily).
>
> This is just for identifying the set of files to
> measure/appraise/audit, not which file is going to be evaluated, which
> will be always the persistent one.
>
> I have to admit, things are not very clear also to me.
>
> Suppose you have a file in the lower filesystem with SELinux label
> user_t, and then on overlayfs with metadata copy, you change the label
> of this file to unconfined_t.
>
> What will happen exactly? On the overlayfs layer, you will have a
> permission request with the new label unconfined_t, but when overlayfs
> calls vfs_open(), there will be another permission request with the old
> label.

CC Vivek who was involved with ovl+selinux, but I think the answer is
that ovl sepolicy is expected to be associated with the mount ctx and
not the objects and there was a need to implement the security hook
security_inode_copy_up() to be able to compose a safe sepolicy for
overlayfs.

>
> It is kind of the same challenge we are facing with IMA, we can observe
> the file operations at different layers. That is why I think having
> stacked IMA calls is a good idea (other than really fixing the
> violations).
>
> The current problem, that is very difficult to solve, is that the
> policy should cover all layers, or some events will be missed. Now we
> have overlayfs-specific code to detect changes in the backing inode,
> while with stacked IMA calls, we can detect the change at the layer
> where it happened (and we can remove the overlayfs-specific code).
>
> Ideally, what I would do to cover all layers is that if there is a
> match at one layer, the lower layers should automatically match too,
> but it is not that easy since after the vfs_open() recursive calls we
> start calling IMA in the botton most layer first.
>
> (What I did with the stacked IMA calls is just an experiment to see how
> far we can go, but still we didn't make any decision with Mimi).
>
> > > AFAIK, IMA/EVM policy is system-wide and not namespace aware
> > > so the policy has to be set on the container's host and not inside
> > > containers. Is that correct?
>
> I know that overlayfs is primarily aiming at containers, but I would
> suggest to not add that complexity yet and just consider the host.
>
> > > If those statements are true then please try to explain to me what is
> > > the thread model for tampering with overlayfs files, without tampering
> > > with the real upper and/or lower files.
>
> I hope at this point is clear that what we care about is that, or the
> process is reading the content of the file whose digest is recorded in
> the measurement list, or we must signal to remote verifiers concurrent
> accesses that make the statement above false.
>
> > > My thesis is that if an IMA/EVM policy is good enough to prevent
> > > tampering with the real lower/upper files, then no extra measures
> > > are needed to protect the virtual overlayfs files against tampering.
>
> What you say is correct, but the way you identify files to
> measure/appraise/audit can be different.
>

IIUC, the problem as you described it, is similar to the problem
of how to display the overlayfs path of mmaped files in /proc/self/maps
when the actual inode mapped to memory is the real inode.

The solution for this problem was the somewhat awkward
"file with fake path" - it was recently changed to use the new
file_user_path() helper.

I'm not sure how this can help IMA, but in theory, you could
always attach iint state to the real inode and only the the real inode
but the rules that filter by path/object in the IMA hooks that take a file
argument could take file_user_path() into account.

For example, if you have security_file_post_open(f) in
backing_file_open(), then you can use d_real_inode()
in process_measurement to get to the IMA state and check the
rules referring to the real inode and you can use file_user_path(file)
to check the rules referring to "front object".

In case of a nested overlayfs, you should get two post_open
hook, both will refer to the same IMA state on the real inode, but
each hook with have a different file_user_path(), so you will have
an opportunity to apply the path/object based rules on every layer
in the nested overlayfs.

I hope what I tried to explain is clear and I hope there are not
many traps down this road, but you won't know unless you try...

Thanks,
Amir.

Re: [RFC PATCH v2 0/2] ima: Fix detection of read/write violations on stacked filesystems

[Roberto Sassu]

On Sat, 2024-04-27 at 12:03 +0300, Amir Goldstein wrote:
> On Fri, Apr 26, 2024 at 10:34 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > 
> > On Thu, 2024-04-25 at 15:37 +0300, Amir Goldstein wrote:
> > > > On Thu, Apr 25, 2024 at 2:30 PM Roberto Sassu
> > > > <roberto.sassu@huaweicloud.com> wrote:
> > > > > > 
> > > > > > On Tue, 2024-04-23 at 09:02 +0300, Amir Goldstein wrote:
> > > > > > > > On Mon, Apr 22, 2024 at 6:07 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > > > > > > > > 
> > > > > > > > > > This series fixes the detection of read/write violations on stacked
> > > > > > > > > > filesystems. To be able to access the relevant dentries necessary to
> > > > > > > > > > detect files opened for writing on a stacked filesystem a new d_real_type
> > > > > > > > > > D_REAL_FILEDATA is introduced that allows callers to access all relevant
> > > > > > > > > > files involved in a stacked filesystem while traversing the layers.
> > > > > > > > > > 
> > > > > > > > 
> > > > > > > > Stefan,
> > > > > > > > 
> > > > > > > > Both Miklos and myself objected to this solution:
> > > > > > > > https://lore.kernel.org/linux-unionfs/CAJfpeguctirEYECoigcAsJwpGPCX2NyfMZ8H8GHGW-0UyKfjgg@mail.gmail.com/
> > > > > > > > 
> > > > > > > > Not sure what you are hoping to achieve from re-posting the same solution.
> > > > > > > > 
> > > > > > > > I stopped counting how many times I already argued that *all* IMA/EVM
> > > > > > > > assertions,
> > > > > > > > including rw-ro violations should be enforced only on the real inode.
> > > > > > > > I know this does not work - so you should find out why it does not work and fix
> > > > > > > > the problem.
> > > > > > > > 
> > > > > > > > Enforcing IMA/EVM on the overlayfs inode layer is just the wrong way IMO.
> > > > > > > > Not once have I heard an argument from IMA/EVM developers why it is really
> > > > > > > > needed to enforce IMA/EVM on the overlayfs inode layer and not on the
> > > > > > > > real inode.
> > > > > > 
> > > > > > Ok, I try to provide an example regarding this, and we see if it makes
> > > > > > sense.
> > > > > > 
> > > > > > # echo test > test-file
> > > > > > # chown 2000 d/test-file
> > > > > > # ls -l a/test-file
> > > > > > -rw-r--r--. 1 2000 root 25 Apr 25 10:50 a/test-file
> > > > > > 
> > > > > > Initially there is a file in the lower layer with UID 2000.
> > > > > > 
> > > > > > 
> > > > > > # mount -t overlay -olowerdir=a,upperdir=b,workdir=c,metacopy=on overlay d
> > > > > > # chown 3000 d/test-file
> > > > > > # ls -l d/test-file
> > > > > > -rw-r--r--. 1 3000 root 25 Apr 25 10:50 d/test-file
> > > > > > # ls -l a/test-file
> > > > > > -rw-r--r--. 1 2000 root 25 Apr 25 10:50 a/test-file
> > > > > > # ls -l b/test-file
> > > > > > -rw-r--r--. 1 3000 root 25 Apr 25 10:50 b/test-file
> > > > > > 
> > > > > > If I have a policy like this:
> > > > > > 
> > > > > > # echo "measure fsname=overlay fowner=3000" > /sys/kernel/security/ima/policy
> > > > > > 
> > > > > > there won't be any match on the real file which still has UID 2000. But
> > > > > > what is observable by the processes through overlayfs is UID 3000.
> > > > > > 
> > > > 
> > > > ok, it is simple to write an ima policy that is not respected by overlayfs.
> > > > 
> > > > My question is: under what circumstances is a policy like the above
> > > > useful in the real world?
> > > > 
> > > > Correct me if I am wrong, but AFAIK, the purpose of IMA/EVM is to
> > > > mitigate attack vectors of tampering with files offline or after the
> > > > file's data/metadata were measured. Is that a correct description?
> > 
> > (For now I would talk about IMA, EVM can be considered separately).
> > 
> > The main purpose of IMA is to evaluate files being accessed, and record
> > the access together with a file digest in a measurement list,
> > allow/deny access to the file (appraisal), or add a new event to audit
> > logs.
> > 
> > How files are selected depends on the IMA policy. A rule can be
> > subject-based or object-based, depending on whether respectively
> > process or file attributes are matched. It can also be both.
> > 
> > A subject-based rule means that you identify a process/set of
> > processes, and you evaluate everything it/they read.
> > 
> > An object-based rule means that you identify a file/set of files, and
> > you evaluate any process accessing them.
> > 
> > Since processes normally would access the top most layer (overlayfs),
> > the IMA policy should be written in terms of metadata seen in that
> > layer (but not necessarily).
> > 
> > This is just for identifying the set of files to
> > measure/appraise/audit, not which file is going to be evaluated, which
> > will be always the persistent one.
> > 
> > I have to admit, things are not very clear also to me.
> > 
> > Suppose you have a file in the lower filesystem with SELinux label
> > user_t, and then on overlayfs with metadata copy, you change the label
> > of this file to unconfined_t.
> > 
> > What will happen exactly? On the overlayfs layer, you will have a
> > permission request with the new label unconfined_t, but when overlayfs
> > calls vfs_open(), there will be another permission request with the old
> > label.
> 
> CC Vivek who was involved with ovl+selinux, but I think the answer is
> that ovl sepolicy is expected to be associated with the mount ctx and
> not the objects and there was a need to implement the security hook
> security_inode_copy_up() to be able to compose a safe sepolicy for
> overlayfs.
> 
> > 
> > It is kind of the same challenge we are facing with IMA, we can observe
> > the file operations at different layers. That is why I think having
> > stacked IMA calls is a good idea (other than really fixing the
> > violations).
> > 
> > The current problem, that is very difficult to solve, is that the
> > policy should cover all layers, or some events will be missed. Now we
> > have overlayfs-specific code to detect changes in the backing inode,
> > while with stacked IMA calls, we can detect the change at the layer
> > where it happened (and we can remove the overlayfs-specific code).
> > 
> > Ideally, what I would do to cover all layers is that if there is a
> > match at one layer, the lower layers should automatically match too,
> > but it is not that easy since after the vfs_open() recursive calls we
> > start calling IMA in the botton most layer first.
> > 
> > (What I did with the stacked IMA calls is just an experiment to see how
> > far we can go, but still we didn't make any decision with Mimi).
> > 
> > > > AFAIK, IMA/EVM policy is system-wide and not namespace aware
> > > > so the policy has to be set on the container's host and not inside
> > > > containers. Is that correct?
> > 
> > I know that overlayfs is primarily aiming at containers, but I would
> > suggest to not add that complexity yet and just consider the host.
> > 
> > > > If those statements are true then please try to explain to me what is
> > > > the thread model for tampering with overlayfs files, without tampering
> > > > with the real upper and/or lower files.
> > 
> > I hope at this point is clear that what we care about is that, or the
> > process is reading the content of the file whose digest is recorded in
> > the measurement list, or we must signal to remote verifiers concurrent
> > accesses that make the statement above false.
> > 
> > > > My thesis is that if an IMA/EVM policy is good enough to prevent
> > > > tampering with the real lower/upper files, then no extra measures
> > > > are needed to protect the virtual overlayfs files against tampering.
> > 
> > What you say is correct, but the way you identify files to
> > measure/appraise/audit can be different.
> > 
> 
> IIUC, the problem as you described it, is similar to the problem
> of how to display the overlayfs path of mmaped files in /proc/self/maps
> when the actual inode mapped to memory is the real inode.
> 
> The solution for this problem was the somewhat awkward
> "file with fake path" - it was recently changed to use the new
> file_user_path() helper.
> 
> I'm not sure how this can help IMA, but in theory, you could
> always attach iint state to the real inode and only the the real inode
> but the rules that filter by path/object in the IMA hooks that take a file
> argument could take file_user_path() into account.

Sorry, it took some time to elaborate this proposal, but I think it is
mostly correct. I even tried it (as an experiment, we didn't make any
decision yet).

So, the basic idea is to use the stacked IMA calls I proposed (by
adding security_file_post_open() in backing-file.c, and evaluate each
layer independently.

The policy match on each IMA call will be on the overlayfs inode, so
that we see the same metadata as the process opening the file, but we
attach the state to the real inode (regardless of the layer).

Violations are going to work because there is no layer mismatch (write
on one layer, read on a different layer), they are checked always on
the real inode. Since overlayfs has to always open the real inode,
i_write/readcount will be always incremented.

If there is a match at one layer, IMA is not going to process the file
again, since the result is cached from the previous IMA call.

i_version comparison will also be on the real inode, no need to provide
one on overlayfs.

Invalidation (like by setting security.ima) will happen on the real
inode.

I went even further, to think that when we check the authenticity of
security.ima with EVM we can pass the real inode, but we are not sure
yet.

We will continue to validate this proposal.

Thanks

Roberto

> For example, if you have security_file_post_open(f) in
> backing_file_open(), then you can use d_real_inode()
> in process_measurement to get to the IMA state and check the
> rules referring to the real inode and you can use file_user_path(file)
> to check the rules referring to "front object".
> 
> In case of a nested overlayfs, you should get two post_open
> hook, both will refer to the same IMA state on the real inode, but
> each hook with have a different file_user_path(), so you will have
> an opportunity to apply the path/object based rules on every layer
> in the nested overlayfs.
> 
> I hope what I tried to explain is clear and I hope there are not
> many traps down this road, but you won't know unless you try...
> 
> Thanks,
> Amir.