Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support
@ 2020-07-09  6:18 Tyler Hicks
  2020-07-09  6:19 ` [PATCH v3 01/12] ima: Have the LSM free its audit rule Tyler Hicks
                   ` (13 more replies)
  0 siblings, 14 replies; 28+ messages in thread
From: Tyler Hicks @ 2020-07-09  6:18 UTC (permalink / raw)
  To: Mimi Zohar, Dmitry Kasatkin
  Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module, Janne Karhunen, Eric Biederman, kexec,
	Casey Schaufler, Nayna Jain

This series ultimately extends the supported IMA rule conditionals for
the KEXEC_CMDLINE hook function. As of today, there's an imbalance in
IMA language conditional support for KEXEC_CMDLINE rules in comparison
to KEXEC_KERNEL_CHECK and KEXEC_INITRAMFS_CHECK rules. The KEXEC_CMDLINE
rules do not support *any* conditionals so you cannot have a sequence of
rules like this:

 dont_measure func=KEXEC_KERNEL_CHECK obj_type=foo_t
 dont_measure func=KEXEC_INITRAMFS_CHECK obj_type=foo_t
 dont_measure func=KEXEC_CMDLINE obj_type=foo_t
 measure func=KEXEC_KERNEL_CHECK
 measure func=KEXEC_INITRAMFS_CHECK
 measure func=KEXEC_CMDLINE

Instead, KEXEC_CMDLINE rules can only be measured or not measured and
there's no additional flexibility in today's implementation of the
KEXEC_CMDLINE hook function.

With this series, the above sequence of rules becomes valid and any
calls to kexec_file_load() with a kernel and initramfs inode type of
foo_t will not be measured (that includes the kernel cmdline buffer)
while all other objects given to a kexec_file_load() syscall will be
measured. There's obviously not an inode directly associated with the
kernel cmdline buffer but this patch series ties the inode based
decision making for KEXEC_CMDLINE to the kernel's inode. I think this
will be intuitive to policy authors.

While reading IMA code and preparing to make this change, I realized
that the buffer based hook functions (KEXEC_CMDLINE and KEY_CHECK) are
quite special in comparison to longer standing hook functions. These
buffer based hook functions can only support measure actions and there
are some restrictions on the conditionals that they support. However,
the rule parser isn't enforcing any of those restrictions and IMA policy
authors wouldn't have any immediate way of knowing that the policy that
they wrote is invalid. For example, the sequence of rules above parses
successfully in today's kernel but the
"dont_measure func=KEXEC_CMDLINE ..." rule is incorrectly handled in
ima_match_rules(). The dont_measure rule is *always* considered to be a
match so, surprisingly, no KEXEC_CMDLINE measurements are made.

While making the rule parser more strict, I realized that the parser
does not correctly free all of the allocated memory associated with an
ima_rule_entry when going down some error paths. Invalid policy loaded
by the policy administrator could result in small memory leaks.

I envision patches 1-7 going to stable. The series is ordered in a way
that has all the fixes up front, followed by cleanups, followed by the
feature patch. The breakdown of patches looks like so:

 Memory leak fixes: 1-3
 Parser strictness fixes: 4-7
 Code cleanups made possible by the fixes: 8-11
 Extend KEXEC_CMDLINE rule support: 12

Perhaps the most logical ordering for code review is:

 1, 2, 3, 8, 9, 4, 5, 6, 7, 10, 11, 12

If you'd like me to re-order or split up the series, just let me know.
Thanks for considering these patches!

* Series-wide v3 changes
  - Indentation changes in patch #4 which caused some churn
  - Added patch #7
  - Significant changes to patch #10 to address Mimi's requests
* Series-wide v2 changes
  - Rebased onto next-integrity-testing
  - Squashed patches 2 and 3 from v1
    + Updated this cover letter to account for changes to patch index
      changes
    + See patch 2 for specific code changes

Tyler

Tyler Hicks (12):
  ima: Have the LSM free its audit rule
  ima: Free the entire rule when deleting a list of rules
  ima: Free the entire rule if it fails to parse
  ima: Fail rule parsing when buffer hook functions have an invalid
    action
  ima: Fail rule parsing when the KEXEC_CMDLINE hook is combined with an
    invalid cond
  ima: Fail rule parsing when the KEY_CHECK hook is combined with an
    invalid cond
  ima: Fail rule parsing when appraise_flag=blacklist is unsupportable
  ima: Shallow copy the args_p member of ima_rule_entry.lsm elements
  ima: Use correct type for the args_p member of ima_rule_entry.lsm
    elements
  ima: Move comprehensive rule validation checks out of the token parser
  ima: Use the common function to detect LSM conditionals in a rule
  ima: Support additional conditionals in the KEXEC_CMDLINE hook
    function

 include/linux/ima.h                          |   4 +-
 kernel/kexec_file.c                          |   2 +-
 security/integrity/ima/ima.h                 |  13 +-
 security/integrity/ima/ima_api.c             |   2 +-
 security/integrity/ima/ima_appraise.c        |   2 +-
 security/integrity/ima/ima_asymmetric_keys.c |   2 +-
 security/integrity/ima/ima_main.c            |  23 ++-
 security/integrity/ima/ima_modsig.c          |  20 --
 security/integrity/ima/ima_policy.c          | 206 ++++++++++++++-----
 security/integrity/ima/ima_queue_keys.c      |   2 +-
 10 files changed, 182 insertions(+), 94 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 01/12] ima: Have the LSM free its audit rule
  2020-07-09  6:18 [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Tyler Hicks
@ 2020-07-09  6:19 ` Tyler Hicks
  2020-07-17 19:20   ` Nayna
  2020-07-09  6:19 ` [PATCH v3 02/12] ima: Free the entire rule when deleting a list of rules Tyler Hicks
                   ` (12 subsequent siblings)
  13 siblings, 1 reply; 28+ messages in thread
From: Tyler Hicks @ 2020-07-09  6:19 UTC (permalink / raw)
  To: Mimi Zohar, Dmitry Kasatkin
  Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module, Janne Karhunen, Casey Schaufler

Ask the LSM to free its audit rule rather than directly calling kfree().
Both AppArmor and SELinux do additional work in their audit_rule_free()
hooks. Fix memory leaks by allowing the LSMs to perform necessary work.

Fixes: b16942455193 ("ima: use the lsm policy update notifier")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Cc: Janne Karhunen <janne.karhunen@gmail.com>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---

* v3
  - No change
* v2
  - Fixed build warning by dropping the 'return -EINVAL' from
    the stubbed out security_filter_rule_free() since it has a void
    return type
  - Added Mimi's Reviewed-by
  - Developed a follow-on patch to rename security_filter_rule_*()
    functions, to address Casey's request, but I'll submit it
    independently of this patch series since it is somewhat unrelated

 security/integrity/ima/ima.h        | 5 +++++
 security/integrity/ima/ima_policy.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 4515975cc540..59ec28f5c117 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -420,6 +420,7 @@ static inline void ima_free_modsig(struct modsig *modsig)
 #ifdef CONFIG_IMA_LSM_RULES
 
 #define security_filter_rule_init security_audit_rule_init
+#define security_filter_rule_free security_audit_rule_free
 #define security_filter_rule_match security_audit_rule_match
 
 #else
@@ -430,6 +431,10 @@ static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr,
 	return -EINVAL;
 }
 
+static inline void security_filter_rule_free(void *lsmrule)
+{
+}
+
 static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
 					     void *lsmrule)
 {
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 66aa3e17a888..d7c268c2b0ce 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -258,7 +258,7 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry)
 	int i;
 
 	for (i = 0; i < MAX_LSM_RULES; i++) {
-		kfree(entry->lsm[i].rule);
+		security_filter_rule_free(entry->lsm[i].rule);
 		kfree(entry->lsm[i].args_p);
 	}
 	kfree(entry);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 02/12] ima: Free the entire rule when deleting a list of rules
  2020-07-09  6:18 [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Tyler Hicks
  2020-07-09  6:19 ` [PATCH v3 01/12] ima: Have the LSM free its audit rule Tyler Hicks
@ 2020-07-09  6:19 ` Tyler Hicks
  2020-07-09  6:19 ` [PATCH v3 03/12] ima: Free the entire rule if it fails to parse Tyler Hicks
                   ` (11 subsequent siblings)
  13 siblings, 0 replies; 28+ messages in thread
From: Tyler Hicks @ 2020-07-09  6:19 UTC (permalink / raw)
  To: Mimi Zohar, Dmitry Kasatkin
  Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module

Create a function, ima_free_rule(), to free all memory associated with
an ima_rule_entry. Use the new function to fix memory leaks of allocated
ima_rule_entry members, such as .fsname and .keyrings, when deleting a
list of rules.

Make the existing ima_lsm_free_rule() function specific to the LSM
audit rule array of an ima_rule_entry and require that callers make an
additional call to kfree to free the ima_rule_entry itself.

This fixes a memory leak seen when loading by a valid rule that contains
an additional piece of allocated memory, such as an fsname, followed by
an invalid rule that triggers a policy load failure:

 # echo -e "dont_measure fsname=securityfs\nbad syntax" > \
    /sys/kernel/security/ima/policy
 -bash: echo: write error: Invalid argument
 # echo scan > /sys/kernel/debug/kmemleak
 # cat /sys/kernel/debug/kmemleak
 unreferenced object 0xffff9bab67ca12c0 (size 16):
   comm "bash", pid 684, jiffies 4295212803 (age 252.344s)
   hex dump (first 16 bytes):
     73 65 63 75 72 69 74 79 66 73 00 6b 6b 6b 6b a5  securityfs.kkkk.
   backtrace:
     [<00000000adc80b1b>] kstrdup+0x2e/0x60
     [<00000000d504cb0d>] ima_parse_add_rule+0x7d4/0x1020
     [<00000000444825ac>] ima_write_policy+0xab/0x1d0
     [<000000002b7f0d6c>] vfs_write+0xde/0x1d0
     [<0000000096feedcf>] ksys_write+0x68/0xe0
     [<0000000052b544a2>] do_syscall_64+0x56/0xa0
     [<000000007ead1ba7>] entry_SYSCALL_64_after_hwframe+0x44/0xa9

Fixes: f1b08bbcbdaf ("ima: define a new policy condition based on the filesystem name")
Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---

* v3
  - No change
* v2
  - Collapsed patch #2 from v1 of this series, into this patch. This
    patch now introduces ima_free_rule().
  - Existing callers of ima_lsm_free_rule() are doing so to free rules
    after a successful or failed ima_lsm_copy_rule() and those callers
    continue to directly call ima_lsm_copy_rule() rather than doing
    explicit reference ownership and calling ima_free_rule().
  - The kfree(entry) of ima_lsm_free_rule() was removed from that
    function to make it focused on freeing the LSM references. Direct
    callers of ima_lsm_free_rule() must now call kfree(entry) after
    ima_lsm_free_rule().
  - A comment was added in ima_lsm_update_rule() to clarify why
    ima_free_rule() isn't being used.

 security/integrity/ima/ima_policy.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index d7c268c2b0ce..bf00b966e87f 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -261,6 +261,21 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry)
 		security_filter_rule_free(entry->lsm[i].rule);
 		kfree(entry->lsm[i].args_p);
 	}
+}
+
+static void ima_free_rule(struct ima_rule_entry *entry)
+{
+	if (!entry)
+		return;
+
+	/*
+	 * entry->template->fields may be allocated in ima_parse_rule() but that
+	 * reference is owned by the corresponding ima_template_desc element in
+	 * the defined_templates list and cannot be freed here
+	 */
+	kfree(entry->fsname);
+	kfree(entry->keyrings);
+	ima_lsm_free_rule(entry);
 	kfree(entry);
 }
 
@@ -302,6 +317,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
 
 out_err:
 	ima_lsm_free_rule(nentry);
+	kfree(nentry);
 	return NULL;
 }
 
@@ -315,7 +331,14 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry)
 
 	list_replace_rcu(&entry->list, &nentry->list);
 	synchronize_rcu();
