diff mbox series

[v8,7/8] ima: check against blacklisted hashes for files with modsig

Message ID 1571508377-23603-8-git-send-email-nayna@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series powerpc: Enabling IMA arch specific secure boot policies | expand

Commit Message

Nayna Jain Oct. 19, 2019, 6:06 p.m. UTC
Asymmetric private keys are used to sign multiple files. The kernel
currently support checking against blacklisted keys. However, if the
public key is blacklisted, any file signed by the blacklisted key will
automatically fail signature verification. We might not want to blacklist
all the files signed by a particular key, but just a single file.
Blacklisting the public key is not fine enough granularity.

This patch adds support for checking against the blacklisted hash of the
file based on the IMA policy. The blacklisted hash is the file hash
without the appended signature. Defined is a new policy option
"appraise_flag=check_blacklist".

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 Documentation/ABI/testing/ima_policy  |  1 +
 security/integrity/ima/ima.h          |  8 +++++++
 security/integrity/ima/ima_appraise.c | 31 +++++++++++++++++++++++++++
 security/integrity/ima/ima_main.c     | 12 +++++++----
 security/integrity/ima/ima_policy.c   | 10 +++++++--
 security/integrity/integrity.h        |  1 +
 6 files changed, 57 insertions(+), 6 deletions(-)

Comments

Mimi Zohar Oct. 20, 2019, 12:58 a.m. UTC | #1
On Sat, 2019-10-19 at 14:06 -0400, Nayna Jain wrote:

> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index 29ebe9afdac4..4c97afcc0f3c 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -25,6 +25,7 @@ Description:
>  			lsm:	[[subj_user=] [subj_role=] [subj_type=]
>  				 [obj_user=] [obj_role=] [obj_type=]]
>  			option:	[[appraise_type=]] [template=] [permit_directio]
> +				[appraise_flag=[check_blacklist]]

Like the other options, only "[[appraise_flag=]]" should be defined
here.  The values should be defined in the "option:" section.

>  		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
>  				[FIRMWARE_CHECK]
> 

>  				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 136ae4e0ee92..7a002b08dde8 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c

