diff mbox series

[v4,5/6] IMA: add hook to measure critical data from kernel components

Message ID 20200923192011.5293-6-tusharsu@linux.microsoft.com
State Changes Requested
Headers show
Series IMA: Infrastructure for measurement of critical kernel data | expand

Commit Message

Tushar Sugandhi Sept. 23, 2020, 7:20 p.m. UTC
Currently, IMA does not provide a generic function for kernel components
to measure their data. A generic function provided by IMA would
enable various parts of the kernel with easier and faster on-boarding to
use IMA infrastructure, would avoid code duplication, and consistent
usage of IMA policy option "data_sources:=" across the kernel.

Add a new IMA func CRITICAL_DATA and a corresponding IMA hook
ima_measure_critical_data() to support measuring various critical kernel
components. Limit the measurement to the components that are specified
in the IMA policy - CRITICAL_DATA+data_sources.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 Documentation/ABI/testing/ima_policy |  8 ++++++-
 include/linux/ima.h                  |  8 +++++++
 security/integrity/ima/ima.h         |  1 +
 security/integrity/ima/ima_api.c     |  2 +-
 security/integrity/ima/ima_main.c    | 26 +++++++++++++++++++++
 security/integrity/ima/ima_policy.c  | 34 ++++++++++++++++++++++++----
 6 files changed, 72 insertions(+), 7 deletions(-)

Comments

Mimi Zohar Oct. 22, 2020, 10:35 p.m. UTC | #1
Hi Tushar,

On Wed, 2020-09-23 at 12:20 -0700, Tushar Sugandhi wrote:
> Currently, IMA does not provide a generic function for kernel components
> to measure their data. A generic function provided by IMA would
> enable various parts of the kernel with easier and faster on-boarding to
> use IMA infrastructure, would avoid code duplication, and consistent
> usage of IMA policy option "data_sources:=" across the kernel.
> 
> Add a new IMA func CRITICAL_DATA and a corresponding IMA hook
> ima_measure_critical_data() to support measuring various critical kernel
> components. Limit the measurement to the components that are specified
> in the IMA policy - CRITICAL_DATA+data_sources.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>

Normally the new LSM or IMA hook is defined before defining a method of
constraining that hook.  Please drop 2/6 (IMA: conditionally allow
empty rule data) and reverse the order of 4/6 and 5/6.   That will
allow each patch to update the Documentation appropriately, making the
change self contained.

> ---
>  Documentation/ABI/testing/ima_policy |  8 ++++++-
>  include/linux/ima.h                  |  8 +++++++
>  security/integrity/ima/ima.h         |  1 +
>  security/integrity/ima/ima_api.c     |  2 +-
>  security/integrity/ima/ima_main.c    | 26 +++++++++++++++++++++
>  security/integrity/ima/ima_policy.c  | 34 ++++++++++++++++++++++++----
>  6 files changed, 72 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index a81cf79fb255..d33bb51309fc 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -29,7 +29,7 @@ Description:
>  		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
>  				[FIRMWARE_CHECK]
>  				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
> -				[KEXEC_CMDLINE] [KEY_CHECK]
> +				[KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA]
>  			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
>  			       [[^]MAY_EXEC]
>  			fsmagic:= hex value
> @@ -51,6 +51,8 @@ Description:
>  			data_sources:= list of kernel components
>  			(eg, selinux|apparmor|dm-crypt) that contain data critical
>  			to the security of the kernel.
> +			Only valid when action is "measure" and func is
> +			CRITICAL_DATA.
>  
>  		default policy:
>  			# PROC_SUPER_MAGIC
> @@ -128,3 +130,7 @@ Description:
>  		keys added to .builtin_trusted_keys or .ima keyring:
>  
>  			measure func=KEY_CHECK keyrings=.builtin_trusted_keys|.ima
> +
> +		Example of measure rule using CRITICAL_DATA to measure critical data
> +
> +			measure func=CRITICAL_DATA data_sources=selinux|apparmor|dm-crypt


