RE: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure

[Roberto Sassu <roberto.sassu@huawei.com>]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Tuesday, May 12, 2020 5:50 PM
> On Tue, 2020-05-12 at 15:31 +0000, Roberto Sassu wrote:
> > > From: owner-linux-security-module@vger.kernel.org [mailto:owner-
> linux-
> > > security-module@vger.kernel.org] On Behalf Of Mimi Zohar
> > > Sent: Tuesday, May 12, 2020 4:17 PM
> > > On Tue, 2020-05-12 at 07:54 +0000, Roberto Sassu wrote:
> > > > > > > Roberto, EVM is only triggered by IMA, unless you've modified
> the
> > > > > > > kernel to do otherwise.
> > > > > >
> > > > > > EVM would deny xattr/attr operations even if IMA is disabled in
> the
> > > > > > kernel configuration. For example, evm_setxattr() returns the
> value
> > > > > > from evm_protect_xattr(). IMA is not involved there.
> > > > >
> > > > > Commit ae1ba1676b88 ("EVM: Allow userland to permit modification
> of
> > > > > EVM-protected metadata")
> > > introduced EVM_ALLOW_METADATA_WRITES
> > > > > to allow writing the EVM portable and immutable file signatures.
> > > >
> > > > According to Documentation/ABI/testing/evm:
> > > >
> > > > Note that once a key has been loaded, it will no longer be
> > > > possible to enable metadata modification.
> > >
> > > Not any key, but the HMAC key.
> > >
> > > 2         Permit modification of EVM-protected metadata at
> > >           runtime. Not supported if HMAC validation and
> > >           creation is enabled.
> >
> > #ifdef CONFIG_EVM_LOAD_X509
> > void __init evm_load_x509(void)
> > {
> > [...]
> >         rc = integrity_load_x509(INTEGRITY_KEYRING_EVM,
> CONFIG_EVM_X509_PATH);
> >         if (!rc)
> >                 evm_initialized |= EVM_INIT_X509;
> >
> >
> > static ssize_t evm_write_key(struct file *file, const char __user *buf,
> >                              size_t count, loff_t *ppos)
> > {
> > [...]
> >         /* Don't allow a request to freshly enable metadata writes if
> >          * keys are loaded.
> >          */
> >         if ((i & EVM_ALLOW_METADATA_WRITES) &&
> >             ((evm_initialized & EVM_KEY_MASK) != 0) &&
> >             !(evm_initialized & EVM_ALLOW_METADATA_WRITES))
> >                 return -EPERM;
> >
> > Should have been:
> >
> >         if ((i & EVM_ALLOW_METADATA_WRITES) &&
> >             ((evm_initialized & EVM_INIT_HMAC) != 0) &&
> >             !(evm_initialized & EVM_ALLOW_METADATA_WRITES))
> >                 return -EPERM;
> 
> Ok
> 
> >
> > > Each time the EVM protected file metadata is updated, the EVM HMAC
> is
> > > updated, assuming the existing EVM HMAC is valid.  Userspace should
> > > not have access to the HMAC key, so we only allow writing EVM
> > > signatures.
> > >
> > > The only difference between writing the original EVM signature and the
> > > new portable and immutable signature is the security.ima xattr
> > > requirement.  Since the new EVM signature does not include the
> > > filesystem specific data, something else needs to bind the file
> > > metadata to the file data.  Thus the IMA xattr requirement.
> > >
> > > Assuming that the new EVM signature is written last, as long as there
> > > is an IMA xattr, there shouldn't be a problem writing the new EVM
> > > signature.
> >
> >         /* first need to know the sig type */
> >         rc = vfs_getxattr_alloc(dentry, XATTR_NAME_EVM, (char
> **)&xattr_data, 0,
> >                                 GFP_NOFS);
> >         if (rc <= 0) {
> >                 evm_status = INTEGRITY_FAIL;
> >                 if (rc == -ENODATA) {
> >                         rc = evm_find_protected_xattrs(dentry);
> >                         if (rc > 0)
> >                                 evm_status = INTEGRITY_NOLABEL;
> >                         else if (rc == 0)
> >                                 evm_status = INTEGRITY_NOXATTRS; /* new file */
> >
> > If EVM_ALLOW_METADATA_WRITES is cleared, only the first xattr
> > can be written (status INTEGRITY_NOXATTRS is ok). After,
> > evm_find_protected_xattrs() returns rc > 0, so the status is
> > INTEGRITY_NOLABEL, which is not ignored by evm_protect_xattr().
> 
> With EVM HMAC enabled, as a result of writing the first protected
> xattr, an EVM HMAC should be calculated and written in
> evm_inode_post_setxattr().

To solve the ordering issue, wouldn't allowing setxattr() on a file
with portable signature that does not yet pass verification be safe?
evm_update_evmxattr() checks if the signature is portable and
if yes, does not calculate the HMAC.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli