Menu
▾
▴
Re: [Linux-ima-user] [PATCH v3] evm: provide portable EVM signature version.
|
From: Mikhail K. <vie...@vi...> - 2017-01-09 21:49:25
|
В Mon, 09 Jan 2017 08:40:45 -0500
Mimi Zohar <zo...@li...> пишет:
> On Sat, 2017-01-07 at 01:23 +0300, Mikhail Kurinnoi wrote:
> > 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.
It called for the new portable version only. Dmitry just re-use
available constant from enumeration that not used in this part of code.
Will be fixed.
> > + 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.
Will be fixed.
> > + 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.
Will be fixed.
> > + version = ((const char *)xattr_data)[1];
> > + if (version != 3)
> > + return;
>
> Please use an enumeration.
Will be fixed.
> > + 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?
No, this function should verify and update EVM xattr depending from
results of verification.
I see the point. Will be fixed.
> > + 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.
Will be fixed.
--
Best regards,
Mikhail Kurinnoi
|