As data sources are added, the documentation example should be updated
to reflect the new source.  Please do not include examples that don't
yet exist.


> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 6888fc372abf..d55896f28790 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -867,6 +867,32 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
>  	fdput(f);
>  }
>  
> +/**
> + * ima_measure_critical_data - measure critical data
> + * @event_name: name for the given data
> + * @event_data_source: name of the event data source
> + * @buf: pointer to buffer containing data to measure
> + * @buf_len: length of buffer(in bytes)
> + * @measure_buf_hash: if set to true - will measure hash of the buf,
> + *                    instead of buf
> + *
> + * Buffers can only be measured, not appraised.
> + */

Perhaps the reason for defining both the event_name and
event_data_source will become clearer with an example.  At this point I
can only guess as to why both are needed (e.g. perhaps a data source
defines multiple events).

While "Buffers can only be measured, not appraised" is true, it was cut
& pasted from ima_kexec_cmdline.  Measuring the kexec boot cmdline is
self describing.  Here, a larger, more detailed IMA hook description
would be appropriate.

thanks,

Mimi
Tushar Sugandhi Oct. 23, 2020, 10:54 p.m. UTC | #2
On 2020-10-22 3:35 p.m., Mimi Zohar wrote:
> Hi Tushar,
> 
> On Wed, 2020-09-23 at 12:20 -0700, Tushar Sugandhi wrote:
>> Currently, IMA does not provide a generic function for kernel components
>> to measure their data. A generic function provided by IMA would
>> enable various parts of the kernel with easier and faster on-boarding to
>> use IMA infrastructure, would avoid code duplication, and consistent
>> usage of IMA policy option "data_sources:=" across the kernel.
>>
>> Add a new IMA func CRITICAL_DATA and a corresponding IMA hook
>> ima_measure_critical_data() to support measuring various critical kernel
>> components. Limit the measurement to the components that are specified
>> in the IMA policy - CRITICAL_DATA+data_sources.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> 
> Normally the new LSM or IMA hook is defined before defining a method of
> constraining that hook.  Please drop 2/6 (IMA: conditionally allow
> empty rule data) and reverse the order of 4/6 and 5/6.   That will
> allow each patch to update the Documentation appropriately, making the
> change self contained.
> 
Sure. I will drop 2/6, and reverse the order of 4/6 and 5/6.
>> ---
>>   Documentation/ABI/testing/ima_policy |  8 ++++++-
>>   include/linux/ima.h                  |  8 +++++++
>>   security/integrity/ima/ima.h         |  1 +
>>   security/integrity/ima/ima_api.c     |  2 +-
>>   security/integrity/ima/ima_main.c    | 26 +++++++++++++++++++++
>>   security/integrity/ima/ima_policy.c  | 34 ++++++++++++++++++++++++----
>>   6 files changed, 72 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
>> index a81cf79fb255..d33bb51309fc 100644
>> --- a/Documentation/ABI/testing/ima_policy
>> +++ b/Documentation/ABI/testing/ima_policy
>> @@ -29,7 +29,7 @@ Description:
>>   		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
>>   				[FIRMWARE_CHECK]
>>   				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
>> -				[KEXEC_CMDLINE] [KEY_CHECK]
>> +				[KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA]
>>   			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
>>   			       [[^]MAY_EXEC]
>>   			fsmagic:= hex value
>> @@ -51,6 +51,8 @@ Description:
>>   			data_sources:= list of kernel components
>>   			(eg, selinux|apparmor|dm-crypt) that contain data critical
>>   			to the security of the kernel.
>> +			Only valid when action is "measure" and func is
>> +			CRITICAL_DATA.
>>   
>>   		default policy:
>>   			# PROC_SUPER_MAGIC
>> @@ -128,3 +130,7 @@ Description:
>>   		keys added to .builtin_trusted_keys or .ima keyring:
>>   
>>   			measure func=KEY_CHECK keyrings=.builtin_trusted_keys|.ima
>> +
>> +		Example of measure rule using CRITICAL_DATA to measure critical data
>> +
>> +			measure func=CRITICAL_DATA data_sources=selinux|apparmor|dm-crypt
> 
> 
> As data sources are added, the documentation example should be updated
> to reflect the new source.  Please do not include examples that don't
> yet exist.
> 
Makes sense. Will do.
> 
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index 6888fc372abf..d55896f28790 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -867,6 +867,32 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
>>   	fdput(f);
>>   }
>>   
>> +/**
>> + * ima_measure_critical_data - measure critical data
>> + * @event_name: name for the given data
>> + * @event_data_source: name of the event data source
>> + * @buf: pointer to buffer containing data to measure
>> + * @buf_len: length of buffer(in bytes)
>> + * @measure_buf_hash: if set to true - will measure hash of the buf,
>> + *                    instead of buf
>> + *
>> + * Buffers can only be measured, not appraised.
>> + */
> 
> Perhaps the reason for defining both the event_name and
> event_data_source will become clearer with an example.  At this point I
> can only guess as to why both are needed (e.g. perhaps a data source
> defines multiple events).
> 
Yes. Precisely. For example, in “dm-crypt” case: the data source is
“dm-crypt” and possible events are “add_target”, “post_suspend”,
"resume" etc. I will add a more detailed hook description as you
suggested below, and explain this point in it.
> While "Buffers can only be measured, not appraised" is true, it was cut
> & pasted from ima_kexec_cmdline.  Measuring the kexec boot cmdline is
> self describing.  Here, a larger, more detailed IMA hook description
> would be appropriate.
Will add. Thanks Mimi.
> 
> thanks,
> 
> Mimi
>
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index a81cf79fb255..d33bb51309fc 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -29,7 +29,7 @@  Description:
 		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
 				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
