[PATCH v2 0/6] Appended signatures support for IMA appraisal

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 0/6] Appended signatures support for IMA appraisal

Thiago Jung Bauermann
On the OpenPOWER platform, secure boot and trusted boot are being
implemented using IMA for taking measurements and verifying signatures.
Since the kernel image on Power servers is an ELF binary, kernels are
signed using the scripts/sign-file tool and thus use the same signature
format as signed kernel modules.

This patch series adds support in IMA for verifying those signatures.
It adds flexibility to OpenPOWER secure boot, because it allows it to boot
kernels with the signature appended to them as well as kernels where the
signature is stored in the IMA extended attribute.

The first four patches are cleanups and improvements that can be taken
independently from the others (and from each other as well). The last two
are the ones actually focused on this feature.

Since modsig is only supported on some specific hooks which don't get
called often (cf. ima_hook_supports_modsig), it's possible to always check
for the presence of an appended modsig before looking for the xattr sig. In
that case, the policy doesn't need to be changed to support the modsig
keyword. Is that preferable than requiring the policy to explicitly allow a
modsig like this code does?

I tested these patches with EVM and I believe they don't break it and
things work as expected, but I'm not really familiar with EVM and its use
cases so this should be taken with a grain of salt.

I also verified that the code correctly recalculates the file hash if the
modsig verification fails and the file also has an xattr signature which
uses a different hash algorithm.

These patches apply on top of today's linux-integrity/next.

Changes since v1:
- Patch "integrity: Small code improvements"
  - Add missing #endif comment in ima.h.

- Patch "ima: Tidy up constant strings"
  - Squashed into previous patch.

- Patch "ima: Simplify policy_func_show."
  - Generate ima_hooks enum and func_tokens array from a single macro.
    (suggested by Mimi)
  - Further simplify policy_func_show by not using the printf format string
    from the policy_tokens table.

- Patch "integrity: Introduce struct evm_hmac_xattr"
  - New patch.

- Patch "MODSIGN: Export module signature definitions."
  - Add function verify_pkcs7_message_signature which takes a
    struct pkcs7_message.
  - Move MODULE_SIG_STRING definition from <linux/module.h> to
    <linux/module_signature.h>.

- Patch "ima: Support appended signatures for appraisal"
  - Changed name from appended_sig to modsig. (suggested by Mehmet Kayaalp)
  - Don't add key_being_used_for value VERIFYING_KEXEC_CMS_SIGNATURE. Use
    existing VERIFYING_MODULE_SIGNATURE. (suggested by Mimi)
  - Moved modsig code to its own file. (suggested by Mimi)
  - Added new xattr "subtype" IMA_MODSIG. (suggested by Mimi)
  - Check whether a hook supports modsig when the policy is being parsed.
    (suggested by Mimi)
  - If the modsig verification fails, look for an xattr signature.
    (suggested by Mimi)
  - Add integrity_keyring_from_id function.
  - Put modsig to measurement list if the template requires the signature
    contents. (suggested by Mimi).

Thiago Jung Bauermann (6):
  integrity: Small code improvements
  ima: Simplify policy_func_show.
  ima: Log the same audit cause whenever a file has no signature
  integrity: Introduce struct evm_hmac_xattr
  MODSIGN: Export module signature definitions.
  ima: Support module-style appended signatures for appraisal

 certs/system_keyring.c                    |  62 ++++++++---
 crypto/asymmetric_keys/pkcs7_parser.c     |  12 +++
 include/crypto/pkcs7.h                    |   3 +
 include/linux/module.h                    |   3 -
 include/linux/module_signature.h          |  48 +++++++++
 include/linux/verification.h              |  10 ++
 init/Kconfig                              |   6 +-
 kernel/Makefile                           |   2 +-
 kernel/module.c                           |   1 +
 kernel/module_signing.c                   |  74 ++++++-------
 security/integrity/Kconfig                |   2 +-
 security/integrity/digsig.c               |  28 +++--
 security/integrity/digsig_asymmetric.c    |   4 +-
 security/integrity/evm/evm.h              |   5 +
 security/integrity/evm/evm_crypto.c       |   2 +-
 security/integrity/evm/evm_main.c         |   8 +-
 security/integrity/iint.c                 |   2 +-
 security/integrity/ima/Kconfig            |  13 +++
 security/integrity/ima/Makefile           |   1 +
 security/integrity/ima/ima.h              |  78 ++++++++++++--
 security/integrity/ima/ima_api.c          |   2 +-
 security/integrity/ima/ima_appraise.c     |  52 +++++++---
 security/integrity/ima/ima_main.c         |  91 ++++++++++++----
 security/integrity/ima/ima_modsig.c       | 167 ++++++++++++++++++++++++++++++
 security/integrity/ima/ima_policy.c       |  82 ++++-----------
 security/integrity/ima/ima_template_lib.c |  14 ++-
 security/integrity/integrity.h            |  14 ++-
 27 files changed, 591 insertions(+), 195 deletions(-)
 create mode 100644 include/linux/module_signature.h
 create mode 100644 security/integrity/ima/ima_modsig.c

--
2.7.4

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 1/6] integrity: Small code improvements

Thiago Jung Bauermann
These changes are too small to warrant their own patches:

The keyid and sig_size members of struct signature_v2_hdr are in BE format,
so use a type that makes this assumption explicit. Also, use beXX_to_cpu
instead of __beXX_to_cpu to read them.

Change integrity_kernel_read to take a void * buffer instead of char *
buffer, so that callers don't have to use a cast if they provide a buffer
that isn't a char *.

Add missing #endif comment in ima.h pointing out which macro it refers to.

Add missing fall through comment in ima_appraise.c.

Constify mask_tokens and func_tokens arrays.

Signed-off-by: Thiago Jung Bauermann <[hidden email]>
---
 security/integrity/digsig_asymmetric.c | 4 ++--
 security/integrity/iint.c              | 2 +-
 security/integrity/ima/ima.h           | 2 +-
 security/integrity/ima/ima_appraise.c  | 1 +
 security/integrity/ima/ima_policy.c    | 4 ++--
 security/integrity/integrity.h         | 7 ++++---
 6 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c
