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

[PATCH 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 can 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.

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

Thiago Jung Bauermann (6):
  integrity: Small code improvements
  ima: Tidy up constant strings
  ima: Simplify policy_func_show.
  ima: Log the same audit cause whenever a file has no signature
  MODSIGN: Export module signature definitions.
  ima: Support appended signatures for appraisal

 crypto/asymmetric_keys/asymmetric_type.c |  1 +
 crypto/asymmetric_keys/pkcs7_parser.c    | 12 +++++
 crypto/asymmetric_keys/pkcs7_verify.c    | 13 +++++
 include/crypto/pkcs7.h                   |  3 ++
 include/linux/module_signature.h         | 45 ++++++++++++++++
 include/linux/verification.h             |  1 +
 init/Kconfig                             |  6 ++-
 kernel/Makefile                          |  2 +-
 kernel/module_signing.c                  | 74 +++++++++++----------------
 security/integrity/Kconfig               |  2 +-
 security/integrity/digsig_asymmetric.c   |  4 +-
 security/integrity/iint.c                |  2 +-
 security/integrity/ima/Kconfig           | 13 +++++
 security/integrity/ima/ima.h             |  8 +++
 security/integrity/ima/ima_appraise.c    | 86 ++++++++++++++++++++++++++++++-
 security/integrity/ima/ima_init.c        |  2 +-
 security/integrity/ima/ima_main.c        | 30 +++++++++--
 security/integrity/ima/ima_policy.c      | 88 ++++++++++++--------------------
 security/integrity/integrity.h           | 27 ++++++----
 19 files changed, 302 insertions(+), 117 deletions(-)
 create mode 100644 include/linux/module_signature.h

-- 
2.7.4

[PATCH 1/6] integrity: Small code improvements

[Thiago Jung Bauermann]

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 *.

Also, add missing fall through comment in ima_appraise.c.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/digsig_asymmetric.c | 4 ++--
 security/integrity/iint.c              | 2 +-
 security/integrity/ima/ima_appraise.c  | 1 +
 security/integrity/integrity.h         | 7 ++++---
 4 files changed, 8 insertions(+), 6 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_appraise.c b/security/integrity/ima/ima_appraise.c
index 1fd9539a969d..427a896bc806 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -227,6 +227,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/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

[PATCH 2/6] ima: Tidy up constant strings

[Thiago Jung Bauermann]

Strictly speaking, boot_aggregate_name is a constant string, not a
modifiable pointer to a constant string.

Also, constify mask_tokens and func_tokens arrays.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_init.c   | 2 +-
 security/integrity/ima/ima_policy.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 2967d497a665..24c703366ba8 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -25,7 +25,7 @@
 #include "ima.h"
 
 /* name for boot aggregate entry */
-static const char *boot_aggregate_name = "boot_aggregate";
+static const char boot_aggregate_name[] = "boot_aggregate";
 int ima_used_chip;
 
 /* Add the boot aggregate to the IMA measurement list and extend
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index aed47b777a57..cfda5d7b17ec 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -889,7 +889,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",
@@ -903,7 +903,7 @@ enum {
 	func_policy
 };
 
-static char *func_tokens[] = {
+static const char *const func_tokens[] = {
 	"FILE_CHECK",
 	"MMAP_CHECK",
 	"BPRM_CHECK",
-- 
2.7.4

[PATCH 3/6] ima: Simplify policy_func_show.

[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.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_policy.c | 47 ++++++-------------------------------
 1 file changed, 7 insertions(+), 40 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index cfda5d7b17ec..158eafef64e8 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -896,20 +896,14 @@ 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
-};
-
 static const char *const func_tokens[] = {
+	NULL,
 	"FILE_CHECK",
 	"MMAP_CHECK",
 	"BPRM_CHECK",
+	"POST_SETATTR",
 	"MODULE_CHECK",
 	"FIRMWARE_CHECK",
-	"POST_SETATTR",
 	"KEXEC_KERNEL_CHECK",
 	"KEXEC_INITRAMFS_CHECK",
 	"POLICY_CHECK"
@@ -949,48 +943,21 @@ 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,};
+	if (func > 0 && func < MAX_CHECK)
+		seq_printf(m, pt(Opt_func), func_tokens[func]);
+	else {
+		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, " ");
 }
 
-- 
2.7.4

Re: [PATCH 3/6] ima: Simplify policy_func_show.

[Mimi Zohar]

On Tue, 2017-04-18 at 17:17 -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.

My main concern with separating the enumeration from the string
definition is that they might become out of sync.  Perhaps using
macros, similar to those used for kernel_read_file_id_str(), would be
better?

> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> ---
>  security/integrity/ima/ima_policy.c | 47 ++++++-------------------------------
>  1 file changed, 7 insertions(+), 40 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index cfda5d7b17ec..158eafef64e8 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -896,20 +896,14 @@ 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
> -};
> -

At least, add a comment here and near the ima_hooks enumeration to
prevent them from becoming out of sync.

Mimi

>  static const char *const func_tokens[] = {
> +	NULL,
>  	"FILE_CHECK",
>  	"MMAP_CHECK",
>  	"BPRM_CHECK",
> +	"POST_SETATTR",
>  	"MODULE_CHECK",
>  	"FIRMWARE_CHECK",
> -	"POST_SETATTR",
>  	"KEXEC_KERNEL_CHECK",
>  	"KEXEC_INITRAMFS_CHECK",
>  	"POLICY_CHECK"
> @@ -949,48 +943,21 @@ 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,};
> +	if (func > 0 && func < MAX_CHECK)
> +		seq_printf(m, pt(Opt_func), func_tokens[func]);
> +	else {
> +		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, " ");
>  }
> 

Re: [PATCH 3/6] ima: Simplify policy_func_show.

[Thiago Jung Bauermann]

Am Donnerstag, 20. April 2017, 08:13:23 BRT schrieb Mimi Zohar:
> On Tue, 2017-04-18 at 17:17 -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.
> 
> My main concern with separating the enumeration from the string
> definition is that they might become out of sync.  Perhaps using
> macros, similar to those used for kernel_read_file_id_str(), would be
> better?

I agree that it would be better. Is the patch below what you had in mind?

I also noticed that policy_func_show can be even simpler if we stop using the 
printf format string from the policy_tokens table. What do you think?

-- 
Thiago Jung Bauermann
IBM Linux Technology Center


>From 594628c94f5dd7c6d2624944a76b6a01f9668128 Mon Sep 17 00:00:00 2001
From: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
Date: Mon, 10 Apr 2017 14:59:44 -0300
Subject: [PATCH 3/6] ima: Simplify policy_func_show.

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 the need of having a temporary buffer. Since the only use of
Opt_func's printf pattern in policy_tokens was in policy_func_show, we
don't need it at all anymore so remove it.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/ima/ima.h        | 25 +++++++++-------
 security/integrity/ima/ima_policy.c | 60 +++++--------------------------------
 2 files changed, 22 insertions(+), 63 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index b563fbd4d122..51ef805cf7f3 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 cfda5d7b17ec..39d43a5beb5a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -503,7 +503,7 @@ static match_table_t policy_tokens = {
 	{Opt_subj_user, "subj_user=%s"},
 	{Opt_subj_role, "subj_role=%s"},
 	{Opt_subj_type, "subj_type=%s"},
-	{Opt_func, "func=%s"},
+	{Opt_func, NULL},
 	{Opt_mask, "mask=%s"},
 	{Opt_fsmagic, "fsmagic=%s"},
 	{Opt_fsuuid, "fsuuid=%s"},
@@ -896,23 +896,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)
@@ -949,49 +936,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

Re: [PATCH 3/6] ima: Simplify policy_func_show.

[Mimi Zohar]

On Thu, 2017-04-20 at 17:40 -0300, Thiago Jung Bauermann wrote:
> Am Donnerstag, 20. April 2017, 08:13:23 BRT schrieb Mimi Zohar:
> > On Tue, 2017-04-18 at 17:17 -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.
> > 
> > My main concern with separating the enumeration from the string
> > definition is that they might become out of sync.  Perhaps using
> > macros, similar to those used for kernel_read_file_id_str(), would be
> > better?
> 
> I agree that it would be better. Is the patch below what you had in mind?

Yes, I haven't tested it yet, but it looks right.
> 
> I also noticed that policy_func_show can be even simpler if we stop using the 
> printf format string from the policy_tokens table. What do you think?
> 
> -- 
> Thiago Jung Bauermann
> IBM Linux Technology Center
> 
> 
> From 594628c94f5dd7c6d2624944a76b6a01f9668128 Mon Sep 17 00:00:00 2001
> From: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> Date: Mon, 10 Apr 2017 14:59:44 -0300
> Subject: [PATCH 3/6] ima: Simplify policy_func_show.
> 
> 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 the need of having a temporary buffer. Since the only use of
> Opt_func's printf pattern in policy_tokens was in policy_func_show, we
> don't need it at all anymore so remove it.
> 
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> ---
>  security/integrity/ima/ima.h        | 25 +++++++++-------
>  security/integrity/ima/ima_policy.c | 60 +++++--------------------------------
>  2 files changed, 22 insertions(+), 63 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index b563fbd4d122..51ef805cf7f3 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 cfda5d7b17ec..39d43a5beb5a 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -503,7 +503,7 @@ static match_table_t policy_tokens = {
>  	{Opt_subj_user, "subj_user=%s"},
>  	{Opt_subj_role, "subj_role=%s"},
>  	{Opt_subj_type, "subj_type=%s"},
> -	{Opt_func, "func=%s"},
> +	{Opt_func, NULL},
>  	{Opt_mask, "mask=%s"},
>  	{Opt_fsmagic, "fsmagic=%s"},
>  	{Opt_fsuuid, "fsuuid=%s"},
> @@ -896,23 +896,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)
> @@ -949,49 +936,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);

The only time this can happen is when __kernel_read_file_id() is
updated without updating the read_idmap[].  Perhaps we can display the
number and the appropriate __kernel_read_file_id string.

Mimi

>  }
> 
>  int ima_policy_show(struct seq_file *m, void *v)

Re: [PATCH 3/6] ima: Simplify policy_func_show.

[Thiago Jung Bauermann]

Am Freitag, 21. April 2017, 09:57:56 BRT schrieb Mimi Zohar:
> On Thu, 2017-04-20 at 17:40 -0300, Thiago Jung Bauermann wrote:
> > @@ -949,49 +936,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);
> 
> The only time this can happen is when __kernel_read_file_id() is
> updated without updating the read_idmap[].  Perhaps we can display the
> number and the appropriate __kernel_read_file_id string.

>From what I understood of the code func comes from ima_parse_rule, so that 
condition would only happen if ima_parse_rule got out of sync with 
func_tokens. Since that code only initializes func with constants from enum 
ima_hooks and this patch makes ima_hooks automatically sync with func_tokens, 
the else branch is more like a "can't happen" safety net.

read_idmap is only used in ima_post_read_file, and I couldn't see a relation 
between that code path and the one for ima_policy_show.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

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

[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 <bauerman@linux.vnet.ibm.com>
---
 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 427a896bc806..8c8030a29117 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -205,7 +205,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

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

[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, 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 <bauerman@linux.vnet.ibm.com>
---
 include/linux/module_signature.h | 45 ++++++++++++++++++++++++
 init/Kconfig                     |  6 +++-
 kernel/Makefile                  |  2 +-
 kernel/module_signing.c          | 74 +++++++++++++++++-----------------------
 4 files changed, 82 insertions(+), 45 deletions(-)

diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
new file mode 100644
index 000000000000..b04f16559b47
--- /dev/null
+++ b/include/linux/module_signature.h
@@ -0,0 +1,45 @@
+/* Module signature handling.
+ *
+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ *
+ * 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
+
+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/init/Kconfig b/init/Kconfig
index a92f27da4a27..891325e5aeff 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -2024,7 +2024,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
@@ -2039,6 +2039,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 b302b4731d16..4451bbc70a08 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_KEXEC_CORE) += kexec_core.o
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

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

[Mimi Zohar]

On Tue, 2017-04-18 at 17:17 -0300, Thiago Jung Bauermann wrote:
> 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, 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.

Basically we want to generalize the concept of an appended signature.
 Referring to it as a "module signature format" seems a bit confusing.

David, would you have a problem with changing the appended string from
"~Module signature appended~\n" to something more generic?  The
appended signature format could be used by anything calling
kernel_read_file() (eg. kexec kernel image/initramfs).

Mimi

> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> ---
>  include/linux/module_signature.h | 45 ++++++++++++++++++++++++
>  init/Kconfig                     |  6 +++-
>  kernel/Makefile                  |  2 +-
>  kernel/module_signing.c          | 74 +++++++++++++++++-----------------------
>  4 files changed, 82 insertions(+), 45 deletions(-)
> 
> diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
> new file mode 100644
> index 000000000000..b04f16559b47
> --- /dev/null
> +++ b/include/linux/module_signature.h
> @@ -0,0 +1,45 @@
> +/* Module signature handling.
> + *
> + * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
> + * Written by David Howells (dhowells@redhat.com)
> + *
> + * 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
> +
> +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/init/Kconfig b/init/Kconfig
> index a92f27da4a27..891325e5aeff 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -2024,7 +2024,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
> @@ -2039,6 +2039,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 b302b4731d16..4451bbc70a08 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_KEXEC_CORE) += kexec_core.o
> 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);

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

[David Howells]

Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> On Tue, 2017-04-18 at 17:17 -0300, Thiago Jung Bauermann wrote:
> > 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, 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.
> 
> Basically we want to generalize the concept of an appended signature.
>  Referring to it as a "module signature format" seems a bit confusing.
> 
> David, would you have a problem with changing the appended string from
> "~Module signature appended~\n" to something more generic?

Conceptually, no.  Is it possible that doing so could break someone's module
that they load on multiple versions of the kernel?  Say a module that only
exports things and doesn't use anything from the core or any other module.

Also, it needs to reasonably long and distinct enough to prevent a false
positive match.

David

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

[Thiago Jung Bauermann]

Am Donnerstag, 20. April 2017, 15:37:37 BRT schrieb David Howells:
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Tue, 2017-04-18 at 17:17 -0300, Thiago Jung Bauermann wrote:
> > > 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, 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.
> > 
> > Basically we want to generalize the concept of an appended signature.
> >  Referring to it as a "module signature format" seems a bit confusing.
> > 
> > David, would you have a problem with changing the appended string from
> > "~Module signature appended~\n" to something more generic?
> 
> Conceptually, no.  Is it possible that doing so could break someone's module
> that they load on multiple versions of the kernel?  Say a module that only
> exports things and doesn't use anything from the core or any other module.

I think that changing the appended string has limited value because very few 
people actually see them. It's just a marker. We could s/module_signature/
appended_signature/ in the code but keep the actual string unchanged. What do 
you think?

Alternatively, we could change the string but accept both the old and the new 
string for backwards compatibility.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

[PATCH 6/6] ima: Support appended signatures for appraisal

[Thiago Jung Bauermann]

This patch introduces the appended_imasig keyword to the IMA policy syntax
to specify that a given hook should expect the file to have the IMA
signature appended to it. Here is how it can be used in a rule:

appraise func=KEXEC_KERNEL_CHECK appraise_type=appended_imasig
appraise func=KEXEC_KERNEL_CHECK appraise_type=appended_imasig|imasig

In the second form, IMA will accept either an appended signature or a
signature stored in the extended attribute. In that case, it will first
check whether there is an appended signature, and if not it will read it
from the extended attribute.

The format of the appended signature is the same used for signed kernel
modules. This means that the file can be signed with the scripts/sign-file
tool, with a command line such as this:

$ sign-file sha256 privkey_ima.pem x509_ima.der vmlinux

This code only works for files that are hashed from a memory buffer, not
for files that are read from disk at the time of hash calculation. In other
words, only hooks that use kernel_read_file can support appended
signatures.

The change in CONFIG_INTEGRITY_SIGNATURE to select CONFIG_KEYS instead of
depending on it is to avoid a dependency recursion in
CONFIG_IMA_APPRAISE_APPENDED_SIG, because CONFIG_MODULE_SIG_FORMAT selects
CONFIG_KEYS and Kconfig complains that CONFIG_INTEGRITY_SIGNATURE depends
on it.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 crypto/asymmetric_keys/asymmetric_type.c |  1 +
 crypto/asymmetric_keys/pkcs7_parser.c    | 12 +++++
 crypto/asymmetric_keys/pkcs7_verify.c    | 13 +++++
 include/crypto/pkcs7.h                   |  3 ++
 include/linux/verification.h             |  1 +
 security/integrity/Kconfig               |  2 +-
 security/integrity/ima/Kconfig           | 13 +++++
 security/integrity/ima/ima.h             |  8 +++
 security/integrity/ima/ima_appraise.c    | 84 +++++++++++++++++++++++++++++++-
 security/integrity/ima/ima_main.c        | 30 ++++++++++--
 security/integrity/ima/ima_policy.c      | 43 ++++++++++------
 security/integrity/integrity.h           | 20 +++++---
 12 files changed, 204 insertions(+), 26 deletions(-)

diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index e4b0ed386bc8..0d58ecfba2ea 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -28,6 +28,7 @@ const char *const key_being_used_for[NR__KEY_BEING_USED_FOR] = {
 	[VERIFYING_KEXEC_PE_SIGNATURE]		= "kexec PE sig",
 	[VERIFYING_KEY_SIGNATURE]		= "key sig",
 	[VERIFYING_KEY_SELF_SIGNATURE]		= "key self sig",
+	[VERIFYING_KEXEC_CMS_SIGNATURE]		= "kexec CMS sig",
 	[VERIFYING_UNSPECIFIED_SIGNATURE]	= "unspec sig",
 };
 EXPORT_SYMBOL_GPL(key_being_used_for);
diff --git a/crypto/asymmetric_keys/pkcs7_parser.c b/crypto/asymmetric_keys/pkcs7_parser.c
index af4cd8649117..e41beda297a8 100644
--- a/crypto/asymmetric_keys/pkcs7_parser.c
+++ b/crypto/asymmetric_keys/pkcs7_parser.c
@@ -673,3 +673,15 @@ int pkcs7_note_signed_info(void *context, size_t hdrlen,
 		return -ENOMEM;
 	return 0;
 }
+
+/**
+ * pkcs7_get_message_sig - get signature in @pkcs7
+ *
+ * This function doesn't meaningfully support messages with more than one
+ * signature. It will always return the first signature.
+ */
+const struct public_key_signature *pkcs7_get_message_sig(
+					const struct pkcs7_message *pkcs7)
+{
+	return pkcs7->signed_infos ? pkcs7->signed_infos->sig : NULL;
+}
diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index 2d93d9eccb4d..eee78074580a 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -420,6 +420,19 @@ int pkcs7_verify(struct pkcs7_message *pkcs7,
 			return -EKEYREJECTED;
 		}
 		break;
+	case VERIFYING_KEXEC_CMS_SIGNATURE:
+		/* Shipping certificates in the CMS message is not allowed. */
+		if (pkcs7->certs) {
+			pr_warn("Signature isn't allowed to contain certificates.\n");
+			return -EBADMSG;
+		}
+
+		/* Shipping CRLs in the CMS message is not allowed. */
+		if (pkcs7->crl) {
+			pr_warn("Signature isn't allowed to contain CRLs.\n");
+			return -EBADMSG;
+		}
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h
index 583f199400a3..a05a0d7214e6 100644
--- a/include/crypto/pkcs7.h
+++ b/include/crypto/pkcs7.h
@@ -29,6 +29,9 @@ extern int pkcs7_get_content_data(const struct pkcs7_message *pkcs7,
 				  const void **_data, size_t *_datalen,
 				  size_t *_headerlen);
 
+extern const struct public_key_signature *pkcs7_get_message_sig(
+					const struct pkcs7_message *pkcs7);
+
 /*
  * pkcs7_trust.c
  */
diff --git a/include/linux/verification.h b/include/linux/verification.h
index a10549a6c7cd..b7298d804440 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -22,6 +22,7 @@ enum key_being_used_for {
 	VERIFYING_KEY_SIGNATURE,
 	VERIFYING_KEY_SELF_SIGNATURE,
 	VERIFYING_UNSPECIFIED_SIGNATURE,
+	VERIFYING_KEXEC_CMS_SIGNATURE,
 	NR__KEY_BEING_USED_FOR
 };
 extern const char *const key_being_used_for[NR__KEY_BEING_USED_FOR];
diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index da9565891738..0d642e0317c7 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -17,8 +17,8 @@ if INTEGRITY
 
 config INTEGRITY_SIGNATURE
 	bool "Digital signature verification using multiple keyrings"
-	depends on KEYS
 	default n
+	select KEYS
 	select SIGNATURE
 	help
 	  This option enables digital signature verification support
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 370eb2f4dd37..13662043bf90 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -155,6 +155,19 @@ config IMA_APPRAISE
 	  <http://linux-ima.sourceforge.net>
 	  If unsure, say N.
 
+config IMA_APPRAISE_APPENDED_SIG
+	bool "Support appended signatures for appraisal"
+	depends on IMA_APPRAISE
+	depends on INTEGRITY_ASYMMETRIC_KEYS
+	select PKCS7_MESSAGE_PARSER
+	select MODULE_SIG_FORMAT
+	default n
+	help
+	   Adds support for signatures appended to files. The format of the
+	   appended signature is the same used for signed kernel modules.
+	   The appended_imasig keyword can be used in the IMA policy for this
+	   purpose.
+
 config IMA_TRUSTED_KEYRING
 	bool "Require all keys on the .ima keyring be signed (deprecated)"
 	depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index b563fbd4d122..63ce5a1179c3 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -185,6 +185,8 @@ enum ima_hooks {
 	MAX_CHECK
 };
 
+extern const char *const func_tokens[];
+
 /* LIM API function definitions */
 int ima_get_action(struct inode *inode, int mask,
 		   enum ima_hooks func, int *pcr);
@@ -243,6 +245,12 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
 int ima_read_xattr(struct dentry *dentry,
 		   struct evm_ima_xattr_data **xattr_value);
 
+#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG
+void ima_read_appended_sig(const void *buf, loff_t *buf_len,
+			  struct evm_ima_xattr_data **xattr_value,
+			  int *xattr_len);
+#endif
+
 #else
 static inline int ima_appraise_measurement(enum ima_hooks func,
 					   struct integrity_iint_cache *iint,
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 8c8030a29117..64a42f16c4ee 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -9,12 +9,15 @@
  * the Free Software Foundation, version 2 of the License.
  */
 #include <linux/module.h>
+#include <linux/module_signature.h>
 #include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/xattr.h>
 #include <linux/magic.h>
 #include <linux/ima.h>
 #include <linux/evm.h>
+#include <keys/asymmetric-type.h>
+#include <crypto/pkcs7.h>
 
 #include "ima.h"
 
@@ -177,6 +180,85 @@ int ima_read_xattr(struct dentry *dentry,
 	return ret;
 }
 
+#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG
+void ima_read_appended_sig(const void *buf, loff_t *buf_len,
+			  struct evm_ima_xattr_data **xattr_value,
+			  int *xattr_len)
+{
+	const size_t marker_len = sizeof(MODULE_SIG_STRING) - 1;
+	const struct public_key_signature *pks;
+	const struct module_signature *sig;
+	struct signature_v2_hdr *hdr;
+	struct pkcs7_message *pkcs7;
+	int i, hdr_len;
+	loff_t file_len = *buf_len;
+	size_t sig_len;
+	const void *p;
+
+	if (file_len <= marker_len + sizeof(*sig))
+		return;
+
+	p = buf + file_len - marker_len;
+	if (memcmp(p, MODULE_SIG_STRING, marker_len))
+		return;
+
+	file_len -= marker_len;
+	sig = (const struct module_signature *) (p - sizeof(*sig));
+
+	if (validate_module_signature(sig, file_len))
+		return;
+
+	sig_len = be32_to_cpu(sig->sig_len);
+	file_len -= sig_len + sizeof(*sig);
+
+	pkcs7 = pkcs7_parse_message(buf + file_len, sig_len);
+	if (IS_ERR(pkcs7))
+		return;
+
+	if (pkcs7_verify(pkcs7, VERIFYING_KEXEC_CMS_SIGNATURE))
+		goto out;
+
+	pks = pkcs7_get_message_sig(pkcs7);
+	if (!pks)
+		goto out;
+
+	/* IMA only supports RSA keys. */
+	if (strcmp(pks->pkey_algo, "rsa"))
+		goto out;
+
+	if (!pks->auth_ids[0])
+		goto out;
+
+	for (i = 0; i < HASH_ALGO__LAST; i++)
+		if (!strcmp(hash_algo_name[i], pks->hash_algo))
+			break;
+
+	if (i == HASH_ALGO__LAST)
+		goto out;
+
+	hdr_len = sizeof(*hdr) + pks->s_size;
+	hdr = kmalloc(hdr_len, GFP_KERNEL);
+	if (!hdr)
+		goto out;
+
+	hdr->type = EVM_IMA_XATTR_DIGSIG;
+	hdr->version = 2;
+	hdr->hash_algo = i;
+	memcpy(hdr->sig, pks->s, pks->s_size);
+	hdr->sig_size = cpu_to_be16(pks->s_size);
+
+	p = pks->auth_ids[0]->data + pks->auth_ids[0]->len - sizeof(hdr->keyid);
+	memcpy(&hdr->keyid, p, sizeof(hdr->keyid));
+
+	*xattr_value = (typeof(*xattr_value)) hdr;
+	*xattr_len = hdr_len;
+	*buf_len = file_len;
+
+ out:
+	pkcs7_free_message(pkcs7);
+}
+#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */
+
 /*
  * ima_appraise_measurement - appraise file measurement
  *
@@ -205,7 +287,7 @@ int ima_appraise_measurement(enum ima_hooks func,
 		if (rc && rc != -ENODATA)
 			goto out;
 
-		cause = iint->flags & IMA_DIGSIG_REQUIRED ?
+		cause = iint->flags & IMA_DIGSIG_REQUIRED_MASK ?
 				"IMA-signature-required" : "missing-hash";
 		status = INTEGRITY_NOLABEL;
 		if (opened & FILE_CREATED) {
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2aebb7984437..994ee420b2ec 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -16,6 +16,9 @@
  *	implements the IMA hooks: ima_bprm_check, ima_file_mmap,
  *	and ima_file_check.
  */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/module.h>
 #include <linux/file.h>
 #include <linux/binfmts.h>
@@ -228,9 +231,30 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 
 	template_desc = ima_template_desc_current();
 	if ((action & IMA_APPRAISE_SUBMASK) ||
-		    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0)
-		/* read 'security.ima' */
-		xattr_len = ima_read_xattr(file_dentry(file), &xattr_value);
+		    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) {
+#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG
+		unsigned long digsig_req;
+
+		if (iint->flags & IMA_APPENDED_DIGSIG_REQUIRED) {
+			if (!buf || !size)
+				pr_err("%s doesn't support appended_imasig\n",
+				       func_tokens[func]);
+			else
+				ima_read_appended_sig(buf, &size, &xattr_value,
+						      &xattr_len);
+		}
+
+		/*
+		 * Don't try to read the xattr if we require an appended
+		 * signature but failed to get one.
+		 */
+		digsig_req = iint->flags & IMA_DIGSIG_REQUIRED_MASK;
+		if (!xattr_len && digsig_req != IMA_APPENDED_DIGSIG_REQUIRED)
+#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */
+			/* read 'security.ima' */
+			xattr_len = ima_read_xattr(file_dentry(file),
+						   &xattr_value);
+	}
 
 	hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
 
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 158eafef64e8..96601b5ba411 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -777,8 +777,15 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			}
 
 			ima_log_string(ab, "appraise_type", args[0].from);