+	/*
+	 * ima_lsm_copy_rule() shallow copied all references, except for the
+	 * LSM references, from entry to nentry so we only want to free the LSM
+	 * references and the entry itself. All other memory refrences will now
+	 * be owned by nentry.
+	 */
 	ima_lsm_free_rule(entry);
+	kfree(entry);
 
 	return 0;
 }
@@ -1402,15 +1425,11 @@ ssize_t ima_parse_add_rule(char *rule)
 void ima_delete_rules(void)
 {
 	struct ima_rule_entry *entry, *tmp;
-	int i;
 
 	temp_ima_appraise = 0;
 	list_for_each_entry_safe(entry, tmp, &ima_temp_rules, list) {
-		for (i = 0; i < MAX_LSM_RULES; i++)
-			kfree(entry->lsm[i].args_p);
-
 		list_del(&entry->list);
-		kfree(entry);
+		ima_free_rule(entry);
 	}
 }
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 03/12] ima: Free the entire rule if it fails to parse
  2020-07-09  6:18 [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Tyler Hicks
  2020-07-09  6:19 ` [PATCH v3 01/12] ima: Have the LSM free its audit rule Tyler Hicks
  2020-07-09  6:19 ` [PATCH v3 02/12] ima: Free the entire rule when deleting a list of rules Tyler Hicks
@ 2020-07-09  6:19 ` Tyler Hicks
  2020-07-09  6:19 ` [PATCH v3 04/12] ima: Fail rule parsing when buffer hook functions have an invalid action Tyler Hicks
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 28+ messages in thread
From: Tyler Hicks @ 2020-07-09  6:19 UTC (permalink / raw)
  To: Mimi Zohar, Dmitry Kasatkin
  Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module

Use ima_free_rule() to fix memory leaks of allocated ima_rule_entry
members, such as .fsname and .keyrings, when an error is encountered
during rule parsing.

Set the args_p pointer to NULL after freeing it in the error path of
ima_lsm_rule_init() so that it isn't freed twice.

This fixes a memory leak seen when loading an rule that contains an
additional piece of allocated memory, such as an fsname, followed by an
invalid conditional:

 # echo "measure fsname=tmpfs bad=cond" > /sys/kernel/security/ima/policy
 -bash: echo: write error: Invalid argument
 # echo scan > /sys/kernel/debug/kmemleak
 # cat /sys/kernel/debug/kmemleak
 unreferenced object 0xffff98e7e4ece6c0 (size 8):
   comm "bash", pid 672, jiffies 4294791843 (age 21.855s)
   hex dump (first 8 bytes):
     74 6d 70 66 73 00 6b a5                          tmpfs.k.
   backtrace:
     [<00000000abab7413>] kstrdup+0x2e/0x60
     [<00000000f11ede32>] ima_parse_add_rule+0x7d4/0x1020
     [<00000000f883dd7a>] ima_write_policy+0xab/0x1d0
     [<00000000b17cf753>] vfs_write+0xde/0x1d0
     [<00000000b8ddfdea>] ksys_write+0x68/0xe0
     [<00000000b8e21e87>] do_syscall_64+0x56/0xa0
     [<0000000089ea7b98>] entry_SYSCALL_64_after_hwframe+0x44/0xa9

Fixes: f1b08bbcbdaf ("ima: define a new policy condition based on the filesystem name")
Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---

* v3
  - No change
* v2
  - No change

 security/integrity/ima/ima_policy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index bf00b966e87f..e458cd47c099 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -913,6 +913,7 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
 
 		if (ima_rules == &ima_default_rules) {
 			kfree(entry->lsm[lsm_rule].args_p);
+			entry->lsm[lsm_rule].args_p = NULL;
 			result = -EINVAL;
 		} else
 			result = 0;
@@ -1404,7 +1405,7 @@ ssize_t ima_parse_add_rule(char *rule)
 
 	result = ima_parse_rule(p, entry);
 	if (result) {
-		kfree(entry);
+		ima_free_rule(entry);
 		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
 				    NULL, op, "invalid-policy", result,
 				    audit_info);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 04/12] ima: Fail rule parsing when buffer hook functions have an invalid action
  2020-07-09  6:18 [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Tyler Hicks
                   ` (2 preceding siblings ...)
  2020-07-09  6:19 ` [PATCH v3 03/12] ima: Free the entire rule if it fails to parse Tyler Hicks
@ 2020-07-09  6:19 ` Tyler Hicks
  2020-07-09  6:19 ` [PATCH v3 05/12] ima: Fail rule parsing when the KEXEC_CMDLINE hook is combined with an invalid cond Tyler Hicks
                   ` (9 subsequent siblings)
  13 siblings, 0 replies; 28+ messages in thread
From: Tyler Hicks @ 2020-07-09  6:19 UTC (permalink / raw)
  To: Mimi Zohar, Dmitry Kasatkin
  Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module

Buffer based hook functions, such as KEXEC_CMDLINE and KEY_CHECK, can
only measure. The process_buffer_measurement() function quietly ignores
all actions except measure so make this behavior clear at the time of
policy load.

The parsing of the keyrings conditional had a check to ensure that it
was only specified with measure actions but the check should be on the
hook function and not the keyrings conditional since
"appraise func=KEY_CHECK" is not a valid rule.

Fixes: b0935123a183 ("IMA: Define a new hook to measure the kexec boot command line arguments")
Fixes: 5808611cccb2 ("IMA: Add KEY_CHECK func to measure keys")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---

* v3
  - Add comments to ima_validate_rule() to separate/explain the types of
    validation checks (section for action checks, section for hook
    function checks, soon to be a section for combination of options
    checks, etc.)
  - Removed the "if (entry->flags & IMA_FUNC)" conditional around the
    switch statement in ima_validate_rule() which reduced the overall indention
    by a tab. This could be removed because entry->func is NONE when the
    IMA_FUNC flag is not set. We'll explicitly enforce and then leverage
    that property in a later patch when we start validating all hook
    functions in ima_validate_rule().
  - Add comment explicitly stating that all hook functions except
    KEXEC_CMDLINE and KEY_CHECK are still being validated in
    ima_parse_rule().
* v2
  - No change

 security/integrity/ima/ima_policy.c | 40 +++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e458cd47c099..40c28f1a6a5a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -973,6 +973,43 @@ static void check_template_modsig(const struct ima_template_desc *template)
 #undef MSG
 }
 
+static bool ima_validate_rule(struct ima_rule_entry *entry)
+{
+	/* Ensure that the action is set */
+	if (entry->action == UNKNOWN)
+		return false;
+
+	/*
+	 * Ensure that the hook function is compatible with the other
+	 * components of the rule
+	 */
+	switch (entry->func) {
+	case NONE:
+	case FILE_CHECK:
+	case MMAP_CHECK:
+	case BPRM_CHECK:
+	case CREDS_CHECK:
+	case POST_SETATTR:
+	case MODULE_CHECK:
+	case FIRMWARE_CHECK:
+	case KEXEC_KERNEL_CHECK:
+	case KEXEC_INITRAMFS_CHECK:
+	case POLICY_CHECK:
+		/* Validation of these hook functions is in ima_parse_rule() */
+		break;
+	case KEXEC_CMDLINE:
+	case KEY_CHECK:
+		if (entry->action & ~(MEASURE | DONT_MEASURE))
+			return false;
+
+		break;
+	default:
+		return false;
+	}
+
+	return true;
+}
+
 static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 {
 	struct audit_buffer *ab;
@@ -1150,7 +1187,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			keyrings_len = strlen(args[0].from) + 1;
 
 			if ((entry->keyrings) ||
-			    (entry->action != MEASURE) ||
 			    (entry->func != KEY_CHECK) ||
 			    (keyrings_len < 2)) {
 				result = -EINVAL;
@@ -1356,7 +1392,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			break;
 		}
 	}
-	if (!result && (entry->action == UNKNOWN))
+	if (!result && !ima_validate_rule(entry))
 		result = -EINVAL;
 	else if (entry->action == APPRAISE)
 		temp_ima_appraise |= ima_appraise_flag(entry->func);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 05/12] ima: Fail rule parsing when the KEXEC_CMDLINE hook is combined with an invalid cond
  2020-07-09  6:18 [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Tyler Hicks
                   ` (3 preceding siblings ...)
  2020-07-09  6:19 ` [PATCH v3 04/12] ima: Fail rule parsing when buffer hook functions have an invalid action Tyler Hicks
@ 2020-07-09  6:19 ` Tyler Hicks
  2020-07-09  6:19 ` [PATCH v3 06/12] ima: Fail rule parsing when the KEY_CHECK " Tyler Hicks
                   ` (8 subsequent siblings)
  13 siblings, 0 replies; 28+ messages in thread
From: Tyler Hicks @ 2020-07-09  6:19 UTC (permalink / raw)
  To: Mimi Zohar, Dmitry Kasatkin
  Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module

The KEXEC_CMDLINE hook function only supports the pcr conditional. Make
this clear at policy load so that IMA policy authors don't assume that
other conditionals are supported.

Since KEXEC_CMDLINE's inception, ima_match_rules() has always returned
true on any loaded KEXEC_CMDLINE rule without any consideration for
other conditionals present in the rule. Make it clear that pcr is the
only supported KEXEC_CMDLINE conditional by returning an error during
policy load.

An example of why this is a problem can be explained with the following
rule:

 dont_measure func=KEXEC_CMDLINE obj_type=foo_t

An IMA policy author would have assumed that rule is valid because the
parser accepted it but the result was that measurements for all
KEXEC_CMDLINE operations would be disabled.

Fixes: b0935123a183 ("IMA: Define a new hook to measure the kexec boot command line arguments")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---

* v3
  - Adjust for the indentation change introduced in patch #4
  - Added Lakshmi's Reviewed-by
* v2
  - Added Mimi's Reviewed-by

 security/integrity/ima/ima_policy.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 40c28f1a6a5a..1c64bd6f1728 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -343,6 +343,17 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry)
 	return 0;
 }
 
+static bool ima_rule_contains_lsm_cond(struct ima_rule_entry *entry)
+{
+	int i;
+
+	for (i = 0; i < MAX_LSM_RULES; i++)
+		if (entry->lsm[i].args_p)
+			return true;
+
+	return false;
+}
+
 /*
  * The LSM policy can be reloaded, leaving the IMA LSM based rules referring
  * to the old, stale LSM policy.  Update the IMA LSM based rules to reflect
@@ -998,6 +1009,16 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
 		/* Validation of these hook functions is in ima_parse_rule() */
 		break;
 	case KEXEC_CMDLINE:
+		if (entry->action & ~(MEASURE | DONT_MEASURE))
+			return false;
+
+		if (entry->flags & ~(IMA_FUNC | IMA_PCR))
+			return false;
+
+		if (ima_rule_contains_lsm_cond(entry))
+			return false;
+
+		break;
 	case KEY_CHECK:
 		if (entry->action & ~(MEASURE | DONT_MEASURE))
 			return false;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 06/12] ima: Fail rule parsing when the KEY_CHECK hook is combined with an invalid cond
  2020-07-09  6:18 [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Tyler Hicks
                   ` (4 preceding siblings ...)
  2020-07-09  6:19 ` [PATCH v3 05/12] ima: Fail rule parsing when the KEXEC_CMDLINE hook is combined with an invalid cond Tyler Hicks
@ 2020-07-09  6:19 ` Tyler Hicks
  2020-07-17 18:56   ` Nayna
  2020-07-09  6:19 ` [PATCH v3 07/12] ima: Fail rule parsing when appraise_flag=blacklist is unsupportable Tyler Hicks
                   ` (7 subsequent siblings)
  13 siblings, 1 reply; 28+ messages in thread
From: Tyler Hicks @ 2020-07-09  6:19 UTC (permalink / raw)
  To: Mimi Zohar, Dmitry Kasatkin
  Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module

The KEY_CHECK function only supports the uid, pcr, and keyrings
conditionals. Make this clear at policy load so that IMA policy authors
don't assume that other conditionals are supported.

Fixes: 5808611cccb2 ("IMA: Add KEY_CHECK func to measure keys")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---

* v3
  - Added Lakshmi's Reviewed-by
  - Adjust for the indentation change introduced in patch #4
* v2
  - No change

 security/integrity/ima/ima_policy.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 1c64bd6f1728..81da02071d41 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1023,6 +1023,13 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
 		if (entry->action & ~(MEASURE | DONT_MEASURE))
 			return false;
 
+		if (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
+				     IMA_KEYRINGS))
+			return false;
+
+		if (ima_rule_contains_lsm_cond(entry))
+			return false;
+
 		break;
 	default:
 		return false;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 07/12] ima: Fail rule parsing when appraise_flag=blacklist is unsupportable
  2020-07-09  6:18 [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Tyler Hicks
                   ` (5 preceding siblings ...)
  2020-07-09  6:19 ` [PATCH v3 06/12] ima: Fail rule parsing when the KEY_CHECK " Tyler Hicks
@ 2020-07-09  6:19 ` Tyler Hicks
  2020-07-16 18:14   ` Mimi Zohar
       [not found]   ` <76d2b27b-3b59-1852-046a-b1718c62b167@linux.vnet.ibm.com>
  2020-07-09  6:19 ` [PATCH v3 08/12] ima: Shallow copy the args_p member of ima_rule_entry.lsm elements Tyler Hicks
                   ` (6 subsequent siblings)
  13 siblings, 2 replies; 28+ messages in thread
From: Tyler Hicks @ 2020-07-09  6:19 UTC (permalink / raw)
  To: Mimi Zohar, Dmitry Kasatkin
  Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module, Nayna Jain

The "appraise_flag" option is only appropriate for appraise actions
and its "blacklist" value is only appropriate when
CONFIG_IMA_APPRAISE_MODSIG is enabled and "appraise_flag=blacklist" is
only appropriate when "appraise_type=imasig|modsig" is also present.
Make this clear at policy load so that IMA policy authors don't assume
that other uses of "appraise_flag=blacklist" are supported.

Fixes: 273df864cf74 ("ima: Check against blacklisted hashes for files with modsig")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Cc: Nayna Jain <nayna@linux.ibm.com>
---

* v3
  - New patch

 security/integrity/ima/ima_policy.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 81da02071d41..9842e2e0bc6d 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1035,6 +1035,11 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
 		return false;
 	}
 
+	/* Ensure that combinations of flags are compatible with each other */
+	if (entry->flags & IMA_CHECK_BLACKLIST &&
+	    !(entry->flags & IMA_MODSIG_ALLOWED))
+		return false;
+
 	return true;
 }
 
@@ -1371,8 +1376,14 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 				result = -EINVAL;
 			break;
 		case Opt_appraise_flag:
+			if (entry->action != APPRAISE) {
+				result = -EINVAL;
+				break;
+			}
+
 			ima_log_string(ab, "appraise_flag", args[0].from);
-			if (strstr(args[0].from, "blacklist"))
+			if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
+			    strstr(args[0].from, "blacklist"))
 				entry->flags |= IMA_CHECK_BLACKLIST;
 			break;
 		case Opt_permit_directio:
-- 
2.25.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 08/12] ima: Shallow copy the args_p member of ima_rule_entry.lsm elements
  2020-07-09  6:18 [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Tyler Hicks
                   ` (6 preceding siblings ...)
  2020-07-09  6:19 ` [PATCH v3 07/12] ima: Fail rule parsing when appraise_flag=blacklist is unsupportable Tyler Hicks
@ 2020-07-09  6:19 ` Tyler Hicks
  2020-07-17 15:35   ` Konsta Karsisto
  2020-07-09  6:19 ` [PATCH v3 09/12] ima: Use correct type for " Tyler Hicks
                   ` (5 subsequent siblings)
  13 siblings, 1 reply; 28+ messages in thread
From: Tyler Hicks @ 2020-07-09  6:19 UTC (permalink / raw)
  To: Mimi Zohar, Dmitry Kasatkin
  Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module

The args_p member is a simple string that is allocated by
ima_rule_init(). Shallow copy it like other non-LSM references in
ima_rule_entry structs.

There are no longer any necessary error path cleanups to do in
ima_lsm_copy_rule().

Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---

* v3
  - No change
* v2
  - Adjusted context to account for ima_lsm_copy_rule() directly calling
    ima_lsm_free_rule() and the lack of explicit reference ownership
    transfers
  - Added comment to ima_lsm_copy_rule() to document the args_p
    reference ownership transfer

 security/integrity/ima/ima_policy.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 9842e2e0bc6d..b02e1ffd10c9 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -300,10 +300,13 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
 			continue;
 
 		nentry->lsm[i].type = entry->lsm[i].type;
-		nentry->lsm[i].args_p = kstrdup(entry->lsm[i].args_p,
-						GFP_KERNEL);
-		if (!nentry->lsm[i].args_p)
-			goto out_err;
+		nentry->lsm[i].args_p = entry->lsm[i].args_p;
+		/*
+		 * Remove the reference from entry so that the associated
+		 * memory will not be freed during a later call to
+		 * ima_lsm_free_rule(entry).
+		 */
+		entry->lsm[i].args_p = NULL;
 
 		security_filter_rule_init(nentry->lsm[i].type,
 					  Audit_equal,
@@ -314,11 +317,6 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
 				(char *)entry->lsm[i].args_p);
 	}
 	return nentry;
-
-out_err:
-	ima_lsm_free_rule(nentry);
-	kfree(nentry);
-	return NULL;
 }
 
 static int ima_lsm_update_rule(struct ima_rule_entry *entry)
-- 
2.25.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 09/12] ima: Use correct type for the args_p member of ima_rule_entry.lsm elements
  2020-07-09  6:18 [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Tyler Hicks
                   ` (7 preceding siblings ...)
  2020-07-09  6:19 ` [PATCH v3 08/12] ima: Shallow copy the args_p member of ima_rule_entry.lsm elements Tyler Hicks
@ 2020-07-09  6:19 ` Tyler Hicks
  2020-07-09  6:19 ` [PATCH v3 10/12] ima: Move comprehensive rule validation checks out of the token parser Tyler Hicks
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 28+ messages in thread
From: Tyler Hicks @ 2020-07-09  6:19 UTC (permalink / raw)
  To: Mimi Zohar, Dmitry Kasatkin
  Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module

Make args_p be of the char pointer type rather than have it be a void
pointer that gets casted to char pointer when it is used. It is a simple
NUL-terminated string as returned by match_strdup().

Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---

* v3
  - No change
* v2
  - No change

 security/integrity/ima/ima_policy.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index b02e1ffd10c9..13a178c70b44 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -74,7 +74,7 @@ struct ima_rule_entry {
 	int pcr;
 	struct {
 		void *rule;	/* LSM file metadata specific */
-		void *args_p;	/* audit value */
+		char *args_p;	/* audit value */
 		int type;	/* audit type */
 	} lsm[MAX_LSM_RULES];
 	char *fsname;
@@ -314,7 +314,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
 					  &nentry->lsm[i].rule);
 		if (!nentry->lsm[i].rule)
 			pr_warn("rule for LSM \'%s\' is undefined\n",
-				(char *)entry->lsm[i].args_p);
+				entry->lsm[i].args_p);
 	}
 	return nentry;
 }
@@ -918,7 +918,7 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
 					   &entry->lsm[lsm_rule].rule);
 	if (!entry->lsm[lsm_rule].rule) {
 		pr_warn("rule for LSM \'%s\' is undefined\n",
-			(char *)entry->lsm[lsm_rule].args_p);
+			entry->lsm[lsm_rule].args_p);
 
 		if (ima_rules == &ima_default_rules) {
 			kfree(entry->lsm[lsm_rule].args_p);
@@ -1682,27 +1682,27 @@ int ima_policy_show(struct seq_file *m, void *v)
 			switch (i) {
 			case LSM_OBJ_USER:
 				seq_printf(m, pt(Opt_obj_user),
-					   (char *)entry->lsm[i].args_p);
+					   entry->lsm[i].args_p);
 				break;
 			case LSM_OBJ_ROLE:
 				seq_printf(m, pt(Opt_obj_role),
-					   (char *)entry->lsm[i].args_p);
+					   entry->lsm[i].args_p);
 				break;
 			case LSM_OBJ_TYPE:
 				seq_printf(m, pt(Opt_obj_type),
-					   (char *)entry->lsm[i].args_p);
+					   entry->lsm[i].args_p);
 				break;
 			case LSM_SUBJ_USER:
 				seq_printf(m, pt(Opt_subj_user),
-					   (char *)entry->lsm[i].args_p);
+					   entry->lsm[i].args_p);
 				break;
 			case LSM_SUBJ_ROLE:
 				seq_printf(m, pt(Opt_subj_role),
-					   (char *)entry->lsm[i].args_p);
+					   entry->lsm[i].args_p);
 				break;
 			case LSM_SUBJ_TYPE:
 				seq_printf(m, pt(Opt_subj_type),
-					   (char *)entry->lsm[i].args_p);
+					   entry->lsm[i].args_p);
 				break;
 			}
 			seq_puts(m, " ");
-- 
2.25.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 10/12] ima: Move comprehensive rule validation checks out of the token parser
  2020-07-09  6:18 [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Tyler Hicks
                   ` (8 preceding siblings ...)
  2020-07-09  6:19 ` [PATCH v3 09/12] ima: Use correct type for " Tyler Hicks
@ 2020-07-09  6:19 ` Tyler Hicks
  2020-07-09  6:19 ` [PATCH v3 11/12] ima: Use the common function to detect LSM conditionals in a rule Tyler Hicks
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 28+ messages in thread
From: Tyler Hicks @ 2020-07-09  6:19 UTC (permalink / raw)
  To: Mimi Zohar, Dmitry Kasatkin
  Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module

Use ima_validate_rule(), at the end of the token parsing stage, to
verify combinations of actions, hooks, and flags. This is useful to
increase readability by consolidating such checks into a single function
and also because rule conditionals can be specified in arbitrary order
making it difficult to do comprehensive rule validation until the entire
rule has been parsed.

This allows for the check that ties together the "keyrings" conditional
with the KEY_CHECK function hook to be moved into the final rule
validation.

The modsig check no longer needs to compiled conditionally because the
token parser will ensure that modsig support is enabled before accepting
"imasig|modsig" appraise type values. The final rule validation will
ensure that appraise_type and appraise_flag options are only present in
appraise rules.

Finally, this allows for the check that ties together the "pcr"
conditional with the measure action to be moved into the final rule
validation.

Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---

* v3
  - Significant broadening of the patch's scope along with renaming and
    re-describing the patch. ima_validate_rule() is now the consolidated
    location for checking combinations of
    actions/functions/conditionals and the existing checks are now
    removed from the token parsing code.
    + Ensure that the IMA_FUNC flag is only set when a function hook is
      specified, and vice versa, which allows us to use the NONE case in
      the switch statement to enforce that "keyrings=",
      "appraise_type=imasig|modsig", and "appraise_flag=blacklist"
      cannot be specified on a rule without an appropriate hook function
  - Adjust for the indentation change introduced in patch #4
  - Adjust for new fixes introduced in patch #7
* v2
  - Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO,
    IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be
    present in the rule entry flags for non-buffer hook functions.

 security/integrity/ima/ima.h        |  6 ---
 security/integrity/ima/ima_modsig.c | 20 ----------
 security/integrity/ima/ima_policy.c | 57 +++++++++++++++++++----------
 3 files changed, 37 insertions(+), 46 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 59ec28f5c117..ea7e77536f3c 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -372,7 +372,6 @@ static inline int ima_read_xattr(struct dentry *dentry,
 #endif /* CONFIG_IMA_APPRAISE */
 
 #ifdef CONFIG_IMA_APPRAISE_MODSIG
-bool ima_hook_supports_modsig(enum ima_hooks func);
 int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
 		    struct modsig **modsig);
 void ima_collect_modsig(struct modsig *modsig, const void *buf, loff_t size);
@@ -382,11 +381,6 @@ int ima_get_raw_modsig(const struct modsig *modsig, const void **data,
 		       u32 *data_len);
 void ima_free_modsig(struct modsig *modsig);
 #else
-static inline bool ima_hook_supports_modsig(enum ima_hooks func)
-{
-	return false;
-}
-
 static inline int ima_read_modsig(enum ima_hooks func, const void *buf,
 				  loff_t buf_len, struct modsig **modsig)
 {
diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
index d106885cc495..fb25723c65bc 100644
--- a/security/integrity/ima/ima_modsig.c
+++ b/security/integrity/ima/ima_modsig.c
@@ -32,26 +32,6 @@ struct modsig {
 	u8 raw_pkcs7[];
 };
 
-/**
- * ima_hook_supports_modsig - can the policy allow modsig for this hook?
- *
- * modsig is only supported by hooks using ima_post_read_file(), because only
- * they preload the contents of the file in a buffer. FILE_CHECK does that in
- * some cases, but not when reached from vfs_open(). POLICY_CHECK can support
- * it, but it's not useful in practice because it's a text file so deny.
- */
-bool ima_hook_supports_modsig(enum ima_hooks func)
-{
-	switch (func) {
-	case KEXEC_KERNEL_CHECK:
-	case KEXEC_INITRAMFS_CHECK:
-	case MODULE_CHECK:
-		return true;
-	default:
-		return false;
-	}
-}
-
 /*
  * ima_read_modsig - Read modsig from buf.
  *
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 13a178c70b44..c4d0a0c1f896 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -984,10 +984,27 @@ static void check_template_modsig(const struct ima_template_desc *template)
 
 static bool ima_validate_rule(struct ima_rule_entry *entry)
 {
-	/* Ensure that the action is set */
+	/* Ensure that the action is set and is compatible with the flags */
 	if (entry->action == UNKNOWN)
 		return false;
 
+	if (entry->action != MEASURE && entry->flags & IMA_PCR)
+		return false;
+
+	if (entry->action != APPRAISE &&
+	    entry->flags & (IMA_DIGSIG_REQUIRED | IMA_MODSIG_ALLOWED | IMA_CHECK_BLACKLIST))
+		return false;
+
+	/*
+	 * The IMA_FUNC bit must be set if and only if there's a valid hook
+	 * function specified, and vice versa. Enforcing this property allows
+	 * for the NONE case below to validate a rule without an explicit hook
+	 * function.
+	 */
+	if (((entry->flags & IMA_FUNC) && entry->func == NONE) ||
+	    (!(entry->flags & IMA_FUNC) && entry->func != NONE))
+		return false;
+
 	/*
 	 * Ensure that the hook function is compatible with the other
 	 * components of the rule
@@ -999,12 +1016,27 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
 	case BPRM_CHECK:
 	case CREDS_CHECK:
 	case POST_SETATTR:
-	case MODULE_CHECK:
 	case FIRMWARE_CHECK:
+	case POLICY_CHECK:
+		if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC |
+				     IMA_UID | IMA_FOWNER | IMA_FSUUID |
+				     IMA_INMASK | IMA_EUID | IMA_PCR |
+				     IMA_FSNAME | IMA_DIGSIG_REQUIRED |
+				     IMA_PERMIT_DIRECTIO))
+			return false;
+
+		break;
+	case MODULE_CHECK:
 	case KEXEC_KERNEL_CHECK:
 	case KEXEC_INITRAMFS_CHECK:
-	case POLICY_CHECK:
-		/* Validation of these hook functions is in ima_parse_rule() */
+		if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC |
+				     IMA_UID | IMA_FOWNER | IMA_FSUUID |
+				     IMA_INMASK | IMA_EUID | IMA_PCR |
+				     IMA_FSNAME | IMA_DIGSIG_REQUIRED |
+				     IMA_PERMIT_DIRECTIO | IMA_MODSIG_ALLOWED |
+				     IMA_CHECK_BLACKLIST))
+			return false;
+
 		break;
 	case KEXEC_CMDLINE:
 		if (entry->action & ~(MEASURE | DONT_MEASURE))
@@ -1218,7 +1250,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			keyrings_len = strlen(args[0].from) + 1;
 
 			if ((entry->keyrings) ||
-			    (entry->func != KEY_CHECK) ||
 			    (keyrings_len < 2)) {
 				result = -EINVAL;
 				break;
@@ -1358,15 +1389,10 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 						   AUDIT_SUBJ_TYPE);
 			break;
 		case Opt_appraise_type:
-			if (entry->action != APPRAISE) {
-				result = -EINVAL;
-				break;
-			}
-
 			ima_log_string(ab, "appraise_type", args[0].from);
 			if ((strcmp(args[0].from, "imasig")) == 0)
 				entry->flags |= IMA_DIGSIG_REQUIRED;
-			else if (ima_hook_supports_modsig(entry->func) &&
+			else if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
 				 strcmp(args[0].from, "imasig|modsig") == 0)
 				entry->flags |= IMA_DIGSIG_REQUIRED |
 						IMA_MODSIG_ALLOWED;
@@ -1374,11 +1400,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 				result = -EINVAL;
 			break;
 		case Opt_appraise_flag:
-			if (entry->action != APPRAISE) {
-				result = -EINVAL;
-				break;
-			}
-
 			ima_log_string(ab, "appraise_flag", args[0].from);
 			if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
 			    strstr(args[0].from, "blacklist"))
@@ -1388,10 +1409,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			entry->flags |= IMA_PERMIT_DIRECTIO;
 			break;
 		case Opt_pcr:
-			if (entry->action != MEASURE) {
-				result = -EINVAL;
-				break;
-			}
 			ima_log_string(ab, "pcr", args[0].from);
 
 			result = kstrtoint(args[0].from, 10, &entry->pcr);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 11/12] ima: Use the common function to detect LSM conditionals in a rule
  2020-07-09  6:18 [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Tyler Hicks
                   ` (9 preceding siblings ...)
  2020-07-09  6:19 ` [PATCH v3 10/12] ima: Move comprehensive rule validation checks out of the token parser Tyler Hicks
@ 2020-07-09  6:19 ` Tyler Hicks
  2020-07-09  6:19 ` [PATCH v3 12/12] ima: Support additional conditionals in the KEXEC_CMDLINE hook function Tyler Hicks
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 28+ messages in thread
From: Tyler Hicks @ 2020-07-09  6:19 UTC (permalink / raw)
  To: Mimi Zohar, Dmitry Kasatkin
  Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module

Make broader use of ima_rule_contains_lsm_cond() to check if a given
rule contains an LSM conditional. This is a code cleanup and has no
user-facing change.

Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---

* v3
  - No change
* v2
  - No change

 security/integrity/ima/ima_policy.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index c4d0a0c1f896..81ee8fd1d83a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -360,17 +360,10 @@ static bool ima_rule_contains_lsm_cond(struct ima_rule_entry *entry)
 static void ima_lsm_update_rules(void)
 {
 	struct ima_rule_entry *entry, *e;
-	int i, result, needs_update;
+	int result;
 
 	list_for_each_entry_safe(entry, e, &ima_policy_rules, list) {
-		needs_update = 0;
-		for (i = 0; i < MAX_LSM_RULES; i++) {
-			if (entry->lsm[i].args_p) {
-				needs_update = 1;
-				break;
-			}
-		}
-		if (!needs_update)
+		if (!ima_rule_contains_lsm_cond(entry))
 			continue;
 
 		result = ima_lsm_update_rule(entry);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 12/12] ima: Support additional conditionals in the KEXEC_CMDLINE hook function
  2020-07-09  6:18 [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Tyler Hicks
                   ` (10 preceding siblings ...)
  2020-07-09  6:19 ` [PATCH v3 11/12] ima: Use the common function to detect LSM conditionals in a rule Tyler Hicks
@ 2020-07-09  6:19 ` Tyler Hicks
  2020-07-17  4:31 ` [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Mimi Zohar
  2020-07-20 21:38 ` Mimi Zohar
  13 siblings, 0 replies; 28+ messages in thread
From: Tyler Hicks @ 2020-07-09  6:19 UTC (permalink / raw)
  To: Mimi Zohar, Dmitry Kasatkin
  Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module, Eric Biederman, kexec

Take the properties of the kexec kernel's inode and the current task
ownership into consideration when matching a KEXEC_CMDLINE operation to
the rules in the IMA policy. This allows for some uniformity when
writing IMA policy rules for KEXEC_KERNEL_CHECK, KEXEC_INITRAMFS_CHECK,
and KEXEC_CMDLINE operations.

Prior to this patch, it was not possible to write a set of rules like
this:

 dont_measure func=KEXEC_KERNEL_CHECK obj_type=foo_t
 dont_measure func=KEXEC_INITRAMFS_CHECK obj_type=foo_t
 dont_measure func=KEXEC_CMDLINE obj_type=foo_t
 measure func=KEXEC_KERNEL_CHECK
 measure func=KEXEC_INITRAMFS_CHECK
 measure func=KEXEC_CMDLINE

The inode information associated with the kernel being loaded by a
kexec_kernel_load(2) syscall can now be included in the decision to
measure or not

Additonally, the uid, euid, and subj_* conditionals can also now be
used in KEXEC_CMDLINE rules. There was no technical reason as to why
those conditionals weren't being considered previously other than
ima_match_rules() didn't have a valid inode to use so it immediately
bailed out for KEXEC_CMDLINE operations rather than going through the
full list of conditional comparisons.

Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: kexec@lists.infradead.org
Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---

* v3
  - Added Lakshmi's Reviewed-by
  - Adjust for the indentation change introduced in patch #4
* v2
  - Moved the inode parameter of process_buffer_measurement() to be the
    first parameter so that it more closely matches process_masurement()

 include/linux/ima.h                          |  4 ++--
 kernel/kexec_file.c                          |  2 +-
 security/integrity/ima/ima.h                 |  2 +-
 security/integrity/ima/ima_api.c             |  2 +-
 security/integrity/ima/ima_appraise.c        |  2 +-
 security/integrity/ima/ima_asymmetric_keys.c |  2 +-
 security/integrity/ima/ima_main.c            | 23 +++++++++++++++-----
 security/integrity/ima/ima_policy.c          | 17 +++++----------
 security/integrity/ima/ima_queue_keys.c      |  2 +-
 9 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 9164e1534ec9..d15100de6cdd 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -25,7 +25,7 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
 			      enum kernel_read_file_id id);
 extern void ima_post_path_mknod(struct dentry *dentry);
 extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
