[PATCH] ovl: IMA Call ima_post_mknod_path() on copy_up'd dentry

[PATCH] ovl: IMA Call ima_post_mknod_path() on copy_up'd dentry

[Goldwyn Rodrigues]

Since copy_up() happens when you are modifying a file on overlay,
it is still a new file for the underlying filesystem. Mark it
in IMA for re-evaluating as a new file.

Putting ima calls within overlayfs may not be the best method, but this is
the only one which I thought would work.

Here is a test case:
mount /dev/vdb /lower
mount /dev/vdc /upper
echo "Original contents" > /lower/existingfile.txt
mount -t overlay overlay /mnt -o upperdir=/upper/upper,workdir=/upper/workdir,lowerdir=/lower
echo "New contents" > /mnt/existingfile.txt

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/overlayfs/copy_up.c            | 8 ++++++++
 security/integrity/ima/ima_main.c | 1 +
 2 files changed, 9 insertions(+)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 9e62dcf06fc4..f3f7f65ce4d3 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -21,6 +21,7 @@
 #include <linux/fdtable.h>
 #include <linux/ratelimit.h>
 #include <linux/exportfs.h>
+#include <linux/ima.h>
 #include "overlayfs.h"
 
 #define OVL_COPY_UP_CHUNK_SIZE (1 << 20)
@@ -102,6 +103,11 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
 			goto retry;
 		}
 
+		if (!strcmp(name, XATTR_NAME_IMA)) {
+			ima_post_path_mknod(new);
+			continue;
+		}
+
 		error = security_inode_copy_up_xattr(name);
 		if (error < 0 && error != -EOPNOTSUPP)
 			break;
@@ -485,6 +491,8 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 		err = ovl_set_size(temp, &c->stat);
 	if (!err)
 		err = ovl_set_attr(temp, &c->stat);
+	if (!err)
+		ima_post_path_mknod(c->dentry);
 	inode_unlock(temp->d_inode);
 
 	return err;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index dbd4c8decde0..2229ea2a0ba6 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -449,6 +449,7 @@ void ima_post_path_mknod(struct dentry *dentry)
 	/* needed for re-opening empty files */
 	iint->flags |= IMA_NEW_FILE;
 }
+EXPORT_SYMBOL_GPL(ima_post_path_mknod);
 
 /**
  * ima_read_file - pre-measure/appraise hook decision based on policy
-- 
2.16.4


-- 
Goldwyn

Re: [PATCH] ovl: IMA Call ima_post_mknod_path() on copy_up'd dentry

[Amir Goldstein]

On Thu, Jan 17, 2019 at 1:22 AM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
> Since copy_up() happens when you are modifying a file on overlay,
> it is still a new file for the underlying filesystem. Mark it
> in IMA for re-evaluating as a new file.
>
> Putting ima calls within overlayfs may not be the best method, but this is
> the only one which I thought would work.
>

Doesn't look right.
Overlayfs creates the new inode with vfs_tmpfile() and I think that is
where you should plug the IMA hook.

> Here is a test case:
> mount /dev/vdb /lower
> mount /dev/vdc /upper
> echo "Original contents" > /lower/existingfile.txt
> mount -t overlay overlay /mnt -o upperdir=/upper/upper,workdir=/upper/workdir,lowerdir=/lower
> echo "New contents" > /mnt/existingfile.txt
>

I bet you can reproduce that same issue without overlayfs
by creating an O_TMPFILE from userspace.

The ima_file_check() hook in do_last() does not cover the O_TMPFILE
case.

Thanks,
Amir.

Re: [PATCH] ovl: IMA Call ima_post_mknod_path() on copy_up'd dentry

[Goldwyn Rodrigues]

On  8:57 17/01, Amir Goldstein wrote:
> On Thu, Jan 17, 2019 at 1:22 AM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> >
> > Since copy_up() happens when you are modifying a file on overlay,
> > it is still a new file for the underlying filesystem. Mark it
> > in IMA for re-evaluating as a new file.
> >
> > Putting ima calls within overlayfs may not be the best method, but this is
> > the only one which I thought would work.
> >
> 
> Doesn't look right.
> Overlayfs creates the new inode with vfs_tmpfile() and I think that is
> where you should plug the IMA hook.
> 
> > Here is a test case:
> > mount /dev/vdb /lower
> > mount /dev/vdc /upper
> > echo "Original contents" > /lower/existingfile.txt
> > mount -t overlay overlay /mnt -o upperdir=/upper/upper,workdir=/upper/workdir,lowerdir=/lower
> > echo "New contents" > /mnt/existingfile.txt
> >
> 
> I bet you can reproduce that same issue without overlayfs
> by creating an O_TMPFILE from userspace.
> 
> The ima_file_check() hook in do_last() does not cover the O_TMPFILE
> case.
> 

The problem you mention was resolved by https://lkml.org/lkml/2018/12/18/809
which I have in my tree.

The current patch is on top of that.

-- 
Goldwyn

Re: [PATCH] ovl: IMA Call ima_post_mknod_path() on copy_up'd dentry

[Amir Goldstein]

On Thu, Jan 17, 2019 at 5:02 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
> On  8:57 17/01, Amir Goldstein wrote:
> > On Thu, Jan 17, 2019 at 1:22 AM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> > >
> > > Since copy_up() happens when you are modifying a file on overlay,
> > > it is still a new file for the underlying filesystem. Mark it
> > > in IMA for re-evaluating as a new file.
> > >
> > > Putting ima calls within overlayfs may not be the best method, but this is
> > > the only one which I thought would work.
> > >
> >
> > Doesn't look right.
> > Overlayfs creates the new inode with vfs_tmpfile() and I think that is
> > where you should plug the IMA hook.
> >
> > > Here is a test case:
> > > mount /dev/vdb /lower
> > > mount /dev/vdc /upper
> > > echo "Original contents" > /lower/existingfile.txt
> > > mount -t overlay overlay /mnt -o upperdir=/upper/upper,workdir=/upper/workdir,lowerdir=/lower
> > > echo "New contents" > /mnt/existingfile.txt
> > >
> >
> > I bet you can reproduce that same issue without overlayfs
> > by creating an O_TMPFILE from userspace.
> >
> > The ima_file_check() hook in do_last() does not cover the O_TMPFILE
> > case.
> >
>
> The problem you mention was resolved by https://lkml.org/lkml/2018/12/18/809
> which I have in my tree.
>

The proposed hook ima_post_create_tmpfile() inside do_tmpfile()
takes a file argument, uses only file_inode() and sets IMA_NEW_FILE.

Now because that hook does not get called from vfs_tmpfile()
you want to add more ima hook inside overlayfs code after calling
vfs_tmpfile().

If you move the IMA hook inside vfs_tmpfile() and pass the dentry
or inode, you will get the same result and you won't need to change
overlayfs code.

Is there a problem with that proposal?

Thanks,
Amir.