-			if ((strcmp(args[0].from, "imasig")) == 0)
+			if (!strcmp(args[0].from, "imasig"))
 				entry->flags |= IMA_DIGSIG_REQUIRED;
+#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG
+			else if (!strcmp(args[0].from, "appended_imasig"))
+				entry->flags |= IMA_APPENDED_DIGSIG_REQUIRED;
+			else if (!strcmp(args[0].from, "appended_imasig|imasig"))
+				entry->flags |= IMA_DIGSIG_REQUIRED
+						| IMA_APPENDED_DIGSIG_REQUIRED;
+#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */
 			else
 				result = -EINVAL;
 			break;
@@ -884,19 +891,7 @@ void ima_delete_rules(void)
 	}
 }
 
-#ifdef	CONFIG_IMA_READ_POLICY
-enum {
-	mask_exec = 0, mask_write, mask_read, mask_append
-};
-
-static const char *const mask_tokens[] = {
-	"MAY_EXEC",
-	"MAY_WRITE",
-	"MAY_READ",
-	"MAY_APPEND"
-};
-
-static const char *const func_tokens[] = {
+const char *const func_tokens[] = {
 	NULL,
 	"FILE_CHECK",
 	"MMAP_CHECK",
@@ -909,6 +904,18 @@ static const char *const func_tokens[] = {
 	"POLICY_CHECK"
 };
 
+#ifdef	CONFIG_IMA_READ_POLICY
+enum {
+	mask_exec = 0, mask_write, mask_read, mask_append
+};
+
+static const char *const mask_tokens[] = {
+	"MAY_EXEC",
+	"MAY_WRITE",
+	"MAY_READ",
+	"MAY_APPEND"
+};
+
 void *ima_policy_start(struct seq_file *m, loff_t *pos)
 {
 	loff_t l = *pos;
@@ -1062,7 +1069,13 @@ int ima_policy_show(struct seq_file *m, void *v)
 			}
 		}
 	}