-extern void ima_kexec_cmdline(const void *buf, int size);
+extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
 
 #ifdef CONFIG_IMA_KEXEC
 extern void ima_add_kexec_buffer(struct kimage *image);
@@ -103,7 +103,7 @@ static inline int ima_file_hash(struct file *file, char *buf, size_t buf_size)
 	return -EOPNOTSUPP;
 }
 
-static inline void ima_kexec_cmdline(const void *buf, int size) {}
+static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {}
 #endif /* CONFIG_IMA */
 
 #ifndef CONFIG_IMA_KEXEC
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index bb05fd52de85..07df431c1f21 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -287,7 +287,7 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 			goto out;
 		}
 
-		ima_kexec_cmdline(image->cmdline_buf,
+		ima_kexec_cmdline(kernel_fd, image->cmdline_buf,
 				  image->cmdline_buf_len - 1);
 	}
 
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index ea7e77536f3c..576ae2c6d418 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -265,7 +265,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
 			   struct evm_ima_xattr_data *xattr_value,
 			   int xattr_len, const struct modsig *modsig, int pcr,
 			   struct ima_template_desc *template_desc);
-void process_buffer_measurement(const void *buf, int size,
+void process_buffer_measurement(struct inode *inode, const void *buf, int size,
 				const char *eventname, enum ima_hooks func,
 				int pcr, const char *keyring);
 void ima_audit_measurement(struct integrity_iint_cache *iint,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index bf22de8b7ce0..4f39fb93f278 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -162,7 +162,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
 
 /**
  * ima_get_action - appraise & measure decision based on policy.
- * @inode: pointer to inode to measure
+ * @inode: pointer to the inode associated with the object being validated
  * @cred: pointer to credentials structure to validate
  * @secid: secid of the task being validated
  * @mask: contains the permission mask (MAY_READ, MAY_WRITE, MAY_EXEC,
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index a9649b04b9f1..6c52bf7ea7f0 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -328,7 +328,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
 
 		rc = is_binary_blacklisted(digest, digestsize);
 		if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
-			process_buffer_measurement(digest, digestsize,
+			process_buffer_measurement(NULL, digest, digestsize,
 						   "blacklisted-hash", NONE,
 						   pcr, NULL);
 	}
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index aaae80c4e376..1c68c500c26f 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -58,7 +58,7 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
 	 * if the IMA policy is configured to measure a key linked
 	 * to the given keyring.
 	 */
-	process_buffer_measurement(payload, payload_len,
+	process_buffer_measurement(NULL, payload, payload_len,
 				   keyring->description, KEY_CHECK, 0,
 				   keyring->description);
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8351b2fd48e0..8a91711ca79b 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -726,6 +726,7 @@ int ima_load_data(enum kernel_load_data_id id)
 
 /*
  * process_buffer_measurement - Measure the buffer to ima log.
+ * @inode: inode associated with the object being measured (NULL for KEY_CHECK)
  * @buf: pointer to the buffer that needs to be added to the log.
  * @size: size of buffer(in bytes).
  * @eventname: event name to be used for the buffer entry.
@@ -735,7 +736,7 @@ int ima_load_data(enum kernel_load_data_id id)
  *
  * Based on policy, the buffer is measured into the ima log.
  */
-void process_buffer_measurement(const void *buf, int size,
+void process_buffer_measurement(struct inode *inode, const void *buf, int size,
 				const char *eventname, enum ima_hooks func,
 				int pcr, const char *keyring)
 {
@@ -768,7 +769,7 @@ void process_buffer_measurement(const void *buf, int size,
 	 */
 	if (func) {
 		security_task_getsecid(current, &secid);
-		action = ima_get_action(NULL, current_cred(), secid, 0, func,
+		action = ima_get_action(inode, current_cred(), secid, 0, func,
 					&pcr, &template, keyring);
 		if (!(action & IMA_MEASURE))
 			return;
@@ -823,16 +824,26 @@ void process_buffer_measurement(const void *buf, int size,
 
 /**
  * ima_kexec_cmdline - measure kexec cmdline boot args
+ * @kernel_fd: file descriptor of the kexec kernel being loaded
  * @buf: pointer to buffer
  * @size: size of buffer
  *
  * Buffers can only be measured, not appraised.
  */
-void ima_kexec_cmdline(const void *buf, int size)
+void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
 {
-	if (buf && size != 0)
-		process_buffer_measurement(buf, size, "kexec-cmdline",
-					   KEXEC_CMDLINE, 0, NULL);
+	struct fd f;
+
+	if (!buf || !size)
+		return;
+
+	f = fdget(kernel_fd);
+	if (!f.file)
+		return;
+
+	process_buffer_measurement(file_inode(f.file), buf, size,
+				   "kexec-cmdline", KEXEC_CMDLINE, 0, NULL);
+	fdput(f);
 }
 
 static int __init init_ima(void)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 81ee8fd1d83a..2e87154c9296 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -443,13 +443,9 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 {
 	int i;
 
-	if ((func == KEXEC_CMDLINE) || (func == KEY_CHECK)) {
-		if ((rule->flags & IMA_FUNC) && (rule->func == func)) {
-			if (func == KEY_CHECK)
-				return ima_match_keyring(rule, keyring, cred);
-			return true;
-		}
-		return false;
+	if (func == KEY_CHECK) {
+		return (rule->flags & IMA_FUNC) && (rule->func == func) &&
+		       ima_match_keyring(rule, keyring, cred);
 	}
 	if ((rule->flags & IMA_FUNC) &&
 	    (rule->func != func && func != POST_SETATTR))
@@ -1035,10 +1031,9 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
 		if (entry->action & ~(MEASURE | DONT_MEASURE))
 			return false;
 
-		if (entry->flags & ~(IMA_FUNC | IMA_PCR))
-			return false;
-
-		if (ima_rule_contains_lsm_cond(entry))
+		if (entry->flags & ~(IMA_FUNC | IMA_FSMAGIC | IMA_UID |
+				     IMA_FOWNER | IMA_FSUUID | IMA_EUID |
+				     IMA_PCR | IMA_FSNAME))
 			return false;
 
 		break;
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index 56ce24a18b66..69a8626a35c0 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -158,7 +158,7 @@ void ima_process_queued_keys(void)
 
 	list_for_each_entry_safe(entry, tmp, &ima_keys, list) {
 		if (!timer_expired)
-			process_buffer_measurement(entry->payload,
+			process_buffer_measurement(NULL, entry->payload,
 						   entry->payload_len,
 						   entry->keyring_name,
 						   KEY_CHECK, 0,
-- 
2.25.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 07/12] ima: Fail rule parsing when appraise_flag=blacklist is unsupportable
  2020-07-09  6:19 ` [PATCH v3 07/12] ima: Fail rule parsing when appraise_flag=blacklist is unsupportable Tyler Hicks
@ 2020-07-16 18:14   ` Mimi Zohar
  2020-07-16 18:20     ` Tyler Hicks
       [not found]   ` <76d2b27b-3b59-1852-046a-b1718c62b167@linux.vnet.ibm.com>
  1 sibling, 1 reply; 28+ messages in thread
From: Mimi Zohar @ 2020-07-16 18:14 UTC (permalink / raw)
  To: Tyler Hicks, Dmitry Kasatkin
  Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module, Nayna Jain

On Thu, 2020-07-09 at 01:19 -0500, Tyler Hicks wrote:
> The "appraise_flag" option is only appropriate for appraise actions
> and its "blacklist" value is only appropriate when
> CONFIG_IMA_APPRAISE_MODSIG is enabled and "appraise_flag=blacklist" is
> only appropriate when "appraise_type=imasig|modsig" is also present.
> Make this clear at policy load so that IMA policy authors don't assume
> that other uses of "appraise_flag=blacklist" are supported.

The code looks correct, but this patch description could be written at
a higher level.  Perhaps it just needs to be prefixed with something
like this:

Verifying that a file hash is not blacklisted is currently only
supported for files with appended signatures (modsig).  In the future,
this might change.  For now, ...

Mimi

> 
> Fixes: 273df864cf74 ("ima: Check against blacklisted hashes for files with modsig")
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> Cc: Nayna Jain <nayna@linux.ibm.com>

> ---
> 
> * v3
>   - New patch
> 
>  security/integrity/ima/ima_policy.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 81da02071d41..9842e2e0bc6d 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1035,6 +1035,11 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>  		return false;
>  	}
>  
> +	/* Ensure that combinations of flags are compatible with each other */
> +	if (entry->flags & IMA_CHECK_BLACKLIST &&
> +	    !(entry->flags & IMA_MODSIG_ALLOWED))
> +		return false;
> +
>  	return true;
>  }
>  
> @@ -1371,8 +1376,14 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>  				result = -EINVAL;
>  			break;
>  		case Opt_appraise_flag:
> +			if (entry->action != APPRAISE) {
> +				result = -EINVAL;
> +				break;
> +			}
> +
>  			ima_log_string(ab, "appraise_flag", args[0].from);
> -			if (strstr(args[0].from, "blacklist"))
> +			if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
> +			    strstr(args[0].from, "blacklist"))
>  				entry->flags |= IMA_CHECK_BLACKLIST;
>  			break;
>  		case Opt_permit_directio:


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 07/12] ima: Fail rule parsing when appraise_flag=blacklist is unsupportable
  2020-07-16 18:14   ` Mimi Zohar
@ 2020-07-16 18:20     ` Tyler Hicks
  0 siblings, 0 replies; 28+ messages in thread
From: Tyler Hicks @ 2020-07-16 18:20 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Dmitry Kasatkin, James Morris, Serge E . Hallyn,
	Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
	linux-integrity, linux-security-module, Nayna Jain

On 2020-07-16 14:14:50, Mimi Zohar wrote:
> On Thu, 2020-07-09 at 01:19 -0500, Tyler Hicks wrote:
> > The "appraise_flag" option is only appropriate for appraise actions
> > and its "blacklist" value is only appropriate when
> > CONFIG_IMA_APPRAISE_MODSIG is enabled and "appraise_flag=blacklist" is
> > only appropriate when "appraise_type=imasig|modsig" is also present.
> > Make this clear at policy load so that IMA policy authors don't assume
> > that other uses of "appraise_flag=blacklist" are supported.
> 
> The code looks correct, but this patch description could be written at
> a higher level.  Perhaps it just needs to be prefixed with something
> like this:
> 
> Verifying that a file hash is not blacklisted is currently only
> supported for files with appended signatures (modsig).  In the future,
> this might change.  For now, ...

That makes sense. I'm not up to speed on the intent behind the blacklist
feature or where it may go in the future so I didn't think to add
anything along those lines.

If you are happy with the rest of the series, please feel free to append
this to the commit message. Otherwise, I can add it if I need to submit
a new revision of the series.

Tyler

> 
> Mimi
> 
> > 
> > Fixes: 273df864cf74 ("ima: Check against blacklisted hashes for files with modsig")
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > Cc: Nayna Jain <nayna@linux.ibm.com>
> 
> > ---
> > 
> > * v3
> >   - New patch
> > 
> >  security/integrity/ima/ima_policy.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 81da02071d41..9842e2e0bc6d 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -1035,6 +1035,11 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> >  		return false;
> >  	}
> >  
> > +	/* Ensure that combinations of flags are compatible with each other */
> > +	if (entry->flags & IMA_CHECK_BLACKLIST &&
> > +	    !(entry->flags & IMA_MODSIG_ALLOWED))
> > +		return false;
> > +
> >  	return true;
> >  }
> >  
> > @@ -1371,8 +1376,14 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> >  				result = -EINVAL;
> >  			break;
> >  		case Opt_appraise_flag:
> > +			if (entry->action != APPRAISE) {
> > +				result = -EINVAL;
> > +				break;
> > +			}
> > +
> >  			ima_log_string(ab, "appraise_flag", args[0].from);
> > -			if (strstr(args[0].from, "blacklist"))
> > +			if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
> > +			    strstr(args[0].from, "blacklist"))
> >  				entry->flags |= IMA_CHECK_BLACKLIST;
> >  			break;
> >  		case Opt_permit_directio:

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support
  2020-07-09  6:18 [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Tyler Hicks
                   ` (11 preceding siblings ...)
  2020-07-09  6:19 ` [PATCH v3 12/12] ima: Support additional conditionals in the KEXEC_CMDLINE hook function Tyler Hicks
@ 2020-07-17  4:31 ` Mimi Zohar
  2020-07-17  4:34   ` Tyler Hicks
  2020-07-20 21:38 ` Mimi Zohar
  13 siblings, 1 reply; 28+ messages in thread
From: Mimi Zohar @ 2020-07-17  4:31 UTC (permalink / raw)
  To: Tyler Hicks, Dmitry Kasatkin
  Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module, Janne Karhunen, Eric Biederman, kexec,
	Casey Schaufler, Nayna Jain

On Thu, 2020-07-09 at 01:18 -0500, Tyler Hicks wrote:
> This series ultimately extends the supported IMA rule conditionals for
> the KEXEC_CMDLINE hook function. As of today, there's an imbalance in
> IMA language conditional support for KEXEC_CMDLINE rules in comparison
> to KEXEC_KERNEL_CHECK and KEXEC_INITRAMFS_CHECK rules. The KEXEC_CMDLINE
> rules do not support *any* conditionals so you cannot have a sequence of
> rules like this:
> 
>  dont_measure func=KEXEC_KERNEL_CHECK obj_type=foo_t
>  dont_measure func=KEXEC_INITRAMFS_CHECK obj_type=foo_t
>  dont_measure func=KEXEC_CMDLINE obj_type=foo_t
>  measure func=KEXEC_KERNEL_CHECK
>  measure func=KEXEC_INITRAMFS_CHECK
>  measure func=KEXEC_CMDLINE
> 
> Instead, KEXEC_CMDLINE rules can only be measured or not measured and
> there's no additional flexibility in today's implementation of the
> KEXEC_CMDLINE hook function.
> 
> With this series, the above sequence of rules becomes valid and any
> calls to kexec_file_load() with a kernel and initramfs inode type of
> foo_t will not be measured (that includes the kernel cmdline buffer)
> while all other objects given to a kexec_file_load() syscall will be
> measured. There's obviously not an inode directly associated with the
> kernel cmdline buffer but this patch series ties the inode based
> decision making for KEXEC_CMDLINE to the kernel's inode. I think this
> will be intuitive to policy authors.
> 
> While reading IMA code and preparing to make this change, I realized
> that the buffer based hook functions (KEXEC_CMDLINE and KEY_CHECK) are
> quite special in comparison to longer standing hook functions. These
> buffer based hook functions can only support measure actions and there
> are some restrictions on the conditionals that they support. However,
> the rule parser isn't enforcing any of those restrictions and IMA policy
> authors wouldn't have any immediate way of knowing that the policy that
> they wrote is invalid. For example, the sequence of rules above parses
> successfully in today's kernel but the
> "dont_measure func=KEXEC_CMDLINE ..." rule is incorrectly handled in
> ima_match_rules(). The dont_measure rule is *always* considered to be a
> match so, surprisingly, no KEXEC_CMDLINE measurements are made.
> 
> While making the rule parser more strict, I realized that the parser
> does not correctly free all of the allocated memory associated with an
> ima_rule_entry when going down some error paths. Invalid policy loaded
> by the policy administrator could result in small memory leaks.
> 
> I envision patches 1-7 going to stable. The series is ordered in a way
> that has all the fixes up front, followed by cleanups, followed by the
> feature patch. The breakdown of patches looks like so:
> 
>  Memory leak fixes: 1-3
>  Parser strictness fixes: 4-7
>  Code cleanups made possible by the fixes: 8-11
>  Extend KEXEC_CMDLINE rule support: 12

Thanks, Tyler.  This is a really nice patch set.  The patches are now
in the "next-integrity-testing" branch.

Mimi

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support
  2020-07-17  4:31 ` [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Mimi Zohar
@ 2020-07-17  4:34   ` Tyler Hicks
  0 siblings, 0 replies; 28+ messages in thread
From: Tyler Hicks @ 2020-07-17  4:34 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Dmitry Kasatkin, James Morris, Serge E . Hallyn,
	Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
	linux-integrity, linux-security-module, Janne Karhunen,
	Eric Biederman, kexec, Casey Schaufler, Nayna Jain

On 2020-07-17 00:31:33, Mimi Zohar wrote:
> On Thu, 2020-07-09 at 01:18 -0500, Tyler Hicks wrote:
> > This series ultimately extends the supported IMA rule conditionals for
> > the KEXEC_CMDLINE hook function. As of today, there's an imbalance in
> > IMA language conditional support for KEXEC_CMDLINE rules in comparison
> > to KEXEC_KERNEL_CHECK and KEXEC_INITRAMFS_CHECK rules. The KEXEC_CMDLINE
> > rules do not support *any* conditionals so you cannot have a sequence of
> > rules like this:
> > 
> >  dont_measure func=KEXEC_KERNEL_CHECK obj_type=foo_t
> >  dont_measure func=KEXEC_INITRAMFS_CHECK obj_type=foo_t
> >  dont_measure func=KEXEC_CMDLINE obj_type=foo_t
> >  measure func=KEXEC_KERNEL_CHECK
> >  measure func=KEXEC_INITRAMFS_CHECK
> >  measure func=KEXEC_CMDLINE
> > 
> > Instead, KEXEC_CMDLINE rules can only be measured or not measured and
> > there's no additional flexibility in today's implementation of the
> > KEXEC_CMDLINE hook function.
> > 
> > With this series, the above sequence of rules becomes valid and any
> > calls to kexec_file_load() with a kernel and initramfs inode type of
> > foo_t will not be measured (that includes the kernel cmdline buffer)
> > while all other objects given to a kexec_file_load() syscall will be
> > measured. There's obviously not an inode directly associated with the
> > kernel cmdline buffer but this patch series ties the inode based
> > decision making for KEXEC_CMDLINE to the kernel's inode. I think this
> > will be intuitive to policy authors.
> > 
> > While reading IMA code and preparing to make this change, I realized
> > that the buffer based hook functions (KEXEC_CMDLINE and KEY_CHECK) are
> > quite special in comparison to longer standing hook functions. These
> > buffer based hook functions can only support measure actions and there
> > are some restrictions on the conditionals that they support. However,
> > the rule parser isn't enforcing any of those restrictions and IMA policy
> > authors wouldn't have any immediate way of knowing that the policy that
> > they wrote is invalid. For example, the sequence of rules above parses
> > successfully in today's kernel but the
> > "dont_measure func=KEXEC_CMDLINE ..." rule is incorrectly handled in
> > ima_match_rules(). The dont_measure rule is *always* considered to be a
> > match so, surprisingly, no KEXEC_CMDLINE measurements are made.
> > 
> > While making the rule parser more strict, I realized that the parser
> > does not correctly free all of the allocated memory associated with an
> > ima_rule_entry when going down some error paths. Invalid policy loaded
> > by the policy administrator could result in small memory leaks.
> > 
> > I envision patches 1-7 going to stable. The series is ordered in a way
> > that has all the fixes up front, followed by cleanups, followed by the
> > feature patch. The breakdown of patches looks like so:
> > 
> >  Memory leak fixes: 1-3
> >  Parser strictness fixes: 4-7
> >  Code cleanups made possible by the fixes: 8-11
> >  Extend KEXEC_CMDLINE rule support: 12
> 
> Thanks, Tyler.  This is a really nice patch set.  The patches are now
> in the "next-integrity-testing" branch.

Thank you for all the helpful review comments. You know where to find me
if any bugs pop up during testing. :)

Tyler

> 
> Mimi

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 08/12] ima: Shallow copy the args_p member of ima_rule_entry.lsm elements
  2020-07-09  6:19 ` [PATCH v3 08/12] ima: Shallow copy the args_p member of ima_rule_entry.lsm elements Tyler Hicks
@ 2020-07-17 15:35   ` Konsta Karsisto
  2020-07-17 16:51     ` Tyler Hicks
  0 siblings, 1 reply; 28+ messages in thread
From: Konsta Karsisto @ 2020-07-17 15:35 UTC (permalink / raw)
  To: Tyler Hicks, linux-integrity
  Cc: Mimi Zohar, Dmitry Kasatkin, James Morris, Serge E . Hallyn,
	Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
	linux-security-module

Hi,

Found one glitch with this change, see below:

On Thu, Jul 9, 2020 at 9:22 AM Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
>
> The args_p member is a simple string that is allocated by
> ima_rule_init(). Shallow copy it like other non-LSM references in
> ima_rule_entry structs.
>
> There are no longer any necessary error path cleanups to do in
> ima_lsm_copy_rule().
>
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> ---
>
> * v3
>   - No change
> * v2
>   - Adjusted context to account for ima_lsm_copy_rule() directly calling
>     ima_lsm_free_rule() and the lack of explicit reference ownership
>     transfers
>   - Added comment to ima_lsm_copy_rule() to document the args_p
>     reference ownership transfer
>
>  security/integrity/ima/ima_policy.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 9842e2e0bc6d..b02e1ffd10c9 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -300,10 +300,13 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
>                         continue;
>
>                 nentry->lsm[i].type = entry->lsm[i].type;
> -               nentry->lsm[i].args_p = kstrdup(entry->lsm[i].args_p,
> -                                               GFP_KERNEL);
> -               if (!nentry->lsm[i].args_p)
> -                       goto out_err;
> +               nentry->lsm[i].args_p = entry->lsm[i].args_p;
> +               /*
> +                * Remove the reference from entry so that the associated
> +                * memory will not be freed during a later call to
> +                * ima_lsm_free_rule(entry).
> +                */
> +               entry->lsm[i].args_p = NULL;

This assignment necessitates a change in the code below...

>                 security_filter_rule_init(nentry->lsm[i].type,
>                                           Audit_equal,
> @@ -314,11 +317,6 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
>                                 (char *)entry->lsm[i].args_p);

... you should refer to nentry->lsm[i].args_p here!

Other than that,

Reviewed-by: Konsta Karsisto <konsta.karsisto@gmail.com>


Konsta

>         }
>         return nentry;
> -
> -out_err:
> -       ima_lsm_free_rule(nentry);
> -       kfree(nentry);
> -       return NULL;
>  }
>
>  static int ima_lsm_update_rule(struct ima_rule_entry *entry)
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 08/12] ima: Shallow copy the args_p member of ima_rule_entry.lsm elements
  2020-07-17 15:35   ` Konsta Karsisto
@ 2020-07-17 16:51     ` Tyler Hicks
  0 siblings, 0 replies; 28+ messages in thread
From: Tyler Hicks @ 2020-07-17 16:51 UTC (permalink / raw)
  To: Konsta Karsisto, Mimi Zohar
  Cc: linux-integrity, Dmitry Kasatkin, James Morris, Serge E . Hallyn,
	Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
	linux-security-module

On 2020-07-17 18:35:03, Konsta Karsisto wrote:
> Hi,
> 
> Found one glitch with this change, see below:
> 
> On Thu, Jul 9, 2020 at 9:22 AM Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> >
> > The args_p member is a simple string that is allocated by
> > ima_rule_init(). Shallow copy it like other non-LSM references in
> > ima_rule_entry structs.
> >
> > There are no longer any necessary error path cleanups to do in
> > ima_lsm_copy_rule().
> >
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > ---
> >
> > * v3
> >   - No change
> > * v2
> >   - Adjusted context to account for ima_lsm_copy_rule() directly calling
> >     ima_lsm_free_rule() and the lack of explicit reference ownership
> >     transfers
> >   - Added comment to ima_lsm_copy_rule() to document the args_p
> >     reference ownership transfer
> >
> >  security/integrity/ima/ima_policy.c | 16 +++++++---------
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 9842e2e0bc6d..b02e1ffd10c9 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -300,10 +300,13 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
> >                         continue;
> >
> >                 nentry->lsm[i].type = entry->lsm[i].type;
> > -               nentry->lsm[i].args_p = kstrdup(entry->lsm[i].args_p,
> > -                                               GFP_KERNEL);
> > -               if (!nentry->lsm[i].args_p)
> > -                       goto out_err;
> > +               nentry->lsm[i].args_p = entry->lsm[i].args_p;
> > +               /*
> > +                * Remove the reference from entry so that the associated
> > +                * memory will not be freed during a later call to
> > +                * ima_lsm_free_rule(entry).
> > +                */
> > +               entry->lsm[i].args_p = NULL;
> 
> This assignment necessitates a change in the code below...
> 
> >                 security_filter_rule_init(nentry->lsm[i].type,
> >                                           Audit_equal,
> > @@ -314,11 +317,6 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
> >                                 (char *)entry->lsm[i].args_p);
> 
> ... you should refer to nentry->lsm[i].args_p here!
> 
> Other than that,
> 
> Reviewed-by: Konsta Karsisto <konsta.karsisto@gmail.com>

Thank you, Konsta. You're exactly right about the required change.
Without it, the pr_warn() in the error path will always attempt to print
a NULL pointer.

Mimi, the following change (along with adding Konsta's Reviewed-by)
needs to be made to this patch in next-integrity-testing:

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index b02e1ffd10c9..330a4e216349 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -314,7 +314,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
 					  &nentry->lsm[i].rule);
 		if (!nentry->lsm[i].rule)
 			pr_warn("rule for LSM \'%s\' is undefined\n",
-				(char *)entry->lsm[i].args_p);
+				(char *)nentry->lsm[i].args_p);
 	}
 	return nentry;
 }

It will then cause the next patch in the series to not apply but the
fixup is minor.

Let me know if you are alright with doing these changes yourself or if
you'd like me to submit a new series revision.

If you do this rebase work in next-integrity-testing yourself, I've
prepared a copy branch of next-integrity-testing with all of the
necessary changes for you to compare against:

 https://git.kernel.org/pub/scm/linux/kernel/git/tyhicks/linux.git/log/?h=next-integrity-testing-fixup

My apologies for the trouble.

Tyler

> 
> 
> Konsta
> 
> >         }
> >         return nentry;
> > -
> > -out_err:
> > -       ima_lsm_free_rule(nentry);
> > -       kfree(nentry);
> > -       return NULL;
> >  }
> >
> >  static int ima_lsm_update_rule(struct ima_rule_entry *entry)
> > --
> > 2.25.1
> >

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 07/12] ima: Fail rule parsing when appraise_flag=blacklist is unsupportable
       [not found]   ` <76d2b27b-3b59-1852-046a-b1718c62b167@linux.vnet.ibm.com>
@ 2020-07-17 18:11     ` Tyler Hicks
  2020-07-20 17:02       ` Nayna
  0 siblings, 1 reply; 28+ messages in thread
From: Tyler Hicks @ 2020-07-17 18:11 UTC (permalink / raw)
  To: Nayna, Mimi Zohar
  Cc: Dmitry Kasatkin, James Morris, Serge E . Hallyn,
	Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
	linux-integrity, linux-security-module, Nayna Jain

On 2020-07-17 13:40:22, Nayna wrote:
> 
> On 7/9/20 2:19 AM, Tyler Hicks wrote:
> > The "appraise_flag" option is only appropriate for appraise actions
> > and its "blacklist" value is only appropriate when
> > CONFIG_IMA_APPRAISE_MODSIG is enabled and "appraise_flag=blacklist" is
> > only appropriate when "appraise_type=imasig|modsig" is also present.
> > Make this clear at policy load so that IMA policy authors don't assume
> > that other uses of "appraise_flag=blacklist" are supported.
> > 
> > Fixes: 273df864cf74 ("ima: Check against blacklisted hashes for files with modsig")
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > Cc: Nayna Jain <nayna@linux.ibm.com>
> > ---
> > 
> > * v3
> >    - New patch
> > 
> >   security/integrity/ima/ima_policy.c | 13 ++++++++++++-
> >   1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 81da02071d41..9842e2e0bc6d 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -1035,6 +1035,11 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> >   		return false;
> >   	}
> > +	/* Ensure that combinations of flags are compatible with each other */
> > +	if (entry->flags & IMA_CHECK_BLACKLIST &&
> > +	    !(entry->flags & IMA_MODSIG_ALLOWED))
> > +		return false;
> > +
> >   	return true;
> >   }
> > @@ -1371,8 +1376,14 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> >   				result = -EINVAL;
> >   			break;
> >   		case Opt_appraise_flag:
> > +			if (entry->action != APPRAISE) {
> > +				result = -EINVAL;
> > +				break;
> > +			}
> > +
> >   			ima_log_string(ab, "appraise_flag", args[0].from);
> > -			if (strstr(args[0].from, "blacklist"))
> > +			if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
> > +			    strstr(args[0].from, "blacklist"))
> >   				entry->flags |= IMA_CHECK_BLACKLIST;
> 
> If IMA_APPRAISE_MODSIG is disabled, it will allow the following rule to
> load, which is not as expected.
> 
> "appraise func=xxx_CHECK appraise_flag=blacklist appraise_type=imasig"
> 
> Missing is the "else" condition to immediately reject the policy rule.

Thanks for the review. You're right. This change is needed:

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 9842e2e0bc6d..cf3ddb38dfa8 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1385,6 +1385,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
 			    strstr(args[0].from, "blacklist"))
 				entry->flags |= IMA_CHECK_BLACKLIST;
+			else
+				result = -EINVAL;
 			break;
 		case Opt_permit_directio:
 			entry->flags |= IMA_PERMIT_DIRECTIO;


Making this change does not conflict with any later patches in the
series.

Mimi, I've rebased and force pushed to my fixup branch with this change,
for your comparison:

 https://git.kernel.org/pub/scm/linux/kernel/git/tyhicks/linux.git/log/?h=next-integrity-testing-fixup

Tyler

> 
> Thanks & Regards,
> 
>      - Nayna
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 06/12] ima: Fail rule parsing when the KEY_CHECK hook is combined with an invalid cond
  2020-07-09  6:19 ` [PATCH v3 06/12] ima: Fail rule parsing when the KEY_CHECK " Tyler Hicks
