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 <bauer...@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 <bauer...@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);
> 

Reply via email to