-	if (entry->flags & IMA_DIGSIG_REQUIRED)
+	if ((entry->flags & IMA_DIGSIG_REQUIRED_MASK) == IMA_DIGSIG_REQUIRED_MASK)
+#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG
+		seq_puts(m, "appraise_type=appended_imasig|imasig ");
+	else if (entry->flags & IMA_APPENDED_DIGSIG_REQUIRED)
+		seq_puts(m, "appraise_type=appended_imasig ");
+	else if (entry->flags & IMA_DIGSIG_REQUIRED)
+#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */
 		seq_puts(m, "appraise_type=imasig ");
 	if (entry->flags & IMA_PERMIT_DIRECTIO)
 		seq_puts(m, "permit_directio ");
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index a53e7e4ab06c..de2a666740c1 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -27,12 +27,20 @@
 #define IMA_AUDITED		0x00000080
 
 /* iint cache flags */
-#define IMA_ACTION_FLAGS	0xff000000
-#define IMA_ACTION_RULE_FLAGS	0x06000000
-#define IMA_DIGSIG		0x01000000
-#define IMA_DIGSIG_REQUIRED	0x02000000
-#define IMA_PERMIT_DIRECTIO	0x04000000
-#define IMA_NEW_FILE		0x08000000
+#define IMA_ACTION_FLAGS		0xff000000
+#define IMA_ACTION_RULE_FLAGS		0x16000000
+#define IMA_DIGSIG			0x01000000
+#define IMA_DIGSIG_REQUIRED		0x02000000
+#define IMA_PERMIT_DIRECTIO		0x04000000
+#define IMA_NEW_FILE			0x08000000
+#define IMA_APPENDED_DIGSIG_REQUIRED	0x10000000
+
+#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG
+#define IMA_DIGSIG_REQUIRED_MASK	(IMA_DIGSIG_REQUIRED | \
+					 IMA_APPENDED_DIGSIG_REQUIRED)
+#else
+#define IMA_DIGSIG_REQUIRED_MASK	IMA_DIGSIG_REQUIRED
+#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */
 
 #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
 				 IMA_APPRAISE_SUBMASK)
