Message-ID: <579803fe4750b2ac1cbf31f4d38929c9ec901a41.camel@linux.ibm.com>
Date:   Thu, 14 Dec 2023 11:09:41 -0500
From:   Mimi Zohar <zohar@...ux.ibm.com>
To:     Amir Goldstein <amir73il@...il.com>,
        Roberto Sassu <roberto.sassu@...weicloud.com>
Cc:     Christian Brauner <brauner@...nel.org>,
        Seth Forshee <sforshee@...nel.org>, miklos@...redi.hu,
        linux-unionfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        paul@...l-moore.com, stefanb@...ux.ibm.com, jlayton@...nel.org,
        linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-fsdevel@...r.kernel.org,
        Roberto Sassu <roberto.sassu@...wei.com>,
        Eric Snowberg <eric.snowberg@...cle.com>
Subject: Re: [RFC][PATCH] overlayfs: Redirect xattr ops on security.evm to
 security.evm_overlayfs

On Thu, 2023-12-14 at 17:09 +0200, Amir Goldstein wrote:
> On Thu, Dec 14, 2023 at 3:43 PM Roberto Sassu
> <roberto.sassu@...weicloud.com> wrote:
> >
> > On Tue, 2023-12-12 at 10:27 -0500, Mimi Zohar wrote:
> > > On Tue, 2023-12-12 at 14:13 +0100, Roberto Sassu wrote:
> > > > On 12.12.23 11:44, Amir Goldstein wrote:
> > > > > On Tue, Dec 12, 2023 at 12:25 PM Roberto Sassu
> > > > > <roberto.sassu@...weicloud.com> wrote:
> > > > > >
> > > > > > On 11.12.23 19:01, Christian Brauner wrote:
> > > > > > > > The second problem is that one security.evm is not enough. We need two,
> > > > > > > > to store the two different HMACs. And we need both at the same time,
> > > > > > > > since when overlayfs is mounted the lower/upper directories can be
> > > > > > > > still accessible.
> > > > > > >
> > > > > > > "Changes to the underlying filesystems while part of a mounted overlay
> > > > > > > filesystem are not allowed. If the underlying filesystem is changed, the
> > > > > > > behavior of the overlay is undefined, though it will not result in a
> > > > > > > crash or deadlock."
> > > > > > >
> > > > > > > https://docs.kernel.org/filesystems/overlayfs.html#changes-to-underlying-filesystems
> > > > > > >
> > > > > > > So I don't know why this would be a problem.
> > > > > >
> > > > > > + Eric Snowberg
> > > > > >
> > > > > > Ok, that would reduce the surface of attack. However, when looking at:
> > > > > >
> > > > > >        ovl: Always reevaluate the file signature for IMA
> > > > > >
> > > > > >        Commit db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the
> > > > > > i_version")
> > > > > >        partially closed an IMA integrity issue when directly modifying a file
> > > > > >        on the lower filesystem.  If the overlay file is first opened by a
> > > > > > user
> > > > > >        and later the lower backing file is modified by root, but the extended
> > > > > >        attribute is NOT updated, the signature validation succeeds with
> > > > > > the old
> > > > > >        original signature.
> > > > > >
> > > > > > Ok, so if the behavior of overlayfs is undefined if the lower backing
> > > > > > file is modified by root, do we need to reevaluate? Or instead would be
> > > > > > better to forbid the write from IMA (legitimate, I think, since the
> > > > > > behavior is documented)? I just saw that we have d_real_inode(), we can
> > > > > > use it to determine if the write should be denied.
> > > > > >
> > > > >
> > > > > There may be several possible legitimate actions in this case, but the
> > > > > overall concept IMO should be the same as I said about EVM -
> > > > > overlayfs does not need an IMA signature of its own, because it
> > > > > can use the IMA signature of the underlying file.
> > > > >
> > > > > Whether overlayfs reads a file from lower fs or upper fs, it does not
> > > > > matter, the only thing that matters is that the underlying file content
> > > > > is attested when needed.
> > > > >
> > > > > The only incident that requires special attention is copy-up.
> > > > > This is what the security hooks security_inode_copy_up() and
> > > > > security_inode_copy_up_xattr() are for.
> > > > >
> > > > > When a file starts in state "lower" and has security.ima,evm xattrs
> > > > > then before a user changes the file, it is copied up to upper fs
> > > > > and suppose that security.ima,evm xattrs are copied as is?
> > >
> > > For IMA copying up security.ima is fine.  Other than EVM portable
> > > signatures, security.evm contains filesystem specific metadata.
> > > Copying security.evm up only works if the metadata is the same on both
> > > filesystems.  Currently the i_generation and i_sb->s_uuid are
> > > different.
> > >
> > > > > When later the overlayfs file content is read from the upper copy
> > > > > the security.ima signature should be enough to attest that file content
> > > > > was not tampered with between going from "lower" to "upper".
> > > > >
> > > > > security.evm may need to be fixed on copy up, but that should be
> > > > > easy to do with the security_inode_copy_up_xattr() hook. No?
> > >
> > > Writing security.evm requires the existing security.evm to be valid.
> > > After each security xattr in the protected list is modified,
> > > security.evm HMAC needs to be updated.  Perhaps calculating and writing
> > > security.evm could be triggered by security_inode_copy_up_xattr().
> > > Just copying a non-portable EVM signature wouldn't work, or for that
> > > matter copying an EVM HMAC with different filesystem metadata.
> >
> > There is another problem, when delayed copy is used. The content comes
> > from one source, metadata from another.
> >
> > I initially created test-file-lower on the lower directory
> > (overlayfs/data), before mounting overlayfs. After mount on
> > overlayfs/mnt:
> >
> > # getfattr -m - -e hex -d overlayfs/mnt/test-file-lower
> > # file: overlayfs/mnt/test-file-lower
> > security.evm=0x02c86ec91a4c0cf024537fd24347b780b90973402e
> > security.ima=0x0404f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2
> > security.selinux=0x73797374656d5f753a6f626a6563745f723a756e6c6162656c65645f743a733000
> >
> > # chcon -t unconfined_t overlayfs/mnt/test-file-lower
> >
> > After this, IMA creates an empty file in the upper directory
> > (overlayfs/root/data), and writes security.ima at file close.
> > Unfortunately, this is what is presented from overlayfs, which is not
> > in sync with the content.
> >
> > # getfattr -m - -e hex -d overlayfs/mnt/test-file-lower
> > # file: overlayfs/mnt/test-file-lower
> > security.evm=0x021d71e7df78c36745e3b651ce29cb9f47dc301248
> > security.ima=0x04048855508aade16ec573d21e6a485dfd0a7624085c1a14b5ecdd6485de0c6839a4
> > security.selinux=0x73797374656d5f753a6f626a6563745f723a756e636f6e66696e65645f743a733000
> >
> > # sha256sum overlayfs/mnt/test-file-lower
> > f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2  overlayfs/mnt/test-file-lower
> >
> > # sha256sum overlayfs/root/data/test-file-lower
> > 8855508aade16ec573d21e6a485dfd0a7624085c1a14b5ecdd6485de0c6839a4  overlayfs/root/data/test-file-lower (upperdir)
> >
> > We would need to use the lower security.ima until the copy is made, but
> > at the same time we need to keep the upper valid (with all xattrs) so
> > that IMA can update the next time overlayfs requests that.
> >
> 
> Yap.
> 
> As Seth wrote, overlayfs is a combination of upper and lower.
> The information that IMA needs should be accessible from either lower
> or upper, but sometimes we will need to make the right choice.
> 
> The case of security.ima is similar to that of st_blocks -
> it is a data-related metadata, so it needs to be taken from the lowerdata inode
> (not even the lower inode). See example of getting STATX_BLOCKS
> in ovl_getattr().
> 
> I would accept a patch that special cases security.ima in ovl_xattr_get()
> and gets it from ovl_i_path_lowerdata(), which would need to be
> factored out of ovl_path_lowerdata().
> 
> I would also accept filtering out security.{ima,evm} from
> 
> But I would only accept it if I know that IMA is not trying to write the
> security.ima xattr when closing an overlayfs file, only when closing the
> real underlying upper file.

I don't see how that would be possible.  As far as I'm aware, the
correlation is between the overlay and the underlying lower/uppper
file, not the other way around.  How could a close on the underlying
file trigger IMA on an overlay file?

> 
> I would also expect IMA to filter out security.{ima,evm} xattrs in
> security_inode_copy_up_xattr() (i.e. return 1).
> and most importantly, a documentation of the model of IMA/EVM
> and overlayfs.

Ok.

Mimi

Powered by blists - more mailing lists