| Message ID | 1492546666-16615-7-git-send-email-bauerman@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Thiago,
[auto build test ERROR on security/next]
[also build test ERROR on v4.11-rc7 next-20170419]
[cannot apply to integrity/next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Thiago-Jung-Bauermann/Appended-signatures-support-for-IMA-appraisal/20170419-053422
base: https://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next
config: i386-randconfig-h0-04201028 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
In file included from security/integrity/ima/ima_appraise.c:19:0:
include/keys/asymmetric-type.h: In function 'asymmetric_key_ids':
>> include/keys/asymmetric-type.h:76:12: error: dereferencing pointer to incomplete type 'const struct key'
return key->payload.data[asym_key_ids];
^~
vim +76 include/keys/asymmetric-type.h
7901c1a8 David Howells 2014-09-16 70 size_t len_1,
7901c1a8 David Howells 2014-09-16 71 const void *val_2,
7901c1a8 David Howells 2014-09-16 72 size_t len_2);
146aa8b1 David Howells 2015-10-21 73 static inline
146aa8b1 David Howells 2015-10-21 74 const struct asymmetric_key_ids *asymmetric_key_ids(const struct key *key)
146aa8b1 David Howells 2015-10-21 75 {
146aa8b1 David Howells 2015-10-21 @76 return key->payload.data[asym_key_ids];
146aa8b1 David Howells 2015-10-21 77 }
7901c1a8 David Howells 2014-09-16 78
9eb02989 David Howells 2016-04-06 79 extern struct key *find_asymmetric_key(struct key *keyring,
:::::: The code at line 76 was first introduced by commit
:::::: 146aa8b1453bd8f1ff2304ffb71b4ee0eb9acdcc KEYS: Merge the type-specific data with the payload data
:::::: TO: David Howells <dhowells@redhat.com>
:::::: CC: David Howells <dhowells@redhat.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Thiago, On Tue, 2017-04-18 at 17:17 -0300, Thiago Jung Bauermann wrote: > This patch introduces the appended_imasig 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=appended_imasig > appraise func=KEXEC_KERNEL_CHECK appraise_type=appended_imasig|imasig > In the second form, 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. An appended signature should be another place to look for a signature, when a signature is required, but it shouldn't make a difference where the signature is located. "imasig" could have implied to look for the signature in both places - xattr or appended. So the new option is just a hint - a performance improvement. This might seem picayune, but the difference between "expect" vs. "hint" impacts the code. (Further explanation inline.) > 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. > > 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_APPENDED_SIG, 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> > --- snip > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 2aebb7984437..994ee420b2ec 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> > @@ -228,9 +231,30 @@ 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); > + strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) { > +#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG Using "ifdef" in C code is really discouraged. Normally, it is an indication that the code needs to be re-factored. Assuming we really need a new CONFIG option, which I'm not sure that we do, I would move the appended signature code to its own file, define stub functions in ima.h, and update the Makefile. > + unsigned long digsig_req; > + > + if (iint->flags & IMA_APPENDED_DIGSIG_REQUIRED) { > + if (!buf || !size) > + pr_err("%s doesn't support appended_imasig\n", > + func_tokens[func]); The policy parsing should prevent defining appended_imasig on inappropriate hooks. Since the iint->flags might be shared between hooks, we might still need to test buf, but it could be simplified to: if (!buf && iint->flags & IMA_APPENDED_DIGSIG_REQUIRED) { > + else > + ima_read_appended_sig(buf, &size, &xattr_value, > + &xattr_len); > + } > + > + /* > + * Don't try to read the xattr if we require an appended > + * signature but failed to get one. > + */ If the appended_sig is just a hint as to where the signature is located, then we should read the xattr, even if IMA_DIGSIG_REQUIRED is not specified. ima_appraise_measurement() should be updated to require a signature if either IMA_DIGSIG_REQUIRED or IMA_APPENDED_SIGNATURE_REQUIRED are specified. Part of the confusion might be due to the naming -"IMA_APPENEDED_SIGNATURE_REQUIRED". > + digsig_req = iint->flags & IMA_DIGSIG_REQUIRED_MASK; > + if (!xattr_len && digsig_req != IMA_APPENDED_DIGSIG_REQUIRED) > +#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */ Is limiting the "if" to the ifdef really necessary? > + /* read 'security.ima' */ > + xattr_len = ima_read_xattr(file_dentry(file), > + &xattr_value); > + } > Suppose an appended signature and an xattr both exist (eg. kernel modules), but for some reason the appended signature validation fails. The code should somehow retry the signature validation with the xattr. > hash_algo = ima_get_hash_algo(xattr_value, xattr_len); > Unfortunately, if the hash algorithm in the appended signature and the xattr are not the same, then we would need to re-calculate the file hash. 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
Hello Mimi, Thanks for your review. Am Mittwoch, 26. April 2017, 07:21:19 BRT schrieb Mimi Zohar: > On Tue, 2017-04-18 at 17:17 -0300, Thiago Jung Bauermann wrote: > > This patch introduces the appended_imasig 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=appended_imasig > > appraise func=KEXEC_KERNEL_CHECK appraise_type=appended_imasig|imasig > > > > In the second form, 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. > > An appended signature should be another place to look for a signature, > when a signature is required, but it shouldn't make a difference where > the signature is located. "imasig" could have implied to look for the > signature in both places - xattr or appended. So the new option is > just a hint - a performance improvement. > > This might seem picayune, but the difference between "expect" vs. > "hint" impacts the code. (Further explanation inline.) Thanks, I just learned a new word. :-) If appended_imasig is just a performance improvement, is it really necessary? Perhaps if CONFIG_IMA_APPENDED_SIG is enabled then imasig would imply always first looking for an appended signature. My first impression is that yes, it's necessary because pointlessly checking for an appended signature at the end of every file covered by FILE_CHECK could have a performance impact on the system. On the other hand, it's just a memcmp and that may be negligible compared to the cost of reading an xattr. II'll have to measure that... > > diff --git a/security/integrity/ima/ima_main.c > > b/security/integrity/ima/ima_main.c index 2aebb7984437..994ee420b2ec > > 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> > > > > @@ -228,9 +231,30 @@ 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); > > + strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) { > > +#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG > > Using "ifdef" in C code is really discouraged. Normally, it is an > indication that the code needs to be re-factored. Assuming we really > need a new CONFIG option, which I'm not sure that we do, I would move > the appended signature code to its own file, define stub functions in > ima.h, and update the Makefile. I didn't like the #ifdef here either and tried to find an alternative, but couldn't come up with something that was actually better. Perhaps it'll be easier with the changes needed in process_measurement to try reading the xattr if the appended sig fails. I decided to add a new CONFIG option because otherwise CONFIG_IMA_APPRAISE would have to start depending on CONFIG_KEYS, CONFIG_PKCS7_MESSAGE_PARSER, CONFIG_X509_CERTIFICATE_PARSER and others. If you don't mind the extra dependencies, I can remove the CONFIG option. > > + unsigned long digsig_req; > > + > > + if (iint->flags & IMA_APPENDED_DIGSIG_REQUIRED) { > > + if (!buf || !size) > > + pr_err("%s doesn't support appended_imasig\n", > > + func_tokens[func]); > > The policy parsing should prevent defining appended_imasig on > inappropriate hooks. Since the iint->flags might be shared between > hooks, we might still need to test buf, but it could be simplified to: > > if (!buf && iint->flags & IMA_APPENDED_DIGSIG_REQUIRED) { That would require that me maintain a list of IMA hooks that use kernel_read_file and update it if any hook is converted to use it. I can do that if you think it's worth it (it is if it's unlikely or infrequent that hooks get converted to kernel_read_file). I could also implement appended_imasig for hooks which need IMA to read the file and remove the restriction altogether. I'd have to check how hard that would be. Also not sure how useful it would be in practice to have those hooks supporting an appended signature. > > + else > > + ima_read_appended_sig(buf, &size, &xattr_value, > > + &xattr_len); > > + } > > + > > + /* > > + * Don't try to read the xattr if we require an appended > > + * signature but failed to get one. > > + */ > > If the appended_sig is just a hint as to where the signature is > located, then we should read the xattr, even if IMA_DIGSIG_REQUIRED is > not specified. ima_appraise_measurement() should be updated to > require a signature if either IMA_DIGSIG_REQUIRED or > IMA_APPENDED_SIGNATURE_REQUIRED are specified. Indeed. > Part of the confusion might be due to the naming > -"IMA_APPENEDED_SIGNATURE_REQUIRED". Right, not a good name. I'll change that. > > + digsig_req = iint->flags & IMA_DIGSIG_REQUIRED_MASK; > > + if (!xattr_len && digsig_req != IMA_APPENDED_DIGSIG_REQUIRED) > > +#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */ > > Is limiting the "if" to the ifdef really necessary? Hopefully not after the changes to make process_measurement try one sig and then the other. > > hash_algo = ima_get_hash_algo(xattr_value, xattr_len); > > Unfortunately, if the hash algorithm in the appended signature and the > xattr are not the same, then we would need to re-calculate the file > hash. I think the most common case for the appended sig verification to fail will be that it won't be there than the signature actually failing. Having to rehash the file is unfortunate but how frequently would that happen?
diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c index e4b0ed386bc8..0d58ecfba2ea 100644 --- a/crypto/asymmetric_keys/asymmetric_type.c +++ b/crypto/asymmetric_keys/asymmetric_type.c @@ -28,6 +28,7 @@ const char *const key_being_used_for[NR__KEY_BEING_USED_FOR] = { [VERIFYING_KEXEC_PE_SIGNATURE] = "kexec PE sig", [VERIFYING_KEY_SIGNATURE] = "key sig", [VERIFYING_KEY_SELF_SIGNATURE] = "key self sig", + [VERIFYING_KEXEC_CMS_SIGNATURE] = "kexec CMS sig", [VERIFYING_UNSPECIFIED_SIGNATURE] = "unspec sig", }; EXPORT_SYMBOL_GPL(key_being_used_for); 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/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c index 2d93d9eccb4d..eee78074580a 100644 --- a/crypto/asymmetric_keys/pkcs7_verify.c +++ b/crypto/asymmetric_keys/pkcs7_verify.c @@ -420,6 +420,19 @@ int pkcs7_verify(struct pkcs7_message *pkcs7, return -EKEYREJECTED; } break; + case VERIFYING_KEXEC_CMS_SIGNATURE: + /* Shipping certificates in the CMS message is not allowed. */ + if (pkcs7->certs) { + pr_warn("Signature isn't allowed to contain certificates.\n"); + return -EBADMSG; + } + + /* Shipping CRLs in the CMS message is not allowed. */ + if (pkcs7->crl) { + pr_warn("Signature isn't allowed to contain CRLs.\n"); + return -EBADMSG; + } + break; default: return -EINVAL; } 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/include/linux/verification.h b/include/linux/verification.h index a10549a6c7cd..b7298d804440 100644 --- a/include/linux/verification.h +++ b/include/linux/verification.h @@ -22,6 +22,7 @@ enum key_being_used_for { VERIFYING_KEY_SIGNATURE, VERIFYING_KEY_SELF_SIGNATURE, VERIFYING_UNSPECIFIED_SIGNATURE, + VERIFYING_KEXEC_CMS_SIGNATURE, NR__KEY_BEING_USED_FOR }; extern const char *const key_being_used_for[NR__KEY_BEING_USED_FOR]; 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/ima/Kconfig b/security/integrity/ima/Kconfig index 370eb2f4dd37..13662043bf90 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -155,6 +155,19 @@ config IMA_APPRAISE <http://linux-ima.sourceforge.net> If unsure, say N. +config IMA_APPRAISE_APPENDED_SIG + bool "Support appended 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 appended_imasig keyword can be used in the IMA policy for this + purpose. + 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/ima.h b/security/integrity/ima/ima.h index b563fbd4d122..63ce5a1179c3 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -185,6 +185,8 @@ enum ima_hooks { MAX_CHECK }; +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); @@ -243,6 +245,12 @@ 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_APPENDED_SIG +void ima_read_appended_sig(const void *buf, loff_t *buf_len, + struct evm_ima_xattr_data **xattr_value, + int *xattr_len); +#endif + #else static inline int ima_appraise_measurement(enum ima_hooks func, struct integrity_iint_cache *iint, diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 8c8030a29117..64a42f16c4ee 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -9,12 +9,15 @@ * the Free Software Foundation, version 2 of the License. */ #include <linux/module.h> +#include <linux/module_signature.h> #include <linux/file.h> #include <linux/fs.h> #include <linux/xattr.h> #include <linux/magic.h> #include <linux/ima.h> #include <linux/evm.h> +#include <keys/asymmetric-type.h> +#include <crypto/pkcs7.h> #include "ima.h" @@ -177,6 +180,85 @@ int ima_read_xattr(struct dentry *dentry, return ret; } +#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG +void ima_read_appended_sig(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 public_key_signature *pks; + const struct module_signature *sig; + struct signature_v2_hdr *hdr; + struct pkcs7_message *pkcs7; + int i, hdr_len; + loff_t file_len = *buf_len; + size_t sig_len; + const void *p; + + if (file_len <= marker_len + sizeof(*sig)) + return; + + p = buf + file_len - marker_len; + if (memcmp(p, MODULE_SIG_STRING, marker_len)) + return; + + file_len -= marker_len; + sig = (const struct module_signature *) (p - sizeof(*sig)); + + if (validate_module_signature(sig, file_len)) + return; + + sig_len = be32_to_cpu(sig->sig_len); + file_len -= sig_len + sizeof(*sig); + + pkcs7 = pkcs7_parse_message(buf + file_len, sig_len); + if (IS_ERR(pkcs7)) + return; + + if (pkcs7_verify(pkcs7, VERIFYING_KEXEC_CMS_SIGNATURE)) + goto out; + + pks = pkcs7_get_message_sig(pkcs7); + if (!pks) + goto out; + + /* IMA only supports RSA keys. */ + if (strcmp(pks->pkey_algo, "rsa")) + goto out; + + if (!pks->auth_ids[0]) + goto out; + + for (i = 0; i < HASH_ALGO__LAST; i++) + if (!strcmp(hash_algo_name[i], pks->hash_algo)) + break; + + if (i == HASH_ALGO__LAST) + goto out; + + hdr_len = sizeof(*hdr) + pks->s_size; + hdr = kmalloc(hdr_len, GFP_KERNEL); + if (!hdr) + goto out; + + hdr->type = EVM_IMA_XATTR_DIGSIG; + hdr->version = 2; + hdr->hash_algo = i; + memcpy(hdr->sig, pks->s, pks->s_size); + hdr->sig_size = cpu_to_be16(pks->s_size); + + p = pks->auth_ids[0]->data + pks->auth_ids[0]->len - sizeof(hdr->keyid); + memcpy(&hdr->keyid, p, sizeof(hdr->keyid)); + + *xattr_value = (typeof(*xattr_value)) hdr; + *xattr_len = hdr_len; + *buf_len = file_len; + + out: + pkcs7_free_message(pkcs7); +} +#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */ + /* * ima_appraise_measurement - appraise file measurement * @@ -205,7 +287,7 @@ int ima_appraise_measurement(enum ima_hooks func, if (rc && rc != -ENODATA) goto out; - cause = iint->flags & IMA_DIGSIG_REQUIRED ? + cause = iint->flags & IMA_DIGSIG_REQUIRED_MASK ? "IMA-signature-required" : "missing-hash"; status = INTEGRITY_NOLABEL; if (opened & FILE_CREATED) { diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 2aebb7984437..994ee420b2ec 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> @@ -228,9 +231,30 @@ 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); + strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) { +#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG + unsigned long digsig_req; + + if (iint->flags & IMA_APPENDED_DIGSIG_REQUIRED) { + if (!buf || !size) + pr_err("%s doesn't support appended_imasig\n", + func_tokens[func]); + else + ima_read_appended_sig(buf, &size, &xattr_value, + &xattr_len); + } + + /* + * Don't try to read the xattr if we require an appended + * signature but failed to get one. + */ + digsig_req = iint->flags & IMA_DIGSIG_REQUIRED_MASK; + if (!xattr_len && digsig_req != IMA_APPENDED_DIGSIG_REQUIRED) +#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */ + /* read 'security.ima' */ + xattr_len = ima_read_xattr(file_dentry(file), + &xattr_value); + } hash_algo = ima_get_hash_algo(xattr_value, xattr_len); diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 158eafef64e8..96601b5ba411 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -777,8 +777,15 @@ 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")) entry->flags |= IMA_DIGSIG_REQUIRED; +#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG + else if (!strcmp(args[0].from, "appended_imasig")) + entry->flags |= IMA_APPENDED_DIGSIG_REQUIRED; + else if (!strcmp(args[0].from, "appended_imasig|imasig")) + entry->flags |= IMA_DIGSIG_REQUIRED + | IMA_APPENDED_DIGSIG_REQUIRED; +#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */ else result = -EINVAL; break; @@ -884,19 +891,7 @@ void ima_delete_rules(void) } } -#ifdef CONFIG_IMA_READ_POLICY -enum { - mask_exec = 0, mask_write, mask_read, mask_append -}; - -static const char *const mask_tokens[] = { - "MAY_EXEC", - "MAY_WRITE", - "MAY_READ", - "MAY_APPEND" -}; - -static const char *const func_tokens[] = { +const char *const func_tokens[] = { NULL, "FILE_CHECK", "MMAP_CHECK", @@ -909,6 +904,18 @@ static const char *const func_tokens[] = { "POLICY_CHECK" }; +#ifdef CONFIG_IMA_READ_POLICY +enum { + mask_exec = 0, mask_write, mask_read, mask_append +}; + +static const char *const mask_tokens[] = { + "MAY_EXEC", + "MAY_WRITE", + "MAY_READ", + "MAY_APPEND" +}; + void *ima_policy_start(struct seq_file *m, loff_t *pos) { loff_t l = *pos; @@ -1062,7 +1069,13 @@ int ima_policy_show(struct seq_file *m, void *v) } } } - if (entry->flags & IMA_DIGSIG_REQUIRED) + if ((entry->flags & IMA_DIGSIG_REQUIRED_MASK) == IMA_DIGSIG_REQUIRED_MASK) +#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG + seq_puts(m, "appraise_type=appended_imasig|imasig "); + else if (entry->flags & IMA_APPENDED_DIGSIG_REQUIRED) + seq_puts(m, "appraise_type=appended_imasig "); + else if (entry->flags & IMA_DIGSIG_REQUIRED) +#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */ seq_puts(m, "appraise_type=imasig "); if (entry->flags & IMA_PERMIT_DIRECTIO) seq_puts(m, "permit_directio "); diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index a53e7e4ab06c..de2a666740c1 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -27,12 +27,20 @@ #define IMA_AUDITED 0x00000080 /* iint cache flags */ -#define IMA_ACTION_FLAGS 0xff000000 -#define IMA_ACTION_RULE_FLAGS 0x06000000 -#define IMA_DIGSIG 0x01000000 -#define IMA_DIGSIG_REQUIRED 0x02000000 -#define IMA_PERMIT_DIRECTIO 0x04000000 -#define IMA_NEW_FILE 0x08000000 +#define IMA_ACTION_FLAGS 0xff000000 +#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_APPENDED_DIGSIG_REQUIRED 0x10000000 + +#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG +#define IMA_DIGSIG_REQUIRED_MASK (IMA_DIGSIG_REQUIRED | \ + IMA_APPENDED_DIGSIG_REQUIRED) +#else +#define IMA_DIGSIG_REQUIRED_MASK IMA_DIGSIG_REQUIRED +#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */ #define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \ IMA_APPRAISE_SUBMASK)
This patch introduces the appended_imasig 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=appended_imasig appraise func=KEXEC_KERNEL_CHECK appraise_type=appended_imasig|imasig In the second form, 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. 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_APPENDED_SIG, 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/asymmetric_type.c | 1 + crypto/asymmetric_keys/pkcs7_parser.c | 12 +++++ crypto/asymmetric_keys/pkcs7_verify.c | 13 +++++ include/crypto/pkcs7.h | 3 ++ include/linux/verification.h | 1 + security/integrity/Kconfig | 2 +- security/integrity/ima/Kconfig | 13 +++++ security/integrity/ima/ima.h | 8 +++ security/integrity/ima/ima_appraise.c | 84 +++++++++++++++++++++++++++++++- security/integrity/ima/ima_main.c | 30 ++++++++++-- security/integrity/ima/ima_policy.c | 43 ++++++++++------ security/integrity/integrity.h | 20 +++++--- 12 files changed, 204 insertions(+), 26 deletions(-)