-				[KEXEC_CMDLINE] [KEY_CHECK]
+				[KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA]
 			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
 			       [[^]MAY_EXEC]
 			fsmagic:= hex value
@@ -51,6 +51,8 @@  Description:
 			data_sources:= list of kernel components
 			(eg, selinux|apparmor|dm-crypt) that contain data critical
 			to the security of the kernel.
+			Only valid when action is "measure" and func is
+			CRITICAL_DATA.
 
 		default policy:
 			# PROC_SUPER_MAGIC
@@ -128,3 +130,7 @@  Description:
 		keys added to .builtin_trusted_keys or .ima keyring:
 
 			measure func=KEY_CHECK keyrings=.builtin_trusted_keys|.ima
+
+		Example of measure rule using CRITICAL_DATA to measure critical data
+
+			measure func=CRITICAL_DATA data_sources=selinux|apparmor|dm-crypt
diff --git a/include/linux/ima.h b/include/linux/ima.h
index d15100de6cdd..4040f484ac63 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -26,6 +26,10 @@  extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
 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(int kernel_fd, const void *buf, int size);
+extern void ima_measure_critical_data(const char *event_name,
+				      const char *event_data_source,
+				      const void *buf, int buf_len,
+				      bool measure_buf_hash);
 
 #ifdef CONFIG_IMA_KEXEC
 extern void ima_add_kexec_buffer(struct kimage *image);
@@ -104,6 +108,10 @@  static inline int ima_file_hash(struct file *file, char *buf, size_t buf_size)
 }
 
 static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {}
+static inline void ima_measure_critical_data(const char *event_name,
+					     const char *event_data_source,
+					     const void *buf, int buf_len,
+					     bool measure_buf_hash) {}
 #endif /* CONFIG_IMA */
 
 #ifndef CONFIG_IMA_KEXEC
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 0f77e0b697a3..c1acf88e1b5d 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -200,6 +200,7 @@  static inline unsigned int ima_hash_key(u8 *digest)
 	hook(POLICY_CHECK, policy)			\
 	hook(KEXEC_CMDLINE, kexec_cmdline)		\
 	hook(KEY_CHECK, key)				\