@ 2020-07-17 18:56   ` Nayna
  2020-07-17 19:18     ` Tyler Hicks
  0 siblings, 1 reply; 28+ messages in thread
From: Nayna @ 2020-07-17 18:56 UTC (permalink / raw)
  To: Tyler Hicks
  Cc: Mimi Zohar, Dmitry Kasatkin, James Morris, Serge E . Hallyn,
	Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
	linux-integrity, linux-security-module


On 7/9/20 2:19 AM, Tyler Hicks wrote:
> The KEY_CHECK function only supports the uid, pcr, and keyrings
> conditionals. Make this clear at policy load so that IMA policy authors
> don't assume that other conditionals are supported.
>
> Fixes: 5808611cccb2 ("IMA: Add KEY_CHECK func to measure keys")
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
>
> * v3
>    - Added Lakshmi's Reviewed-by
>    - Adjust for the indentation change introduced in patch #4
> * v2
>    - No change
>
>   security/integrity/ima/ima_policy.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 1c64bd6f1728..81da02071d41 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1023,6 +1023,13 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>   		if (entry->action & ~(MEASURE | DONT_MEASURE))
>   			return false;
>
> +		if (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
> +				     IMA_KEYRINGS))
> +			return false;
> +
> +		if (ima_rule_contains_lsm_cond(entry))
> +			return false;
> +
>   		break;
>   	default:
>   		return false;