-- 
2.7.4

Re: [PATCH 6/6] ima: Support appended signatures for appraisal

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2131 bytes --]

Hi Thiago,

[auto build test ERROR on security/next]
[also build test ERROR on v4.11-rc7 next-20170419]
[cannot apply to integrity/next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Thiago-Jung-Bauermann/Appended-signatures-support-for-IMA-appraisal/20170419-053422
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next
config: i386-randconfig-h0-04201028 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   In file included from security/integrity/ima/ima_appraise.c:19:0:
   include/keys/asymmetric-type.h: In function 'asymmetric_key_ids':
>> include/keys/asymmetric-type.h:76:12: error: dereferencing pointer to incomplete type 'const struct key'
     return key->payload.data[asym_key_ids];
               ^~

vim +76 include/keys/asymmetric-type.h

7901c1a8 David Howells 2014-09-16  70  							    size_t len_1,
7901c1a8 David Howells 2014-09-16  71  							    const void *val_2,
7901c1a8 David Howells 2014-09-16  72  							    size_t len_2);
146aa8b1 David Howells 2015-10-21  73  static inline
146aa8b1 David Howells 2015-10-21  74  const struct asymmetric_key_ids *asymmetric_key_ids(const struct key *key)
146aa8b1 David Howells 2015-10-21  75  {
146aa8b1 David Howells 2015-10-21 @76  	return key->payload.data[asym_key_ids];
146aa8b1 David Howells 2015-10-21  77  }
7901c1a8 David Howells 2014-09-16  78  
9eb02989 David Howells 2016-04-06  79  extern struct key *find_asymmetric_key(struct key *keyring,

:::::: The code at line 76 was first introduced by commit
:::::: 146aa8b1453bd8f1ff2304ffb71b4ee0eb9acdcc KEYS: Merge the type-specific data with the payload data

:::::: TO: David Howells <dhowells@redhat.com>
:::::: CC: David Howells <dhowells@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25689 bytes --]

