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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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