Should there be a check for IMA_MEASURE_ASYMMETRIC_KEYS in Opt_keyrings 
in ima_parse_rule() to return immediately if not enabled ?

Thanks & Regards,

      - Nayna



^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 06/12] ima: Fail rule parsing when the KEY_CHECK hook is combined with an invalid cond
  2020-07-17 18:56   ` Nayna
@ 2020-07-17 19:18     ` Tyler Hicks
  2020-07-17 23:39       ` Tyler Hicks
  0 siblings, 1 reply; 28+ messages in thread
From: Tyler Hicks @ 2020-07-17 19:18 UTC (permalink / raw)
  To: Nayna, Lakshmi Ramasubramanian
  Cc: Mimi Zohar, Dmitry Kasatkin, James Morris, Serge E . Hallyn,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module

On 2020-07-17 14:56:46, Nayna wrote:
> 
> On 7/9/20 2:19 AM, Tyler Hicks wrote:
> > The KEY_CHECK function only supports the uid, pcr, and keyrings
> > conditionals. Make this clear at policy load so that IMA policy authors
> > don't assume that other conditionals are supported.
> > 
> > Fixes: 5808611cccb2 ("IMA: Add KEY_CHECK func to measure keys")
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> > ---
> > 
> > * v3
> >    - Added Lakshmi's Reviewed-by
> >    - Adjust for the indentation change introduced in patch #4
> > * v2
> >    - No change
> > 
> >   security/integrity/ima/ima_policy.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> > 
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 1c64bd6f1728..81da02071d41 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -1023,6 +1023,13 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> >   		if (entry->action & ~(MEASURE | DONT_MEASURE))
> >   			return false;
> > 
> > +		if (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
> > +				     IMA_KEYRINGS))
> > +			return false;
> > +
> > +		if (ima_rule_contains_lsm_cond(entry))
> > +			return false;
> > +
> >   		break;
> >   	default:
> >   		return false;
> 
> Should there be a check for IMA_MEASURE_ASYMMETRIC_KEYS in Opt_keyrings in
> ima_parse_rule() to return immediately if not enabled ?

I didn't notice that "keyrings=" could be disabled at build time. I
think you're right that something like what I have below would be a good idea.

@Lakshmi, do you agree?

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 81da02071d41..bd687560f88e 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1212,6 +1212,11 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 		case Opt_keyrings:
 			ima_log_string(ab, "keyrings", args[0].from);
 
+			if (!IS_ENABLED(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS)) {
+				result = -EINVAL;
+				break;
+			}
+
 			keyrings_len = strlen(args[0].from) + 1;
 
 			if ((entry->keyrings) ||

Tyler

> 
> Thanks & Regards,
> 
>      - Nayna
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 01/12] ima: Have the LSM free its audit rule
  2020-07-09  6:19 ` [PATCH v3 01/12] ima: Have the LSM free its audit rule Tyler Hicks