> @@ -303,6 +304,36 @@ static int modsig_verify(enum ima_hooks func, const struct modsig *modsig,
>  	return rc;
>  }
>  
> +/*
> + * ima_blacklist_measurement - Checks whether the binary is blacklisted. If

Please update the function name to reflect the actual function name.

> + * yes, then adds the hash of the blacklisted binary to the measurement list.

Refer to Documentation/process/coding-style.rst section "8)
Commenting" on how to format function comments.  Don't start a
sentence with "If yes,".

> + *
> + * Returns -EPERM if the hash is blacklisted.
> + */
> +int ima_check_blacklist(struct integrity_iint_cache *iint,
> +			const struct modsig *modsig, int pcr)
> +{
> +	enum hash_algo hash_algo;

> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 5380aca2b351..bfaae7a8443a 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c

> @@ -1172,6 +1173,11 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>  			else
>  				result = -EINVAL;
>  			break;
> +		case Opt_appraise_flag:
> +			ima_log_string(ab, "appraise_flag", args[0].from);
> +			if (strstr(args[0].from, "blacklist"))
> +				entry->flags |= IMA_CHECK_BLACKLIST;
> +			break;

When adding a new policy rule option, ima_policy_show() needs to be
updated as well.

Mimi

>  		case Opt_permit_directio:
>  			entry->flags |= IMA_PERMIT_DIRECTIO;
>  			break;
>
Mimi Zohar Oct. 20, 2019, 4:06 p.m. UTC | #2
On Sat, 2019-10-19 at 14:06 -0400, Nayna Jain wrote:
> Asymmetric private keys are used to sign multiple files. The kernel
> currently support checking against blacklisted keys. However, if the
> public key is blacklisted, any file signed by the blacklisted key will
> automatically fail signature verification. We might not want to blacklist
> all the files signed by a particular key, but just a single file.
> Blacklisting the public key is not fine enough granularity.
> 
> This patch adds support for checking against the blacklisted hash of the
> file based on the IMA policy. The blacklisted hash is the file hash
> without the appended signature. Defined is a new policy option
> "appraise_flag=check_blacklist".

Please add an example of how to blacklist a file with an appended
signature.  The simplest example that works on x86 as well as Power
would be blacklisting a kernel module.  The example should include
calculating the kernel module hash without the appended signature,
enabling the Kconfig option (CONFIG_SYSTEM_BLACKLIST_HASH_LIST), and
the blacklist hash format (eg. "bin:<file hash>").

thanks, 

Mimi
Mimi Zohar Oct. 20, 2019, 4:09 p.m. UTC | #3
On Sun, 2019-10-20 at 12:06 -0400, Mimi Zohar wrote:
> On Sat, 2019-10-19 at 14:06 -0400, Nayna Jain wrote:
> > Asymmetric private keys are used to sign multiple files. The kernel
> > currently support checking against blacklisted keys. However, if the
> > public key is blacklisted, any file signed by the blacklisted key will
> > automatically fail signature verification. We might not want to blacklist
> > all the files signed by a particular key, but just a single file.
> > Blacklisting the public key is not fine enough granularity.
> > 
> > This patch adds support for checking against the blacklisted hash of the
> > file based on the IMA policy. The blacklisted hash is the file hash
> > without the appended signature. Defined is a new policy option
> > "appraise_flag=check_blacklist".
> 
> Please add an example of how to blacklist a file with an appended
> signature.  The simplest example that works on x86 as well as Power
> would be blacklisting a kernel module.  The example should include
> calculating the kernel module hash without the appended signature,
> enabling the Kconfig option (CONFIG_SYSTEM_BLACKLIST_HASH_LIST), and
> the blacklist hash format (eg. "bin:<file hash>").

And of course, the IMA appraise kernel module policy rule containing
"appraise_flag=check_blacklist".

thanks,

Mimi
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 29ebe9afdac4..4c97afcc0f3c 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -25,6 +25,7 @@  Description:
 			lsm:	[[subj_user=] [subj_role=] [subj_type=]
 				 [obj_user=] [obj_role=] [obj_type=]]
 			option:	[[appraise_type=]] [template=] [permit_directio]
+				[appraise_flag=[check_blacklist]]
 		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
 				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index a65772ffa427..df4ca482fb53 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -256,6 +256,8 @@  int ima_policy_show(struct seq_file *m, void *v);
 #define IMA_APPRAISE_KEXEC	0x40
 
 #ifdef CONFIG_IMA_APPRAISE
+int ima_check_blacklist(struct integrity_iint_cache *iint,
+			const struct modsig *modsig, int pcr);
 int ima_appraise_measurement(enum ima_hooks func,
 			     struct integrity_iint_cache *iint,
 			     struct file *file, const unsigned char *filename,
@@ -271,6 +273,12 @@  int ima_read_xattr(struct dentry *dentry,
 		   struct evm_ima_xattr_data **xattr_value);
 
 #else
+static inline int ima_check_blacklist(struct integrity_iint_cache *iint,
+				      const struct modsig *modsig, int pcr)
+{
+	return 0;
+}
+
 static inline int ima_appraise_measurement(enum ima_hooks func,
 					   struct integrity_iint_cache *iint,
 					   struct file *file,
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 136ae4e0ee92..7a002b08dde8 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -12,6 +12,7 @@ 
 #include <linux/magic.h>
 #include <linux/ima.h>
 #include <linux/evm.h>
+#include <keys/system_keyring.h>
 
 #include "ima.h"
 
@@ -303,6 +304,36 @@  static int modsig_verify(enum ima_hooks func, const struct modsig *modsig,
 	return rc;
 }
 
+/*
+ * ima_blacklist_measurement - Checks whether the binary is blacklisted. If
+ * yes, then adds the hash of the blacklisted binary to the measurement list.
+ *
+ * Returns -EPERM if the hash is blacklisted.
+ */
+int ima_check_blacklist(struct integrity_iint_cache *iint,
+			const struct modsig *modsig, int pcr)
+{
+	enum hash_algo hash_algo;
+	const u8 *digest = NULL;
+	u32 digestsize = 0;
+	int rc = 0;
+
+	if (!(iint->flags & IMA_CHECK_BLACKLIST))
+		return 0;
+
+	if (iint->flags & IMA_MODSIG_ALLOWED && modsig) {
+		ima_get_modsig_digest(modsig, &hash_algo, &digest, &digestsize);
+
+		rc = is_binary_blacklisted(digest, digestsize);
+		if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
+			process_buffer_measurement(digest, digestsize,
+						   "blacklisted-hash", NONE,
+						   pcr);
+	}
+
+	return rc;
+}
+
 /*
  * ima_appraise_measurement - appraise file measurement
  *
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index fe0b704ffdeb..13a0d64580ef 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -335,10 +335,14 @@  static int process_measurement(struct file *file, const struct cred *cred,
 				      xattr_value, xattr_len, modsig, pcr,
 				      template_desc);
 	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
-		inode_lock(inode);
-		rc = ima_appraise_measurement(func, iint, file, pathname,
-					      xattr_value, xattr_len, modsig);
-		inode_unlock(inode);
+		rc = ima_check_blacklist(iint, modsig, pcr);
+		if (rc != -EPERM) {
+			inode_lock(inode);
+			rc = ima_appraise_measurement(func, iint, file,
+						      pathname, xattr_value,
+						      xattr_len, modsig);
+			inode_unlock(inode);
+		}
 		if (!rc)
 			rc = mmap_violation_check(func, file, &pathbuf,
 						  &pathname, filename);
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 5380aca2b351..bfaae7a8443a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -765,8 +765,8 @@  enum {
 	Opt_fsuuid, Opt_uid_eq, Opt_euid_eq, Opt_fowner_eq,
 	Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
 	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
-	Opt_appraise_type, Opt_permit_directio,
-	Opt_pcr, Opt_template, Opt_err
+	Opt_appraise_type, Opt_appraise_flag,
+	Opt_permit_directio, Opt_pcr, Opt_template, Opt_err
 };
 
 static const match_table_t policy_tokens = {
@@ -798,6 +798,7 @@  static const match_table_t policy_tokens = {
 	{Opt_euid_lt, "euid<%s"},
 	{Opt_fowner_lt, "fowner<%s"},
 	{Opt_appraise_type, "appraise_type=%s"},
+	{Opt_appraise_flag, "appraise_flag=%s"},
 	{Opt_permit_directio, "permit_directio"},
 	{Opt_pcr, "pcr=%s"},
 	{Opt_template, "template=%s"},
@@ -1172,6 +1173,11 @@  static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			else
 				result = -EINVAL;
 			break;
+		case Opt_appraise_flag:
+			ima_log_string(ab, "appraise_flag", args[0].from);
+			if (strstr(args[0].from, "blacklist"))
+				entry->flags |= IMA_CHECK_BLACKLIST;
+			break;
 		case Opt_permit_directio:
 			entry->flags |= IMA_PERMIT_DIRECTIO;
 			break;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index d9323d31a3a8..73fc286834d7 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -32,6 +32,7 @@ 
 #define EVM_IMMUTABLE_DIGSIG	0x08000000
 #define IMA_FAIL_UNVERIFIABLE_SIGS	0x10000000
 #define IMA_MODSIG_ALLOWED	0x20000000
+#define IMA_CHECK_BLACKLIST	0x40000000
 
 #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
 				 IMA_HASH | IMA_APPRAISE_SUBMASK)