Message-ID: <CAOQ4uxjjLLGQM5BUgDrFdghYsFgShNA6tDpvC8vNg_jOh9WGAQ@mail.gmail.com>
Date:   Mon, 22 May 2023 17:00:09 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Mimi Zohar <zohar@...ux.ibm.com>
Cc:     Christian Brauner <brauner@...nel.org>,
        Jeff Layton <jlayton@...nel.org>,
        Stefan Berger <stefanb@...ux.ibm.com>,
        Paul Moore <paul@...l-moore.com>,
        linux-integrity@...r.kernel.org, miklos@...redi.hu,
        linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-unionfs@...r.kernel.org,
        Ignaz Forster <iforster@...e.de>, Petr Vorel <pvorel@...e.cz>
Subject: Re: [PATCH] overlayfs: Trigger file re-evaluation by IMA / EVM after writes

On Mon, May 22, 2023 at 3:18 PM Mimi Zohar <zohar@...ux.ibm.com> wrote:
>
> On Sat, 2023-05-20 at 12:15 +0300, Amir Goldstein wrote:
> > On Fri, May 19, 2023 at 10:42 PM Mimi Zohar <zohar@...ux.ibm.com> wrote:
> > >
> > > On Fri, 2023-04-07 at 10:31 +0200, Christian Brauner wrote:
> > > > So, I think we want both; we want the ovl_copyattr() and the
> > > > vfs_getattr_nosec() change:
> > > >
> > > > (1) overlayfs should copy up the inode version in ovl_copyattr(). That
> > > >     is in line what we do with all other inode attributes. IOW, the
> > > >     overlayfs inode's i_version counter should aim to mirror the
> > > >     relevant layer's i_version counter. I wouldn't know why that
> > > >     shouldn't be the case. Asking the other way around there doesn't
> > > >     seem to be any use for overlayfs inodes to have an i_version that
> > > >     isn't just mirroring the relevant layer's i_version.
> > > > (2) Jeff's changes for ima to make it rely on vfs_getattr_nosec().
> > > >     Currently, ima assumes that it will get the correct i_version from
> > > >     an inode but that just doesn't hold for stacking filesystem.
> > > >
> > > > While (1) would likely just fix the immediate bug (2) is correct and
> > > > _robust_. If we change how attributes are handled vfs_*() helpers will
> > > > get updated and ima with it. Poking at raw inodes without using
> > > > appropriate helpers is much more likely to get ima into trouble.
> > >
> > > In addition to properly setting the i_version for IMA, EVM has a
> > > similar issue with i_generation and s_uuid. Adding them to
> > > ovl_copyattr() seems to resolve it.   Does that make sense?
> > >
> > > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > > index 923d66d131c1..cd0aeb828868 100644
> > > --- a/fs/overlayfs/util.c
> > > +++ b/fs/overlayfs/util.c
> > > @@ -1118,5 +1118,8 @@ void ovl_copyattr(struct inode *inode)
> > >         inode->i_atime = realinode->i_atime;
> > >         inode->i_mtime = realinode->i_mtime;
> > >         inode->i_ctime = realinode->i_ctime;
> > > +       inode->i_generation = realinode->i_generation;
> > > +       if (inode->i_sb)
> > > +               uuid_copy(&inode->i_sb->s_uuid, &realinode->i_sb-
> > > >s_uuid);
> >
> > That is not a possible solution Mimi.
> >
> > The i_gneration copy *may* be acceptable in "all layers on same fs"
> > setup, but changing overlayfs s_uuid over and over is a non-starter.
> >
> > If you explain the problem, I may be able to help you find a better solution.
>
> EVM calculates an HMAC of the file metadata (security xattrs, i_ino,
> i_generation, i_uid, i_gid, i_mode, s_uuid)  and stores it as
> security.evm.  Notrmally this would be used for mutable files, which
> cannot be signed.  The i_generation and s_uuid on the lower layer and
> the overlay are not the same, causing the EVM HMAC verification to
> fail.
>

OK, so EVM expects i_ino, i_generation, i_uid, i_gid, i_mode, s_uuid
and security xattr to remain stable and persistent (survive umount/mount).
Correct?

You cannot expect that the same EVM xattr will correctly describe both
the overlayfs inode and the underlying real fs inode, because they may
vary in some of the metadata, so need to decide if you only want to attest
overlayfs inodes, real underlying inodes or both.
If both, then the same EVM xattr cannot be used, but as it is, overlayfs
inode has no "private" xattr version, it stores its xattr on the underlying
real inode.

i_uid, i_gid, i_mode:
Should be stable and persistent for overlayfs inode and survive copy up.
Should be identical to the underlying inode.

security xattr:
Overlayfs tries to copy up all security.* xattr and also calls the LSM
hook security_inode_copy_up_xattr() to approve each copied xattr.
Should be identical to the underlying inode.

s_uuid:
So far, overlayfs sb has a null uuid.
With this patch, overlayfs will gain a persistent s_uuid, just like any
other disk fs with the opt-in feature index=on:
https://lore.kernel.org/linux-unionfs/20230425132223.2608226-4-amir73il@gmail.com/
Should be different from the underlying fs uuid when there is more
than one underlying fs.
We can consider inheriting s_uuid from underlying fs when all layers
are on the same fs.

i_ino:
As documented in:
https://github.com/torvalds/linux/blob/master/Documentation/filesystems/overlayfs.rst#inode-properties
It should be persistent and survive copy up with the
xino=auto feature (module param or mount option) or
CONFIG_OVERLAY_FS_XINO_AUTO=y
which is not the kernel default, but already set by some distros.
Will be identical to the underlying inode only in some special cases
such as pure upper (not copied up) inodes.
Will be different from the underlying lower file inode many in other cases.

i_generation:
For xino=auto, we could follow the same rules as i_ino and get similar
qualities -
i_generation will become persistent and survive copy up, but it will not be
identical to the real underlying inode i_generation in many cases.

Bottom line:
If you only want to attest overlayfs inodes - shouldn't be too hard
If you want to attest both overlayfs inodes AND their backing "real" inodes -
much more challenging.

Hope that this writeup helps more than it confuses.

Thanks,
Amir.

Powered by blists - more mailing lists