On Thu, 2017-11-02 at 15:10 -0700, Matthew Garrett wrote:
> When EVM is enabled it forbids modification of metadata are protected by
> EVM unless there is already a valid EVM signature. If any modification
> is made, the kernel will then generate a new EVM HMAC. However, this
> does not map well on use cases which use only asymmetric EVM signatures,
> as in this scenario the kernel is unable to generate new signatures.
>
> This patch extends the /sys/kernel/security/evm interface to allow
> userland to request that modification of these xattrs be permitted. This
> is only permitted if no keys have already been loaded. In this
> configuration, modifying the metadata will invalidate the EVM appraisal
> on the file in question. This allows packaging systems to write out new
> files, set the relevant extended attributes and then move them into
> place.
>
> There's also some refactoring of the use of evm_initialized in order to
> avoid heading down codepaths that assume there's a key available.
>
> Signed-off-by: Matthew Garrett <mjg59@xxxxxxxxxx>
> ---
> Documentation/ABI/testing/evm | 51 +++++++++++++++++++++++++-------------
> security/integrity/evm/evm.h | 5 +++-
> security/integrity/evm/evm_main.c | 32 ++++++++++++++++++------
> security/integrity/evm/evm_secfs.c | 14 +++++++++++
> 4 files changed, 77 insertions(+), 25 deletions(-)
>
> diff --git a/Documentation/ABI/testing/evm b/Documentation/ABI/testing/evm
> index d2782afb0d96..b0f54fe8d0c6 100644
> --- a/Documentation/ABI/testing/evm
> +++ b/Documentation/ABI/testing/evm
> @@ -14,28 +14,45 @@ Description:
> generated either locally or remotely using an
> asymmetric key. These keys are loaded onto root's
> keyring using keyctl, and EVM is then enabled by
> - echoing a value to <securityfs>/evm:
> + echoing a value to <securityfs>/evm made up of the
> + following bits:
>
> - 1: enable HMAC validation and creation
> - 2: enable digital signature validation
> - 3: enable HMAC and digital signature validation and HMAC
> - creation
> + Bit Effect
> + 0 Enable HMAC validation and creation
The code and documentation do not seem to be in sync. Dracut is
currently using 1 to indicate the HMAC key has been loaded.
> + 1 Enable digital signature validation
> + 2 Permit modification of EVM-protected metadata at
> + runtime. Not supported if HMAC validation and
> + creation is enabled.
> + 31 Disable further runtime modification of EVM policy
>
> - Further writes will be blocked if HMAC support is enabled or
> - if bit 32 is set:
> + For example:
>
> - echo 0x80000002 ><securityfs>/evm
> + echo 1 ><securityfs>/evm
>
> - will enable digital signature validation and block
> - further writes to <securityfs>/evm.
> + will enable HMAC validation and creation
>
> - Until this is done, EVM can not create or validate the
> - 'security.evm' xattr, but returns INTEGRITY_UNKNOWN.
> - Loading keys and signaling EVM should be done as early
> - as possible. Normally this is done in the initramfs,
> - which has already been measured as part of the trusted
> - boot. For more information on creating and loading
> - existing trusted/encrypted keys, refer to:
> + echo 0x80000003 ><securityfs>/evm
> +
> + will enable HMAC and digital signature validation and
> + HMAC creation and disable all further modification of policy.
> +
> + echo 0x80000006 ><securityfs>/evm
4 is missing above.
> +
> + will enable digital signature validation, permit
> + modification of EVM-protected metadata and
> + disable all further modification of policy
> +
> + Note that once a key has been loaded, it will no longer be
> + possible to enable metadata modification.
> +
> + Until key loading has been signaled EVM can not create
> + or validate the 'security.evm' xattr, but returns
> + INTEGRITY_UNKNOWN. Loading keys and signaling EVM
> + should be done as early as possible. Normally this is
> + done in the initramfs, which has already been measured
> + as part of the trusted boot. For more information on
> + creating and loading existing trusted/encrypted keys,
> + refer to:
> Documentation/keys-trusted-encrypted.txt. Both dracut
> (via 97masterkey and 98integrity) and systemd (via
> core/ima-setup) have support for loading keys at boot
> diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
> index 946efffcc389..6c63db19fbcf 100644
> --- a/security/integrity/evm/evm.h
> +++ b/security/integrity/evm/evm.h
> @@ -23,9 +23,12 @@
>
> #define EVM_INIT_HMAC 0x0001
> #define EVM_INIT_X509 0x0002
> +#define EVM_ALLOW_METADATA_WRITES 0x0004
> #define EVM_SETUP 0x80000000 /* userland has signaled key load */
Sorry for not commenting earlier on this bit's naming, but it's
confusing. Perhaps something like EVM_SETUP_COMPLETED/FINISHED?
>
> -#define EVM_INIT_MASK (EVM_INIT_HMAC | EVM_INIT_X509 | EVM_SETUP)
> +#define EVM_KEY_MASK (EVM_INIT_HMAC | EVM_INIT_X509)
> +#define EVM_INIT_MASK (EVM_INIT_HMAC | EVM_INIT_X509 | EVM_SETUP | \
> + EVM_ALLOW_METADATA_WRITES)
>
> extern int evm_initialized;
> extern char *evm_hmac;
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 675a835b6d6d..0788fd08b509 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -73,6 +73,11 @@ static void __init evm_init_config(void)
> pr_info("HMAC attrs: 0x%x\n", evm_hmac_attrs);
> }
>
> +static bool evm_key_loaded(void)
> +{
> + return (bool)(evm_initialized & EVM_KEY_MASK);
> +}
> +/
> static int evm_find_protected_xattrs(struct dentry *dentry)
> {
> struct inode *inode = d_backing_inode(dentry);
> @@ -243,7 +248,7 @@ enum integrity_status evm_verifyxattr(struct dentry *dentry,
> void *xattr_value, size_t xattr_value_len,
> struct integrity_iint_cache *iint)
> {
> - if (!evm_initialized || !evm_protected_xattr(xattr_name))
> + if (!evm_key_loaded() || !evm_protected_xattr(xattr_name))
> return INTEGRITY_UNKNOWN;
>
> if (!iint) {
> @@ -267,7 +272,7 @@ static enum integrity_status evm_verify_current_integrity(struct dentry *dentry)
> {
> struct inode *inode = d_backing_inode(dentry);
>
> - if (!evm_initialized || !S_ISREG(inode->i_mode) || evm_fixmode)
> + if (!evm_key_loaded() || !S_ISREG(inode->i_mode) || evm_fixmode)
> return 0;
> return evm_verify_hmac(dentry, NULL, NULL, 0, NULL);
> }
> @@ -302,6 +307,7 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
> return 0;
> goto out;
> }
> +
> evm_status = evm_verify_current_integrity(dentry);
> if (evm_status == INTEGRITY_NOXATTRS) {
> struct integrity_iint_cache *iint;
> @@ -348,6 +354,10 @@ int evm_inode_setxattr(struct dentry *dentry, const char *xattr_name,
> {
> const struct evm_ima_xattr_data *xattr_data = xattr_value;
>
> + /* Policy permits modification of the protected xattrs */
> + if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> + return 0;
> +
> if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) {
> if (!xattr_value_len)
> return -EINVAL;
> @@ -369,6 +379,10 @@ int evm_inode_setxattr(struct dentry *dentry, const char *xattr_name,
> */
> int evm_inode_removexattr(struct dentry *dentry, const char *xattr_name)
> {
> + /* Policy permits modification of the protected xattrs */
> + if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> + return 0;
> +
> return evm_protect_xattr(dentry, xattr_name, NULL, 0);
> }
>
> @@ -397,8 +411,8 @@ static void evm_reset_status(struct inode *inode)
> void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
> const void *xattr_value, size_t xattr_value_len)
> {
> - if (!evm_initialized || (!evm_protected_xattr(xattr_name)
> - && !posix_xattr_acl(xattr_name)))
> + if (!evm_key_loaded() || (!evm_protected_xattr(xattr_name)
> + && !posix_xattr_acl(xattr_name)))
> return;
>
> evm_reset_status(dentry->d_inode);
> @@ -418,7 +432,7 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
> */
> void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
> {
> - if (!evm_initialized || !evm_protected_xattr(xattr_name))
> + if (!evm_key_loaded() || !evm_protected_xattr(xattr_name))
> return;
>
> evm_reset_status(dentry->d_inode);
> @@ -435,6 +449,10 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
> unsigned int ia_valid = attr->ia_valid;
> enum integrity_status evm_status;
>
> + /* Policy permits modification of the protected attrs */
Could we indicate that there is no HMAC key loaded.
> + if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> + return 0;
> +
> if (!(ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)))
> return 0;
> evm_status = evm_verify_current_integrity(dentry);
> @@ -460,7 +478,7 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
> */
> void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
> {
> - if (!evm_initialized)
> + if (!evm_key_loaded())
> return;
>
> if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
> @@ -477,7 +495,7 @@ int evm_inode_init_security(struct inode *inode,
> struct evm_ima_xattr_data *xattr_data;
> int rc;
>
> - if (!evm_initialized || !evm_protected_xattr(lsm_xattr->name))
> + if (!evm_key_loaded() || !evm_protected_xattr(lsm_xattr->name))
> return 0;
>
> xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
> diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
> index 319cf16d6603..ee8e3abb7dbb 100644
> --- a/security/integrity/evm/evm_secfs.c
> +++ b/security/integrity/evm/evm_secfs.c
> @@ -75,6 +75,14 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf,
> if (!i || (i & ~EVM_INIT_MASK) != 0)
> return -EINVAL;
>
> + /* 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))
Ok, not sure that the "(evm_initialized & EVM_ALLOW_METADATA_WRITES)"
is needed, but it doesn't hurt.
Mimi
> + return -EPERM;
> +
> if (i & EVM_INIT_HMAC) {
> ret = evm_init_key();
> if (ret != 0)
> @@ -85,6 +93,12 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf,
>
> evm_initialized |= i;
>
> + /* Don't allow protected metadata modification if a symmetric key
> + * is loaded
> + */
> + if (evm_initialized & EVM_INIT_HMAC)
> + evm_initialized &= ~(EVM_ALLOW_METADATA_WRITES);
> +
> return count;
> }
>