@ 2020-07-17 19:20   ` Nayna
  2020-07-17 19:24     ` Tyler Hicks
  0 siblings, 1 reply; 28+ messages in thread
From: Nayna @ 2020-07-17 19:20 UTC (permalink / raw)
  To: Tyler Hicks
  Cc: Mimi Zohar, Dmitry Kasatkin, James Morris, Serge E . Hallyn,
	Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
	linux-integrity, linux-security-module, Janne Karhunen,
	Casey Schaufler


On 7/9/20 2:19 AM, Tyler Hicks wrote:
> Ask the LSM to free its audit rule rather than directly calling kfree().

Is it to be called audit rule or filter rule ?  Likewise in subject line.

Thanks & Regards,

     - Nayna


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 01/12] ima: Have the LSM free its audit rule
  2020-07-17 19:20   ` Nayna
@ 2020-07-17 19:24     ` Tyler Hicks
  2020-07-19 11:02       ` Mimi Zohar
  0 siblings, 1 reply; 28+ messages in thread
From: Tyler Hicks @ 2020-07-17 19:24 UTC (permalink / raw)
  To: Nayna
  Cc: Mimi Zohar, Dmitry Kasatkin, James Morris, Serge E . Hallyn,
	Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
	linux-integrity, linux-security-module, Janne Karhunen,
	Casey Schaufler

On 2020-07-17 15:20:22, Nayna wrote:
> 
> On 7/9/20 2:19 AM, Tyler Hicks wrote:
> > Ask the LSM to free its audit rule rather than directly calling kfree().
> 
> Is it to be called audit rule or filter rule ?  Likewise in subject line.

The security hooks call this "audit rule" but Mimi explained the
reasoning for IMA referring to this as an "audit filter" here:

 https://lore.kernel.org/lkml/1593466203.5085.62.camel@linux.ibm.com/

I would be fine with her renaming/rewording this patch, accordingly, in
next-integrity-testing.

Tyler

> 
> Thanks & Regards,
> 
>     - Nayna

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 06/12] ima: Fail rule parsing when the KEY_CHECK hook is combined with an invalid cond
  2020-07-17 19:18     ` Tyler Hicks