Re: [PATCH 6/6] ima: Support appended signatures for appraisal

[Thiago Jung Bauermann]

Am Donnerstag, 20. April 2017, 11:04:40 BRT schrieb kbuild test robot:
>    In file included from security/integrity/ima/ima_appraise.c:19:0:
> 
>    include/keys/asymmetric-type.h: In function 'asymmetric_key_ids':
> >> include/keys/asymmetric-type.h:76:12: error: dereferencing pointer to
> >> incomplete type 'const struct key'
>      return key->payload.data[asym_key_ids];
>                ^~

This happens with CONFIG_IMA_APPRAISE=y and CONFIG_KEYS=n.
Fixed by only including the new header files in ima_appraise.c if 
CONFIG_IMA_APPRAISE_APPENDED_SIG=y

-- 
Thiago Jung Bauermann
IBM Linux Technology Center


>From e2d3143b13d6601f6a1f4936dfb569a04a1ce0cc Mon Sep 17 00:00:00 2001
From: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
Date: Mon, 20 Mar 2017 11:08:21 -0300
Subject: [PATCH 6/6] ima: Support appended signatures for appraisal

This patch introduces the appended_imasig keyword to the IMA policy syntax
to specify that a given hook should expect the file to have the IMA
signature appended to it. Here is how it can be used in a rule:

appraise func=KEXEC_KERNEL_CHECK appraise_type=appended_imasig
appraise func=KEXEC_KERNEL_CHECK appraise_type=appended_imasig|imasig

In the second form, IMA will accept either an appended signature or a
signature stored in the extended attribute. In that case, it will first
check whether there is an appended signature, and if not it will read it
from the extended attribute.

The format of the appended signature is the same used for signed kernel
modules. This means that the file can be signed with the scripts/sign-file
tool, with a command line such as this:

$ sign-file sha256 privkey_ima.pem x509_ima.der vmlinux

This code only works for files that are hashed from a memory buffer, not
for files that are read from disk at the time of hash calculation. In other
words, only hooks that use kernel_read_file can support appended
signatures.

The change in CONFIG_INTEGRITY_SIGNATURE to select CONFIG_KEYS instead of
depending on it is to avoid a dependency recursion in
CONFIG_IMA_APPRAISE_APPENDED_SIG, because CONFIG_MODULE_SIG_FORMAT selects
CONFIG_KEYS and Kconfig complains that CONFIG_INTEGRITY_SIGNATURE depends
on it.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 crypto/asymmetric_keys/asymmetric_type.c |  1 +
 crypto/asymmetric_keys/pkcs7_parser.c    | 12 +++++
 crypto/asymmetric_keys/pkcs7_verify.c    | 13 +++++
 include/crypto/pkcs7.h                   |  3 ++
 include/linux/verification.h             |  1 +
 security/integrity/Kconfig               |  2 +-
 security/integrity/ima/Kconfig           | 13 +++++
 security/integrity/ima/ima.h             |  8 +++
 security/integrity/ima/ima_appraise.c    | 86 +++++++++++++++++++++++++++++++-
 security/integrity/ima/ima_main.c        | 30 +++++++++--
 security/integrity/ima/ima_policy.c      | 29 ++++++++---
 security/integrity/integrity.h           | 20 +++++---
 12 files changed, 199 insertions(+), 19 deletions(-)

diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index e4b0ed386bc8..0d58ecfba2ea 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -28,6 +28,7 @@ const char *const key_being_used_for[NR__KEY_BEING_USED_FOR] = {
 	[VERIFYING_KEXEC_PE_SIGNATURE]		= "kexec PE sig",
 	[VERIFYING_KEY_SIGNATURE]		= "key sig",
 	[VERIFYING_KEY_SELF_SIGNATURE]		= "key self sig",
+	[VERIFYING_KEXEC_CMS_SIGNATURE]		= "kexec CMS sig",
 	[VERIFYING_UNSPECIFIED_SIGNATURE]	= "unspec sig",
 };
 EXPORT_SYMBOL_GPL(key_being_used_for);
diff --git a/crypto/asymmetric_keys/pkcs7_parser.c b/crypto/asymmetric_keys/pkcs7_parser.c
index af4cd8649117..e41beda297a8 100644
--- a/crypto/asymmetric_keys/pkcs7_parser.c
+++ b/crypto/asymmetric_keys/pkcs7_parser.c
@@ -673,3 +673,15 @@ int pkcs7_note_signed_info(void *context, size_t hdrlen,
 		return -ENOMEM;
 	return 0;
 }
+
+/**
+ * pkcs7_get_message_sig - get signature in @pkcs7
+ *
+ * This function doesn't meaningfully support messages with more than one
+ * signature. It will always return the first signature.
+ */
+const struct public_key_signature *pkcs7_get_message_sig(
+					const struct pkcs7_message *pkcs7)
+{
+	return pkcs7->signed_infos ? pkcs7->signed_infos->sig : NULL;
+}
diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index 2d93d9eccb4d..eee78074580a 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -420,6 +420,19 @@ int pkcs7_verify(struct pkcs7_message *pkcs7,
 			return -EKEYREJECTED;
 		}
 		break;
+	case VERIFYING_KEXEC_CMS_SIGNATURE:
+		/* Shipping certificates in the CMS message is not allowed. */
+		if (pkcs7->certs) {
+			pr_warn("Signature isn't allowed to contain certificates.\n");
+			return -EBADMSG;
+		}
+
+		/* Shipping CRLs in the CMS message is not allowed. */
+		if (pkcs7->crl) {
+			pr_warn("Signature isn't allowed to contain CRLs.\n");
+			return -EBADMSG;
+		}
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h
index 583f199400a3..a05a0d7214e6 100644
--- a/include/crypto/pkcs7.h
+++ b/include/crypto/pkcs7.h
@@ -29,6 +29,9 @@ extern int pkcs7_get_content_data(const struct pkcs7_message *pkcs7,
 				  const void **_data, size_t *_datalen,
 				  size_t *_headerlen);
 
+extern const struct public_key_signature *pkcs7_get_message_sig(
+					const struct pkcs7_message *pkcs7);
+
 /*
  * pkcs7_trust.c
  */
diff --git a/include/linux/verification.h b/include/linux/verification.h
index a10549a6c7cd..b7298d804440 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -22,6 +22,7 @@ enum key_being_used_for {
 	VERIFYING_KEY_SIGNATURE,
 	VERIFYING_KEY_SELF_SIGNATURE,
 	VERIFYING_UNSPECIFIED_SIGNATURE,
+	VERIFYING_KEXEC_CMS_SIGNATURE,
 	NR__KEY_BEING_USED_FOR
 };
 extern const char *const key_being_used_for[NR__KEY_BEING_USED_FOR];
diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index da9565891738..0d642e0317c7 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -17,8 +17,8 @@ if INTEGRITY
 
 config INTEGRITY_SIGNATURE
 	bool "Digital signature verification using multiple keyrings"
-	depends on KEYS
 	default n
