Re: [PATCH] ima: fix wrong dereferences of file->f_path

[Mimi Zohar <zohar@linux.ibm.com>]

On Wed, 2023-09-13 at 15:09 +0300, Amir Goldstein wrote:
> On Wed, Sep 13, 2023 at 10:38 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > When storing IMA xattr on an overlayfs inode, the xattr is actually
> > stored in the inode of the underlying (a.k.a real) filesystem, so there
> > is an ambiguity whether this IMA xattr describes the integrity of the
> > overlayfs inode or the real inode.
> >
> > For this reason and other reasons, IMA is not supported on overlayfs,
> > in the sense that integrity checking on the overlayfs inode/file/path
> > do not work correctly and have undefined behavior and the IMA xattr
> > always describes the integrity of the real inode.
> >
> > When a user operates on an overlayfs file, whose underlying real file
> > has IMA enabled, IMA should always operate on the real path and not
> > on the overlayfs path.
> >
> > IMA code already uses the helper file_dentry() to get the dentry
> > of the real file. Dereferencing file->f_path directly means that IMA
> > will operate on the overlayfs inode, which is wrong.
> >
> > Therefore, all dereferences to f_path were converted to use the
> > file_real_path() helper.

Thanks, Amir.  This sounds right.

> >
> > Reported-by: syzbot+a67fc5321ffb4b311c98@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/linux-unionfs/0000000000005bd097060530b758@google.com/
> > Fixes: db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version")
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Jeff Layton <jlayton@kernel.org>
> > Cc: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > Mimi,
> >
> > Some of the wrong f_path dereferences are much older than the Fixes
> > commit, but they did not have as big an impact as the wrong f_path
> > dereference that the Fixes commit introduced.
> >
> > For example, commit a408e4a86b36 ("ima: open a new file instance if no
> > read permissions") worked because reading the content of the overlayfs
> > file has the same result as reading the content of the real file, but it
> > is actually the real file integrity that we want to verify.
> >
> > Anyway, the real path information, that is now available via the
> > file_real_path() helper, was not available in IMA integrity check context
> > at the time that commit a408e4a86b36 was merged.
> 
> Only problem is that fix did not resolve the syzbot bug, which
> seems to do the IMA integrity check on overlayfs file (not sure).
> 
> I am pretty sure that this patch fixes "a bug" when IMA is on the filesystem
> under overlayfs and this is a pretty important use case.

Agreed.

> But I guess there are still issues with IMA over overlayfs and this is not
> the only one.

Sigh

> Is this really a use case that needs to be supported?
> Isn't the newly added SB_I_IMA_UNVERIFIABLE_SIGNATUREh flag
> a hint that IMA on overlayfs is not a good idea at all?

With  SB_I_IMA_UNVERIFIABLE_SIGNATURE enabled for overlayfs, signature
verification will then fail immediately for all overlayfs files in
policy.  I don't think that's the right solution.  Verification should
be limited to when the overlayfs file is the same as the underlying
backing store, the real inode, not the overlay upper files.

-- 
Thanks,

Mimi