@ 2020-07-17 23:39       ` Tyler Hicks
  0 siblings, 0 replies; 28+ messages in thread
From: Tyler Hicks @ 2020-07-17 23:39 UTC (permalink / raw)
  To: Nayna, Lakshmi Ramasubramanian, Mimi Zohar
  Cc: Dmitry Kasatkin, James Morris, Serge E . Hallyn,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module

On 2020-07-17 14:19:03, Tyler Hicks wrote:
> On 2020-07-17 14:56:46, Nayna wrote:
> > 
> > On 7/9/20 2:19 AM, Tyler Hicks wrote:
> > > The KEY_CHECK function only supports the uid, pcr, and keyrings
> > > conditionals. Make this clear at policy load so that IMA policy authors
> > > don't assume that other conditionals are supported.
> > > 
> > > Fixes: 5808611cccb2 ("IMA: Add KEY_CHECK func to measure keys")
> > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > > Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> > > ---
> > > 
> > > * v3
> > >    - Added Lakshmi's Reviewed-by
> > >    - Adjust for the indentation change introduced in patch #4
> > > * v2
> > >    - No change
> > > 
> > >   security/integrity/ima/ima_policy.c | 7 +++++++
> > >   1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > > index 1c64bd6f1728..81da02071d41 100644
> > > --- a/security/integrity/ima/ima_policy.c
> > > +++ b/security/integrity/ima/ima_policy.c
> > > @@ -1023,6 +1023,13 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> > >   		if (entry->action & ~(MEASURE | DONT_MEASURE))
> > >   			return false;
> > > 
> > > +		if (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
> > > +				     IMA_KEYRINGS))
> > > +			return false;
> > > +
> > > +		if (ima_rule_contains_lsm_cond(entry))
> > > +			return false;
> > > +
> > >   		break;
> > >   	default:
> > >   		return false;
> > 
> > Should there be a check for IMA_MEASURE_ASYMMETRIC_KEYS in Opt_keyrings in
> > ima_parse_rule() to return immediately if not enabled ?
> 
> I didn't notice that "keyrings=" could be disabled at build time. I
> think you're right that something like what I have below would be a good idea.
> 
> @Lakshmi, do you agree?
> 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 81da02071d41..bd687560f88e 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1212,6 +1212,11 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>  		case Opt_keyrings:
>  			ima_log_string(ab, "keyrings", args[0].from);
>  
> +			if (!IS_ENABLED(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS)) {
> +				result = -EINVAL;
> +				break;
> +			}
> +
>  			keyrings_len = strlen(args[0].from) + 1;
>  
>  			if ((entry->keyrings) ||
> 

Actually, this change introduces a new compiler warning in another part
of the code that I need to think some more about. I'd like to leave this
patch as-is for now and work on the !CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
case in a separate, later patch when I have some more time to think
about it and test properly.

Tyler

> Tyler
> 
> > 
> > Thanks & Regards,
> > 
> >      - Nayna
> > 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 01/12] ima: Have the LSM free its audit rule
  2020-07-17 19:24     ` Tyler Hicks
@ 2020-07-19 11:02       ` Mimi Zohar
  0 siblings, 0 replies; 28+ messages in thread
From: Mimi Zohar @ 2020-07-19 11:02 UTC (permalink / raw)
  To: Tyler Hicks, Nayna
  Cc: Dmitry Kasatkin, James Morris, Serge E . Hallyn,
	Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
	linux-integrity, linux-security-module, Janne Karhunen,
	Casey Schaufler

On Fri, 2020-07-17 at 14:24 -0500, Tyler Hicks wrote:
> On 2020-07-17 15:20:22, Nayna wrote:
> > 
> > On 7/9/20 2:19 AM, Tyler Hicks wrote:
> > > Ask the LSM to free its audit rule rather than directly calling kfree().
> > 
> > Is it to be called audit rule or filter rule ?  Likewise in subject line.
> gt
> The security hooks call this "audit rule" but Mimi explained the
> reasoning for IMA referring to this as an "audit filter" here:
> 
>  https://lore.kernel.org/lkml/1593466203.5085.62.camel@linux.ibm.com/
> 
> I would be fine with her renaming/rewording this patch, accordingly, in
> next-integrity-testing.

Both here and "ima: AppArmor satisfies the audit rule requirements",
the subject is AppArmor/LSM, which do refer to the rules as "audit"
rules.  In the "ima: Rename internal audit rule functions" case, the
rule rename is internal to IMA.  Here it makes sense to replace
"audit" with "filter".  Tyler, I've gone ahead and made the change.

Mimi


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 07/12] ima: Fail rule parsing when appraise_flag=blacklist is unsupportable
  2020-07-17 18:11     ` Tyler Hicks
@ 2020-07-20 17:02       ` Nayna
  0 siblings, 0 replies; 28+ messages in thread
From: Nayna @ 2020-07-20 17:02 UTC (permalink / raw)
  To: Tyler Hicks, Mimi Zohar
  Cc: Dmitry Kasatkin, James Morris, Serge E . Hallyn,
	Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
	linux-integrity, linux-security-module, Nayna Jain


On 7/17/20 2:11 PM, Tyler Hicks wrote:
> On 2020-07-17 13:40:22, Nayna wrote:
>> On 7/9/20 2:19 AM, Tyler Hicks wrote:
>>> The "appraise_flag" option is only appropriate for appraise actions
>>> and its "blacklist" value is only appropriate when
>>> CONFIG_IMA_APPRAISE_MODSIG is enabled and "appraise_flag=blacklist" is
>>> only appropriate when "appraise_type=imasig|modsig" is also present.
>>> Make this clear at policy load so that IMA policy authors don't assume
>>> that other uses of "appraise_flag=blacklist" are supported.
>>>
>>> Fixes: 273df864cf74 ("ima: Check against blacklisted hashes for files with modsig")
>>> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
>>> Cc: Nayna Jain <nayna@linux.ibm.com>
>>> ---
>>>
>>> * v3
>>>     - New patch
>>>
>>>    security/integrity/ima/ima_policy.c | 13 ++++++++++++-
>>>    1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>>> index 81da02071d41..9842e2e0bc6d 100644
>>> --- a/security/integrity/ima/ima_policy.c
>>> +++ b/security/integrity/ima/ima_policy.c
>>> @@ -1035,6 +1035,11 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>>>    		return false;
>>>    	}
>>> +	/* Ensure that combinations of flags are compatible with each other */
>>> +	if (entry->flags & IMA_CHECK_BLACKLIST &&
>>> +	    !(entry->flags & IMA_MODSIG_ALLOWED))
>>> +		return false;
>>> +
>>>    	return true;
>>>    }
>>> @@ -1371,8 +1376,14 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>>>    				result = -EINVAL;
>>>    			break;
>>>    		case Opt_appraise_flag:
>>> +			if (entry->action != APPRAISE) {
>>> +				result = -EINVAL;
>>> +				break;
>>> +			}
>>> +
>>>    			ima_log_string(ab, "appraise_flag", args[0].from);
>>> -			if (strstr(args[0].from, "blacklist"))
>>> +			if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
>>> +			    strstr(args[0].from, "blacklist"))
>>>    				entry->flags |= IMA_CHECK_BLACKLIST;
>> If IMA_APPRAISE_MODSIG is disabled, it will allow the following rule to
>> load, which is not as expected.
>>
>> "appraise func=xxx_CHECK appraise_flag=blacklist appraise_type=imasig"
>>
>> Missing is the "else" condition to immediately reject the policy rule.
> Thanks for the review. You're right. This change is needed:
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 9842e2e0bc6d..cf3ddb38dfa8 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1385,6 +1385,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>   			if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
>   			    strstr(args[0].from, "blacklist"))
>   				entry->flags |= IMA_CHECK_BLACKLIST;
> +			else
> +				result = -EINVAL;
>   			break;
>   		case Opt_permit_directio:
>   			entry->flags |= IMA_PERMIT_DIRECTIO;
>
Reviewed-by: Nayna Jain<nayna@linux.ibm.com>

Tested-by: Nayna Jain<nayna@linux.ibm.com>

Thanks & Regards,

       - Nayna


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support
  2020-07-09  6:18 [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Tyler Hicks
                   ` (12 preceding siblings ...)
  2020-07-17  4:31 ` [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Mimi Zohar
@ 2020-07-20 21:38 ` Mimi Zohar
  13 siblings, 0 replies; 28+ messages in thread
From: Mimi Zohar @ 2020-07-20 21:38 UTC (permalink / raw)
  To: Tyler Hicks, Dmitry Kasatkin, Sasha Levin
  Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
	Prakhar Srivastava, linux-kernel, linux-integrity,
	linux-security-module, Janne Karhunen, Eric Biederman, kexec,
	Casey Schaufler, Nayna Jain

[Cc'ing Sasha]

On Thu, 2020-07-09 at 01:18 -0500, Tyler Hicks wrote:

> I envision patches 1-7 going to stable. The series is ordered in a way
> that has all the fixes up front, followed by cleanups, followed by the
> feature patch. The breakdown of patches looks like so:
> 
>  Memory leak fixes: 1-3
>  Parser strictness fixes: 4-7
>  Code cleanups made possible by the fixes: 8-11
>  Extend KEXEC_CMDLINE rule support: 12

I agree they should be backported, but they don't apply cleanly before
linux-5.6.  The changes aren't that major.  Some patch hunks apply
cleanly, but won't compile, while others patch hunks need to be
dropped based on when the feature was upstreamed.  For these reasons,
I'm not Cc'ing stable.

Feature upstreamed:
- LSM policy update: linux 5.3
- key command line: linux 5.3
- blacklist: linux 5.5
- keyrings: linux 5.6

For Linux 5.3:
- Dependency on backporting commit 483ec26eed42 ("ima: ima/lsm policy
rule loading logic bug fixes") to apply " ima: Free the entire rule if
it fails to parse".

Mimi

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, back to index

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-09  6:18 [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Tyler Hicks
2020-07-09  6:19 ` [PATCH v3 01/12] ima: Have the LSM free its audit rule Tyler Hicks
2020-07-17 19:20   ` Nayna
2020-07-17 19:24     ` Tyler Hicks
2020-07-19 11:02       ` Mimi Zohar
2020-07-09  6:19 ` [PATCH v3 02/12] ima: Free the entire rule when deleting a list of rules Tyler Hicks
2020-07-09  6:19 ` [PATCH v3 03/12] ima: Free the entire rule if it fails to parse Tyler Hicks
2020-07-09  6:19 ` [PATCH v3 04/12] ima: Fail rule parsing when buffer hook functions have an invalid action Tyler Hicks
2020-07-09  6:19 ` [PATCH v3 05/12] ima: Fail rule parsing when the KEXEC_CMDLINE hook is combined with an invalid cond Tyler Hicks
2020-07-09  6:19 ` [PATCH v3 06/12] ima: Fail rule parsing when the KEY_CHECK " Tyler Hicks
2020-07-17 18:56   ` Nayna
2020-07-17 19:18     ` Tyler Hicks
2020-07-17 23:39       ` Tyler Hicks
2020-07-09  6:19 ` [PATCH v3 07/12] ima: Fail rule parsing when appraise_flag=blacklist is unsupportable Tyler Hicks
2020-07-16 18:14   ` Mimi Zohar
2020-07-16 18:20     ` Tyler Hicks
     [not found]   ` <76d2b27b-3b59-1852-046a-b1718c62b167@linux.vnet.ibm.com>
2020-07-17 18:11     ` Tyler Hicks
2020-07-20 17:02       ` Nayna
2020-07-09  6:19 ` [PATCH v3 08/12] ima: Shallow copy the args_p member of ima_rule_entry.lsm elements Tyler Hicks
2020-07-17 15:35   ` Konsta Karsisto
2020-07-17 16:51     ` Tyler Hicks
2020-07-09  6:19 ` [PATCH v3 09/12] ima: Use correct type for " Tyler Hicks
2020-07-09  6:19 ` [PATCH v3 10/12] ima: Move comprehensive rule validation checks out of the token parser Tyler Hicks
2020-07-09  6:19 ` [PATCH v3 11/12] ima: Use the common function to detect LSM conditionals in a rule Tyler Hicks
2020-07-09  6:19 ` [PATCH v3 12/12] ima: Support additional conditionals in the KEXEC_CMDLINE hook function Tyler Hicks
2020-07-17  4:31 ` [PATCH v3 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Mimi Zohar
2020-07-17  4:34   ` Tyler Hicks
2020-07-20 21:38 ` Mimi Zohar

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org
	public-inbox-index linux-integrity

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git