Re: [PATCH v2 14/25] evm: add support for fscaps security hooks

[Roberto Sassu]

On Fri, 2024-03-01 at 13:54 +0100, Christian Brauner wrote:
> On Fri, Mar 01, 2024 at 10:19:13AM +0100, Roberto Sassu wrote:
> > On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) wrote:
> > > Support the new fscaps security hooks by converting the vfs_caps to raw
> > > xattr data and then handling them the same as other xattrs.
> > 
> > Hi Seth
> > 
> > I started looking at this patch set.
> > 
> > The first question I have is if you are also going to update libcap
> > (and also tar, I guess), since both deal with the raw xattr.
> > 
> > From IMA/EVM perspective (Mimi will add on that), I guess it is
> > important that files with a signature/HMAC continue to be accessible
> > after applying this patch set.
> > 
> > Looking at the code, it seems the case (if I understood correctly,
> > vfs_getxattr_alloc() is still allowed).
> > 
> > To be sure that everything works, it would be really nice if you could
> > also extend our test suite:
> > 
> > https://github.com/mimizohar/ima-evm-utils/blob/next-testing/tests/portable_signatures.test
> > 
> > and
> > 
> > https://github.com/mimizohar/ima-evm-utils/blob/next-testing/tests/evm_hmac.test
> > 
> > 
> > The first test we would need to extend is check_cp_preserve_xattrs,
> > which basically does a cp -a. We would need to set fscaps in the
> > origin, copy to the destination, and see if the latter is accessible.
> > 
> > I would also extend:
> > 
> > check_tar_extract_xattrs_different_owner
> > check_tar_extract_xattrs_same_owner
> > check_metadata_change
> > check_evm_revalidate
> > check_evm_portable_sig_ima_appraisal
> > check_evm_portable_sig_ima_measurement_list
> > 
> > It should not be too complicated. The purpose would be to exercise your
> > code below.
> > 
> > 
> > Regarding the second test, we would need to extend just check_evm_hmac.
> > 
> > 
> > Just realized, before extending the tests, it would be necessary to
> > modify also evmctl.c, to retrieve fscaps through the new interfaces,
> > and to let users provide custom fscaps the HMAC or portable signature
> > is calculated on.
> 
> While request for tests are obviously fine they should be added by the
> respective experts for IMA/EVM in this case. I don't think it's
> appropriate to expect Seth to do that especially because you seem to
> imply that you currently don't have any tests for fscaps at all. We're
> always happy to test things and if that'd be adding new IMA/EVM specific
> features than it would be something to discuss but really we're
> refactoring so the fact that you don't have tests we can run is not the
> fault of this patchset and IMA/EVM is just a small portion of it. 

Hi Christian

I have seen this policy of adding tests in other subsystems (eBPF),
which in my opinion makes sense, since you want anyway to check that
you didn't break existing code.

And yes, I agree that we should have better tests, and a better
workflow (we are working on improving it).

In this particular case, I was not asking to write a test from scratch,
that should not be difficult per se, but adding additional commands.

If I got it correctly, even if current tests for fscaps would have
existed, they would not work anyway, since they would have been based
on getting/setting the raw xattrs (as far as I know, at least for tar).

Happy to try adding the tests, would appreciate your help to review if
the tests are done in the correct way.

Thanks

Roberto