diff mbox series

[v3,4/6] IMA: add policy to measure critical data from kernel components

Message ID 20200828015704.6629-5-tusharsu@linux.microsoft.com (mailing list archive)
State New, archived
Headers show
Series IMA: Infrastructure for measurement of critical kernel data | expand

Commit Message

Tushar Sugandhi Aug. 28, 2020, 1:57 a.m. UTC
There would be several candidate kernel components suitable for IMA
measurement. Not all of them would have support for IMA measurement.
Also, system administrators may not want to measure data for all of
them, even when they support IMA measurement. An IMA policy specific
to various kernel components is needed to measure their respective
critical data.

Add a new IMA policy "critical_kernel_data_sources" to support measuring
various critical kernel components. This policy would enable the
system administrators to limit the measurement to the components,
if the components support IMA measurement.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 Documentation/ABI/testing/ima_policy |  3 +++
 security/integrity/ima/ima_policy.c  | 29 +++++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

Comments

Mimi Zohar Aug. 31, 2020, 6:15 p.m. UTC | #1
On Thu, 2020-08-27 at 18:57 -0700, Tushar Sugandhi wrote:
> There would be several candidate kernel components suitable for IMA
> measurement. Not all of them would have support for IMA measurement.
> Also, system administrators may not want to measure data for all of
> them, even when they support IMA measurement. An IMA policy specific
> to various kernel components is needed to measure their respective
> critical data.

The base policy rules are wide, but may be constrained by specifying
different options.  For example the builtin policy rules cannot be
written in terms LSM labels, which would constrain them.  A policy rule
may measure all keyrings or may constrain which keyrings need to be
measured.  Measuring critical data is not any different.

Please rewrite the above paragraph accordingly.

> 
> Add a new IMA policy "critical_kernel_data_sources" to support measuring
> various critical kernel components. This policy would enable the
> system administrators to limit the measurement to the components,
> if the components support IMA measurement.

"critical_kernel_data_sources" is really wordy.   Find a better, self
defining term for describing the type of data, one that isn't so wordy,
and reflect it in the code.

> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>  Documentation/ABI/testing/ima_policy |  3 +++
>  security/integrity/ima/ima_policy.c  | 29 +++++++++++++++++++++++++++-
>  2 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index cd572912c593..7ccdc1964e29 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -48,6 +48,9 @@ Description:
>  			template:= name of a defined IMA template type
>  			(eg, ima-ng). Only valid when action is "measure".
>  			pcr:= decimal value
> +			critical_kernel_data_sources:= list of kernel
> +			components (eg, selinux|apparmor|dm-crypt) that
> +			contain data critical to the security of the kernel.

This original policy definition, for the most part, is in Backus–Naur
format.   The keyring names is an exception, because it is not limited
to pre-defined kernel objects.  The critical data hook is measuring
things in kernel memory.  As new calls to measure critical data are
added, new identifiers would be added here.

For example, if SELinux is the first example of measuring critical
data, then the SELinux critical data patch would include
"critical_data:= [selinux]".  Each subsequent critical data being
measured would extend this list.  At the same time, the list of known
"critical data" defined in patch 6/6 would be updated.

Normally a new feature and the first usage of that feature are included
in the same patch set.  Separating them like this makes it difficult to
write, review and upstream.

Mimi
Tushar Sugandhi Sept. 11, 2020, 5:29 p.m. UTC | #2
On 2020-08-31 11:15 a.m., Mimi Zohar wrote:
> On Thu, 2020-08-27 at 18:57 -0700, Tushar Sugandhi wrote:
>> There would be several candidate kernel components suitable for IMA
>> measurement. Not all of them would have support for IMA measurement.
>> Also, system administrators may not want to measure data for all of
>> them, even when they support IMA measurement. An IMA policy specific
>> to various kernel components is needed to measure their respective
>> critical data.
> 
> The base policy rules are wide, but may be constrained by specifying
> different options.  For example the builtin policy rules cannot be
> written in terms LSM labels, which would constrain them.  A policy rule
> may measure all keyrings or may constrain which keyrings need to be
> measured.  Measuring critical data is not any different.
> 
> Please rewrite the above paragraph accordingly.
> 
Ok. Will do.
>>
>> Add a new IMA policy "critical_kernel_data_sources" to support measuring
>> various critical kernel components. This policy would enable the
>> system administrators to limit the measurement to the components,
>> if the components support IMA measurement.
> 
> "critical_kernel_data_sources" is really wordy.   Find a better, self
> defining term for describing the type of data, one that isn't so wordy,
> and reflect it in the code.
> 
Will do. I will go with "critical_data". You also have suggested it in
the comment below.

"critical_data_sources" also seems right, but that's more wordy than
"critical_data".

Some more options we considered, but they don’t sound right.
Please let us know what do you think.
1. "critical_data_sources="
2. "critical_kernel_components=" -or- "crit_krnl_comps="
3. "critical_data_providers="
4. "critical_kernel_data_providers=" -or- "crit_krnl_dt_provs="
5. "critical_kernel_data_sources=" -or- "crit_krnl_dt_srcs="
6. "security_critical_data="
7. "protectable_data="
8. "protected_data="
9. "vital_protected_data="