+	hook(CRITICAL_DATA, critical_data)		\
 	hook(MAX_CHECK, none)
 
 #define __ima_hook_enumify(ENUM, str)	ENUM,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index af218babd198..9917e1730cb6 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -176,7 +176,7 @@  void ima_add_violation(struct file *file, const unsigned char *filename,
  *		subj=, obj=, type=, func=, mask=, fsmagic=
  *	subj,obj, and type: are LSM specific.
  *	func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
- *	| KEXEC_CMDLINE | KEY_CHECK
+ *	| KEXEC_CMDLINE | KEY_CHECK | CRITICAL_DATA
  *	mask: contains the permission mask
  *	fsmagic: hex value
  *
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 6888fc372abf..d55896f28790 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -867,6 +867,32 @@  void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
 	fdput(f);
 }
 
+/**
+ * ima_measure_critical_data - measure critical data
+ * @event_name: name for the given data
+ * @event_data_source: name of the event data source
+ * @buf: pointer to buffer containing data to measure
+ * @buf_len: length of buffer(in bytes)
+ * @measure_buf_hash: if set to true - will measure hash of the buf,
+ *                    instead of buf
+ *
+ * Buffers can only be measured, not appraised.
+ */
+void ima_measure_critical_data(const char *event_name,
+			       const char *event_data_source,
+			       const void *buf, int buf_len,
+			       bool measure_buf_hash)
+{
+	if (!event_name || !event_data_source || !buf || !buf_len) {
+		pr_err("Invalid arguments passed to %s().\n", __func__);
+		return;
+	}
+
+	process_buffer_measurement(NULL, buf, buf_len, event_name,
+				   CRITICAL_DATA, 0, event_data_source,
+				   measure_buf_hash);
+}
+
 static int __init init_ima(void)
 {
 	int error;
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 89452245f54a..491017df7589 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -510,14 +510,23 @@  static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 {
 	int i;
 
-	if (func == KEY_CHECK) {
-		return (rule->flags & IMA_FUNC) && (rule->func == func) &&
-		       ima_match_rule_data(rule, rule->keyrings, func_data,
-					   true, cred);
-	}
 	if ((rule->flags & IMA_FUNC) &&
 	    (rule->func != func && func != POST_SETATTR))
 		return false;
+
+	switch (func) {
+	case KEY_CHECK:
+		return ((rule->func == func) &&
+			ima_match_rule_data(rule, rule->keyrings,
+					    func_data, true, cred));
+	case CRITICAL_DATA:
+		return ((rule->func == func) &&
+			ima_match_rule_data(rule, rule->data_sources,
+					    func_data, false, cred));
+	default:
+		break;
+	}
+
 	if ((rule->flags & IMA_MASK) &&
 	    (rule->mask != mask && func != POST_SETATTR))
 		return false;
@@ -1113,6 +1122,19 @@  static bool ima_validate_rule(struct ima_rule_entry *entry)
 		if (ima_rule_contains_lsm_cond(entry))
 			return false;
 
+		break;
+	case CRITICAL_DATA:
+		if (entry->action & ~(MEASURE | DONT_MEASURE))
+			return false;
+
+		if (!(entry->flags & IMA_DATA_SOURCES) ||
+		    (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
+		    IMA_DATA_SOURCES)))
+			return false;
+
+		if (ima_rule_contains_lsm_cond(entry))
+			return false;
+
 		break;
 	default:
 		return false;
@@ -1245,6 +1267,8 @@  static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			else if (IS_ENABLED(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) &&
 				 strcmp(args[0].from, "KEY_CHECK") == 0)
 				entry->func = KEY_CHECK;
+			else if (strcmp(args[0].from, "CRITICAL_DATA") == 0)
+				entry->func = CRITICAL_DATA;
 			else
 				result = -EINVAL;
 			if (!result)