+	select KEYS
 	select SIGNATURE
 	help
 	  This option enables digital signature verification support
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 370eb2f4dd37..13662043bf90 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -155,6 +155,19 @@ config IMA_APPRAISE
 	  <http://linux-ima.sourceforge.net>
 	  If unsure, say N.
 
+config IMA_APPRAISE_APPENDED_SIG
+	bool "Support appended signatures for appraisal"
+	depends on IMA_APPRAISE
+	depends on INTEGRITY_ASYMMETRIC_KEYS
+	select PKCS7_MESSAGE_PARSER
+	select MODULE_SIG_FORMAT
+	default n
+	help
+	   Adds support for signatures appended to files. The format of the
+	   appended signature is the same used for signed kernel modules.
+	   The appended_imasig keyword can be used in the IMA policy for this
+	   purpose.
+
 config IMA_TRUSTED_KEYRING
 	bool "Require all keys on the .ima keyring be signed (deprecated)"
 	depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 51ef805cf7f3..60a823630d2a 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,12 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
 int ima_read_xattr(struct dentry *dentry,
 		   struct evm_ima_xattr_data **xattr_value);
 
+#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG
+void ima_read_appended_sig(const void *buf, loff_t *buf_len,
+			  struct evm_ima_xattr_data **xattr_value,
+			  int *xattr_len);
+#endif
+
 #else
 static inline int ima_appraise_measurement(enum ima_hooks func,
 					   struct integrity_iint_cache *iint,
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 8c8030a29117..789bcb1b2d89 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -15,6 +15,11 @@
 #include <linux/magic.h>
 #include <linux/ima.h>
 #include <linux/evm.h>
+#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG
+#include <linux/module_signature.h>
+#include <keys/asymmetric-type.h>
+#include <crypto/pkcs7.h>
+#endif
 
 #include "ima.h"
 
@@ -177,6 +182,85 @@ int ima_read_xattr(struct dentry *dentry,
 	return ret;
 }
 
+#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG
+void ima_read_appended_sig(const void *buf, loff_t *buf_len,
+			  struct evm_ima_xattr_data **xattr_value,
+			  int *xattr_len)
+{
+	const size_t marker_len = sizeof(MODULE_SIG_STRING) - 1;
+	const struct public_key_signature *pks;
+	const struct module_signature *sig;
+	struct signature_v2_hdr *hdr;
+	struct pkcs7_message *pkcs7;
+	int i, hdr_len;
+	loff_t file_len = *buf_len;
+	size_t sig_len;
+	const void *p;
+
+	if (file_len <= marker_len + sizeof(*sig))
+		return;
+
+	p = buf + file_len - marker_len;
+	if (memcmp(p, MODULE_SIG_STRING, marker_len))
+		return;
+
+	file_len -= marker_len;
+	sig = (const struct module_signature *) (p - sizeof(*sig));
+
+	if (validate_module_signature(sig, file_len))
+		return;
+
+	sig_len = be32_to_cpu(sig->sig_len);
+	file_len -= sig_len + sizeof(*sig);
+
+	pkcs7 = pkcs7_parse_message(buf + file_len, sig_len);
+	if (IS_ERR(pkcs7))
+		return;
+
+	if (pkcs7_verify(pkcs7, VERIFYING_KEXEC_CMS_SIGNATURE))
+		goto out;
+
+	pks = pkcs7_get_message_sig(pkcs7);
+	if (!pks)
+		goto out;
+
+	/* IMA only supports RSA keys. */
+	if (strcmp(pks->pkey_algo, "rsa"))
+		goto out;
+
+	if (!pks->auth_ids[0])
+		goto out;
+
+	for (i = 0; i < HASH_ALGO__LAST; i++)
+		if (!strcmp(hash_algo_name[i], pks->hash_algo))
+			break;
+
+	if (i == HASH_ALGO__LAST)
+		goto out;
+
+	hdr_len = sizeof(*hdr) + pks->s_size;
+	hdr = kmalloc(hdr_len, GFP_KERNEL);
+	if (!hdr)
+		goto out;
+
+	hdr->type = EVM_IMA_XATTR_DIGSIG;
+	hdr->version = 2;
+	hdr->hash_algo = i;
+	memcpy(hdr->sig, pks->s, pks->s_size);
+	hdr->sig_size = cpu_to_be16(pks->s_size);
+
+	p = pks->auth_ids[0]->data + pks->auth_ids[0]->len - sizeof(hdr->keyid);
+	memcpy(&hdr->keyid, p, sizeof(hdr->keyid));
+
+	*xattr_value = (typeof(*xattr_value)) hdr;
+	*xattr_len = hdr_len;
+	*buf_len = file_len;
+
+ out:
+	pkcs7_free_message(pkcs7);
+}
+#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */
+
 /*
  * ima_appraise_measurement - appraise file measurement
  *
@@ -205,7 +289,7 @@ int ima_appraise_measurement(enum ima_hooks func,
 		if (rc && rc != -ENODATA)
 			goto out;
 
-		cause = iint->flags & IMA_DIGSIG_REQUIRED ?
+		cause = iint->flags & IMA_DIGSIG_REQUIRED_MASK ?
 				"IMA-signature-required" : "missing-hash";
 		status = INTEGRITY_NOLABEL;
 		if (opened & FILE_CREATED) {
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2aebb7984437..994ee420b2ec 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -16,6 +16,9 @@
  *	implements the IMA hooks: ima_bprm_check, ima_file_mmap,
  *	and ima_file_check.
  */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/module.h>
 #include <linux/file.h>
 #include <linux/binfmts.h>
@@ -228,9 +231,30 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 
 	template_desc = ima_template_desc_current();
 	if ((action & IMA_APPRAISE_SUBMASK) ||
-		    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0)
-		/* read 'security.ima' */
-		xattr_len = ima_read_xattr(file_dentry(file), &xattr_value);
+		    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) {
+#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG
+		unsigned long digsig_req;
+
+		if (iint->flags & IMA_APPENDED_DIGSIG_REQUIRED) {
+			if (!buf || !size)
+				pr_err("%s doesn't support appended_imasig\n",
+				       func_tokens[func]);
+			else
+				ima_read_appended_sig(buf, &size, &xattr_value,
+						      &xattr_len);
+		}
+
+		/*
+		 * Don't try to read the xattr if we require an appended
+		 * signature but failed to get one.
+		 */
+		digsig_req = iint->flags & IMA_DIGSIG_REQUIRED_MASK;
+		if (!xattr_len && digsig_req != IMA_APPENDED_DIGSIG_REQUIRED)
+#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */
+			/* read 'security.ima' */
+			xattr_len = ima_read_xattr(file_dentry(file),
+						   &xattr_value);
+	}
 
 	hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
 
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 39d43a5beb5a..fb652f5e3999 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -777,8 +777,15 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			}
 
 			ima_log_string(ab, "appraise_type", args[0].from);
-			if ((strcmp(args[0].from, "imasig")) == 0)
+			if (!strcmp(args[0].from, "imasig"))
 				entry->flags |= IMA_DIGSIG_REQUIRED;
+#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG
+			else if (!strcmp(args[0].from, "appended_imasig"))
+				entry->flags |= IMA_APPENDED_DIGSIG_REQUIRED;
+			else if (!strcmp(args[0].from, "appended_imasig|imasig"))
+				entry->flags |= IMA_DIGSIG_REQUIRED
+						| IMA_APPENDED_DIGSIG_REQUIRED;
+#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */
 			else
 				result = -EINVAL;
 			break;
@@ -884,6 +891,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
@@ -896,12 +909,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;
@@ -1049,7 +1056,13 @@ int ima_policy_show(struct seq_file *m, void *v)
 			}
 		}
 	}
-	if (entry->flags & IMA_DIGSIG_REQUIRED)
+	if ((entry->flags & IMA_DIGSIG_REQUIRED_MASK) == IMA_DIGSIG_REQUIRED_MASK)
+#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG
+		seq_puts(m, "appraise_type=appended_imasig|imasig ");
+	else if (entry->flags & IMA_APPENDED_DIGSIG_REQUIRED)
+		seq_puts(m, "appraise_type=appended_imasig ");
+	else if (entry->flags & IMA_DIGSIG_REQUIRED)
+#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */
 		seq_puts(m, "appraise_type=imasig ");
 	if (entry->flags & IMA_PERMIT_DIRECTIO)
 		seq_puts(m, "permit_directio ");
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index a53e7e4ab06c..de2a666740c1 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -27,12 +27,20 @@
 #define IMA_AUDITED		0x00000080
 
 /* iint cache flags */