index 80052ed8d467..ab6a029062a1 100644
--- a/security/integrity/digsig_asymmetric.c
+++ b/security/integrity/digsig_asymmetric.c
@@ -92,13 +92,13 @@ int asymmetric_verify(struct key *keyring, const char *sig,
 
  siglen -= sizeof(*hdr);
 
- if (siglen != __be16_to_cpu(hdr->sig_size))
+ if (siglen != be16_to_cpu(hdr->sig_size))
  return -EBADMSG;
 
  if (hdr->hash_algo >= HASH_ALGO__LAST)
  return -ENOPKG;
 
- key = request_asymmetric_key(keyring, __be32_to_cpu(hdr->keyid));
+ key = request_asymmetric_key(keyring, be32_to_cpu(hdr->keyid));
  if (IS_ERR(key))
  return PTR_ERR(key);
 
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index c710d22042f9..6fc888ca468e 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -182,7 +182,7 @@ security_initcall(integrity_iintcache_init);
  *
  */
 int integrity_kernel_read(struct file *file, loff_t offset,
-  char *addr, unsigned long count)
+  void *addr, unsigned long count)
 {
  mm_segment_t old_fs;
  char __user *buf = (char __user *)addr;
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d26a30e37d13..215a93c41b51 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -284,7 +284,7 @@ static inline int ima_read_xattr(struct dentry *dentry,
  return 0;
 }
 
-#endif
+#endif /* CONFIG_IMA_APPRAISE */
 
 /* LSM based policy rules require audit */
 #ifdef CONFIG_IMA_LSM_RULES
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 7fe0566142d8..ea36a4f134f4 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -240,6 +240,7 @@ int ima_appraise_measurement(enum ima_hooks func,
  case IMA_XATTR_DIGEST_NG:
  /* first byte contains algorithm id */
  hash_start = 1;
+ /* fall through */
  case IMA_XATTR_DIGEST:
  if (iint->flags & IMA_DIGSIG_REQUIRED) {
  cause = "IMA-signature-required";
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 0acd68decb17..949ad3858327 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -965,7 +965,7 @@ enum {
  mask_exec = 0, mask_write, mask_read, mask_append
 };
 
-static char *mask_tokens[] = {
+static const char *const mask_tokens[] = {
  "MAY_EXEC",
  "MAY_WRITE",
  "MAY_READ",
@@ -979,7 +979,7 @@ enum {
  func_policy
 };
 
-static char *func_tokens[] = {
+static const char *const func_tokens[] = {
  "FILE_CHECK",
  "MMAP_CHECK",
  "BPRM_CHECK",
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 24520b4ef3b0..a53e7e4ab06c 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -92,8 +92,8 @@ struct signature_v2_hdr {
  uint8_t type; /* xattr type */
  uint8_t version; /* signature format version */
  uint8_t hash_algo; /* Digest algorithm [enum hash_algo] */
- uint32_t keyid; /* IMA key identifier - not X509/PGP specific */
- uint16_t sig_size; /* signature size */
+ __be32 keyid; /* IMA key identifier - not X509/PGP specific */
+ __be16 sig_size; /* signature size */
  uint8_t sig[0]; /* signature payload */
 } __packed;
 
@@ -118,7 +118,8 @@ struct integrity_iint_cache {
 struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
 
 int integrity_kernel_read(struct file *file, loff_t offset,
-  char *addr, unsigned long count);
+  void *addr, unsigned long count);
+
 int __init integrity_read_file(const char *path, char **data);
 
 #define INTEGRITY_KEYRING_EVM 0
--
2.7.4

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 2/6] ima: Simplify policy_func_show.

Thiago Jung Bauermann
In reply to this post by Thiago Jung Bauermann
If the func_tokens array uses the same indices as enum ima_hooks,
policy_func_show can be a lot simpler, and the func_* enum becomes
unnecessary.

Also, if we use the same macro trick used by kernel_read_file_id_str we can
use one hooks list for both the enum and the string array, making sure they
are always in sync (suggested by Mimi Zohar).

Finally, by using the printf pattern for the function token directly
instead of using the pt macro we can simplify policy_func_show even further
and avoid needing a temporary buffer.

Signed-off-by: Thiago Jung Bauermann <[hidden email]>
---
 security/integrity/ima/ima.h        | 25 +++++++++-------
 security/integrity/ima/ima_policy.c | 58 ++++---------------------------------
 2 files changed, 21 insertions(+), 62 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 215a93c41b51..d52b487ad259 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -172,17 +172,22 @@ static inline unsigned long ima_hash_key(u8 *digest)
  return hash_long(*digest, IMA_HASH_BITS);
 }
 
+#define __ima_hooks(hook) \
+ hook(NONE) \
+ hook(FILE_CHECK) \
+ hook(MMAP_CHECK) \
+ hook(BPRM_CHECK) \
+ hook(POST_SETATTR) \
+ hook(MODULE_CHECK) \
+ hook(FIRMWARE_CHECK) \
+ hook(KEXEC_KERNEL_CHECK) \
+ hook(KEXEC_INITRAMFS_CHECK) \
+ hook(POLICY_CHECK) \
+ hook(MAX_CHECK)
+#define __ima_hook_enumify(ENUM) ENUM,
+
 enum ima_hooks {
- FILE_CHECK = 1,
- MMAP_CHECK,
- BPRM_CHECK,
- POST_SETATTR,
- MODULE_CHECK,
- FIRMWARE_CHECK,
- KEXEC_KERNEL_CHECK,
- KEXEC_INITRAMFS_CHECK,
- POLICY_CHECK,
- MAX_CHECK
+ __ima_hooks(__ima_hook_enumify)
 };
 
 /* LIM API function definitions */
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 949ad3858327..f4436626ccb7 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -972,23 +972,10 @@ static const char *const mask_tokens[] = {
  "MAY_APPEND"
 };
 
-enum {
- func_file = 0, func_mmap, func_bprm,
- func_module, func_firmware, func_post,
- func_kexec_kernel, func_kexec_initramfs,
- func_policy
-};
+#define __ima_hook_stringify(str) (#str),
 
 static const char *const func_tokens[] = {
- "FILE_CHECK",
- "MMAP_CHECK",
- "BPRM_CHECK",
- "MODULE_CHECK",
- "FIRMWARE_CHECK",
- "POST_SETATTR",
- "KEXEC_KERNEL_CHECK",
- "KEXEC_INITRAMFS_CHECK",
- "POLICY_CHECK"
+ __ima_hooks(__ima_hook_stringify)
 };
 
 void *ima_policy_start(struct seq_file *m, loff_t *pos)
@@ -1025,49 +1012,16 @@ void ima_policy_stop(struct seq_file *m, void *v)
 
 #define pt(token) policy_tokens[token + Opt_err].pattern
 #define mt(token) mask_tokens[token]
-#define ft(token) func_tokens[token]
 
 /*
  * policy_func_show - display the ima_hooks policy rule
  */
 static void policy_func_show(struct seq_file *m, enum ima_hooks func)
 {
- char tbuf[64] = {0,};
-
- switch (func) {
- case FILE_CHECK:
- seq_printf(m, pt(Opt_func), ft(func_file));
- break;
- case MMAP_CHECK:
- seq_printf(m, pt(Opt_func), ft(func_mmap));
- break;
- case BPRM_CHECK:
- seq_printf(m, pt(Opt_func), ft(func_bprm));
- break;
- case MODULE_CHECK:
- seq_printf(m, pt(Opt_func), ft(func_module));
- break;
- case FIRMWARE_CHECK:
- seq_printf(m, pt(Opt_func), ft(func_firmware));
- break;
- case POST_SETATTR:
- seq_printf(m, pt(Opt_func), ft(func_post));
- break;
- case KEXEC_KERNEL_CHECK:
- seq_printf(m, pt(Opt_func), ft(func_kexec_kernel));
- break;
- case KEXEC_INITRAMFS_CHECK:
- seq_printf(m, pt(Opt_func), ft(func_kexec_initramfs));
- break;
- case POLICY_CHECK:
- seq_printf(m, pt(Opt_func), ft(func_policy));
- break;
- default:
- snprintf(tbuf, sizeof(tbuf), "%d", func);
- seq_printf(m, pt(Opt_func), tbuf);
- break;
- }
- seq_puts(m, " ");
+ if (func > 0 && func < MAX_CHECK)
+ seq_printf(m, "func=%s ", func_tokens[func]);
+ else
+ seq_printf(m, "func=%d ", func);
 }
 
 int ima_policy_show(struct seq_file *m, void *v)
--
2.7.4

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 3/6] ima: Log the same audit cause whenever a file has no signature

Thiago Jung Bauermann
In reply to this post by Thiago Jung Bauermann
If the file doesn't have an xattr, ima_appraise_measurement sets cause to
"missing-hash" while if there's an xattr but it's a digest instead of a
signature it sets cause to "IMA-signature-required".

Fix it by setting cause to "IMA-signature-required" in both cases.

Signed-off-by: Thiago Jung Bauermann <[hidden email]>
---
 security/integrity/ima/ima_appraise.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index ea36a4f134f4..809ba70fbbbf 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -217,7 +217,8 @@ int ima_appraise_measurement(enum ima_hooks func,
  if (rc && rc != -ENODATA)
  goto out;
 
- cause = "missing-hash";
+ cause = iint->flags & IMA_DIGSIG_REQUIRED ?
+ "IMA-signature-required" : "missing-hash";
  status = INTEGRITY_NOLABEL;
  if (opened & FILE_CREATED)
  iint->flags |= IMA_NEW_FILE;
--
2.7.4

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 4/6] integrity: Introduce struct evm_hmac_xattr

Thiago Jung Bauermann
In reply to this post by Thiago Jung Bauermann
Even though struct evm_ima_xattr_data includes a fixed-size array to hold a
SHA1 digest, most of the code ignores the array and uses the struct to mean
"type indicator followed by data of unspecified size" and tracks the real
size of what the struct represents in a separate length variable.

The only exception to that is the EVM code, which correctly uses the
definition of struct evm_ima_xattr_data.

This patch makes this explicit in the code by removing the length
specification from the array in struct evm_ima_xattr_data. It also changes
the name of the element from digest to data, since in most places the array
doesn't hold a digest.

A separate struct evm_hmac_xattr is introduced, with the original
definition of evm_ima_xattr_data to be used in the places that actually
expect that definition.

Signed-off-by: Thiago Jung Bauermann <[hidden email]>
---
 security/integrity/evm/evm.h          | 5 +++++
 security/integrity/evm/evm_crypto.c   | 2 +-
 security/integrity/evm/evm_main.c     | 8 ++++----
 security/integrity/ima/ima_appraise.c | 7 ++++---
 security/integrity/integrity.h        | 2 +-
 5 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
index f5f12727771a..e1081cf2f9c5 100644
--- a/security/integrity/evm/evm.h
+++ b/security/integrity/evm/evm.h
@@ -24,6 +24,11 @@
 #define EVM_INIT_HMAC 0x0001
 #define EVM_INIT_X509 0x0002
 
+struct evm_hmac_xattr {
+ u8 type; /* Should be EVM_XATTR_HMAC. */
+ u8 digest[SHA1_DIGEST_SIZE];
+} __packed;
+
 extern int evm_initialized;
 extern char *evm_hmac;
 extern char *evm_hash;
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index d7f282d75cc1..08dde59f3128 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -252,7 +252,7 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
  const char *xattr_value, size_t xattr_value_len)
 {
  struct inode *inode = d_backing_inode(dentry);
- struct evm_ima_xattr_data xattr_data;
+ struct evm_hmac_xattr xattr_data;
  int rc = 0;
 
  rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 063d38aef64e..b7c1e11a915e 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -116,7 +116,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
      struct integrity_iint_cache *iint)
 {
  struct evm_ima_xattr_data *xattr_data = NULL;
- struct evm_ima_xattr_data calc;
+ struct evm_hmac_xattr calc;
  enum integrity_status evm_status = INTEGRITY_PASS;
  int rc, xattr_len;
 
@@ -147,7 +147,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
  /* check value type */
  switch (xattr_data->type) {
  case EVM_XATTR_HMAC:
- if (xattr_len != sizeof(struct evm_ima_xattr_data)) {
+ if (xattr_len != sizeof(struct evm_hmac_xattr)) {
  evm_status = INTEGRITY_FAIL;
  goto out;
  }
@@ -155,7 +155,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
    xattr_value_len, calc.digest);
  if (rc)
  break;
- rc = crypto_memneq(xattr_data->digest, calc.digest,
+ rc = crypto_memneq(xattr_data->data, calc.digest,
     sizeof(calc.digest));
  if (rc)
  rc = -EINVAL;
@@ -467,7 +467,7 @@ int evm_inode_init_security(struct inode *inode,
  const struct xattr *lsm_xattr,
  struct xattr *evm_xattr)
 {
- struct evm_ima_xattr_data *xattr_data;
+ struct evm_hmac_xattr *xattr_data;
  int rc;
 
  if (!evm_initialized || !evm_protected_xattr(lsm_xattr->name))
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 809ba70fbbbf..87d2b601cf8e 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -156,7 +156,8 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
  return sig->hash_algo;
  break;
  case IMA_XATTR_DIGEST_NG:
- ret = xattr_value->digest[0];
+ /* first byte contains algorithm id */
+ ret = xattr_value->data[0];
  if (ret < HASH_ALGO__LAST)
  return ret;
  break;
@@ -164,7 +165,7 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
  /* this is for backward compatibility */
  if (xattr_len == 21) {
  unsigned int zero = 0;
- if (!memcmp(&xattr_value->digest[16], &zero, 4))
+ if (!memcmp(&xattr_value->data[16], &zero, 4))
  return HASH_ALGO_MD5;
  else
  return HASH_ALGO_SHA1;
@@ -253,7 +254,7 @@ int ima_appraise_measurement(enum ima_hooks func,
  /* xattr length may be longer. md5 hash in previous
    version occupied 20 bytes in xattr, instead of 16
  */
- rc = memcmp(&xattr_value->digest[hash_start],
+ rc = memcmp(&xattr_value->data[hash_start],
     iint->ima_hash->digest,
     iint->ima_hash->length);
  else
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index a53e7e4ab06c..874211aba6e9 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -63,7 +63,7 @@ enum evm_ima_xattr_type {
 
 struct evm_ima_xattr_data {
  u8 type;
- u8 digest[SHA1_DIGEST_SIZE];
+ u8 data[];
 } __packed;
 
 #define IMA_MAX_DIGEST_SIZE 64
--
2.7.4

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 5/6] MODSIGN: Export module signature definitions.

Thiago Jung Bauermann
In reply to this post by Thiago Jung Bauermann
IMA will use the module_signature format for append signatures, so export
the relevant definitions and factor out the code which verifies that the
appended signature trailer is valid.

Also, add function verify_pkcs7_message_signature which takes a struct
pkcs7_message for verification isntead of the raw bytes that
verify_pkcs7_signature takes.

Finally, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it
and be able to use validate_module_signature without having to depend on
CONFIG_MODULE_SIG.

Signed-off-by: Thiago Jung Bauermann <[hidden email]>
---
 certs/system_keyring.c           | 62 ++++++++++++++++++++++++---------
 include/linux/module.h           |  3 --
 include/linux/module_signature.h | 48 ++++++++++++++++++++++++++
 include/linux/verification.h     | 10 ++++++
 init/Kconfig                     |  6 +++-
 kernel/Makefile                  |  2 +-
 kernel/module.c                  |  1 +
 kernel/module_signing.c          | 74 +++++++++++++++++-----------------------
 8 files changed, 142 insertions(+), 64 deletions(-)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 6251d1b27f0c..0597f87a2375 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -190,33 +190,28 @@ late_initcall(load_system_certificate_list);
 #ifdef CONFIG_SYSTEM_DATA_VERIFICATION
 
 /**
- * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
+ * verify_pkcs7_message_signature - Verify PKCS#7-based signature on system data
  * @data: The data to be verified (NULL if expecting internal data).
  * @len: Size of @data.
- * @raw_pkcs7: The PKCS#7 message that is the signature.
- * @pkcs7_len: The size of @raw_pkcs7.
+ * @pkcs7: The PKCS#7 message that is the signature.
  * @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
  * (void *)1UL for all trusted keys).
  * @usage: The use to which the key is being put.
  * @view_content: Callback to gain access to content.
  * @ctx: Context for callback.
  */
-int verify_pkcs7_signature(const void *data, size_t len,
-   const void *raw_pkcs7, size_t pkcs7_len,
-   struct key *trusted_keys,
-   enum key_being_used_for usage,
-   int (*view_content)(void *ctx,
-       const void *data, size_t len,
-       size_t asn1hdrlen),
-   void *ctx)
+int verify_pkcs7_message_signature(const void *data, size_t len,
+   struct pkcs7_message *pkcs7,
+   struct key *trusted_keys,
+   enum key_being_used_for usage,
+   int (*view_content)(void *ctx,
+       const void *data,
+       size_t len,
+       size_t asn1hdrlen),
+   void *ctx)
 {
- struct pkcs7_message *pkcs7;
  int ret;
 
- pkcs7 = pkcs7_parse_message(raw_pkcs7, pkcs7_len);
- if (IS_ERR(pkcs7))
- return PTR_ERR(pkcs7);
-
  /* The data should be detached - so we need to supply it. */
  if (data && pkcs7_supply_detached_data(pkcs7, data, len) < 0) {
  pr_err("PKCS#7 signature with non-detached data\n");
@@ -258,6 +253,41 @@ int verify_pkcs7_signature(const void *data, size_t len,
  }
 
 error:
+ pr_devel("<==%s() = %d\n", __func__, ret);
+ return ret;
+}
+
+/**
+ * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
+ * @data: The data to be verified (NULL if expecting internal data).
+ * @len: Size of @data.
+ * @raw_pkcs7: The PKCS#7 message that is the signature.
+ * @pkcs7_len: The size of @raw_pkcs7.
+ * @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
+ * (void *)1UL for all trusted keys).
+ * @usage: The use to which the key is being put.
+ * @view_content: Callback to gain access to content.
+ * @ctx: Context for callback.
+ */
+int verify_pkcs7_signature(const void *data, size_t len,
+   const void *raw_pkcs7, size_t pkcs7_len,
+   struct key *trusted_keys,
+   enum key_being_used_for usage,
+   int (*view_content)(void *ctx,
+       const void *data, size_t len,
+       size_t asn1hdrlen),
+   void *ctx)
+{
+ struct pkcs7_message *pkcs7;
+ int ret;
+
+ pkcs7 = pkcs7_parse_message(raw_pkcs7, pkcs7_len);
+ if (IS_ERR(pkcs7))
+ return PTR_ERR(pkcs7);
+
+ ret = verify_pkcs7_message_signature(data, len, pkcs7, trusted_keys,
+     usage, view_content, ctx);
+
  pkcs7_free_message(pkcs7);
  pr_devel("<==%s() = %d\n", __func__, ret);
  return ret;
diff --git a/include/linux/module.h b/include/linux/module.h
index 21f56393602f..76a233f120ba 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -23,9 +23,6 @@
 #include <linux/percpu.h>
 #include <asm/module.h>
 
-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
-#define MODULE_SIG_STRING "~Module signature appended~\n"
-
 /* Not Yet Implemented */
 #define MODULE_SUPPORTED_DEVICE(name)
 
diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
new file mode 100644
index 000000000000..4e49fa059931
--- /dev/null
+++ b/include/linux/module_signature.h
@@ -0,0 +1,48 @@
+/* Module signature handling.
+ *
+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells ([hidden email])
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+
+#ifndef _LINUX_MODULE_SIGNATURE_H
+#define _LINUX_MODULE_SIGNATURE_H
+
+/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
+#define MODULE_SIG_STRING "~Module signature appended~\n"
+
+enum pkey_id_type {
+ PKEY_ID_PGP, /* OpenPGP generated key ID */
+ PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */
+ PKEY_ID_PKCS7, /* Signature in PKCS#7 message */
+};
+
+/*
+ * Module signature information block.
+ *
+ * The constituents of the signature section are, in order:
+ *
+ * - Signer's name
+ * - Key identifier
+ * - Signature data
+ * - Information block
+ */
+struct module_signature {
+ u8 algo; /* Public-key crypto algorithm [0] */
+ u8 hash; /* Digest algorithm [0] */
+ u8 id_type; /* Key identifier type [PKEY_ID_PKCS7] */
+ u8 signer_len; /* Length of signer's name [0] */
+ u8 key_id_len; /* Length of key identifier [0] */
+ u8 __pad[3];
+ __be32 sig_len; /* Length of signature data */
+};
+
+int validate_module_signature(const struct module_signature *ms,
+      size_t file_len);
+int mod_verify_sig(const void *mod, unsigned long *_modlen);
+
+#endif /* _LINUX_MODULE_SIGNATURE_H */
diff --git a/include/linux/verification.h b/include/linux/verification.h
index a10549a6c7cd..c7cb8db06ced 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -29,6 +29,7 @@ extern const char *const key_being_used_for[NR__KEY_BEING_USED_FOR];
 #ifdef CONFIG_SYSTEM_DATA_VERIFICATION
 
 struct key;
+struct pkcs7_message;
 
 extern int verify_pkcs7_signature(const void *data, size_t len,
   const void *raw_pkcs7, size_t pkcs7_len,
@@ -38,6 +39,15 @@ extern int verify_pkcs7_signature(const void *data, size_t len,
       const void *data, size_t len,
       size_t asn1hdrlen),
   void *ctx);
+extern int verify_pkcs7_message_signature(const void *data, size_t len,
+  struct pkcs7_message *pkcs7,
+  struct key *trusted_keys,
+  enum key_being_used_for usage,
+  int (*view_content)(void *ctx,
+      const void *data,
+      size_t len,
+      size_t asn1hdrlen),
+  void *ctx);
 
 #ifdef CONFIG_SIGNED_PE_FILE_VERIFICATION
 extern int verify_pefile_signature(const void *pebuf, unsigned pelen,
diff --git a/init/Kconfig b/init/Kconfig
index 1d3475fc9496..5f20513098ff 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -2063,7 +2063,7 @@ config MODULE_SRCVERSION_ALL
 config MODULE_SIG
  bool "Module signature verification"
  depends on MODULES
- select SYSTEM_DATA_VERIFICATION
+ select MODULE_SIG_FORMAT
  help
   Check modules for valid signatures upon load: the signature
   is simply appended to the module. For more information see
@@ -2078,6 +2078,10 @@ config MODULE_SIG
   debuginfo strip done by some packagers (such as rpmbuild) and
   inclusion into an initramfs that wants the module size reduced.
 
+config MODULE_SIG_FORMAT
+ def_bool n
+ select SYSTEM_DATA_VERIFICATION
+
 config MODULE_SIG_FORCE
  bool "Require modules to be validly signed"
  depends on MODULE_SIG
diff --git a/kernel/Makefile b/kernel/Makefile
index 72aa080f91f0..dcb548b68084 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -56,7 +56,7 @@ obj-y += up.o
 endif
 obj-$(CONFIG_UID16) += uid16.o
 obj-$(CONFIG_MODULES) += module.o
-obj-$(CONFIG_MODULE_SIG) += module_signing.o
+obj-$(CONFIG_MODULE_SIG_FORMAT) += module_signing.o
 obj-$(CONFIG_KALLSYMS) += kallsyms.o
 obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
 obj-$(CONFIG_CRASH_CORE) += crash_core.o
diff --git a/kernel/module.c b/kernel/module.c
index 4a3665f8f837..0f973856d6d2 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -19,6 +19,7 @@
 #include <linux/export.h>
 #include <linux/extable.h>
 #include <linux/moduleloader.h>
+#include <linux/module_signature.h>
 #include <linux/trace_events.h>
 #include <linux/init.h>
 #include <linux/kallsyms.h>
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index 937c844bee4a..421e3e668714 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -11,36 +11,38 @@
 
 #include <linux/kernel.h>
 #include <linux/errno.h>
+#include <linux/module_signature.h>
 #include <linux/string.h>
 #include <linux/verification.h>
 #include <crypto/public_key.h>
 #include "module-internal.h"
 
-enum pkey_id_type {
- PKEY_ID_PGP, /* OpenPGP generated key ID */
- PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */
- PKEY_ID_PKCS7, /* Signature in PKCS#7 message */
-};
-
-/*
- * Module signature information block.
- *
- * The constituents of the signature section are, in order:
+/**
+ * validate_module_signature - validate that the given signature is sane
  *
- * - Signer's name
- * - Key identifier
- * - Signature data
- * - Information block
+ * @ms: Signature to validate.
+ * @file_len: Size of the file to which @ms is appended.
  */
-struct module_signature {
- u8 algo; /* Public-key crypto algorithm [0] */
- u8 hash; /* Digest algorithm [0] */
- u8 id_type; /* Key identifier type [PKEY_ID_PKCS7] */
- u8 signer_len; /* Length of signer's name [0] */
- u8 key_id_len; /* Length of key identifier [0] */
- u8 __pad[3];
- __be32 sig_len; /* Length of signature data */
-};
+int validate_module_signature(const struct module_signature *ms, size_t file_len)
+{
+ if (be32_to_cpu(ms->sig_len) >= file_len - sizeof(*ms))
+ return -EBADMSG;
+ else if (ms->id_type != PKEY_ID_PKCS7) {
+ pr_err("Module is not signed with expected PKCS#7 message\n");
+ return -ENOPKG;
+ } else if (ms->algo != 0 ||
+   ms->hash != 0 ||
+   ms->signer_len != 0 ||
+   ms->key_id_len != 0 ||
+   ms->__pad[0] != 0 ||
+   ms->__pad[1] != 0 ||
+   ms->__pad[2] != 0) {
+ pr_err("PKCS#7 signature info has unexpected non-zero params\n");
+ return -EBADMSG;
+ }
+
+ return 0;
+}
 
 /*
  * Verify the signature on a module.
@@ -49,6 +51,7 @@ int mod_verify_sig(const void *mod, unsigned long *_modlen)
 {
  struct module_signature ms;
  size_t modlen = *_modlen, sig_len;
+ int ret;
 
  pr_devel("==>%s(,%zu)\n", __func__, modlen);
 
@@ -56,30 +59,15 @@ int mod_verify_sig(const void *mod, unsigned long *_modlen)
  return -EBADMSG;
 
  memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
- modlen -= sizeof(ms);
+
+ ret = validate_module_signature(&ms, modlen);
+ if (ret)
+ return ret;
 
  sig_len = be32_to_cpu(ms.sig_len);
- if (sig_len >= modlen)
- return -EBADMSG;
- modlen -= sig_len;
+ modlen -= sig_len + sizeof(ms);
  *_modlen = modlen;
 
- if (ms.id_type != PKEY_ID_PKCS7) {
- pr_err("Module is not signed with expected PKCS#7 message\n");
- return -ENOPKG;
- }
-
- if (ms.algo != 0 ||
-    ms.hash != 0 ||
-    ms.signer_len != 0 ||
-    ms.key_id_len != 0 ||
-    ms.__pad[0] != 0 ||
-    ms.__pad[1] != 0 ||
-    ms.__pad[2] != 0) {
- pr_err("PKCS#7 signature info has unexpected non-zero params\n");
- return -EBADMSG;
- }
-
  return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
       NULL, VERIFYING_MODULE_SIGNATURE,
       NULL, NULL);
--
2.7.4

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2 6/6] ima: Support module-style appended signatures for appraisal

Thiago Jung Bauermann
In reply to this post by Thiago Jung Bauermann
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 <[hidden email]>
---
 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);
 
  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 <[hidden email]>
+ *
+ * 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);
 
--
2.7.4

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 0/6] Appended signatures support for IMA appraisal

Michael Ellerman-2
In reply to this post by Thiago Jung Bauermann
Thiago Jung Bauermann <[hidden email]> writes:

> On the OpenPOWER platform, secure boot and trusted boot are being
> implemented using IMA for taking measurements and verifying signatures.

I still want you to implement arch_kexec_kernel_verify_sig() as well :)

cheers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 0/6] Appended signatures support for IMA appraisal

Thiago Jung Bauermann

Michael Ellerman <[hidden email]> writes:

> Thiago Jung Bauermann <[hidden email]> writes:
>
>> On the OpenPOWER platform, secure boot and trusted boot are being
>> implemented using IMA for taking measurements and verifying signatures.
>
> I still want you to implement arch_kexec_kernel_verify_sig() as well :)

Yes, I will implement it! We are still working on loading the public
keys for kernel signing from the firmware into a kernel keyring, so
there's not much point in implementing arch_kexec_kernel_verify_sig
without having that first.

The same problem also affects IMA: even with these patches, new code
still neededs to be added to make IMA use the platform keys for kernel
signature verification.

--
Thiago Jung Bauermann
IBM Linux Technology Center

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 0/6] Appended signatures support for IMA appraisal

Michael Ellerman-2
Thiago Jung Bauermann <[hidden email]> writes:

> Michael Ellerman <[hidden email]> writes:
>
>> Thiago Jung Bauermann <[hidden email]> writes:
>>
>>> On the OpenPOWER platform, secure boot and trusted boot are being
>>> implemented using IMA for taking measurements and verifying signatures.
>>
>> I still want you to implement arch_kexec_kernel_verify_sig() as well :)
>
> Yes, I will implement it! We are still working on loading the public
> keys for kernel signing from the firmware into a kernel keyring, so
> there's not much point in implementing arch_kexec_kernel_verify_sig
> without having that first.

OK. What's the ETA on those patches?

cheers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 6/6] ima: Support module-style appended signatures for appraisal

Mimi Zohar
In reply to this post by Thiago Jung Bauermann
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 <[hidden email]>

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 <[hidden email]>
> + *
> + * 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 | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 1/6] integrity: Small code improvements

Mimi Zohar
In reply to this post by Thiago Jung Bauermann
On Wed, 2017-06-07 at 22:49 -0300, Thiago Jung Bauermann wrote:

> These changes are too small to warrant their own patches:
>
> The keyid and sig_size members of struct signature_v2_hdr are in BE format,
> so use a type that makes this assumption explicit. Also, use beXX_to_cpu
> instead of __beXX_to_cpu to read them.
>
> Change integrity_kernel_read to take a void * buffer instead of char *
> buffer, so that callers don't have to use a cast if they provide a buffer
> that isn't a char *.
>
> Add missing #endif comment in ima.h pointing out which macro it refers to.
>
> Add missing fall through comment in ima_appraise.c.
>
> Constify mask_tokens and func_tokens arrays.
>
> Signed-off-by: Thiago Jung Bauermann <[hidden email]>

Thank you.  Queued to be upstreamed.

Mimi


> ---
>  security/integrity/digsig_asymmetric.c | 4 ++--
>  security/integrity/iint.c              | 2 +-
>  security/integrity/ima/ima.h           | 2 +-
>  security/integrity/ima/ima_appraise.c  | 1 +
>  security/integrity/ima/ima_policy.c    | 4 ++--
>  security/integrity/integrity.h         | 7 ++++---
>  6 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c
> index 80052ed8d467..ab6a029062a1 100644
> --- a/security/integrity/digsig_asymmetric.c
> +++ b/security/integrity/digsig_asymmetric.c
> @@ -92,13 +92,13 @@ int asymmetric_verify(struct key *keyring, const char *sig,
>
>   siglen -= sizeof(*hdr);
>
> - if (siglen != __be16_to_cpu(hdr->sig_size))
> + if (siglen != be16_to_cpu(hdr->sig_size))
>   return -EBADMSG;
>
>   if (hdr->hash_algo >= HASH_ALGO__LAST)
>   return -ENOPKG;
>
> - key = request_asymmetric_key(keyring, __be32_to_cpu(hdr->keyid));
> + key = request_asymmetric_key(keyring, be32_to_cpu(hdr->keyid));
>   if (IS_ERR(key))
>   return PTR_ERR(key);
>
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index c710d22042f9..6fc888ca468e 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -182,7 +182,7 @@ security_initcall(integrity_iintcache_init);
>   *
>   */
>  int integrity_kernel_read(struct file *file, loff_t offset,
> -  char *addr, unsigned long count)
> +  void *addr, unsigned long count)
>  {
>   mm_segment_t old_fs;
>   char __user *buf = (char __user *)addr;
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index d26a30e37d13..215a93c41b51 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -284,7 +284,7 @@ static inline int ima_read_xattr(struct dentry *dentry,
>   return 0;
>  }
>
> -#endif
> +#endif /* CONFIG_IMA_APPRAISE */
>
>  /* LSM based policy rules require audit */
>  #ifdef CONFIG_IMA_LSM_RULES
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 7fe0566142d8..ea36a4f134f4 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -240,6 +240,7 @@ int ima_appraise_measurement(enum ima_hooks func,
>   case IMA_XATTR_DIGEST_NG:
>   /* first byte contains algorithm id */
>   hash_start = 1;
> + /* fall through */
>   case IMA_XATTR_DIGEST:
>   if (iint->flags & IMA_DIGSIG_REQUIRED) {
>   cause = "IMA-signature-required";
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 0acd68decb17..949ad3858327 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -965,7 +965,7 @@ enum {
>   mask_exec = 0, mask_write, mask_read, mask_append
>  };
>
> -static char *mask_tokens[] = {
> +static const char *const mask_tokens[] = {
>   "MAY_EXEC",
>   "MAY_WRITE",
>   "MAY_READ",
> @@ -979,7 +979,7 @@ enum {
>   func_policy
>  };
>
> -static char *func_tokens[] = {
> +static const char *const func_tokens[] = {
>   "FILE_CHECK",
>   "MMAP_CHECK",
>   "BPRM_CHECK",
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 24520b4ef3b0..a53e7e4ab06c 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -92,8 +92,8 @@ struct signature_v2_hdr {
>   uint8_t type; /* xattr type */
>   uint8_t version; /* signature format version */
>   uint8_t hash_algo; /* Digest algorithm [enum hash_algo] */
> - uint32_t keyid; /* IMA key identifier - not X509/PGP specific */
> - uint16_t sig_size; /* signature size */
> + __be32 keyid; /* IMA key identifier - not X509/PGP specific */
> + __be16 sig_size; /* signature size */
>   uint8_t sig[0]; /* signature payload */
>  } __packed;
>
> @@ -118,7 +118,8 @@ struct integrity_iint_cache {
>  struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
>
>  int integrity_kernel_read(struct file *file, loff_t offset,
> -  char *addr, unsigned long count);
> +  void *addr, unsigned long count);
> +
>  int __init integrity_read_file(const char *path, char **data);
>
>  #define INTEGRITY_KEYRING_EVM 0

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 2/6] ima: Simplify policy_func_show.

Mimi Zohar
In reply to this post by Thiago Jung Bauermann
On Wed, 2017-06-07 at 22:49 -0300, Thiago Jung Bauermann wrote:

> If the func_tokens array uses the same indices as enum ima_hooks,
> policy_func_show can be a lot simpler, and the func_* enum becomes
> unnecessary.
>
> Also, if we use the same macro trick used by kernel_read_file_id_str we can
> use one hooks list for both the enum and the string array, making sure they
> are always in sync (suggested by Mimi Zohar).
>
> Finally, by using the printf pattern for the function token directly
> instead of using the pt macro we can simplify policy_func_show even further
> and avoid needing a temporary buffer.
>
> Signed-off-by: Thiago Jung Bauermann <[hidden email]>

Thank you.  Queued to be upstreamed.

Mimi

> ---
>  security/integrity/ima/ima.h        | 25 +++++++++-------
>  security/integrity/ima/ima_policy.c | 58 ++++---------------------------------
>  2 files changed, 21 insertions(+), 62 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 215a93c41b51..d52b487ad259 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -172,17 +172,22 @@ static inline unsigned long ima_hash_key(u8 *digest)
>   return hash_long(*digest, IMA_HASH_BITS);
>  }
>
> +#define __ima_hooks(hook) \
> + hook(NONE) \
> + hook(FILE_CHECK) \
> + hook(MMAP_CHECK) \
> + hook(BPRM_CHECK) \
> + hook(POST_SETATTR) \
> + hook(MODULE_CHECK) \
> + hook(FIRMWARE_CHECK) \
> + hook(KEXEC_KERNEL_CHECK) \
> + hook(KEXEC_INITRAMFS_CHECK) \
> + hook(POLICY_CHECK) \
> + hook(MAX_CHECK)
> +#define __ima_hook_enumify(ENUM) ENUM,
> +
>  enum ima_hooks {
> - FILE_CHECK = 1,
> - MMAP_CHECK,
> - BPRM_CHECK,
> - POST_SETATTR,
> - MODULE_CHECK,
> - FIRMWARE_CHECK,
> - KEXEC_KERNEL_CHECK,
> - KEXEC_INITRAMFS_CHECK,
> - POLICY_CHECK,
> - MAX_CHECK
> + __ima_hooks(__ima_hook_enumify)
>  };
>
>  /* LIM API function definitions */
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 949ad3858327..f4436626ccb7 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -972,23 +972,10 @@ static const char *const mask_tokens[] = {
>   "MAY_APPEND"
>  };
>
> -enum {
> - func_file = 0, func_mmap, func_bprm,
> - func_module, func_firmware, func_post,
> - func_kexec_kernel, func_kexec_initramfs,
> - func_policy
> -};
> +#define __ima_hook_stringify(str) (#str),
>
>  static const char *const func_tokens[] = {
> - "FILE_CHECK",
> - "MMAP_CHECK",
> - "BPRM_CHECK",
> - "MODULE_CHECK",
> - "FIRMWARE_CHECK",
> - "POST_SETATTR",
> - "KEXEC_KERNEL_CHECK",
> - "KEXEC_INITRAMFS_CHECK",
> - "POLICY_CHECK"
> + __ima_hooks(__ima_hook_stringify)
>  };
>
>  void *ima_policy_start(struct seq_file *m, loff_t *pos)
> @@ -1025,49 +1012,16 @@ void ima_policy_stop(struct seq_file *m, void *v)
>
>  #define pt(token) policy_tokens[token + Opt_err].pattern
>  #define mt(token) mask_tokens[token]
> -#define ft(token) func_tokens[token]
>
>  /*
>   * policy_func_show - display the ima_hooks policy rule
>   */
>  static void policy_func_show(struct seq_file *m, enum ima_hooks func)
>  {
> - char tbuf[64] = {0,};
> -
> - switch (func) {
> - case FILE_CHECK:
> - seq_printf(m, pt(Opt_func), ft(func_file));
> - break;
> - case MMAP_CHECK:
> - seq_printf(m, pt(Opt_func), ft(func_mmap));
> - break;
> - case BPRM_CHECK:
> - seq_printf(m, pt(Opt_func), ft(func_bprm));
> - break;
> - case MODULE_CHECK:
> - seq_printf(m, pt(Opt_func), ft(func_module));
> - break;
> - case FIRMWARE_CHECK:
> - seq_printf(m, pt(Opt_func), ft(func_firmware));
> - break;
> - case POST_SETATTR:
> - seq_printf(m, pt(Opt_func), ft(func_post));
> - break;
> - case KEXEC_KERNEL_CHECK:
> - seq_printf(m, pt(Opt_func), ft(func_kexec_kernel));
> - break;
> - case KEXEC_INITRAMFS_CHECK:
> - seq_printf(m, pt(Opt_func), ft(func_kexec_initramfs));
> - break;
> - case POLICY_CHECK:
> - seq_printf(m, pt(Opt_func), ft(func_policy));
> - break;
> - default:
> - snprintf(tbuf, sizeof(tbuf), "%d", func);
> - seq_printf(m, pt(Opt_func), tbuf);
> - break;
> - }
> - seq_puts(m, " ");
> + if (func > 0 && func < MAX_CHECK)
> + seq_printf(m, "func=%s ", func_tokens[func]);
> + else
> + seq_printf(m, "func=%d ", func);
>  }
>
>  int ima_policy_show(struct seq_file *m, void *v)

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 3/6] ima: Log the same audit cause whenever a file has no signature

Mimi Zohar
In reply to this post by Thiago Jung Bauermann
On Wed, 2017-06-07 at 22:49 -0300, Thiago Jung Bauermann wrote:
> If the file doesn't have an xattr, ima_appraise_measurement sets cause to
> "missing-hash" while if there's an xattr but it's a digest instead of a
> signature it sets cause to "IMA-signature-required".
>
> Fix it by setting cause to "IMA-signature-required" in both cases.
>
> Signed-off-by: Thiago Jung Bauermann <[hidden email]>

Thank you.  Queued to be upstreamed.

Mimi

> ---
>  security/integrity/ima/ima_appraise.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index ea36a4f134f4..809ba70fbbbf 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -217,7 +217,8 @@ int ima_appraise_measurement(enum ima_hooks func,
>   if (rc && rc != -ENODATA)
>   goto out;
>
> - cause = "missing-hash";
> + cause = iint->flags & IMA_DIGSIG_REQUIRED ?
> + "IMA-signature-required" : "missing-hash";
>   status = INTEGRITY_NOLABEL;
>   if (opened & FILE_CREATED)
>   iint->flags |= IMA_NEW_FILE;

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 6/6] ima: Support module-style appended signatures for appraisal

Thiago Jung Bauermann
In reply to this post by Mimi Zohar

Hello Mimi,

Thanks for your review, and for queuing the other patches in this series.

Mimi Zohar <[hidden email]> 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.

--
Thiago Jung Bauermann
IBM Linux Technology Center

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2 6/6] ima: Support module-style appended signatures for appraisal

Mimi Zohar
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 <[hidden email]> 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.
>

Loading...