Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <85070b837c134341bb10a2647dbe62e2b5806565.camel@linux.ibm.com>
Subject: Re: [PATCH] overlayfs: Trigger file re-evaluation by IMA / EVM
 after writes
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Amir Goldstein <amir73il@gmail.com>
Cc:     Christian Brauner <brauner@kernel.org>,
        Jeff Layton <jlayton@kernel.org>,
        Stefan Berger <stefanb@linux.ibm.com>,
        Paul Moore <paul@paul-moore.com>,
        linux-integrity@vger.kernel.org, miklos@szeredi.hu,
        linux-kernel@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, linux-unionfs@vger.kernel.org,
        Ignaz Forster <iforster@suse.de>, Petr Vorel <pvorel@suse.cz>
Date:   Tue, 23 May 2023 15:38:52 -0400
In-Reply-To: <CAOQ4uxjjLLGQM5BUgDrFdghYsFgShNA6tDpvC8vNg_jOh9WGAQ@mail.gmail.com>
References: <20230407-trasse-umgearbeitet-d580452b7a9b@brauner>
         <078d8c1fd6b6de59cde8aa85f8e59a056cb78614.camel@linux.ibm.com>
         <CAOQ4uxi7PFPPUuW9CZAZB9tvU2GWVpmpdBt=EUYyw60K=WX-Yg@mail.gmail.com>
         <9aced306f134628221c55530643535b89874ccc0.camel@linux.ibm.com>
         <CAOQ4uxjjLLGQM5BUgDrFdghYsFgShNA6tDpvC8vNg_jOh9WGAQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
Precedence: bulk

On Mon, 2023-05-22 at 17:00 +0300, Amir Goldstein wrote:
> On Mon, May 22, 2023 at 3:18 PM Mimi Zohar <zohar@linux.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@linux.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?

Yes

> 
> 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.

Understood.  Accessing a file on the overlay filesystem then needs to
be verified based on the backing file metadata.  Currently that isn't
being done.  So either all the backing file metadata needs to be copied
up or some other change(s) need to be made.

> 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.   It definitely helps.

To summarize what I'm seeing (IMA hash and EVM HMAC):

- Directly accessing overlay files, "lower" backed file, fails to
verify without copying all the file metadata up.

- Writing directly to the "upper" backing file properly updates the
file metadata.

- Writing directly to the overlay file does not write security.ima
either to the overlayfs  or the "upper" backing file.

policy rules:
appraise func=FILE_CHECK fsuuid=....
measure func=FILE_CHECK fsuuid=....
appraise func=FILE_CHECK fsname=overlay 
measure func=FILE_CHECK fsname=overlay

Mimi