-#define IMA_ACTION_FLAGS	0xff000000
-#define IMA_ACTION_RULE_FLAGS	0x06000000
-#define IMA_DIGSIG		0x01000000
-#define IMA_DIGSIG_REQUIRED	0x02000000
-#define IMA_PERMIT_DIRECTIO	0x04000000
-#define IMA_NEW_FILE		0x08000000
+#define IMA_ACTION_FLAGS		0xff000000
+#define IMA_ACTION_RULE_FLAGS		0x16000000
+#define IMA_DIGSIG			0x01000000
+#define IMA_DIGSIG_REQUIRED		0x02000000
+#define IMA_PERMIT_DIRECTIO		0x04000000
+#define IMA_NEW_FILE			0x08000000
+#define IMA_APPENDED_DIGSIG_REQUIRED	0x10000000
+
+#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG
+#define IMA_DIGSIG_REQUIRED_MASK	(IMA_DIGSIG_REQUIRED | \
+					 IMA_APPENDED_DIGSIG_REQUIRED)
+#else
+#define IMA_DIGSIG_REQUIRED_MASK	IMA_DIGSIG_REQUIRED
+#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */
 
 #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
 				 IMA_APPRAISE_SUBMASK)
-- 
2.7.4

Re: [PATCH 6/6] ima: Support appended signatures for appraisal

[Mehmet Kayaalp]

> On Apr 20, 2017, at 7:41 PM, Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> wrote:
> 
> This patch introduces the appended_imasig keyword to the IMA policy syntax
> to specify that a given hook should expect the file to have the IMA
> signature appended to it. Here is how it can be used in a rule:
> 
> appraise func=KEXEC_KERNEL_CHECK appraise_type=appended_imasig
> appraise func=KEXEC_KERNEL_CHECK appraise_type=appended_imasig|imasig
> 
> In the second form, IMA will accept either an appended signature or a
> signature stored in the extended attribute. In that case, it will first
> check whether there is an appended signature, and if not it will read it
> from the extended attribute.
> 
> 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:

I would suggest naming the appraise_type as modsig (or some variant) to clarify
that the format is defined by how module signatures are handled. Maybe we'd like 
to define a different appended/inline signature format for IMA in the future.

-Mehmet

Re: [PATCH 6/6] ima: Support appended signatures for appraisal

[Thiago Jung Bauermann]

Am Mittwoch, 26. April 2017, 18:18:34 BRT schrieb Mehmet Kayaalp:
> > On Apr 20, 2017, at 7:41 PM, Thiago Jung Bauermann
> > <bauerman@linux.vnet.ibm.com> wrote:
> > 
> > This patch introduces the appended_imasig keyword to the IMA policy syntax
> > to specify that a given hook should expect the file to have the IMA
> > signature appended to it. Here is how it can be used in a rule:
> > 
> > appraise func=KEXEC_KERNEL_CHECK appraise_type=appended_imasig
> > appraise func=KEXEC_KERNEL_CHECK appraise_type=appended_imasig|imasig
> > 
> > In the second form, IMA will accept either an appended signature or a
> > signature stored in the extended attribute. In that case, it will first
> > check whether there is an appended signature, and if not it will read it
> > from the extended attribute.
> > 
> > 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:
> I would suggest naming the appraise_type as modsig (or some variant) to
> clarify that the format is defined by how module signatures are handled.
> Maybe we'd like to define a different appended/inline signature format for
> IMA in the future.

I like the suggestion. Would that mean that we will keep refering to it as 
"module signature format", and thus nothing changes in patch 5?

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH 6/6] ima: Support appended signatures for appraisal

[Mehmet Kayaalp]

> On Apr 27, 2017, at 5:41 PM, Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> wrote:
> 
> Am Mittwoch, 26. April 2017, 18:18:34 BRT schrieb Mehmet Kayaalp:
>>> On Apr 20, 2017, at 7:41 PM, Thiago Jung Bauermann
>>> <bauerman@linux.vnet.ibm.com> wrote:
>>> 
>>> This patch introduces the appended_imasig keyword to the IMA policy syntax
>>> to specify that a given hook should expect the file to have the IMA
>>> signature appended to it. Here is how it can be used in a rule:
>>> 
>>> appraise func=KEXEC_KERNEL_CHECK appraise_type=appended_imasig
>>> appraise func=KEXEC_KERNEL_CHECK appraise_type=appended_imasig|imasig
>>> 
>>> In the second form, IMA will accept either an appended signature or a
>>> signature stored in the extended attribute. In that case, it will first
>>> check whether there is an appended signature, and if not it will read it
>>> from the extended attribute.
>>> 
>>> 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:
>> I would suggest naming the appraise_type as modsig (or some variant) to
>> clarify that the format is defined by how module signatures are handled.
>> Maybe we'd like to define a different appended/inline signature format for
>> IMA in the future.
> 
> I like the suggestion. Would that mean that we will keep refering to it as 
> "module signature format", and thus nothing changes in patch 5?

I think so. If we want IMA to own the format, we might want to go further than
just changing the word "module" in the marker. We might consider having more
flexibility and some additional fields, for example multiple signatures, or certificate
chains, ascii/binary encodings etc. We could maybe add a different type for CMS
Signed-Data.

Mehmet

Re: [PATCH 6/6] ima: Support appended signatures for appraisal

[Mimi Zohar]

Hi Thiago,

On Tue, 2017-04-18 at 17:17 -0300, Thiago Jung Bauermann wrote:
> This patch introduces the appended_imasig keyword to the IMA policy syntax
> to specify that a given hook should expect the file to have the IMA
> signature appended to it.  Here is how it can be used in a rule:
> 
> appraise func=KEXEC_KERNEL_CHECK appraise_type=appended_imasig
> appraise func=KEXEC_KERNEL_CHECK appraise_type=appended_imasig|imasig

> In the second form, IMA will accept either an appended signature or a
> signature stored in the extended attribute. In that case, it will first
> check whether there is an appended signature, and if not it will read it
> from the extended attribute.

An appended signature should be another place to look for a signature,
when a signature is required, but it shouldn't make a difference where
the signature is located.  "imasig" could have implied to look for the
signature in both places - xattr or appended.  So the new option is
just a hint - a performance improvement.

This might seem picayune, but the difference between "expect" vs.
"hint" impacts the code. (Further explanation inline.)

> The format of the appended signature is the same used for signed kernel
> modules. This means that the file can be signed with the scripts/sign-file
> tool, with a command line such as this:
> 
> $ sign-file sha256 privkey_ima.pem x509_ima.der vmlinux
> 
> This code only works for files that are hashed from a memory buffer, not
> for files that are read from disk at the time of hash calculation. In other
> words, only hooks that use kernel_read_file can support appended
> signatures.
> 
> The change in CONFIG_INTEGRITY_SIGNATURE to select CONFIG_KEYS instead of
> depending on it is to avoid a dependency recursion in
> CONFIG_IMA_APPRAISE_APPENDED_SIG, because CONFIG_MODULE_SIG_FORMAT selects
> CONFIG_KEYS and Kconfig complains that CONFIG_INTEGRITY_SIGNATURE depends
> on it.
> 
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> ---

snip

> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 2aebb7984437..994ee420b2ec 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -16,6 +16,9 @@
>   *	implements the IMA hooks: ima_bprm_check, ima_file_mmap,
>   *	and ima_file_check.
>   */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/module.h>
>  #include <linux/file.h>
>  #include <linux/binfmts.h>
> @@ -228,9 +231,30 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> 
>  	template_desc = ima_template_desc_current();
>  	if ((action & IMA_APPRAISE_SUBMASK) ||
> -		    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0)
> -		/* read 'security.ima' */
> -		xattr_len = ima_read_xattr(file_dentry(file), &xattr_value);
> +		    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) {
> +#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG

Using "ifdef" in C code is really discouraged. Normally, it is an
indication that the code needs to be re-factored.  Assuming we really
need a new CONFIG option, which I'm not sure that we do, I would move
the appended signature code to its own file, define stub functions in
ima.h, and update the Makefile.

> +		unsigned long digsig_req;
> +
> +		if (iint->flags & IMA_APPENDED_DIGSIG_REQUIRED) {
> +			if (!buf || !size)
> +				pr_err("%s doesn't support appended_imasig\n",
> +				       func_tokens[func]);

The policy parsing should prevent defining appended_imasig on
inappropriate hooks.  Since the iint->flags might be shared between
hooks, we might still need to test buf, but it could be simplified to:

if (!buf && iint->flags & IMA_APPENDED_DIGSIG_REQUIRED) {

> +			else
> +				ima_read_appended_sig(buf, &size, &xattr_value,
> +						      &xattr_len);
> +		}
> +
> +		/*
> +		 * Don't try to read the xattr if we require an appended
> +		 * signature but failed to get one.
> +		 */

If the appended_sig is just a hint as to where the signature is
located, then we should read the xattr, even if IMA_DIGSIG_REQUIRED is
not specified.  ima_appraise_measurement() should be updated to
require a signature if either IMA_DIGSIG_REQUIRED or
IMA_APPENDED_SIGNATURE_REQUIRED are specified.  

Part of the confusion might be due to the naming
-"IMA_APPENEDED_SIGNATURE_REQUIRED".


> +		digsig_req = iint->flags & IMA_DIGSIG_REQUIRED_MASK;
> +		if (!xattr_len && digsig_req != IMA_APPENDED_DIGSIG_REQUIRED)
> +#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */

Is limiting the "if" to the ifdef really necessary? 

> +			/* read 'security.ima' */
> +			xattr_len = ima_read_xattr(file_dentry(file),
> +						   &xattr_value);
> +	}
> 

Suppose an appended signature and an xattr both exist (eg. kernel
modules), but for some reason the appended signature validation fails.
 The code should somehow retry the signature validation with the
xattr.

>  	hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
> 

Unfortunately, if the hash algorithm in the appended signature and the
xattr are not the same, then we would need to re-calculate the file
hash.

Mimi

Re: [PATCH 6/6] ima: Support appended signatures for appraisal

[Thiago Jung Bauermann]

Hello Mimi,

Thanks for your review.

Am Mittwoch, 26. April 2017, 07:21:19 BRT schrieb Mimi Zohar:
> On Tue, 2017-04-18 at 17:17 -0300, Thiago Jung Bauermann wrote:
> > This patch introduces the appended_imasig keyword to the IMA policy syntax
> > to specify that a given hook should expect the file to have the IMA
> > signature appended to it.  Here is how it can be used in a rule:
> > 
> > appraise func=KEXEC_KERNEL_CHECK appraise_type=appended_imasig
> > appraise func=KEXEC_KERNEL_CHECK appraise_type=appended_imasig|imasig
> > 
> > In the second form, IMA will accept either an appended signature or a
> > signature stored in the extended attribute. In that case, it will first
> > check whether there is an appended signature, and if not it will read it
> > from the extended attribute.
> 
> An appended signature should be another place to look for a signature,
> when a signature is required, but it shouldn't make a difference where
> the signature is located.  "imasig" could have implied to look for the
> signature in both places - xattr or appended.  So the new option is
> just a hint - a performance improvement.
> 
> This might seem picayune, but the difference between "expect" vs.
> "hint" impacts the code. (Further explanation inline.)

Thanks, I just learned a new word. :-)

If appended_imasig is just a performance improvement, is it really necessary? 
Perhaps if CONFIG_IMA_APPENDED_SIG is enabled then imasig would imply always 
first looking for an appended signature.

My first impression is that yes, it's necessary because pointlessly checking 
for an appended signature at the end of every file covered by FILE_CHECK could 
have a performance impact on the system. On the other hand, it's just a memcmp 
and that may be negligible compared to the cost of reading an xattr. II'll 
have to measure that...

> > diff --git a/security/integrity/ima/ima_main.c
> > b/security/integrity/ima/ima_main.c index 2aebb7984437..994ee420b2ec
> > 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -16,6 +16,9 @@
> > 
> >   *	implements the IMA hooks: ima_bprm_check, ima_file_mmap,
> >   *	and ima_file_check.
> >   */
> > 
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > 
> >  #include <linux/module.h>
> >  #include <linux/file.h>
> >  #include <linux/binfmts.h>
> > 
> > @@ -228,9 +231,30 @@ static int process_measurement(struct file *file,
> > char *buf, loff_t size,> 
> >  	template_desc = ima_template_desc_current();
> >  	if ((action & IMA_APPRAISE_SUBMASK) ||
> > 
> > -		    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0)
> > -		/* read 'security.ima' */
> > -		xattr_len = ima_read_xattr(file_dentry(file), &xattr_value);
> > +		    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) {
> > +#ifdef CONFIG_IMA_APPRAISE_APPENDED_SIG
> 
> Using "ifdef" in C code is really discouraged. Normally, it is an
> indication that the code needs to be re-factored.  Assuming we really
> need a new CONFIG option, which I'm not sure that we do, I would move
> the appended signature code to its own file, define stub functions in
> ima.h, and update the Makefile.

I didn't like the #ifdef here either and tried to find an alternative, but 
couldn't come up with something that was actually better. Perhaps it'll be 
easier with the changes needed in process_measurement to try reading the xattr 
if the appended sig fails.

I decided to add a new CONFIG option because otherwise CONFIG_IMA_APPRAISE 
would have to start depending on CONFIG_KEYS, CONFIG_PKCS7_MESSAGE_PARSER, 
CONFIG_X509_CERTIFICATE_PARSER and others. If you don't mind the extra 
dependencies, I can remove the CONFIG option.

> > +		unsigned long digsig_req;
> > +
> > +		if (iint->flags & IMA_APPENDED_DIGSIG_REQUIRED) {
> > +			if (!buf || !size)
> > +				pr_err("%s doesn't support appended_imasig\n",
> > +				       func_tokens[func]);
> 
> The policy parsing should prevent defining appended_imasig on
> inappropriate hooks.  Since the iint->flags might be shared between
> hooks, we might still need to test buf, but it could be simplified to:
> 
> if (!buf && iint->flags & IMA_APPENDED_DIGSIG_REQUIRED) {

That would require that me maintain a list of IMA hooks that use 
kernel_read_file and update it if any hook is converted to use it. I can do 
that if you think it's worth it (it is if it's unlikely or infrequent that 
hooks get converted to kernel_read_file).

I could also implement appended_imasig for hooks which need IMA to read the 
file and remove the restriction altogether. I'd have to check how hard that 
would be. Also not sure how useful it would be in practice to have those hooks 
supporting an appended signature.

> > +			else
> > +				ima_read_appended_sig(buf, &size, &xattr_value,
> > +						      &xattr_len);
> > +		}
> > +
> > +		/*
> > +		 * Don't try to read the xattr if we require an appended
> > +		 * signature but failed to get one.
> > +		 */
> 
> If the appended_sig is just a hint as to where the signature is
> located, then we should read the xattr, even if IMA_DIGSIG_REQUIRED is
> not specified.  ima_appraise_measurement() should be updated to
> require a signature if either IMA_DIGSIG_REQUIRED or
> IMA_APPENDED_SIGNATURE_REQUIRED are specified.  

Indeed.

> Part of the confusion might be due to the naming
> -"IMA_APPENEDED_SIGNATURE_REQUIRED".

Right, not a good name. I'll change that.

> > +		digsig_req = iint->flags & IMA_DIGSIG_REQUIRED_MASK;
> > +		if (!xattr_len && digsig_req != IMA_APPENDED_DIGSIG_REQUIRED)
> > +#endif /* CONFIG_IMA_APPRAISE_APPENDED_SIG */
> 
> Is limiting the "if" to the ifdef really necessary? 

Hopefully not after the changes to make process_measurement try one sig and 
then the other.

> >  	hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
> 
> Unfortunately, if the hash algorithm in the appended signature and the
> xattr are not the same, then we would need to re-calculate the file
> hash.

I think the most common case for the appended sig verification to fail will be 
that it won't be there than the signature actually failing.

Having to rehash the file is unfortunate but how frequently would that happen?

-- 
Thiago Jung Bauermann
IBM Linux Technology Center