>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>> ---
>>   Documentation/ABI/testing/ima_policy |  3 +++
>>   security/integrity/ima/ima_policy.c  | 29 +++++++++++++++++++++++++++-
>>   2 files changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
>> index cd572912c593..7ccdc1964e29 100644
>> --- a/Documentation/ABI/testing/ima_policy
>> +++ b/Documentation/ABI/testing/ima_policy
>> @@ -48,6 +48,9 @@ Description:
>>   			template:= name of a defined IMA template type
>>   			(eg, ima-ng). Only valid when action is "measure".
>>   			pcr:= decimal value
>> +			critical_kernel_data_sources:= list of kernel
>> +			components (eg, selinux|apparmor|dm-crypt) that
>> +			contain data critical to the security of the kernel.
> 
> This original policy definition, for the most part, is in Backus–Naur
> format.   The keyring names is an exception, because it is not limited
> to pre-defined kernel objects.  The critical data hook is measuring
> things in kernel memory.  As new calls to measure critical data are
> added, new identifiers would be added here.
> 
> For example, if SELinux is the first example of measuring critical
> data, then the SELinux critical data patch would include
> "critical_data:= [selinux]".  Each subsequent critical data being
> measured would extend this list.  At the same time, the list of known
> "critical data" defined in patch 6/6 would be updated.
> 
> Normally a new feature and the first usage of that feature are included
> in the same patch set.  Separating them like this makes it difficult to
> write, review and upstream.
> 
> Mimi
> 
I agree. But the unique issue we are facing here is there are two
"first users" of this new "base series".

One, SeLinux work (driven by Lakshmi); and two, device-mapper/dm-crypt 
work (driven by me).

Both of them need to be reviewed by different maintainers, may go 
through several iterations before getting accepted.

That’s why we wanted to keep this "base series" independent of the 
"first users"; and called the "base series" as a dependency in the 
dm-crypt[1] / SeLinux[2] series.

We would appreciate your guidance on how we can better author these
three series - 1.this base series 2. dm-crypt series and 3. SeLinux
series.

[1]dm-crypt Series: https://patchwork.kernel.org/patch/11743715/
[2]SeLinux Series: https://patchwork.kernel.org/patch/11762287/
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index cd572912c593..7ccdc1964e29 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -48,6 +48,9 @@  Description:
 			template:= name of a defined IMA template type
 			(eg, ima-ng). Only valid when action is "measure".
 			pcr:= decimal value
+			critical_kernel_data_sources:= list of kernel
+			components (eg, selinux|apparmor|dm-crypt) that
+			contain data critical to the security of the kernel.
 
 		default policy:
 			# PROC_SUPER_MAGIC
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 8866e84d0062..c8a044705347 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -33,6 +33,7 @@ 
 #define IMA_PCR		0x0100
 #define IMA_FSNAME	0x0200
 #define IMA_KEYRINGS	0x0400
+#define IMA_DATA_SOURCES	0x0800
 
 #define UNKNOWN		0
 #define MEASURE		0x0001	/* same as IMA_MEASURE */
@@ -84,6 +85,7 @@  struct ima_rule_entry {
 	} lsm[MAX_LSM_RULES];
 	char *fsname;
 	struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */
+	struct ima_rule_opt_list *data_sources; /* Measure data from these sources */
 	struct ima_template_desc *template;
 };
 
@@ -911,7 +913,7 @@  enum {
 	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
 	Opt_appraise_type, Opt_appraise_flag,
 	Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
-	Opt_err
+	Opt_data_sources, Opt_err
 };
 
 static const match_table_t policy_tokens = {
@@ -948,6 +950,7 @@  static const match_table_t policy_tokens = {
 	{Opt_pcr, "pcr=%s"},
 	{Opt_template, "template=%s"},
 	{Opt_keyrings, "keyrings=%s"},
+	{Opt_data_sources, "critical_kernel_data_sources=%s"},
 	{Opt_err, NULL}
 };
 
@@ -1312,6 +1315,24 @@  static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 
 			entry->flags |= IMA_KEYRINGS;
 			break;
+		case Opt_data_sources:
+			ima_log_string(ab, "critical_kernel_data_sources",
+				       args[0].from);
+
+			if (entry->data_sources) {
+				result = -EINVAL;
+				break;
+			}
+
+			entry->data_sources = ima_alloc_rule_opt_list(args);
+			if (IS_ERR(entry->data_sources)) {
+				result = PTR_ERR(entry->data_sources);
+				entry->data_sources = NULL;
+				break;
+			}
+
+			entry->flags |= IMA_DATA_SOURCES;
+			break;
 		case Opt_fsuuid:
 			ima_log_string(ab, "fsuuid", args[0].from);
 
@@ -1692,6 +1713,12 @@  int ima_policy_show(struct seq_file *m, void *v)
 		seq_puts(m, " ");
 	}
 
+	if (entry->flags & IMA_DATA_SOURCES) {
+		seq_puts(m, "critical_kernel_data_sources=");
+		ima_show_rule_opt_list(m, entry->data_sources);
+		seq_puts(m, " ");
+	}
+
 	if (entry->flags & IMA_PCR) {
 		snprintf(tbuf, sizeof(tbuf), "%d", entry->pcr);
 		seq_printf(m, pt(Opt_pcr), tbuf);