| Message ID | 1496886555-10082-7-git-send-email-bauerman@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Thiago, On Wed, 2017-06-07 at 22:49 -0300, Thiago Jung Bauermann wrote: > This patch introduces the modsig keyword to the IMA policy syntax to > specify that a given hook should expect the file to have the IMA signature > appended to it. Here is how it can be used in a rule: > > appraise func=KEXEC_KERNEL_CHECK appraise_type=modsig|imasig > > With this rule, IMA will accept either an appended signature or a signature > stored in the extended attribute. In that case, it will first check whether > there is an appended signature, and if not it will read it from the > extended attribute. > > The format of the appended signature is the same used for signed kernel > modules. This means that the file can be signed with the scripts/sign-file > tool, with a command line such as this: > > $ sign-file sha256 privkey_ima.pem x509_ima.der vmlinux > > This code only works for files that are hashed from a memory buffer, not > for files that are read from disk at the time of hash calculation. In other > words, only hooks that use kernel_read_file can support appended > signatures. This means that only FIRMWARE_CHECK, KEXEC_KERNEL_CHECK, > KEXEC_INITRAMFS_CHECK and POLICY_CHECK can be supported. > > This feature warrants a separate config option because it depends on many > other config options: > > ASYMMETRIC_KEY_TYPE n -> y > CRYPTO_RSA n -> y > INTEGRITY_SIGNATURE n -> y > MODULE_SIG_FORMAT n -> y > SYSTEM_DATA_VERIFICATION n -> y > +ASN1 y > +ASYMMETRIC_PUBLIC_KEY_SUBTYPE y > +CLZ_TAB y > +CRYPTO_AKCIPHER y > +IMA_APPRAISE_MODSIG y > +IMA_TRUSTED_KEYRING n > +INTEGRITY_ASYMMETRIC_KEYS y > +INTEGRITY_TRUSTED_KEYRING n > +MPILIB y > +OID_REGISTRY y > +PKCS7_MESSAGE_PARSER y > +PKCS7_TEST_KEY n > +SECONDARY_TRUSTED_KEYRING n > +SIGNATURE y > +SIGNED_PE_FILE_VERIFICATION n > +SYSTEM_EXTRA_CERTIFICATE n > +SYSTEM_TRUSTED_KEYRING y > +SYSTEM_TRUSTED_KEYS "" > +X509_CERTIFICATE_PARSER y > > The change in CONFIG_INTEGRITY_SIGNATURE to select CONFIG_KEYS instead of > depending on it is to avoid a dependency recursion in > CONFIG_IMA_APPRAISE_MODSIG, because CONFIG_MODULE_SIG_FORMAT selects > CONFIG_KEYS and Kconfig complains that CONFIG_INTEGRITY_SIGNATURE depends > on it. > > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> Thank you, Thiago. Appended signatures seem to be working proper now with multiple keys on the IMA keyring. The length of this patch description is a good indication that this patch needs to be broken up for easier review. A few comments/suggestions inline below. > --- > crypto/asymmetric_keys/pkcs7_parser.c | 12 +++ > include/crypto/pkcs7.h | 3 + > security/integrity/Kconfig | 2 +- > security/integrity/digsig.c | 28 +++-- > security/integrity/ima/Kconfig | 13 +++ > security/integrity/ima/Makefile | 1 + > security/integrity/ima/ima.h | 53 ++++++++++ > security/integrity/ima/ima_api.c | 2 +- > security/integrity/ima/ima_appraise.c | 41 ++++++-- > security/integrity/ima/ima_main.c | 91 ++++++++++++---- > security/integrity/ima/ima_modsig.c | 167 ++++++++++++++++++++++++++++++ > security/integrity/ima/ima_policy.c | 26 +++-- > security/integrity/ima/ima_template_lib.c | 14 ++- > security/integrity/integrity.h | 5 +- > 14 files changed, 404 insertions(+), 54 deletions(-) > > diff --git a/crypto/asymmetric_keys/pkcs7_parser.c b/crypto/asymmetric_keys/pkcs7_parser.c > index af4cd8649117..e41beda297a8 100644 > --- a/crypto/asymmetric_keys/pkcs7_parser.c > +++ b/crypto/asymmetric_keys/pkcs7_parser.c > @@ -673,3 +673,15 @@ int pkcs7_note_signed_info(void *context, size_t hdrlen, > return -ENOMEM; > return 0; > } > + > +/** > + * pkcs7_get_message_sig - get signature in @pkcs7 > + * > + * This function doesn't meaningfully support messages with more than one > + * signature. It will always return the first signature. > + */ > +const struct public_key_signature *pkcs7_get_message_sig( > + const struct pkcs7_message *pkcs7) > +{ > + return pkcs7->signed_infos ? pkcs7->signed_infos->sig : NULL; > +} > diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h > index 583f199400a3..a05a0d7214e6 100644 > --- a/include/crypto/pkcs7.h > +++ b/include/crypto/pkcs7.h > @@ -29,6 +29,9 @@ extern int pkcs7_get_content_data(const struct pkcs7_message *pkcs7, > const void **_data, size_t *_datalen, > size_t *_headerlen); > > +extern const struct public_key_signature *pkcs7_get_message_sig( > + const struct pkcs7_message *pkcs7); > + > /* > * pkcs7_trust.c > */ > diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig > index da9565891738..0d642e0317c7 100644 > --- a/security/integrity/Kconfig > +++ b/security/integrity/Kconfig > @@ -17,8 +17,8 @@ if INTEGRITY > > config INTEGRITY_SIGNATURE > bool "Digital signature verification using multiple keyrings" > - depends on KEYS > default n > + select KEYS > select SIGNATURE > help > This option enables digital signature verification support > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c > index 06554c448dce..9190c9058f4f 100644 > --- a/security/integrity/digsig.c > +++ b/security/integrity/digsig.c > @@ -48,11 +48,10 @@ static bool init_keyring __initdata; > #define restrict_link_to_ima restrict_link_by_builtin_trusted > #endif > > -int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen, > - const char *digest, int digestlen) > +struct key *integrity_keyring_from_id(const unsigned int id) > { > - if (id >= INTEGRITY_KEYRING_MAX || siglen < 2) > - return -EINVAL; > + if (id >= INTEGRITY_KEYRING_MAX) > + return ERR_PTR(-EINVAL); > When splitting up this patch, the addition of this new function could be a separate patch. The patch description would explain the need for a new function. > if (!keyring[id]) { > keyring[id] = > @@ -61,18 +60,29 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen, > int err = PTR_ERR(keyring[id]); > pr_err("no %s keyring: %d\n", keyring_name[id], err); > keyring[id] = NULL; > - return err; > + return ERR_PTR(err); > } > } > > + return keyring[id]; > +} > + > +int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen, > + const char *digest, int digestlen) > +{ > + struct key *keyring = integrity_keyring_from_id(id); > + > + if (IS_ERR(keyring) || siglen < 2) > + return -EINVAL; > + > switch (sig[1]) { > case 1: > /* v1 API expect signature without xattr type */ > - return digsig_verify(keyring[id], sig + 1, siglen - 1, > - digest, digestlen); > + return digsig_verify(keyring, sig + 1, siglen - 1, digest, > + digestlen); > case 2: > - return asymmetric_verify(keyring[id], sig, siglen, > - digest, digestlen); > + return asymmetric_verify(keyring, sig, siglen, digest, > + digestlen); > } > > return -EOPNOTSUPP; > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig > index 35ef69312811..55f734a6124b 100644 > --- a/security/integrity/ima/Kconfig > +++ b/security/integrity/ima/Kconfig > @@ -163,6 +163,19 @@ config IMA_APPRAISE_BOOTPARAM > This option enables the different "ima_appraise=" modes > (eg. fix, log) from the boot command line. > > +config IMA_APPRAISE_MODSIG > + bool "Support module-style signatures for appraisal" > + depends on IMA_APPRAISE > + depends on INTEGRITY_ASYMMETRIC_KEYS > + select PKCS7_MESSAGE_PARSER > + select MODULE_SIG_FORMAT > + default n > + help > + Adds support for signatures appended to files. The format of the > + appended signature is the same used for signed kernel modules. > + The modsig keyword can be used in the IMA policy to allow a hook > + to accept such signatures. > + > config IMA_TRUSTED_KEYRING > bool "Require all keys on the .ima keyring be signed (deprecated)" > depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING > diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile > index 29f198bde02b..c72026acecc3 100644 > --- a/security/integrity/ima/Makefile > +++ b/security/integrity/ima/Makefile > @@ -8,5 +8,6 @@ obj-$(CONFIG_IMA) += ima.o > ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \ > ima_policy.o ima_template.o ima_template_lib.o > ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o > +ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o > ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o > obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index d52b487ad259..eb3ff7286b07 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -190,6 +190,8 @@ enum ima_hooks { > __ima_hooks(__ima_hook_enumify) > }; > > +extern const char *const func_tokens[]; > + > /* LIM API function definitions */ > int ima_get_action(struct inode *inode, int mask, > enum ima_hooks func, int *pcr); > @@ -248,6 +250,19 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, > int ima_read_xattr(struct dentry *dentry, > struct evm_ima_xattr_data **xattr_value); > > +#ifdef CONFIG_IMA_APPRAISE_MODSIG > +bool ima_hook_supports_modsig(enum ima_hooks func); > +int ima_read_modsig(const void *buf, loff_t *buf_len, > + struct evm_ima_xattr_data **xattr_value, > + int *xattr_len); > +enum hash_algo ima_get_modsig_hash_algo(struct evm_ima_xattr_data *hdr); > +int ima_modsig_serialize_data(struct evm_ima_xattr_data *hdr, > + struct evm_ima_xattr_data **data, int *data_len); > +int ima_modsig_verify(const unsigned int keyring_id, > + struct evm_ima_xattr_data *hdr); > +void ima_free_xattr_data(struct evm_ima_xattr_data *hdr); > +#endif > + > #else > static inline int ima_appraise_measurement(enum ima_hooks func, > struct integrity_iint_cache *iint, > @@ -291,6 +306,44 @@ static inline int ima_read_xattr(struct dentry *dentry, > > #endif /* CONFIG_IMA_APPRAISE */ > > +#ifndef CONFIG_IMA_APPRAISE_MODSIG > +static inline bool ima_hook_supports_modsig(enum ima_hooks func) > +{ > + return false; > +} > + > +static inline int ima_read_modsig(const void *buf, loff_t *buf_len, > + struct evm_ima_xattr_data **xattr_value, > + int *xattr_len) > +{ > + return -ENOTSUPP; > +} > + > +static inline enum hash_algo ima_get_modsig_hash_algo( > + struct evm_ima_xattr_data *hdr) > +{ > + return HASH_ALGO__LAST; > +} > + > +static inline int ima_modsig_serialize_data(struct evm_ima_xattr_data *hdr, > + struct evm_ima_xattr_data **data, > + int *data_len) > +{ > + return -ENOTSUPP; > +} > + > +static inline int ima_modsig_verify(const unsigned int keyring_id, > + struct evm_ima_xattr_data *hdr) > +{ > + return -ENOTSUPP; > +} > + > +static inline void ima_free_xattr_data(struct evm_ima_xattr_data *hdr) > +{ > + kfree(hdr); > +} > +#endif > + > /* LSM based policy rules require audit */ > #ifdef CONFIG_IMA_LSM_RULES > > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c > index c2edba8de35e..e3328c866362 100644 > --- a/security/integrity/ima/ima_api.c > +++ b/security/integrity/ima/ima_api.c > @@ -204,7 +204,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, > char digest[IMA_MAX_DIGEST_SIZE]; > } hash; > > - if (!(iint->flags & IMA_COLLECTED)) { > + if (!(iint->flags & IMA_COLLECTED) || iint->ima_hash->algo != algo) { > u64 i_version = file_inode(file)->i_version; > > if (file->f_flags & O_DIRECT) { > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > index 87d2b601cf8e..8716c4fb3675 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -172,6 +172,11 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, > } else if (xattr_len == 17) > return HASH_ALGO_MD5; > break; > + case IMA_MODSIG: > + ret = ima_get_modsig_hash_algo(xattr_value); > + if (ret < HASH_ALGO__LAST) > + return ret; > + break; > } > > /* return default hash algo */ > @@ -229,10 +234,14 @@ int ima_appraise_measurement(enum ima_hooks func, > goto out; > } > > - status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint); > - if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) { > - if ((status == INTEGRITY_NOLABEL) > - || (status == INTEGRITY_NOXATTRS)) > + /* Appended signatures aren't protected by EVM. */ > + status = evm_verifyxattr(dentry, XATTR_NAME_IMA, > + xattr_value->type == IMA_MODSIG ? > + NULL : xattr_value, rc, iint); > + if (status != INTEGRITY_PASS && status != INTEGRITY_UNKNOWN && > + !(xattr_value->type == IMA_MODSIG && > + (status == INTEGRITY_NOLABEL || status == INTEGRITY_NOXATTRS))) { This was messy to begin with, and now it is even more messy. For appended signatures, we're only interested in INTEGRITY_FAIL. Maybe leave the existing "if" clause alone and define a new "if" clause. > + if (status == INTEGRITY_NOLABEL || status == INTEGRITY_NOXATTRS) > cause = "missing-HMAC"; > else if (status == INTEGRITY_FAIL) > cause = "invalid-HMAC"; > @@ -267,11 +276,18 @@ int ima_appraise_measurement(enum ima_hooks func, > status = INTEGRITY_PASS; > break; > case EVM_IMA_XATTR_DIGSIG: > + case IMA_MODSIG: > iint->flags |= IMA_DIGSIG; > - rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > - (const char *)xattr_value, rc, > - iint->ima_hash->digest, > - iint->ima_hash->length); > + > + if (xattr_value->type == EVM_IMA_XATTR_DIGSIG) > + rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > + (const char *)xattr_value, > + rc, iint->ima_hash->digest, > + iint->ima_hash->length); > + else > + rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA, > + xattr_value); > + Perhaps allowing IMA_MODSIG to flow into EVM_IMA_XATTR_DIGSIG on failure, would help restore process_measurements() to the way it was. Further explanation below. > if (rc == -EOPNOTSUPP) { > status = INTEGRITY_UNKNOWN; > } else if (rc) { > @@ -291,13 +307,15 @@ int ima_appraise_measurement(enum ima_hooks func, > if (status != INTEGRITY_PASS) { > if ((ima_appraise & IMA_APPRAISE_FIX) && > (!xattr_value || > - xattr_value->type != EVM_IMA_XATTR_DIGSIG)) { > + (xattr_value->type != EVM_IMA_XATTR_DIGSIG && > + xattr_value->type != IMA_MODSIG))) { > if (!ima_fix_xattr(dentry, iint)) > status = INTEGRITY_PASS; > } else if ((inode->i_size == 0) && > (iint->flags & IMA_NEW_FILE) && > (xattr_value && > - xattr_value->type == EVM_IMA_XATTR_DIGSIG)) { > + (xattr_value->type == EVM_IMA_XATTR_DIGSIG || > + xattr_value->type == IMA_MODSIG))) { > status = INTEGRITY_PASS; > } > integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename, > @@ -406,7 +424,8 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name, > if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST)) > return -EINVAL; > ima_reset_appraise_flags(d_backing_inode(dentry), > - (xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0); > + xvalue->type == EVM_IMA_XATTR_DIGSIG || > + xvalue->type == IMA_MODSIG); Probably easier to read if we set a variable, before calling ima_reset_appraise_flags. > result = 0; > } > return result; > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 2aebb7984437..5527acab034e 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -16,6 +16,9 @@ > * implements the IMA hooks: ima_bprm_check, ima_file_mmap, > * and ima_file_check. > */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > #include <linux/module.h> > #include <linux/file.h> > #include <linux/binfmts.h> > @@ -155,12 +158,66 @@ void ima_file_free(struct file *file) > ima_check_last_writer(iint, inode, file); > } > > +static int measure_and_appraise(struct file *file, char *buf, loff_t size, > + enum ima_hooks func, int opened, int action, > + struct integrity_iint_cache *iint, > + struct evm_ima_xattr_data **xattr_value_, > + int *xattr_len_, const char *pathname, > + bool appended_sig) > +{ > + struct ima_template_desc *template_desc; > + struct evm_ima_xattr_data *xattr_value = NULL; > + enum hash_algo hash_algo; > + int rc, xattr_len = 0; > + > + template_desc = ima_template_desc_current(); > + if (action & IMA_APPRAISE_SUBMASK || > + strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) { > + if (appended_sig) { > + /* Shouldn't happen. */ > + if (WARN_ONCE(!buf || !size, > + "%s doesn't support modsig\n", > + func_tokens[func])) > + return -ENOTSUPP; > + > + rc = ima_read_modsig(buf, &size, &xattr_value, > + &xattr_len); > + if (rc) > + return rc; > + } else > + /* read 'security.ima' */ > + xattr_len = ima_read_xattr(file_dentry(file), > + &xattr_value); > + } > + > + hash_algo = ima_get_hash_algo(xattr_value, xattr_len); > + > + rc = ima_collect_measurement(iint, file, buf, size, hash_algo); > + if (rc != 0) { > + if (file->f_flags & O_DIRECT) > + rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES; > + goto out; > + } > + > + if (action & IMA_APPRAISE_SUBMASK) > + rc = ima_appraise_measurement(func, iint, file, pathname, > + xattr_value, xattr_len, opened); > +out: > + if (rc) > + ima_free_xattr_data(xattr_value); > + else { > + *xattr_value_ = xattr_value; > + *xattr_len_ = xattr_len; > + } > + > + return rc; > +} > + > static int process_measurement(struct file *file, char *buf, loff_t size, > int mask, enum ima_hooks func, int opened) > { > struct inode *inode = file_inode(file); > struct integrity_iint_cache *iint = NULL; > - struct ima_template_desc *template_desc; > char *pathbuf = NULL; > char filename[NAME_MAX]; > const char *pathname = NULL; > @@ -169,7 +226,6 @@ static int process_measurement(struct file *file, char *buf, loff_t size, > struct evm_ima_xattr_data *xattr_value = NULL; > int xattr_len = 0; > bool violation_check; > - enum hash_algo hash_algo; > > if (!ima_policy_flag || !S_ISREG(inode->i_mode)) > return 0; > @@ -226,30 +282,23 @@ static int process_measurement(struct file *file, char *buf, loff_t size, > goto out_digsig; > } > > - template_desc = ima_template_desc_current(); > - if ((action & IMA_APPRAISE_SUBMASK) || > - strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) > - /* read 'security.ima' */ > - xattr_len = ima_read_xattr(file_dentry(file), &xattr_value); > - > - hash_algo = ima_get_hash_algo(xattr_value, xattr_len); > - > - rc = ima_collect_measurement(iint, file, buf, size, hash_algo); > - if (rc != 0) { > - if (file->f_flags & O_DIRECT) > - rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES; > - goto out_digsig; > - } > - There are four stages: collect measurement, store measurement, appraise measurement and audit measurement. "Collect" needs to be done if any one of the other stages is needed. > if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */ > pathname = ima_d_path(&file->f_path, &pathbuf, filename); > > + if (iint->flags & IMA_MODSIG_ALLOWED) > + rc = measure_and_appraise(file, buf, size, func, opened, action, > + iint, &xattr_value, &xattr_len, > + pathname, true); > + if (!xattr_len) > + rc = measure_and_appraise(file, buf, size, func, opened, action, > + iint, &xattr_value, &xattr_len, > + pathname, false); I would rather see "collect" extended to support an appended signature rather than trying to combine "collect" and "appraise" together. > + if (rc) > + goto out_digsig; > + > if (action & IMA_MEASURE) > ima_store_measurement(iint, file, pathname, > xattr_value, xattr_len, pcr); > - if (action & IMA_APPRAISE_SUBMASK) > - rc = ima_appraise_measurement(func, iint, file, pathname, > - xattr_value, xattr_len, opened); Moving appraisal before storing the measurement, should be a separate patch with a clear explanation as to why it is needed. Mimi > if (action & IMA_AUDIT) > ima_audit_measurement(iint, pathname); > > @@ -257,7 +306,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size, > if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG) && > !(iint->flags & IMA_NEW_FILE)) > rc = -EACCES; > - kfree(xattr_value); > + ima_free_xattr_data(xattr_value); > out_free: > if (pathbuf) > __putname(pathbuf); > diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c > new file mode 100644 > index 000000000000..f224acb95191 > --- /dev/null > +++ b/security/integrity/ima/ima_modsig.c > @@ -0,0 +1,167 @@ > +/* > + * IMA support for appraising module-style appended signatures. > + * > + * Copyright (C) 2017 IBM Corporation > + * > + * Author: > + * Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation (version 2 of the License). > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/types.h> > +#include <linux/module_signature.h> > +#include <keys/asymmetric-type.h> > +#include <crypto/pkcs7.h> > + > +#include "ima.h" > + > +struct signature_modsig_hdr { > + uint8_t type; /* Should be IMA_MODSIG. */ > + const void *data; /* Pointer to data covered by pkcs7_msg. */ > + size_t data_len; > + struct pkcs7_message *pkcs7_msg; > + int raw_pkcs7_len; > + > + /* This will be in the measurement list if required by the template. */ > + struct evm_ima_xattr_data raw_pkcs7; > +}; > + > +/** > + * ima_hook_supports_modsig - can the policy allow modsig for this hook? > + * > + * modsig is only supported by hooks using ima_post_read_file, because only they > + * preload the contents of the file in a buffer. FILE_CHECK does that in some > + * cases, but not when reached from vfs_open. POLICY_CHECK can support it, but > + * it's not useful in practice because it's a text file so deny. > + */ > +bool ima_hook_supports_modsig(enum ima_hooks func) > +{ > + switch (func) { > + case FIRMWARE_CHECK: > + case KEXEC_KERNEL_CHECK: > + case KEXEC_INITRAMFS_CHECK: > + return true; > + default: > + return false; > + } > +} > + > +int ima_read_modsig(const void *buf, loff_t *buf_len, > + struct evm_ima_xattr_data **xattr_value, > + int *xattr_len) > +{ > + const size_t marker_len = sizeof(MODULE_SIG_STRING) - 1; > + const struct module_signature *sig; > + struct signature_modsig_hdr *hdr; > + loff_t file_len = *buf_len; > + size_t sig_len; > + const void *p; > + int rc; > + > + if (file_len <= marker_len + sizeof(*sig)) > + return -ENOENT; > + > + p = buf + file_len - marker_len; > + if (memcmp(p, MODULE_SIG_STRING, marker_len)) > + return -ENOENT; > + > + file_len -= marker_len; > + sig = (const struct module_signature *) (p - sizeof(*sig)); > + > + rc = validate_module_signature(sig, file_len); > + if (rc) > + return rc; > + > + sig_len = be32_to_cpu(sig->sig_len); > + file_len -= sig_len + sizeof(*sig); > + > + hdr = kmalloc(sizeof(*hdr) + sig_len, GFP_KERNEL); > + if (!hdr) > + return -ENOMEM; > + > + hdr->pkcs7_msg = pkcs7_parse_message(buf + file_len, sig_len); > + if (IS_ERR(hdr->pkcs7_msg)) { > + rc = PTR_ERR(hdr->pkcs7_msg); > + kfree(hdr); > + return rc; > + } > + > + memcpy(hdr->raw_pkcs7.data, buf + file_len, sig_len); > + hdr->raw_pkcs7_len = sig_len + 1; > + hdr->raw_pkcs7.type = IMA_MODSIG; > + > + hdr->type = IMA_MODSIG; > + hdr->data = buf; > + hdr->data_len = file_len; > + > + *xattr_value = (typeof(*xattr_value)) hdr; > + *xattr_len = sizeof(*hdr); > + *buf_len = file_len; > + > + return 0; > +} > + > +enum hash_algo ima_get_modsig_hash_algo(struct evm_ima_xattr_data *hdr) > +{ > + const struct public_key_signature *pks; > + struct signature_modsig_hdr *modsig = (typeof(modsig)) hdr; > + int i; > + > + pks = pkcs7_get_message_sig(modsig->pkcs7_msg); > + if (!pks) > + return HASH_ALGO__LAST; > + > + for (i = 0; i < HASH_ALGO__LAST; i++) > + if (!strcmp(hash_algo_name[i], pks->hash_algo)) > + break; > + > + return i; > +} > + > +int ima_modsig_serialize_data(struct evm_ima_xattr_data *hdr, > + struct evm_ima_xattr_data **data, int *data_len) > +{ > + struct signature_modsig_hdr *modsig = (typeof(modsig)) hdr; > + > + *data = &modsig->raw_pkcs7; > + *data_len = modsig->raw_pkcs7_len; > + > + return 0; > +} > + > +int ima_modsig_verify(const unsigned int keyring_id, > + struct evm_ima_xattr_data *hdr) > +{ > + struct signature_modsig_hdr *modsig = (typeof(modsig)) hdr; > + struct key *trusted_keys = integrity_keyring_from_id(keyring_id); > + > + if (IS_ERR(trusted_keys)) > + return -EINVAL; > + > + return verify_pkcs7_message_signature(modsig->data, modsig->data_len, > + modsig->pkcs7_msg, trusted_keys, > + VERIFYING_MODULE_SIGNATURE, > + NULL, NULL); > +} > + > +void ima_free_xattr_data(struct evm_ima_xattr_data *hdr) > +{ > + if (!hdr) > + return; > + > + if (hdr->type == IMA_MODSIG) { > + struct signature_modsig_hdr *modsig = (typeof(modsig)) hdr; > + > + pkcs7_free_message(modsig->pkcs7_msg); > + } > + > + kfree(hdr); > +} > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index f4436626ccb7..4047ccabcbbf 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -853,8 +853,12 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) > } > > ima_log_string(ab, "appraise_type", args[0].from); > - if ((strcmp(args[0].from, "imasig")) == 0) > + if (strcmp(args[0].from, "imasig") == 0) > entry->flags |= IMA_DIGSIG_REQUIRED; > + else if (ima_hook_supports_modsig(entry->func) && > + strcmp(args[0].from, "modsig|imasig") == 0) > + entry->flags |= IMA_DIGSIG_REQUIRED > + | IMA_MODSIG_ALLOWED; > else > result = -EINVAL; > break; > @@ -960,6 +964,12 @@ void ima_delete_rules(void) > } > } > > +#define __ima_hook_stringify(str) (#str), > + > +const char *const func_tokens[] = { > + __ima_hooks(__ima_hook_stringify) > +}; > + > #ifdef CONFIG_IMA_READ_POLICY > enum { > mask_exec = 0, mask_write, mask_read, mask_append > @@ -972,12 +982,6 @@ static const char *const mask_tokens[] = { > "MAY_APPEND" > }; > > -#define __ima_hook_stringify(str) (#str), > - > -static const char *const func_tokens[] = { > - __ima_hooks(__ima_hook_stringify) > -}; > - > void *ima_policy_start(struct seq_file *m, loff_t *pos) > { > loff_t l = *pos; > @@ -1140,8 +1144,12 @@ int ima_policy_show(struct seq_file *m, void *v) > } > } > } > - if (entry->flags & IMA_DIGSIG_REQUIRED) > - seq_puts(m, "appraise_type=imasig "); > + if (entry->flags & IMA_DIGSIG_REQUIRED) { > + if (entry->flags & IMA_MODSIG_ALLOWED) > + seq_puts(m, "appraise_type=modsig|imasig "); > + else > + seq_puts(m, "appraise_type=imasig "); > + } > if (entry->flags & IMA_PERMIT_DIRECTIO) > seq_puts(m, "permit_directio "); > rcu_read_unlock(); > diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c > index f9ba37b3928d..be123485e486 100644 > --- a/security/integrity/ima/ima_template_lib.c > +++ b/security/integrity/ima/ima_template_lib.c > @@ -322,9 +322,21 @@ int ima_eventsig_init(struct ima_event_data *event_data, > int xattr_len = event_data->xattr_len; > int rc = 0; > > - if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG)) > + if (!xattr_value || (xattr_value->type != EVM_IMA_XATTR_DIGSIG && > + xattr_value->type != IMA_MODSIG)) > goto out; > > + /* > + * The xattr_value for IMA_MODSIG is a runtime structure containing > + * pointers. Get its raw data instead. > + */ > + if (xattr_value->type == IMA_MODSIG) { > + rc = ima_modsig_serialize_data(xattr_value, &xattr_value, > + &xattr_len); > + if (rc) > + goto out; > + } > + > rc = ima_write_template_field_data(xattr_value, xattr_len, fmt, > field_data); > out: > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h > index 874211aba6e9..2c9393cf2860 100644 > --- a/security/integrity/integrity.h > +++ b/security/integrity/integrity.h > @@ -28,11 +28,12 @@ > > /* iint cache flags */ > #define IMA_ACTION_FLAGS 0xff000000 > -#define IMA_ACTION_RULE_FLAGS 0x06000000 > +#define IMA_ACTION_RULE_FLAGS 0x16000000 > #define IMA_DIGSIG 0x01000000 > #define IMA_DIGSIG_REQUIRED 0x02000000 > #define IMA_PERMIT_DIRECTIO 0x04000000 > #define IMA_NEW_FILE 0x08000000 > +#define IMA_MODSIG_ALLOWED 0x10000000 > > #define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \ > IMA_APPRAISE_SUBMASK) > @@ -58,6 +59,7 @@ enum evm_ima_xattr_type { > EVM_XATTR_HMAC, > EVM_IMA_XATTR_DIGSIG, > IMA_XATTR_DIGEST_NG, > + IMA_MODSIG, > IMA_XATTR_LAST > }; > > @@ -129,6 +131,7 @@ int __init integrity_read_file(const char *path, char **data); > > #ifdef CONFIG_INTEGRITY_SIGNATURE > > +struct key *integrity_keyring_from_id(const unsigned int id); > int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen, > const char *digest, int digestlen); > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Mimi, Thanks for your review, and for queuing the other patches in this series. Mimi Zohar <zohar@linux.vnet.ibm.com> writes: > On Wed, 2017-06-07 at 22:49 -0300, Thiago Jung Bauermann wrote: >> This patch introduces the modsig keyword to the IMA policy syntax to >> specify that a given hook should expect the file to have the IMA signature >> appended to it. > > Thank you, Thiago. Appended signatures seem to be working proper now > with multiple keys on the IMA keyring. Great news! > The length of this patch description is a good indication that this > patch needs to be broken up for easier review. A few > comments/suggestions inline below. Ok, I will try to break it up, and also patch 5 as you suggested. >> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c >> index 06554c448dce..9190c9058f4f 100644 >> --- a/security/integrity/digsig.c >> +++ b/security/integrity/digsig.c >> @@ -48,11 +48,10 @@ static bool init_keyring __initdata; >> #define restrict_link_to_ima restrict_link_by_builtin_trusted >> #endif >> >> -int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen, >> - const char *digest, int digestlen) >> +struct key *integrity_keyring_from_id(const unsigned int id) >> { >> - if (id >= INTEGRITY_KEYRING_MAX || siglen < 2) >> - return -EINVAL; >> + if (id >= INTEGRITY_KEYRING_MAX) >> + return ERR_PTR(-EINVAL); >> > > When splitting up this patch, the addition of this new function could > be a separate patch. The patch description would explain the need for > a new function. Ok, will do for v3. >> @@ -229,10 +234,14 @@ int ima_appraise_measurement(enum ima_hooks func, >> goto out; >> } >> >> - status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint); >> - if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) { >> - if ((status == INTEGRITY_NOLABEL) >> - || (status == INTEGRITY_NOXATTRS)) >> + /* Appended signatures aren't protected by EVM. */ >> + status = evm_verifyxattr(dentry, XATTR_NAME_IMA, >> + xattr_value->type == IMA_MODSIG ? >> + NULL : xattr_value, rc, iint); >> + if (status != INTEGRITY_PASS && status != INTEGRITY_UNKNOWN && >> + !(xattr_value->type == IMA_MODSIG && >> + (status == INTEGRITY_NOLABEL || status == INTEGRITY_NOXATTRS))) { > > This was messy to begin with, and now it is even more messy. For > appended signatures, we're only interested in INTEGRITY_FAIL. Maybe > leave the existing "if" clause alone and define a new "if" clause. Ok, is this what you had in mind? @@ -229,8 +237,14 @@ int ima_appraise_measurement(enum ima_hooks func, goto out; } - status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint); - if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) { + /* Appended signatures aren't protected by EVM. */ + status = evm_verifyxattr(dentry, XATTR_NAME_IMA, + xattr_value->type == IMA_MODSIG ? + NULL : xattr_value, rc, iint); + if (xattr_value->type == IMA_MODSIG && status == INTEGRITY_FAIL) { + cause = "invalid-HMAC"; + goto out; + } else if (status != INTEGRITY_PASS && status != INTEGRITY_UNKNOWN) { if ((status == INTEGRITY_NOLABEL) || (status == INTEGRITY_NOXATTRS)) cause = "missing-HMAC"; >> @@ -267,11 +276,18 @@ int ima_appraise_measurement(enum ima_hooks func, >> status = INTEGRITY_PASS; >> break; >> case EVM_IMA_XATTR_DIGSIG: >> + case IMA_MODSIG: >> iint->flags |= IMA_DIGSIG; >> - rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, >> - (const char *)xattr_value, rc, >> - iint->ima_hash->digest, >> - iint->ima_hash->length); >> + >> + if (xattr_value->type == EVM_IMA_XATTR_DIGSIG) >> + rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, >> + (const char *)xattr_value, >> + rc, iint->ima_hash->digest, >> + iint->ima_hash->length); >> + else >> + rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA, >> + xattr_value); >> + > > Perhaps allowing IMA_MODSIG to flow into EVM_IMA_XATTR_DIGSIG on > failure, would help restore process_measurements() to the way it was. > Further explanation below. It's not possible to simply flow into EVM_IMA_XATTR_DIGSIG on failure because after calling ima_read_xattr we need to run again all the logic before the switch statement. Instead, I'm currently testing the following change for v3, what do you think? More about this code further below. @@ -266,6 +280,44 @@ int ima_appraise_measurement(enum ima_hooks func, } status = INTEGRITY_PASS; break; + case IMA_MODSIG: + rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA, xattr_value); + if (!rc) { + iint->flags |= IMA_DIGSIG; + status = INTEGRITY_PASS; + break; + } + + /* + * The appended signature failed verification. Let's try + * reading a signature from the extended attribute instead. + */ + + ima_free_xattr_data(xattr_value); + xattr_value = NULL; + + /* read 'security.ima' */ + xattr_len = ima_read_xattr(file_dentry(file), &xattr_value); + algo = iint->ima_hash->algo; + + if (!xattr_value || xattr_value->type == IMA_MODSIG || + ima_get_hash_algo(xattr_value, xattr_len) != algo) { + iint->flags |= IMA_DIGSIG; + + if (rc == -EOPNOTSUPP) + status = INTEGRITY_UNKNOWN; + else { + cause = "invalid-signature"; + status = INTEGRITY_FAIL; + } + + break; + } + + pr_debug("Trying the xattr signature\n"); + + return ima_appraise_measurement(func, iint, file, filename, + xattr_value, xattr_len, opened); case EVM_IMA_XATTR_DIGSIG: iint->flags |= IMA_DIGSIG; rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, >> @@ -406,7 +424,8 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name, >> if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST)) >> return -EINVAL; >> ima_reset_appraise_flags(d_backing_inode(dentry), >> - (xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0); >> + xvalue->type == EVM_IMA_XATTR_DIGSIG || >> + xvalue->type == IMA_MODSIG); > > Probably easier to read if we set a variable, before calling > ima_reset_appraise_flags. Ok, will do in v3. >> @@ -226,30 +282,23 @@ static int process_measurement(struct file *file, char *buf, loff_t size, >> goto out_digsig; >> } >> >> - template_desc = ima_template_desc_current(); >> - if ((action & IMA_APPRAISE_SUBMASK) || >> - strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) >> - /* read 'security.ima' */ >> - xattr_len = ima_read_xattr(file_dentry(file), &xattr_value); >> - >> - hash_algo = ima_get_hash_algo(xattr_value, xattr_len); >> - >> - rc = ima_collect_measurement(iint, file, buf, size, hash_algo); >> - if (rc != 0) { >> - if (file->f_flags & O_DIRECT) >> - rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES; >> - goto out_digsig; >> - } >> - > > There are four stages: collect measurement, store measurement, > appraise measurement and audit measurement. "Collect" needs to be > done if any one of the other stages is needed. > >> if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */ >> pathname = ima_d_path(&file->f_path, &pathbuf, filename); >> >> + if (iint->flags & IMA_MODSIG_ALLOWED) >> + rc = measure_and_appraise(file, buf, size, func, opened, action, >> + iint, &xattr_value, &xattr_len, >> + pathname, true); >> + if (!xattr_len) >> + rc = measure_and_appraise(file, buf, size, func, opened, action, >> + iint, &xattr_value, &xattr_len, >> + pathname, false); > > I would rather see "collect" extended to support an appended signature > rather than trying to combine "collect" and "appraise" together. I'm not sure I understand what you mean by "extend collect to support an appended signature", but the fundamental problem is that we don't know whether we need to fallback to the xattr sig until the appraise step because that's when we verify the signature, and by then the collect and store steps were already completed and would need to be done again if the hash algorithm in the xattr sig is different. If we don't want to change the order of these steps what I suggest (and what the code I pasted above for ima_appraise_measurement does) is to only allow falling back to the xattr sig if the appended sig and the xattr sig use the same hash algorithm. In that case, collect and store don't need to be redone nor the store step need to be moved after appraise. This is the only change that would be needed in process_measurement: @@ -227,10 +230,16 @@ static int process_measurement(struct file *file, char *buf, loff_t size, } template_desc = ima_template_desc_current(); - if ((action & IMA_APPRAISE_SUBMASK) || - strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) - /* read 'security.ima' */ - xattr_len = ima_read_xattr(file_dentry(file), &xattr_value); + if (action & IMA_APPRAISE_SUBMASK || + strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) { + if (iint->flags & IMA_MODSIG_ALLOWED) + ima_read_modsig(buf, &size, &xattr_value, &xattr_len); + + if (!xattr_len) + /* read 'security.ima' */ + xattr_len = ima_read_xattr(file_dentry(file), + &xattr_value); + } hash_algo = ima_get_hash_algo(xattr_value, xattr_len); @@ -257,7 +266,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size, if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG) && !(iint->flags & IMA_NEW_FILE)) rc = -EACCES; - kfree(xattr_value); + ima_free_xattr_data(xattr_value); out_free: if (pathbuf) __putname(pathbuf); > >> + if (rc) >> + goto out_digsig; >> + >> if (action & IMA_MEASURE) >> ima_store_measurement(iint, file, pathname, >> xattr_value, xattr_len, pcr); >> - if (action & IMA_APPRAISE_SUBMASK) >> - rc = ima_appraise_measurement(func, iint, file, pathname, >> - xattr_value, xattr_len, opened); > > Moving appraisal before storing the measurement, should be a separate > patch with a clear explanation as to why it is needed. Ok, will do in v3 if you don't like the restriction of both the modsig and xattr sig having to use the same hash algorithm.
On Wed, 2017-06-21 at 14:45 -0300, Thiago Jung Bauermann wrote: > Hello Mimi, > > Thanks for your review, and for queuing the other patches in this series. > > Mimi Zohar <zohar@linux.vnet.ibm.com> writes: > > On Wed, 2017-06-07 at 22:49 -0300, Thiago Jung Bauermann wrote: > >> This patch introduces the modsig keyword to the IMA policy syntax to > >> specify that a given hook should expect the file to have the IMA signature > >> appended to it. > > > > Thank you, Thiago. Appended signatures seem to be working proper now > > with multiple keys on the IMA keyring. > > Great news! > > > The length of this patch description is a good indication that this > > patch needs to be broken up for easier review. A few > > comments/suggestions inline below. > > Ok, I will try to break it up, and also patch 5 as you suggested. > > >> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c > >> index 06554c448dce..9190c9058f4f 100644 > >> --- a/security/integrity/digsig.c > >> +++ b/security/integrity/digsig.c > >> @@ -48,11 +48,10 @@ static bool init_keyring __initdata; > >> #define restrict_link_to_ima restrict_link_by_builtin_trusted > >> #endif > >> > >> -int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen, > >> - const char *digest, int digestlen) > >> +struct key *integrity_keyring_from_id(const unsigned int id) > >> { > >> - if (id >= INTEGRITY_KEYRING_MAX || siglen < 2) > >> - return -EINVAL; > >> + if (id >= INTEGRITY_KEYRING_MAX) > >> + return ERR_PTR(-EINVAL); > >> > > > > When splitting up this patch, the addition of this new function could > > be a separate patch. The patch description would explain the need for > > a new function. > > Ok, will do for v3. > > >> @@ -229,10 +234,14 @@ int ima_appraise_measurement(enum ima_hooks func, > >> goto out; > >> } > >> > >> - status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint); > >> - if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) { > >> - if ((status == INTEGRITY_NOLABEL) > >> - || (status == INTEGRITY_NOXATTRS)) > >> + /* Appended signatures aren't protected by EVM. */ > >> + status = evm_verifyxattr(dentry, XATTR_NAME_IMA, > >> + xattr_value->type == IMA_MODSIG ? > >> + NULL : xattr_value, rc, iint); > >> + if (status != INTEGRITY_PASS && status != INTEGRITY_UNKNOWN && > >> + !(xattr_value->type == IMA_MODSIG && > >> + (status == INTEGRITY_NOLABEL || status == INTEGRITY_NOXATTRS))) { > > > > This was messy to begin with, and now it is even more messy. For > > appended signatures, we're only interested in INTEGRITY_FAIL. Maybe > > leave the existing "if" clause alone and define a new "if" clause. > > Ok, is this what you had in mind? > > @@ -229,8 +237,14 @@ int ima_appraise_measurement(enum ima_hooks func, > goto out; > } > > - status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint); > - if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) { > + /* Appended signatures aren't protected by EVM. */ > + status = evm_verifyxattr(dentry, XATTR_NAME_IMA, > + xattr_value->type == IMA_MODSIG ? > + NULL : xattr_value, rc, iint); Yes, maybe add a comment here indicating only verifying other security xattrs, if they exist. > + if (xattr_value->type == IMA_MODSIG && status == INTEGRITY_FAIL) { > + cause = "invalid-HMAC"; > + goto out; > + } else if (status != INTEGRITY_PASS && status != INTEGRITY_UNKNOWN) { > if ((status == INTEGRITY_NOLABEL) > || (status == INTEGRITY_NOXATTRS)) > cause = "missing-HMAC"; > > >> @@ -267,11 +276,18 @@ int ima_appraise_measurement(enum ima_hooks func, > >> status = INTEGRITY_PASS; > >> break; > >> case EVM_IMA_XATTR_DIGSIG: > >> + case IMA_MODSIG: > >> iint->flags |= IMA_DIGSIG; > >> - rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > >> - (const char *)xattr_value, rc, > >> - iint->ima_hash->digest, > >> - iint->ima_hash->length); > >> + > >> + if (xattr_value->type == EVM_IMA_XATTR_DIGSIG) > >> + rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > >> + (const char *)xattr_value, > >> + rc, iint->ima_hash->digest, > >> + iint->ima_hash->length); > >> + else > >> + rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA, > >> + xattr_value); > >> + > > > > Perhaps allowing IMA_MODSIG to flow into EVM_IMA_XATTR_DIGSIG on > > failure, would help restore process_measurements() to the way it was. > > Further explanation below. > > It's not possible to simply flow into EVM_IMA_XATTR_DIGSIG on failure > because after calling ima_read_xattr we need to run again all the logic > before the switch statement. Instead, I'm currently testing the > following change for v3, what do you think? I don't think we can assume that the same algorithm will be used for signing the kernel image. Different entities would be signing the kernel image with different requirements. Suppose for example a stock distro image comes signed using one algorithm (appended signature), but the same kernel image is locally signed using a different algorithm (xattr). Signature verification is dependent on either the distro or local public key being loaded onto the IMA keyring. > More about this code further below. > > @@ -266,6 +280,44 @@ int ima_appraise_measurement(enum ima_hooks func, > } > status = INTEGRITY_PASS; > break; > + case IMA_MODSIG: > + rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA, xattr_value); > + if (!rc) { > + iint->flags |= IMA_DIGSIG; > + status = INTEGRITY_PASS; > + break; > + } > + > + /* > + * The appended signature failed verification. Let's try > + * reading a signature from the extended attribute instead. > + */ > + > + ima_free_xattr_data(xattr_value); > + xattr_value = NULL; > + > + /* read 'security.ima' */ > + xattr_len = ima_read_xattr(file_dentry(file), &xattr_value); > + algo = iint->ima_hash->algo; > + > + if (!xattr_value || xattr_value->type == IMA_MODSIG || > + ima_get_hash_algo(xattr_value, xattr_len) != algo) { > + iint->flags |= IMA_DIGSIG; > + > + if (rc == -EOPNOTSUPP) > + status = INTEGRITY_UNKNOWN; > + else { > + cause = "invalid-signature"; > + status = INTEGRITY_FAIL; > + } > + > + break; > + } > + > + pr_debug("Trying the xattr signature\n"); > + > + return ima_appraise_measurement(func, iint, file, filename, > + xattr_value, xattr_len, opened); > case EVM_IMA_XATTR_DIGSIG: > iint->flags |= IMA_DIGSIG; > rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > > >> @@ -406,7 +424,8 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name, > >> if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST)) > >> return -EINVAL; > >> ima_reset_appraise_flags(d_backing_inode(dentry), > >> - (xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0); > >> + xvalue->type == EVM_IMA_XATTR_DIGSIG || > >> + xvalue->type == IMA_MODSIG); > > > > Probably easier to read if we set a variable, before calling > > ima_reset_appraise_flags. > > Ok, will do in v3. > > >> @@ -226,30 +282,23 @@ static int process_measurement(struct file *file, char *buf, loff_t size, > >> goto out_digsig; > >> } > >> > >> - template_desc = ima_template_desc_current(); > >> - if ((action & IMA_APPRAISE_SUBMASK) || > >> - strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) > >> - /* read 'security.ima' */ > >> - xattr_len = ima_read_xattr(file_dentry(file), &xattr_value); > >> - > >> - hash_algo = ima_get_hash_algo(xattr_value, xattr_len); > >> - > >> - rc = ima_collect_measurement(iint, file, buf, size, hash_algo); > >> - if (rc != 0) { > >> - if (file->f_flags & O_DIRECT) > >> - rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES; > >> - goto out_digsig; > >> - } > >> - > > > > There are four stages: collect measurement, store measurement, > > appraise measurement and audit measurement. "Collect" needs to be > > done if any one of the other stages is needed. > > > >> if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */ > >> pathname = ima_d_path(&file->f_path, &pathbuf, filename); > >> > >> + if (iint->flags & IMA_MODSIG_ALLOWED) > >> + rc = measure_and_appraise(file, buf, size, func, opened, action, > >> + iint, &xattr_value, &xattr_len, > >> + pathname, true); > >> + if (!xattr_len) > >> + rc = measure_and_appraise(file, buf, size, func, opened, action, > >> + iint, &xattr_value, &xattr_len, > >> + pathname, false); > > > > I would rather see "collect" extended to support an appended signature > > rather than trying to combine "collect" and "appraise" together. > > I'm not sure I understand what you mean by "extend collect to support an > appended signature", but the fundamental problem is that we don't know > whether we need to fallback to the xattr sig until the appraise step > because that's when we verify the signature, and by then the collect and > store steps were already completed and would need to be done again if > the hash algorithm in the xattr sig is different. The "appraise" stage could be moved before the "store" stage, like you have. (This should be a separate patch explaining the need for moving it.) Based on an argument to ima_collect_measurement() have it "collect" either the appended signature or the xattr. Maybe something like this: loop [ appended signature, xattr ] { <== list based on policy flags collect_measurement() if failure continue appraise_measurement() if success break } store_measurement() Mimi > If we don't want to change the order of these steps what I suggest (and > what the code I pasted above for ima_appraise_measurement does) is to > only allow falling back to the xattr sig if the appended sig and the > xattr sig use the same hash algorithm. > > In that case, collect and store don't need to be redone nor the store > step need to be moved after appraise. This is the only change that would > be needed in process_measurement: > @@ -227,10 +230,16 @@ static int process_measurement(struct file *file, char *buf, loff_t size, > } > > template_desc = ima_template_desc_current(); > - if ((action & IMA_APPRAISE_SUBMASK) || > - strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) > - /* read 'security.ima' */ > - xattr_len = ima_read_xattr(file_dentry(file), &xattr_value); > + if (action & IMA_APPRAISE_SUBMASK || > + strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) { > + if (iint->flags & IMA_MODSIG_ALLOWED) > + ima_read_modsig(buf, &size, &xattr_value, &xattr_len); > + > + if (!xattr_len) > + /* read 'security.ima' */ > + xattr_len = ima_read_xattr(file_dentry(file), > + &xattr_value); > + } > > hash_algo = ima_get_hash_algo(xattr_value, xattr_len); > > @@ -257,7 +266,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size, > if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG) && > !(iint->flags & IMA_NEW_FILE)) > rc = -EACCES; > - kfree(xattr_value); > + ima_free_xattr_data(xattr_value); > out_free: > if (pathbuf) > __putname(pathbuf); > > > > >> + if (rc) > >> + goto out_digsig; > >> + > >> if (action & IMA_MEASURE) > >> ima_store_measurement(iint, file, pathname, > >> xattr_value, xattr_len, pcr); > >> - if (action & IMA_APPRAISE_SUBMASK) > >> - rc = ima_appraise_measurement(func, iint, file, pathname, > >> - xattr_value, xattr_len, opened); > > > > Moving appraisal before storing the measurement, should be a separate > > patch with a clear explanation as to why it is needed. > > Ok, will do in v3 if you don't like the restriction of both the modsig > and xattr sig having to use the same hash algorithm. > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mimi Zohar <zohar@linux.vnet.ibm.com> writes: > On Wed, 2017-06-21 at 14:45 -0300, Thiago Jung Bauermann wrote: >> Mimi Zohar <zohar@linux.vnet.ibm.com> writes: >> > On Wed, 2017-06-07 at 22:49 -0300, Thiago Jung Bauermann wrote: >> >> @@ -267,11 +276,18 @@ int ima_appraise_measurement(enum ima_hooks func, >> >> status = INTEGRITY_PASS; >> >> break; >> >> case EVM_IMA_XATTR_DIGSIG: >> >> + case IMA_MODSIG: >> >> iint->flags |= IMA_DIGSIG; >> >> - rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, >> >> - (const char *)xattr_value, rc, >> >> - iint->ima_hash->digest, >> >> - iint->ima_hash->length); >> >> + >> >> + if (xattr_value->type == EVM_IMA_XATTR_DIGSIG) >> >> + rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, >> >> + (const char *)xattr_value, >> >> + rc, iint->ima_hash->digest, >> >> + iint->ima_hash->length); >> >> + else >> >> + rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA, >> >> + xattr_value); >> >> + >> > >> > Perhaps allowing IMA_MODSIG to flow into EVM_IMA_XATTR_DIGSIG on >> > failure, would help restore process_measurements() to the way it was. >> > Further explanation below. >> >> It's not possible to simply flow into EVM_IMA_XATTR_DIGSIG on failure >> because after calling ima_read_xattr we need to run again all the logic >> before the switch statement. Instead, I'm currently testing the >> following change for v3, what do you think? > > I don't think we can assume that the same algorithm will be used for > signing the kernel image. Different entities would be signing the > kernel image with different requirements. > > Suppose for example a stock distro image comes signed using one > algorithm (appended signature), but the same kernel image is locally > signed using a different algorithm (xattr). Signature verification is > dependent on either the distro or local public key being loaded onto > the IMA keyring. This example is good, but it raises one question: should the xattr signature cover the entire contents of the stock distro image (i.e., also cover the appended signature), or should it ignore the appended signature and thus cover the same contents that the appended signature covers? If the former, then we can't reuse the iint->ima_hash that was collected when trying to verify the appended signature because it doesn't cover the appended signature itself and won't match the hash expected by the xattr signature. If the latter, then evmctl ima_sign needs to be modified to check whether there's an appended signature in the given file and ignore it when calculating the xattr signature. Which is better? >> >> @@ -226,30 +282,23 @@ static int process_measurement(struct file *file, char *buf, loff_t size, >> >> goto out_digsig; >> >> } >> >> >> >> - template_desc = ima_template_desc_current(); >> >> - if ((action & IMA_APPRAISE_SUBMASK) || >> >> - strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) >> >> - /* read 'security.ima' */ >> >> - xattr_len = ima_read_xattr(file_dentry(file), &xattr_value); >> >> - >> >> - hash_algo = ima_get_hash_algo(xattr_value, xattr_len); >> >> - >> >> - rc = ima_collect_measurement(iint, file, buf, size, hash_algo); >> >> - if (rc != 0) { >> >> - if (file->f_flags & O_DIRECT) >> >> - rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES; >> >> - goto out_digsig; >> >> - } >> >> - >> > >> > There are four stages: collect measurement, store measurement, >> > appraise measurement and audit measurement. "Collect" needs to be >> > done if any one of the other stages is needed. >> > >> >> if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */ >> >> pathname = ima_d_path(&file->f_path, &pathbuf, filename); >> >> >> >> + if (iint->flags & IMA_MODSIG_ALLOWED) >> >> + rc = measure_and_appraise(file, buf, size, func, opened, action, >> >> + iint, &xattr_value, &xattr_len, >> >> + pathname, true); >> >> + if (!xattr_len) >> >> + rc = measure_and_appraise(file, buf, size, func, opened, action, >> >> + iint, &xattr_value, &xattr_len, >> >> + pathname, false); >> > >> > I would rather see "collect" extended to support an appended signature >> > rather than trying to combine "collect" and "appraise" together. >> >> I'm not sure I understand what you mean by "extend collect to support an >> appended signature", but the fundamental problem is that we don't know >> whether we need to fallback to the xattr sig until the appraise step >> because that's when we verify the signature, and by then the collect and >> store steps were already completed and would need to be done again if >> the hash algorithm in the xattr sig is different. > > The "appraise" stage could be moved before the "store" stage, like you > have. (This should be a separate patch explaining the need for moving > it.) Based on an argument to ima_collect_measurement() have it > "collect" either the appended signature or the xattr. Maybe something > like this: > > loop [ appended signature, xattr ] { <== list based on policy flags > collect_measurement() > if failure > continue > appraise_measurement() > if success > break > } > > store_measurement() Ok, the above is what v2 already does, so the only change I made was to rename the function from measure_and_appraise to collect_and_appraise to match the IMA jargon.
On Tue, 2017-07-04 at 23:22 -0300, Thiago Jung Bauermann wrote: > Mimi Zohar <zohar@linux.vnet.ibm.com> writes: > > > On Wed, 2017-06-21 at 14:45 -0300, Thiago Jung Bauermann wrote: > >> Mimi Zohar <zohar@linux.vnet.ibm.com> writes: > >> > On Wed, 2017-06-07 at 22:49 -0300, Thiago Jung Bauermann wrote: > >> >> @@ -267,11 +276,18 @@ int ima_appraise_measurement(enum ima_hooks func, > >> >> status = INTEGRITY_PASS; > >> >> break; > >> >> case EVM_IMA_XATTR_DIGSIG: > >> >> + case IMA_MODSIG: > >> >> iint->flags |= IMA_DIGSIG; > >> >> - rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > >> >> - (const char *)xattr_value, rc, > >> >> - iint->ima_hash->digest, > >> >> - iint->ima_hash->length); > >> >> + > >> >> + if (xattr_value->type == EVM_IMA_XATTR_DIGSIG) > >> >> + rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > >> >> + (const char *)xattr_value, > >> >> + rc, iint->ima_hash->digest, > >> >> + iint->ima_hash->length); > >> >> + else > >> >> + rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA, > >> >> + xattr_value); > >> >> + > >> > > >> > Perhaps allowing IMA_MODSIG to flow into EVM_IMA_XATTR_DIGSIG on > >> > failure, would help restore process_measurements() to the way it was. > >> > Further explanation below. > >> > >> It's not possible to simply flow into EVM_IMA_XATTR_DIGSIG on failure > >> because after calling ima_read_xattr we need to run again all the logic > >> before the switch statement. Instead, I'm currently testing the > >> following change for v3, what do you think? > > > > I don't think we can assume that the same algorithm will be used for > > signing the kernel image. Different entities would be signing the > > kernel image with different requirements. > > > > Suppose for example a stock distro image comes signed using one > > algorithm (appended signature), but the same kernel image is locally > > signed using a different algorithm (xattr). Signature verification is > > dependent on either the distro or local public key being loaded onto > > the IMA keyring. > > This example is good, but it raises one question: should the xattr > signature cover the entire contents of the stock distro image (i.e., > also cover the appended signature), or should it ignore the appended > signature and thus cover the same contents that the appended signature > covers? > > If the former, then we can't reuse the iint->ima_hash that was collected > when trying to verify the appended signature because it doesn't cover > the appended signature itself and won't match the hash expected by the > xattr signature. > > If the latter, then evmctl ima_sign needs to be modified to check > whether there's an appended signature in the given file and ignore it > when calculating the xattr signature. > > Which is better? I realize that having the same file hash for both the appended signature and extended attribute would make things a lot easier, but security.ima is a signature of the file as written to disk, meaning it would include any appended signature > > >> >> @@ -226,30 +282,23 @@ static int process_measurement(struct file *file, char *buf, loff_t size, > >> >> goto out_digsig; > >> >> } > >> >> > >> >> - template_desc = ima_template_desc_current(); > >> >> - if ((action & IMA_APPRAISE_SUBMASK) || > >> >> - strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) > >> >> - /* read 'security.ima' */ > >> >> - xattr_len = ima_read_xattr(file_dentry(file), &xattr_value); > >> >> - > >> >> - hash_algo = ima_get_hash_algo(xattr_value, xattr_len); > >> >> - > >> >> - rc = ima_collect_measurement(iint, file, buf, size, hash_algo); > >> >> - if (rc != 0) { > >> >> - if (file->f_flags & O_DIRECT) > >> >> - rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES; > >> >> - goto out_digsig; > >> >> - } > >> >> - > >> > > >> > There are four stages: collect measurement, store measurement, > >> > appraise measurement and audit measurement. "Collect" needs to be > >> > done if any one of the other stages is needed. > >> > > >> >> if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */ > >> >> pathname = ima_d_path(&file->f_path, &pathbuf, filename); > >> >> > >> >> + if (iint->flags & IMA_MODSIG_ALLOWED) > >> >> + rc = measure_and_appraise(file, buf, size, func, opened, action, > >> >> + iint, &xattr_value, &xattr_len, > >> >> + pathname, true); > >> >> + if (!xattr_len) > >> >> + rc = measure_and_appraise(file, buf, size, func, opened, action, > >> >> + iint, &xattr_value, &xattr_len, > >> >> + pathname, false); > >> > > >> > I would rather see "collect" extended to support an appended signature > >> > rather than trying to combine "collect" and "appraise" together. > >> > >> I'm not sure I understand what you mean by "extend collect to support an > >> appended signature", but the fundamental problem is that we don't know > >> whether we need to fallback to the xattr sig until the appraise step > >> because that's when we verify the signature, and by then the collect and > >> store steps were already completed and would need to be done again if > >> the hash algorithm in the xattr sig is different. > > > > The "appraise" stage could be moved before the "store" stage, like you > > have. (This should be a separate patch explaining the need for moving > > it.) Based on an argument to ima_collect_measurement() have it > > "collect" either the appended signature or the xattr. Maybe something > > like this: > > > > loop [ appended signature, xattr ] { <== list based on policy flags > > collect_measurement() > > if failure > > continue > > appraise_measurement() > > if success > > break > > } > > > > store_measurement() > > Ok, the above is what v2 already does, so the only change I made was to > rename the function from measure_and_appraise to collect_and_appraise to > match the IMA jargon. I would prefer if you did not combine the "collect" and "appraise" functions, but left them independent of each other. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/crypto/asymmetric_keys/pkcs7_parser.c b/crypto/asymmetric_keys/pkcs7_parser.c index af4cd8649117..e41beda297a8 100644 --- a/crypto/asymmetric_keys/pkcs7_parser.c +++ b/crypto/asymmetric_keys/pkcs7_parser.c @@ -673,3 +673,15 @@ int pkcs7_note_signed_info(void *context, size_t hdrlen, return -ENOMEM; return 0; } + +/** + * pkcs7_get_message_sig - get signature in @pkcs7 + * + * This function doesn't meaningfully support messages with more than one + * signature. It will always return the first signature. + */ +const struct public_key_signature *pkcs7_get_message_sig( + const struct pkcs7_message *pkcs7) +{ + return pkcs7->signed_infos ? pkcs7->signed_infos->sig : NULL; +} diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h index 583f199400a3..a05a0d7214e6 100644 --- a/include/crypto/pkcs7.h +++ b/include/crypto/pkcs7.h @@ -29,6 +29,9 @@ extern int pkcs7_get_content_data(const struct pkcs7_message *pkcs7, const void **_data, size_t *_datalen, size_t *_headerlen); +extern const struct public_key_signature *pkcs7_get_message_sig( + const struct pkcs7_message *pkcs7); + /* * pkcs7_trust.c */ diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig index da9565891738..0d642e0317c7 100644 --- a/security/integrity/Kconfig +++ b/security/integrity/Kconfig @@ -17,8 +17,8 @@ if INTEGRITY config INTEGRITY_SIGNATURE bool "Digital signature verification using multiple keyrings" - depends on KEYS default n + select KEYS select SIGNATURE help This option enables digital signature verification support diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c index 06554c448dce..9190c9058f4f 100644 --- a/security/integrity/digsig.c +++ b/security/integrity/digsig.c @@ -48,11 +48,10 @@ static bool init_keyring __initdata; #define restrict_link_to_ima restrict_link_by_builtin_trusted #endif -int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen, - const char *digest, int digestlen) +struct key *integrity_keyring_from_id(const unsigned int id) { - if (id >= INTEGRITY_KEYRING_MAX || siglen < 2) - return -EINVAL; + if (id >= INTEGRITY_KEYRING_MAX) + return ERR_PTR(-EINVAL); if (!keyring[id]) { keyring[id] = @@ -61,18 +60,29 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen, int err = PTR_ERR(keyring[id]); pr_err("no %s keyring: %d\n", keyring_name[id], err); keyring[id] = NULL; - return err; + return ERR_PTR(err); } } + return keyring[id]; +} + +int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen, + const char *digest, int digestlen) +{ + struct key *keyring = integrity_keyring_from_id(id); + + if (IS_ERR(keyring) || siglen < 2) + return -EINVAL; + switch (sig[1]) { case 1: /* v1 API expect signature without xattr type */ - return digsig_verify(keyring[id], sig + 1, siglen - 1, - digest, digestlen); + return digsig_verify(keyring, sig + 1, siglen - 1, digest, + digestlen); case 2: - return asymmetric_verify(keyring[id], sig, siglen, - digest, digestlen); + return asymmetric_verify(keyring, sig, siglen, digest, + digestlen); } return -EOPNOTSUPP; diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index 35ef69312811..55f734a6124b 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -163,6 +163,19 @@ config IMA_APPRAISE_BOOTPARAM This option enables the different "ima_appraise=" modes (eg. fix, log) from the boot command line. +config IMA_APPRAISE_MODSIG + bool "Support module-style signatures for appraisal" + depends on IMA_APPRAISE + depends on INTEGRITY_ASYMMETRIC_KEYS + select PKCS7_MESSAGE_PARSER + select MODULE_SIG_FORMAT + default n + help + Adds support for signatures appended to files. The format of the + appended signature is the same used for signed kernel modules. + The modsig keyword can be used in the IMA policy to allow a hook + to accept such signatures. + config IMA_TRUSTED_KEYRING bool "Require all keys on the .ima keyring be signed (deprecated)" depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile index 29f198bde02b..c72026acecc3 100644 --- a/security/integrity/ima/Makefile +++ b/security/integrity/ima/Makefile @@ -8,5 +8,6 @@ obj-$(CONFIG_IMA) += ima.o ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \ ima_policy.o ima_template.o ima_template_lib.o ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o +ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index d52b487ad259..eb3ff7286b07 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -190,6 +190,8 @@ enum ima_hooks { __ima_hooks(__ima_hook_enumify) }; +extern const char *const func_tokens[]; + /* LIM API function definitions */ int ima_get_action(struct inode *inode, int mask, enum ima_hooks func, int *pcr); @@ -248,6 +250,19 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, int ima_read_xattr(struct dentry *dentry, struct evm_ima_xattr_data **xattr_value); +#ifdef CONFIG_IMA_APPRAISE_MODSIG +bool ima_hook_supports_modsig(enum ima_hooks func); +int ima_read_modsig(const void *buf, loff_t *buf_len, + struct evm_ima_xattr_data **xattr_value, + int *xattr_len); +enum hash_algo ima_get_modsig_hash_algo(struct evm_ima_xattr_data *hdr); +int ima_modsig_serialize_data(struct evm_ima_xattr_data *hdr, + struct evm_ima_xattr_data **data, int *data_len); +int ima_modsig_verify(const unsigned int keyring_id, + struct evm_ima_xattr_data *hdr); +void ima_free_xattr_data(struct evm_ima_xattr_data *hdr); +#endif + #else static inline int ima_appraise_measurement(enum ima_hooks func, struct integrity_iint_cache *iint, @@ -291,6 +306,44 @@ static inline int ima_read_xattr(struct dentry *dentry, #endif /* CONFIG_IMA_APPRAISE */ +#ifndef CONFIG_IMA_APPRAISE_MODSIG +static inline bool ima_hook_supports_modsig(enum ima_hooks func) +{ + return false; +} + +static inline int ima_read_modsig(const void *buf, loff_t *buf_len, + struct evm_ima_xattr_data **xattr_value, + int *xattr_len) +{ + return -ENOTSUPP; +} + +static inline enum hash_algo ima_get_modsig_hash_algo( + struct evm_ima_xattr_data *hdr) +{ + return HASH_ALGO__LAST; +} + +static inline int ima_modsig_serialize_data(struct evm_ima_xattr_data *hdr, + struct evm_ima_xattr_data **data, + int *data_len) +{ + return -ENOTSUPP; +} + +static inline int ima_modsig_verify(const unsigned int keyring_id, + struct evm_ima_xattr_data *hdr) +{ + return -ENOTSUPP; +} + +static inline void ima_free_xattr_data(struct evm_ima_xattr_data *hdr) +{ + kfree(hdr); +} +#endif + /* LSM based policy rules require audit */ #ifdef CONFIG_IMA_LSM_RULES diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index c2edba8de35e..e3328c866362 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -204,7 +204,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, char digest[IMA_MAX_DIGEST_SIZE]; } hash; - if (!(iint->flags & IMA_COLLECTED)) { + if (!(iint->flags & IMA_COLLECTED) || iint->ima_hash->algo != algo) { u64 i_version = file_inode(file)->i_version; if (file->f_flags & O_DIRECT) { diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 87d2b601cf8e..8716c4fb3675 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -172,6 +172,11 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, } else if (xattr_len == 17) return HASH_ALGO_MD5; break; + case IMA_MODSIG: + ret = ima_get_modsig_hash_algo(xattr_value); + if (ret < HASH_ALGO__LAST) + return ret; + break; } /* return default hash algo */ @@ -229,10 +234,14 @@ int ima_appraise_measurement(enum ima_hooks func, goto out; } - status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint); - if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) { - if ((status == INTEGRITY_NOLABEL) - || (status == INTEGRITY_NOXATTRS)) + /* Appended signatures aren't protected by EVM. */ + status = evm_verifyxattr(dentry, XATTR_NAME_IMA, + xattr_value->type == IMA_MODSIG ? + NULL : xattr_value, rc, iint); + if (status != INTEGRITY_PASS && status != INTEGRITY_UNKNOWN && + !(xattr_value->type == IMA_MODSIG && + (status == INTEGRITY_NOLABEL || status == INTEGRITY_NOXATTRS))) { + if (status == INTEGRITY_NOLABEL || status == INTEGRITY_NOXATTRS) cause = "missing-HMAC"; else if (status == INTEGRITY_FAIL) cause = "invalid-HMAC"; @@ -267,11 +276,18 @@ int ima_appraise_measurement(enum ima_hooks func, status = INTEGRITY_PASS; break; case EVM_IMA_XATTR_DIGSIG: + case IMA_MODSIG: iint->flags |= IMA_DIGSIG; - rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, - (const char *)xattr_value, rc, - iint->ima_hash->digest, - iint->ima_hash->length); + + if (xattr_value->type == EVM_IMA_XATTR_DIGSIG) + rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, + (const char *)xattr_value, + rc, iint->ima_hash->digest, + iint->ima_hash->length); + else + rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA, + xattr_value); + if (rc == -EOPNOTSUPP) { status = INTEGRITY_UNKNOWN; } else if (rc) { @@ -291,13 +307,15 @@ int ima_appraise_measurement(enum ima_hooks func, if (status != INTEGRITY_PASS) { if ((ima_appraise & IMA_APPRAISE_FIX) && (!xattr_value || - xattr_value->type != EVM_IMA_XATTR_DIGSIG)) { + (xattr_value->type != EVM_IMA_XATTR_DIGSIG && + xattr_value->type != IMA_MODSIG))) { if (!ima_fix_xattr(dentry, iint)) status = INTEGRITY_PASS; } else if ((inode->i_size == 0) && (iint->flags & IMA_NEW_FILE) && (xattr_value && - xattr_value->type == EVM_IMA_XATTR_DIGSIG)) { + (xattr_value->type == EVM_IMA_XATTR_DIGSIG || + xattr_value->type == IMA_MODSIG))) { status = INTEGRITY_PASS; } integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename, @@ -406,7 +424,8 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name, if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST)) return -EINVAL; ima_reset_appraise_flags(d_backing_inode(dentry), - (xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0); + xvalue->type == EVM_IMA_XATTR_DIGSIG || + xvalue->type == IMA_MODSIG); result = 0; } return result; diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 2aebb7984437..5527acab034e 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -16,6 +16,9 @@ * implements the IMA hooks: ima_bprm_check, ima_file_mmap, * and ima_file_check. */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + #include <linux/module.h> #include <linux/file.h> #include <linux/binfmts.h> @@ -155,12 +158,66 @@ void ima_file_free(struct file *file) ima_check_last_writer(iint, inode, file); } +static int measure_and_appraise(struct file *file, char *buf, loff_t size, + enum ima_hooks func, int opened, int action, + struct integrity_iint_cache *iint, + struct evm_ima_xattr_data **xattr_value_, + int *xattr_len_, const char *pathname, + bool appended_sig) +{ + struct ima_template_desc *template_desc; + struct evm_ima_xattr_data *xattr_value = NULL; + enum hash_algo hash_algo; + int rc, xattr_len = 0; + + template_desc = ima_template_desc_current(); + if (action & IMA_APPRAISE_SUBMASK || + strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) { + if (appended_sig) { + /* Shouldn't happen. */ + if (WARN_ONCE(!buf || !size, + "%s doesn't support modsig\n", + func_tokens[func])) + return -ENOTSUPP; + + rc = ima_read_modsig(buf, &size, &xattr_value, + &xattr_len); + if (rc) + return rc; + } else + /* read 'security.ima' */ + xattr_len = ima_read_xattr(file_dentry(file), + &xattr_value); + } + + hash_algo = ima_get_hash_algo(xattr_value, xattr_len); + + rc = ima_collect_measurement(iint, file, buf, size, hash_algo); + if (rc != 0) { + if (file->f_flags & O_DIRECT) + rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES; + goto out; + } + + if (action & IMA_APPRAISE_SUBMASK) + rc = ima_appraise_measurement(func, iint, file, pathname, + xattr_value, xattr_len, opened); +out: + if (rc) + ima_free_xattr_data(xattr_value); + else { + *xattr_value_ = xattr_value; + *xattr_len_ = xattr_len; + } + + return rc; +} + static int process_measurement(struct file *file, char *buf, loff_t size, int mask, enum ima_hooks func, int opened) { struct inode *inode = file_inode(file); struct integrity_iint_cache *iint = NULL; - struct ima_template_desc *template_desc; char *pathbuf = NULL; char filename[NAME_MAX]; const char *pathname = NULL; @@ -169,7 +226,6 @@ static int process_measurement(struct file *file, char *buf, loff_t size, struct evm_ima_xattr_data *xattr_value = NULL; int xattr_len = 0; bool violation_check; - enum hash_algo hash_algo; if (!ima_policy_flag || !S_ISREG(inode->i_mode)) return 0; @@ -226,30 +282,23 @@ static int process_measurement(struct file *file, char *buf, loff_t size, goto out_digsig; } - template_desc = ima_template_desc_current(); - if ((action & IMA_APPRAISE_SUBMASK) || - strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) - /* read 'security.ima' */ - xattr_len = ima_read_xattr(file_dentry(file), &xattr_value); - - hash_algo = ima_get_hash_algo(xattr_value, xattr_len); - - rc = ima_collect_measurement(iint, file, buf, size, hash_algo); - if (rc != 0) { - if (file->f_flags & O_DIRECT) - rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES; - goto out_digsig; - } - if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */ pathname = ima_d_path(&file->f_path, &pathbuf, filename); + if (iint->flags & IMA_MODSIG_ALLOWED) + rc = measure_and_appraise(file, buf, size, func, opened, action, + iint, &xattr_value, &xattr_len, + pathname, true); + if (!xattr_len) + rc = measure_and_appraise(file, buf, size, func, opened, action, + iint, &xattr_value, &xattr_len, + pathname, false); + if (rc) + goto out_digsig; + if (action & IMA_MEASURE) ima_store_measurement(iint, file, pathname, xattr_value, xattr_len, pcr); - if (action & IMA_APPRAISE_SUBMASK) - rc = ima_appraise_measurement(func, iint, file, pathname, - xattr_value, xattr_len, opened); if (action & IMA_AUDIT) ima_audit_measurement(iint, pathname); @@ -257,7 +306,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size, if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG) && !(iint->flags & IMA_NEW_FILE)) rc = -EACCES; - kfree(xattr_value); + ima_free_xattr_data(xattr_value); out_free: if (pathbuf) __putname(pathbuf); diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c new file mode 100644 index 000000000000..f224acb95191 --- /dev/null +++ b/security/integrity/ima/ima_modsig.c @@ -0,0 +1,167 @@ +/* + * IMA support for appraising module-style appended signatures. + * + * Copyright (C) 2017 IBM Corporation + * + * Author: + * Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation (version 2 of the License). + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/types.h> +#include <linux/module_signature.h> +#include <keys/asymmetric-type.h> +#include <crypto/pkcs7.h> + +#include "ima.h" + +struct signature_modsig_hdr { + uint8_t type; /* Should be IMA_MODSIG. */ + const void *data; /* Pointer to data covered by pkcs7_msg. */ + size_t data_len; + struct pkcs7_message *pkcs7_msg; + int raw_pkcs7_len; + + /* This will be in the measurement list if required by the template. */ + struct evm_ima_xattr_data raw_pkcs7; +}; + +/** + * ima_hook_supports_modsig - can the policy allow modsig for this hook? + * + * modsig is only supported by hooks using ima_post_read_file, because only they + * preload the contents of the file in a buffer. FILE_CHECK does that in some + * cases, but not when reached from vfs_open. POLICY_CHECK can support it, but + * it's not useful in practice because it's a text file so deny. + */ +bool ima_hook_supports_modsig(enum ima_hooks func) +{ + switch (func) { + case FIRMWARE_CHECK: + case KEXEC_KERNEL_CHECK: + case KEXEC_INITRAMFS_CHECK: + return true; + default: + return false; + } +} + +int ima_read_modsig(const void *buf, loff_t *buf_len, + struct evm_ima_xattr_data **xattr_value, + int *xattr_len) +{ + const size_t marker_len = sizeof(MODULE_SIG_STRING) - 1; + const struct module_signature *sig; + struct signature_modsig_hdr *hdr; + loff_t file_len = *buf_len; + size_t sig_len; + const void *p; + int rc; + + if (file_len <= marker_len + sizeof(*sig)) + return -ENOENT; + + p = buf + file_len - marker_len; + if (memcmp(p, MODULE_SIG_STRING, marker_len)) + return -ENOENT; + + file_len -= marker_len; + sig = (const struct module_signature *) (p - sizeof(*sig)); + + rc = validate_module_signature(sig, file_len); + if (rc) + return rc; + + sig_len = be32_to_cpu(sig->sig_len); + file_len -= sig_len + sizeof(*sig); + + hdr = kmalloc(sizeof(*hdr) + sig_len, GFP_KERNEL); + if (!hdr) + return -ENOMEM; + + hdr->pkcs7_msg = pkcs7_parse_message(buf + file_len, sig_len); + if (IS_ERR(hdr->pkcs7_msg)) { + rc = PTR_ERR(hdr->pkcs7_msg); + kfree(hdr); + return rc; + } + + memcpy(hdr->raw_pkcs7.data, buf + file_len, sig_len); + hdr->raw_pkcs7_len = sig_len + 1; + hdr->raw_pkcs7.type = IMA_MODSIG; + + hdr->type = IMA_MODSIG; + hdr->data = buf; + hdr->data_len = file_len; + + *xattr_value = (typeof(*xattr_value)) hdr; + *xattr_len = sizeof(*hdr); + *buf_len = file_len; + + return 0; +} + +enum hash_algo ima_get_modsig_hash_algo(struct evm_ima_xattr_data *hdr) +{ + const struct public_key_signature *pks; + struct signature_modsig_hdr *modsig = (typeof(modsig)) hdr; + int i; + + pks = pkcs7_get_message_sig(modsig->pkcs7_msg); + if (!pks) + return HASH_ALGO__LAST; + + for (i = 0; i < HASH_ALGO__LAST; i++) + if (!strcmp(hash_algo_name[i], pks->hash_algo)) + break; + + return i; +} + +int ima_modsig_serialize_data(struct evm_ima_xattr_data *hdr, + struct evm_ima_xattr_data **data, int *data_len) +{ + struct signature_modsig_hdr *modsig = (typeof(modsig)) hdr; + + *data = &modsig->raw_pkcs7; + *data_len = modsig->raw_pkcs7_len; + + return 0; +} + +int ima_modsig_verify(const unsigned int keyring_id, + struct evm_ima_xattr_data *hdr) +{ + struct signature_modsig_hdr *modsig = (typeof(modsig)) hdr; + struct key *trusted_keys = integrity_keyring_from_id(keyring_id); + + if (IS_ERR(trusted_keys)) + return -EINVAL; + + return verify_pkcs7_message_signature(modsig->data, modsig->data_len, + modsig->pkcs7_msg, trusted_keys, + VERIFYING_MODULE_SIGNATURE, + NULL, NULL); +} + +void ima_free_xattr_data(struct evm_ima_xattr_data *hdr) +{ + if (!hdr) + return; + + if (hdr->type == IMA_MODSIG) { + struct signature_modsig_hdr *modsig = (typeof(modsig)) hdr; + + pkcs7_free_message(modsig->pkcs7_msg); + } + + kfree(hdr); +} diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index f4436626ccb7..4047ccabcbbf 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -853,8 +853,12 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) } ima_log_string(ab, "appraise_type", args[0].from); - if ((strcmp(args[0].from, "imasig")) == 0) + if (strcmp(args[0].from, "imasig") == 0) entry->flags |= IMA_DIGSIG_REQUIRED; + else if (ima_hook_supports_modsig(entry->func) && + strcmp(args[0].from, "modsig|imasig") == 0) + entry->flags |= IMA_DIGSIG_REQUIRED + | IMA_MODSIG_ALLOWED; else result = -EINVAL; break; @@ -960,6 +964,12 @@ void ima_delete_rules(void) } } +#define __ima_hook_stringify(str) (#str), + +const char *const func_tokens[] = { + __ima_hooks(__ima_hook_stringify) +}; + #ifdef CONFIG_IMA_READ_POLICY enum { mask_exec = 0, mask_write, mask_read, mask_append @@ -972,12 +982,6 @@ static const char *const mask_tokens[] = { "MAY_APPEND" }; -#define __ima_hook_stringify(str) (#str), - -static const char *const func_tokens[] = { - __ima_hooks(__ima_hook_stringify) -}; - void *ima_policy_start(struct seq_file *m, loff_t *pos) { loff_t l = *pos; @@ -1140,8 +1144,12 @@ int ima_policy_show(struct seq_file *m, void *v) } } } - if (entry->flags & IMA_DIGSIG_REQUIRED) - seq_puts(m, "appraise_type=imasig "); + if (entry->flags & IMA_DIGSIG_REQUIRED) { + if (entry->flags & IMA_MODSIG_ALLOWED) + seq_puts(m, "appraise_type=modsig|imasig "); + else + seq_puts(m, "appraise_type=imasig "); + } if (entry->flags & IMA_PERMIT_DIRECTIO) seq_puts(m, "permit_directio "); rcu_read_unlock(); diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index f9ba37b3928d..be123485e486 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -322,9 +322,21 @@ int ima_eventsig_init(struct ima_event_data *event_data, int xattr_len = event_data->xattr_len; int rc = 0; - if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG)) + if (!xattr_value || (xattr_value->type != EVM_IMA_XATTR_DIGSIG && + xattr_value->type != IMA_MODSIG)) goto out; + /* + * The xattr_value for IMA_MODSIG is a runtime structure containing + * pointers. Get its raw data instead. + */ + if (xattr_value->type == IMA_MODSIG) { + rc = ima_modsig_serialize_data(xattr_value, &xattr_value, + &xattr_len); + if (rc) + goto out; + } + rc = ima_write_template_field_data(xattr_value, xattr_len, fmt, field_data); out: diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 874211aba6e9..2c9393cf2860 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -28,11 +28,12 @@ /* iint cache flags */ #define IMA_ACTION_FLAGS 0xff000000 -#define IMA_ACTION_RULE_FLAGS 0x06000000 +#define IMA_ACTION_RULE_FLAGS 0x16000000 #define IMA_DIGSIG 0x01000000 #define IMA_DIGSIG_REQUIRED 0x02000000 #define IMA_PERMIT_DIRECTIO 0x04000000 #define IMA_NEW_FILE 0x08000000 +#define IMA_MODSIG_ALLOWED 0x10000000 #define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \ IMA_APPRAISE_SUBMASK) @@ -58,6 +59,7 @@ enum evm_ima_xattr_type { EVM_XATTR_HMAC, EVM_IMA_XATTR_DIGSIG, IMA_XATTR_DIGEST_NG, + IMA_MODSIG, IMA_XATTR_LAST }; @@ -129,6 +131,7 @@ int __init integrity_read_file(const char *path, char **data); #ifdef CONFIG_INTEGRITY_SIGNATURE +struct key *integrity_keyring_from_id(const unsigned int id); int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen, const char *digest, int digestlen);
This patch introduces the modsig keyword to the IMA policy syntax to specify that a given hook should expect the file to have the IMA signature appended to it. Here is how it can be used in a rule: appraise func=KEXEC_KERNEL_CHECK appraise_type=modsig|imasig With this rule, IMA will accept either an appended signature or a signature stored in the extended attribute. In that case, it will first check whether there is an appended signature, and if not it will read it from the extended attribute. The format of the appended signature is the same used for signed kernel modules. This means that the file can be signed with the scripts/sign-file tool, with a command line such as this: $ sign-file sha256 privkey_ima.pem x509_ima.der vmlinux This code only works for files that are hashed from a memory buffer, not for files that are read from disk at the time of hash calculation. In other words, only hooks that use kernel_read_file can support appended signatures. This means that only FIRMWARE_CHECK, KEXEC_KERNEL_CHECK, KEXEC_INITRAMFS_CHECK and POLICY_CHECK can be supported. This feature warrants a separate config option because it depends on many other config options: ASYMMETRIC_KEY_TYPE n -> y CRYPTO_RSA n -> y INTEGRITY_SIGNATURE n -> y MODULE_SIG_FORMAT n -> y SYSTEM_DATA_VERIFICATION n -> y +ASN1 y +ASYMMETRIC_PUBLIC_KEY_SUBTYPE y +CLZ_TAB y +CRYPTO_AKCIPHER y +IMA_APPRAISE_MODSIG y +IMA_TRUSTED_KEYRING n +INTEGRITY_ASYMMETRIC_KEYS y +INTEGRITY_TRUSTED_KEYRING n +MPILIB y +OID_REGISTRY y +PKCS7_MESSAGE_PARSER y +PKCS7_TEST_KEY n +SECONDARY_TRUSTED_KEYRING n +SIGNATURE y +SIGNED_PE_FILE_VERIFICATION n +SYSTEM_EXTRA_CERTIFICATE n +SYSTEM_TRUSTED_KEYRING y +SYSTEM_TRUSTED_KEYS "" +X509_CERTIFICATE_PARSER y The change in CONFIG_INTEGRITY_SIGNATURE to select CONFIG_KEYS instead of depending on it is to avoid a dependency recursion in CONFIG_IMA_APPRAISE_MODSIG, because CONFIG_MODULE_SIG_FORMAT selects CONFIG_KEYS and Kconfig complains that CONFIG_INTEGRITY_SIGNATURE depends on it. Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> --- crypto/asymmetric_keys/pkcs7_parser.c | 12 +++ include/crypto/pkcs7.h | 3 + security/integrity/Kconfig | 2 +- security/integrity/digsig.c | 28 +++-- security/integrity/ima/Kconfig | 13 +++ security/integrity/ima/Makefile | 1 + security/integrity/ima/ima.h | 53 ++++++++++ security/integrity/ima/ima_api.c | 2 +- security/integrity/ima/ima_appraise.c | 41 ++++++-- security/integrity/ima/ima_main.c | 91 ++++++++++++---- security/integrity/ima/ima_modsig.c | 167 ++++++++++++++++++++++++++++++ security/integrity/ima/ima_policy.c | 26 +++-- security/integrity/ima/ima_template_lib.c | 14 ++- security/integrity/integrity.h | 5 +- 14 files changed, 404 insertions(+), 54 deletions(-)