Menu
▾
▴
Re: [Linux-ima-user] [PATCH v3] evm: provide portable EVM signature version.
|
From: Mimi Z. <zo...@li...> - 2017-01-09 13:41:02
|
On Sat, 2017-01-07 at 01:23 +0300, Mikhail Kurinnoi wrote:
> version 3 changes:
> 1) Prevent ima_fix_xattr() use __vfs_setxattr_noperm() if EVM
> signature verification failed.
This can't be right. The boot comnand line option "ima_appraise=fix"
allows IMA to go into a special mode to "fix" the xattrs.
> Since, __vfs_setxattr_noperm() don't count on evm_inode_setxattr(),
> during file unpacking from atchive EVM zero-filled sign could be
> updated by IMA without EVM signature status checking. So, even if you
> have EVM portable signature signed by another key, IMA will update EVM
> signature to HMAC any way, no matter what you have in security.evm
> xattr or you don't have it at all.
EVM and IMA are two separate modules, which communicate with each other
via the evm_verifyxattr() API. Changes to EVM, should not affect IMA.
If they do, then something is not right.
> 2) Added integrity_audit_msg() in order to get some error in audit
> syslog on EVM portable signature verification fail.
>
> version 2 changes:
> 1) evm_verify_hmac() prevent work with EVM portable signature. If file
> signed by EVM portable signature, evm_verify_hmac() return
> INTEGRITY_FAIL;
> 2) evm_inode_post_setxattr() now count on EVM portable signature.
I originally suggested that the portable EVM xattr should be converted
before it is written (in evm_inode_post_setxattr()), but by the time we
get to evm_inode_post_setxattr(), the portable EVM xattr has already
been written. Can the verification be done earlier in
evm_inode_setxattr(), before the portable EVM xattr is written?
Stefan Berger, a colleague, asked if the portable EVM xattr is never
written to disk or immediately converted to an HMAC, how will it be
included in an archive?
> ~~~~~~~~~~~~~~~~~~
> patch for tar v1.29 provide EVM portable signature support for testing.
> ~~~~~~~~~~~~~~~~~~
> --- a/src/extract.c
> +++ b/src/extract.c
> @@ -391,6 +391,10 @@ set_stat (char const *file_name,
> xattrs_xattrs_set (st, file_name, typeflag, 1);
> xattrs_acls_set (st, file_name, typeflag);
> xattrs_selinux_set (st, file_name, typeflag);
> + /* IMA signature. */
> + xattrs_xattrs_set (st, file_name, typeflag, 2);
> + /* EVM portable signature should be the last one. */
> + xattrs_xattrs_set (st, file_name, typeflag, 3);
> }
>
> /* For each entry H in the leading prefix of entries in HEAD that do
>
> --- a/src/xattrs.c
> +++ b/src/xattrs.c
> @@ -668,9 +668,22 @@ xattrs_xattrs_set (struct tar_stat_info const *st,
> For a regular files: all extended attributes are restored during
> the first run except 'security.capability' which is restored in
> 'later_run == 1'. */
> - if (typeflag == REGTYPE
> - && later_run == !!strcmp (keyword, "security.capability"))
> - continue;
> + if (typeflag == REGTYPE) {
> + if (later_run != 1 && !strcmp (keyword, "security.capability"))
> + continue;
> + if (later_run == 1 && strcmp (keyword, "security.capability"))
> + continue;
> + /* IMA signature. */
> + if (later_run != 2 && !strcmp (keyword, "security.ima"))
> + continue;
> + if (later_run == 2 && strcmp (keyword, "security.ima"))
> + continue;
> + /* EVM portable signature should be the last one. */
> + if (later_run != 3 && !strcmp (keyword, "security.evm"))
> + continue;
> + if (later_run == 3 && strcmp (keyword, "security.evm"))
> + continue;
> + }
>
> if (xattrs_masked_out (keyword, false /* extracting */ ))
> /* we don't want to restore this keyword */
>
> ~~~~~~~~~~~~~~~~~~
>
>
>
>
> Signed-off-by: Dmitry Kasatkin <d.k...@sa...>
> Signed-off-by: Mikhail Kurinnoi <vie...@vi...>
>
> --- a/security/integrity/digsig.c 2016-12-31 13:06:26.331828091 +0300
> +++ b/security/integrity/digsig.c 2017-01-05 23:33:23.614541837 +0300
> @@ -71,6 +71,7 @@ int integrity_digsig_verify(const unsign
> return digsig_verify(keyring[id], sig + 1, siglen - 1,
> digest, digestlen);
> case 2:
> + case 3:
> return asymmetric_verify(keyring[id], sig, siglen,
> digest, digestlen);
> }
> --- a/security/integrity/evm/evm_crypto.c 2016-12-31 13:06:26.331828091 +0300
> +++ b/security/integrity/evm/evm_crypto.c 2017-01-05 23:35:48.577389813 +0300
> @@ -169,6 +169,23 @@ static void hmac_add_misc(struct shash_d
> crypto_shash_final(desc, digest);
> }
>
> +static void hmac_add_misc_portable_digsig(struct shash_desc *desc, struct inode *inode,
> + char *digest)
> +{
> + struct h_misc {
> + uid_t uid;
> + gid_t gid;
> + umode_t mode;
> + } hmac_misc;
> +
> + memset(&hmac_misc, 0, sizeof(hmac_misc));
> + hmac_misc.uid = from_kuid(&init_user_ns, inode->i_uid);
> + hmac_misc.gid = from_kgid(&init_user_ns, inode->i_gid);
> + hmac_misc.mode = inode->i_mode;
> + crypto_shash_update(desc, (const u8 *)&hmac_misc, sizeof(hmac_misc));
> + crypto_shash_final(desc, digest);
> +}
> +
> /*
> * Calculate the HMAC value across the set of protected security xattrs.
> *
> @@ -219,7 +236,10 @@ static int evm_calc_hmac_or_hash(struct
> xattr_size = size;
> crypto_shash_update(desc, (const u8 *)xattr_value, xattr_size);
> }
> - hmac_add_misc(desc, inode, digest);
> + if (type == EVM_IMA_XATTR_DIGSIG)
> + hmac_add_misc_portable_digsig(desc, inode, digest);
Calling "hmac_add_misc_portable_digsig" for all signatures is confusing.
It should only be called for the new portable version.
> + else
> + hmac_add_misc(desc, inode, digest);
>
> out:
> kfree(xattr_value);
> @@ -237,10 +257,12 @@ int evm_calc_hmac(struct dentry *dentry,
>
> int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
> const char *req_xattr_value, size_t req_xattr_value_len,
> - char *digest)
> + int version, char *digest)
> {
> return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value,
> - req_xattr_value_len, IMA_XATTR_DIGEST, digest);
> + req_xattr_value_len,
> + version == 3 ? EVM_IMA_XATTR_DIGSIG : IMA_XATTR_DIGEST,
> + digest);
> }
>
> /*
> --- a/security/integrity/evm/evm.h 2016-11-29 01:32:07.248219481 +0300
> +++ b/security/integrity/evm/evm.h 2017-01-05 23:34:06.551383033 +0300
> @@ -48,7 +48,7 @@ int evm_calc_hmac(struct dentry *dentry,
> size_t req_xattr_value_len, char *digest);
> int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
> const char *req_xattr_value,
> - size_t req_xattr_value_len, char *digest);
> + size_t req_xattr_value_len, int version, char *digest);
> int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
> char *hmac_val);
> int evm_init_secfs(void);
> --- a/security/integrity/evm/evm_main.c 2016-12-31 13:06:26.331828091 +0300
> +++ b/security/integrity/evm/evm_main.c 2017-01-07 00:39:24.414082237 +0300
> @@ -116,7 +116,7 @@ static enum integrity_status evm_verify_
> struct evm_ima_xattr_data *xattr_data = NULL;
> struct evm_ima_xattr_data calc;
> enum integrity_status evm_status = INTEGRITY_PASS;
> - int rc, xattr_len;
> + int rc, xattr_len, version;
>
> if (iint && iint->evm_status == INTEGRITY_PASS)
> return iint->evm_status;
> @@ -159,8 +159,11 @@ static enum integrity_status evm_verify_
> rc = -EINVAL;
> break;
> case EVM_IMA_XATTR_DIGSIG:
> + version = ((const char *)xattr_data)[1];
> + if (version == 3)
Please use and define an enumeration.
> + break;
> rc = evm_calc_hash(dentry, xattr_name, xattr_value,
> - xattr_value_len, calc.digest);
> + xattr_value_len, version, calc.digest);
> if (rc)
> break;
> rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
> @@ -236,7 +239,7 @@ enum integrity_status evm_verifyxattr(st
> void *xattr_value, size_t xattr_value_len,
> struct integrity_iint_cache *iint)
> {
> - if (!evm_initialized || !evm_protected_xattr(xattr_name))
> + if (!evm_initialized || (xattr_name && !evm_protected_xattr(xattr_name)))
> return INTEGRITY_UNKNOWN;
>
> if (!iint) {
> @@ -372,6 +375,47 @@ static void evm_reset_status(struct inod
> iint->evm_status = INTEGRITY_UNKNOWN;
> }
>
> +void evm_update_portable_digsig(struct dentry *dentry, const char *xattr_name,
> + const void *xattr_value, size_t xattr_value_len)
> +{
> + const struct evm_ima_xattr_data *xattr_data = xattr_value;
> + struct evm_ima_xattr_data calc;
> + struct evm_ima_xattr_data xattr_data_fail;
> + int version, rc;
> +
> + if ((xattr_data->type != EVM_IMA_XATTR_DIGSIG) || \
> + (strcmp(xattr_name, XATTR_NAME_EVM) != 0))
> + return;
I think it would be clearer if the test was in the caller.
> + version = ((const char *)xattr_data)[1];
> + if (version != 3)
> + return;
Please use an enumeration.
> + evm_reset_status(dentry->d_inode);
> +
> + rc = evm_calc_hash(dentry, xattr_name, xattr_value,
> + xattr_value_len, version, calc.digest);
> + if (rc)
> + goto digsig_fail;
> + rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
> + (const char *)xattr_data, xattr_value_len,
> + calc.digest, sizeof(calc.digest));
> + if (rc)
> + goto digsig_fail;
> +
> + evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len);
Instead of updating the portable EVM xattr, this function should just
verify the portable EVM xattr?
> + return;
> +
> +digsig_fail:
> + integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
> + dentry->d_name.name, "update_metadata",
> + integrity_status_msg[INTEGRITY_FAIL], -EPERM, 0);
> + memset(&xattr_data_fail, 0, sizeof(xattr_data_fail));
> + __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
> + &xattr_data_fail,
> + sizeof(xattr_data_fail), 0);
> +}
> +
> /**
> * evm_inode_post_setxattr - update 'security.evm' to reflect the changes
> * @dentry: pointer to the affected dentry
> @@ -388,8 +432,12 @@ static void evm_reset_status(struct inod
> 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_initialized)
> + return;
> +
> + evm_update_portable_digsig(dentry, xattr_name, xattr_value, xattr_value_len);
If a "protected" xattr was written, security.evm is updated to reflect
the change below.
It would be clearer to move the "security.evm" test here, call to verify
the portable xattr*, and then fall through to do the normal xattr update
below.
*assuming that the portable EVM xattr was written to disk.
Mimi
|
