From: Mimi Zohar <zohar@linux.ibm.com>
To: Amir Goldstein <amir73il@gmail.com>,
Roberto Sassu <roberto.sassu@huaweicloud.com>
Cc: Christian Brauner <brauner@kernel.org>,
Seth Forshee <sforshee@kernel.org>,
miklos@szeredi.hu, linux-unionfs@vger.kernel.org,
linux-kernel@vger.kernel.org, paul@paul-moore.com,
stefanb@linux.ibm.com, jlayton@kernel.org,
linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org,
Roberto Sassu <roberto.sassu@huawei.com>,
Eric Snowberg <eric.snowberg@oracle.com>
Subject: Re: [RFC][PATCH] overlayfs: Redirect xattr ops on security.evm to security.evm_overlayfs
Date: Thu, 14 Dec 2023 11:09:41 -0500 [thread overview]
Message-ID: <579803fe4750b2ac1cbf31f4d38929c9ec901a41.camel@linux.ibm.com> (raw)
In-Reply-To: <CAOQ4uxhwHgj-bE7N5SNcRZfnVHn9yCdY_=LFuOxEBkVBbrZKiw@mail.gmail.com>
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@huaweicloud.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@huaweicloud.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
next prev parent reply other threads:[~2023-12-14 16:10 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-08 17:23 [RFC][PATCH] overlayfs: Redirect xattr ops on security.evm to security.evm_overlayfs Roberto Sassu
2023-12-08 21:55 ` Amir Goldstein
2023-12-08 22:01 ` Christian Brauner
2023-12-11 14:56 ` Roberto Sassu
2023-12-11 15:36 ` Seth Forshee
2023-12-11 15:41 ` Roberto Sassu
2023-12-11 17:15 ` Seth Forshee
2023-12-11 18:24 ` Amir Goldstein
2023-12-11 18:01 ` Christian Brauner
2023-12-12 10:24 ` Roberto Sassu
2023-12-12 10:44 ` Amir Goldstein
2023-12-12 13:13 ` Roberto Sassu
2023-12-12 15:27 ` Mimi Zohar
2023-12-14 13:42 ` Roberto Sassu
2023-12-14 15:09 ` Amir Goldstein
2023-12-14 16:09 ` Mimi Zohar [this message]
2023-12-14 18:06 ` Amir Goldstein
2023-12-14 19:36 ` Mimi Zohar
2023-12-12 16:20 ` Roberto Sassu
2023-12-11 18:31 ` Amir Goldstein
2023-12-12 12:41 ` Roberto Sassu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=579803fe4750b2ac1cbf31f4d38929c9ec901a41.camel@linux.ibm.com \
--to=zohar@linux.ibm.com \
--cc=amir73il@gmail.com \
--cc=brauner@kernel.org \
--cc=eric.snowberg@oracle.com \
--cc=jlayton@kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=linux-unionfs@vger.kernel.org \
--cc=miklos@szeredi.hu \
--cc=paul@paul-moore.com \
--cc=roberto.sassu@huawei.com \
--cc=roberto.sassu@huaweicloud.com \
--cc=sforshee@kernel.org \
--cc=stefanb@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).