Skip to contentSkip to conversation

IMA: support for duplicate data measurement

PATCH Superseded Tushar Sugandhi 2 participants 7 messages Feb 17, 2021 – Feb 22, 2021

Subsystems (4)

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 IMA: support for duplicate data measurement

IMA does not measure duplicate data since TPM extend is a very expensive operation. However, in some cases, the measurement of duplicate data is necessary to accurately determine the current state of the system. Eg, SELinux state changing from 'audit', to 'enforcing', and back to 'audit' again. In this example, currently, IMA will not measure the last state change to 'audit'. This limits the ability of attestation services to accurately determine the current state of the measurements on the system.

Update ima_add_template_entry() to support measurement of duplicate data, driven by a Kconfig option - IMA_DISABLE_HTABLE.

---
Change Log v2:
- Incorporated feedback from Mimi on v1.
- The fix is not just applicable to measurement of critical data, it now applies to other buffers and file data as well.
- the fix is driven by a Kconfig option IMA_DISABLE_HTABLE, rather than a IMA policy condition - allow_dup.

 security/integrity/ima/Kconfig     | 7 +++++++
 security/integrity/ima/ima_queue.c | 5 +++--
 2 files changed, 10 insertions(+), 2 deletions(-)
security/integrity/ima/Kconfig +7 -0
index 12e9250c1bec..057c20b46587 100644
@@ -334,3 +334,10 @@ config IMA_SECURE_AND_OR_TRUSTED_BOOT
        help
           This option is selected by architectures to enable secure and/or
           trusted boot based on IMA runtime policies.
+
+config IMA_DISABLE_HTABLE
+	bool "disable htable to allow measurement of duplicate data"
+	depends on IMA
+	default n
+	help
+	   This option disables htable to allow measurement of duplicate data.
security/integrity/ima/ima_queue.c +3 -2
index c096ef8945c7..532da87ce519 100644
@@ -168,7 +168,7 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 	int result = 0, tpmresult = 0;
 
 	mutex_lock(&ima_extend_list_mutex);
-	if (!violation) {
+	if (!violation && !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE)) {
 		if (ima_lookup_digest_entry(digest, entry->pcr)) {
 			audit_cause = "hash_exists";
 			result = -EEXIST;
@@ -176,7 +176,8 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 		}
 	}
 
-	result = ima_add_digest_entry(entry, 1);
+	result = ima_add_digest_entry(entry,
+				      !IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE));
 	if (result < 0) {
 		audit_cause = "ENOMEM";
 		audit_info = 0;
Signature · 1 line
2.17.1
maintainer
#1 lore raw

Hi Tushar,

The Subject line could be improved. Perhaps something like - "IMA:
support for duplicate measurement records"

Quoted 8 lines · Tushar Sugandhi

IMA does not measure duplicate data since TPM extend is a very expensive operation. However, in some cases, the measurement of duplicate data is necessary to accurately determine the current state of the system. Eg, SELinux state changing from 'audit', to 'enforcing', and back to 'audit' again. In this example, currently, IMA will not measure the last state change to 'audit'. This limits the ability of attestation services to accurately determine the current state of the measurements on the system.

This patch description is written from your specific usecase perspective, but it impacts file and buffer data measurements as well, not only critical data measurements. In all of these situations, with this patch a new measurement record is added/appended to the measurement list. Please re-write the patch description making it more generic.

For example, I would start with something like, "IMA does not include duplicate file, buffer or critical data measurement records ..."

thanks,

Mimi

Quoted 5 lines

Update ima_add_template_entry() to support measurement of duplicate data, driven by a Kconfig option - IMA_DISABLE_HTABLE.

#2 lore raw

Thanks for the feedback Mimi.
Appreciate it.

Quoted 5 lines · Mimi Zohar

Hi Tushar,

The Subject line could be improved. Perhaps something like - "IMA:
support for duplicate measurement records"

Will do.

Quoted 17 lines · Tushar Sugandhi
Quoted 8 lines

IMA does not measure duplicate data since TPM extend is a very expensive operation. However, in some cases, the measurement of duplicate data is necessary to accurately determine the current state of the system. Eg, SELinux state changing from 'audit', to 'enforcing', and back to 'audit' again. In this example, currently, IMA will not measure the last state change to 'audit'. This limits the ability of attestation services to accurately determine the current state of the measurements on the system.

This patch description is written from your specific usecase perspective, but it impacts file and buffer data measurements as well, not only critical data measurements. In all of these situations, with this patch a new measurement record is added/appended to the measurement list. Please re-write the patch description making it more generic.

Quoted 3 lines

For example, I would start with something like, "IMA does not include duplicate file, buffer or critical data measurement records ..."

Agreed.
I will generalize the description further and send the v3 for review.

Thanks,
Tushar

Quoted 9 lines

thanks,

Mimi

Quoted 5 lines

Update ima_add_template_entry() to support measurement of duplicate data, driven by a Kconfig option - IMA_DISABLE_HTABLE.

maintainer
#3 lore raw
Quoted 28 lines · Tushar Sugandhi

Thanks for the feedback Mimi.
Appreciate it.

Quoted 5 lines · Mimi Zohar

Hi Tushar,

The Subject line could be improved. Perhaps something like - "IMA:
support for duplicate measurement records"

Will do.

Quoted 17 lines · Tushar Sugandhi
Quoted 8 lines

IMA does not measure duplicate data since TPM extend is a very expensive operation. However, in some cases, the measurement of duplicate data is necessary to accurately determine the current state of the system. Eg, SELinux state changing from 'audit', to 'enforcing', and back to 'audit' again. In this example, currently, IMA will not measure the last state change to 'audit'. This limits the ability of attestation services to accurately determine the current state of the measurements on the system.

This patch description is written from your specific usecase perspective, but it impacts file and buffer data measurements as well, not only critical data measurements. In all of these situations, with this patch a new measurement record is added/appended to the measurement list. Please re-write the patch description making it more generic.

Quoted 5 lines
Quoted 3 lines

For example, I would start with something like, "IMA does not include duplicate file, buffer or critical data measurement records ..."

Agreed.
I will generalize the description further and send the v3 for review.

It would be good to boot with the ima_policy=tcb policy with/without
your patch and account for the different number of measurements. Are
all the differences related to duplicate measurements - original file hash -> new file hash -> original file hash - similar to what you described.

thanks,

Mimi

#4 lore raw
Quoted 32 lines · Mimi Zohar
Quoted 31 lines · Tushar Sugandhi

Thanks for the feedback Mimi.
Appreciate it.

Quoted 5 lines · Mimi Zohar

Hi Tushar,

The Subject line could be improved. Perhaps something like - "IMA:
support for duplicate measurement records"

Will do.

Quoted 20 lines · Tushar Sugandhi
Quoted 8 lines

IMA does not measure duplicate data since TPM extend is a very expensive operation. However, in some cases, the measurement of duplicate data is necessary to accurately determine the current state of the system. Eg, SELinux state changing from 'audit', to 'enforcing', and back to 'audit' again. In this example, currently, IMA will not measure the last state change to 'audit'. This limits the ability of attestation services to accurately determine the current state of the measurements on the system.

This patch description is written from your specific usecase perspective, but it impacts file and buffer data measurements as well, not only critical data measurements. In all of these situations, with this patch a new measurement record is added/appended to the measurement list. Please re-write the patch description making it more generic.

For example, I would start with something like, "IMA does not include duplicate file, buffer or critical data measurement records ..."

Quoted 9 lines
Quoted 2 lines

Agreed.
I will generalize the description further and send the v3 for review.

It would be good to boot with the ima_policy=tcb policy with/without
your patch and account for the different number of measurements. Are
all the differences related to duplicate measurements - original file hash -> new file hash -> original file hash - similar to what you described.

Thanks for the ima_policy=tcb pointer.

I tested my patch with:
- duplicate buffer content for "measure func=CRITICAL_DATA"
- and reading the same file twice with "measure func=FILE_CHECK
mask=MAY_READ"

In both the above use cases, IMA is measuring the duplicate entries with the patch, and not measuring the duplicate entries w/o the patch.

I will test the "ima_policy=tcb" boot-scenario as you suggested, before posting the next version.

Thanks,
Tushar

Quoted 4 lines

thanks,

Mimi

#5 lore raw

Hi Mimi,

Quoted 46 lines · Tushar Sugandhi
Quoted 32 lines · Mimi Zohar
Quoted 31 lines · Tushar Sugandhi

Thanks for the feedback Mimi.
Appreciate it.

Quoted 5 lines · Mimi Zohar

Hi Tushar,

The Subject line could be improved.  Perhaps something like - "IMA:
support for duplicate measurement records"

Will do.

Quoted 20 lines · Tushar Sugandhi
Quoted 8 lines

IMA does not measure duplicate data since TPM extend is a very expensive operation.  However, in some cases, the measurement of duplicate data is necessary to accurately determine the current state of the system. Eg, SELinux state changing from 'audit', to 'enforcing', and back to 'audit' again.  In this example, currently, IMA will not measure the last state change to 'audit'.  This limits the ability of attestation services to accurately determine the current state of the measurements on the system.

This patch description is written from your specific usecase perspective, but it impacts file and buffer data measurements as well, not only critical data measurements.  In all of these situations, with this patch a new measurement record is added/appended to the measurement list.  Please re-write the patch description making it more generic.

For example, I would start with something like, "IMA does not include duplicate file, buffer or critical data measurement records ..."

Quoted 9 lines
Quoted 2 lines

Agreed.
I will generalize the description further and send the v3 for review.

It would be good to boot with the ima_policy=tcb policy with/without your patch and account for the different number of measurements.   Are all the differences related to duplicate measurements - original file hash -> new file hash -> original file hash - similar to what you described.

Thanks for the ima_policy=tcb pointer.

Quoted 9 lines

I tested my patch with:
 - duplicate buffer content for "measure func=CRITICAL_DATA"
 - and reading the same file twice with "measure func=FILE_CHECK
mask=MAY_READ"

In both the above use cases, IMA is measuring the duplicate entries with the patch, and not measuring the duplicate entries w/o the patch.

I will test the "ima_policy=tcb" boot-scenario as you suggested, before posting the next version.

I booted the system with "ima_policy=tcb" policy with/without my patch. I also removed /etc/ima/ima-policy for testing these use-cases.
(so that it wouldn't override the policy generated by boot param "ima_policy=tcb").

I double checked the contents of the kernel policy:
#cat /sys/kernel/security/integrity/ima/policy

    dont_measure fsmagic=0x9fa0
    dont_measure fsmagic=0x62656572
    dont_measure fsmagic=0x64626720
    dont_measure fsmagic=0x1021994
    dont_measure fsmagic=0x1cd1
    dont_measure fsmagic=0x42494e4d
    dont_measure fsmagic=0x73636673
    dont_measure fsmagic=0xf97cff8c
    dont_measure fsmagic=0x43415d53
    dont_measure fsmagic=0x27e0eb
    dont_measure fsmagic=0x63677270
    dont_measure fsmagic=0x6e736673
    dont_measure fsmagic=0xde5e81e4
    measure func=MMAP_CHECK mask=MAY_EXEC
    measure func=BPRM_CHECK mask=MAY_EXEC
    measure func=FILE_CHECK mask=^MAY_READ euid=0
    measure func=FILE_CHECK mask=^MAY_READ uid=0
    measure func=MODULE_CHECK
    measure func=FIRMWARE_CHECK
    measure func=POLICY_CHECK

And then I compared the contents of the ascii_runtime_measurements with and without my patch.

And here are my findings:

(1) Files like systemd-udevd, x2go_sessions etc. get measured multiple times with the CONFIG_IMA_DISABLE_HTABLE=y.
They only get measured once with the config "=n".

10 668df8723f5a1f57a0afe3b50d44054d66363f3e ima-ng
sha1:51f66e82421b93b21ad1e0a25e5efa4155c6a8e0 /lib/systemd/systemd-udevd

    10 668df8723f5a1f57a0afe3b50d44054d66363f3e ima-ng 

sha1:51f66e82421b93b21ad1e0a25e5efa4155c6a8e0 /lib/systemd/systemd-udevd

(2) There are lot more instances of /tmp/<random> measurement records

    with the CONFIG_IMA_DISABLE_HTABLE=y.
    Eg,

    10 33515851cfee4acbf24de9482ff018d33def1083 ima-ng 
sha1:da39a3ee5e6b4b0d3255bfef95601890afd80709 /tmp/oUWCVeypLR
    10 9d1dc0e1e54ee2e16308a824fc5780bd21b38208 ima-ng 
sha1:da39a3ee5e6b4b0d3255bfef95601890afd80709 /tmp/etX8dy7qqy
    10 8643a5543179b86c02d7e3e01e16b3bd2f8dbb9f ima-ng 
sha1:da39a3ee5e6b4b0d3255bfef95601890afd80709 /tmp/I4zTWEuyMf
    10 56e9547a4ed39036d2e790cfad78b467aa979e32 ima-ng 

sha1:da39a3ee5e6b4b0d3255bfef95601890afd80709 /tmp/Lh5wDm6_Ep

I believe both the observations are consistent with the expected outcome of the patch.

Please let me know if I should test any other scenario.

I will shortly post the v3 patch with updates to description and title as you suggested.

Thanks,
Tushar

Quoted 7 lines

Thanks,
Tushar

Quoted 4 lines

thanks,

Mimi

maintainer
#6 lore raw
Quoted 111 lines · Tushar Sugandhi
Quoted 50 lines · Tushar Sugandhi
Quoted 33 lines · Mimi Zohar
Quoted 32 lines · Tushar Sugandhi

Thanks for the feedback Mimi.
Appreciate it.

Quoted 5 lines · Mimi Zohar

Hi Tushar,

The Subject line could be improved. Perhaps something like - "IMA:
support for duplicate measurement records"

Will do.

Quoted 21 lines · Tushar Sugandhi
Quoted 9 lines

IMA does not measure duplicate data since TPM extend is a very expensive
operation. However, in some cases, the measurement of duplicate data is necessary to accurately determine the current state of the system. Eg, SELinux state changing from 'audit', to 'enforcing', and back to 'audit' again. In this example, currently, IMA will not measure the last state change to 'audit'. This limits the ability of attestation services to accurately determine the current state of the measurements on the system.

This patch description is written from your specific usecase perspective, but it impacts file and buffer data measurements as well, not only critical data measurements. In all of these situations, with this patch a new measurement record is added/appended to the measurement list. Please re-write the patch description making it more generic.

For example, I would start with something like, "IMA does not include duplicate file, buffer or critical data measurement records ..."

Quoted 9 lines
Quoted 2 lines

Agreed.
I will generalize the description further and send the v3 for review.

It would be good to boot with the ima_policy=tcb policy with/without
your patch and account for the different number of measurements. Are
all the differences related to duplicate measurements - original file hash -> new file hash -> original file hash - similar to what you described.

Thanks for the ima_policy=tcb pointer.

I tested my patch with:
- duplicate buffer content for "measure func=CRITICAL_DATA"
- and reading the same file twice with "measure func=FILE_CHECK
mask=MAY_READ"

Quoted 6 lines

In both the above use cases, IMA is measuring the duplicate entries with the patch, and not measuring the duplicate entries w/o the patch.

I will test the "ima_policy=tcb" boot-scenario as you suggested, before posting the next version.

I booted the system with "ima_policy=tcb" policy with/without my patch. I also removed /etc/ima/ima-policy for testing these use-cases. (so that it wouldn't override the policy generated by boot param "ima_policy=tcb").

I double checked the contents of the kernel policy:
#cat /sys/kernel/security/integrity/ima/policy

     dont_measure fsmagic=0x9fa0
     dont_measure fsmagic=0x62656572
     dont_measure fsmagic=0x64626720
     dont_measure fsmagic=0x1021994
     dont_measure fsmagic=0x1cd1
     dont_measure fsmagic=0x42494e4d
     dont_measure fsmagic=0x73636673
     dont_measure fsmagic=0xf97cff8c
     dont_measure fsmagic=0x43415d53
     dont_measure fsmagic=0x27e0eb
     dont_measure fsmagic=0x63677270
     dont_measure fsmagic=0x6e736673
     dont_measure fsmagic=0xde5e81e4
     measure func=MMAP_CHECK mask=MAY_EXEC
     measure func=BPRM_CHECK mask=MAY_EXEC
     measure func=FILE_CHECK mask=^MAY_READ euid=0
     measure func=FILE_CHECK mask=^MAY_READ uid=0
     measure func=MODULE_CHECK
     measure func=FIRMWARE_CHECK
     measure func=POLICY_CHECK

And then I compared the contents of the ascii_runtime_measurements with and without my patch.

And here are my findings:

(1) Files like systemd-udevd, x2go_sessions etc. get measured multiple times with the CONFIG_IMA_DISABLE_HTABLE=y.
They only get measured once with the config "=n".

10 668df8723f5a1f57a0afe3b50d44054d66363f3e ima-ng
sha1:51f66e82421b93b21ad1e0a25e5efa4155c6a8e0 /lib/systemd/systemd-udevd

     10 668df8723f5a1f57a0afe3b50d44054d66363f3e ima-ng 

sha1:51f66e82421b93b21ad1e0a25e5efa4155c6a8e0 /lib/systemd/systemd-udevd

(2) There are lot more instances of /tmp/<random> measurement records

     with the CONFIG_IMA_DISABLE_HTABLE=y.
     Eg,

     10 33515851cfee4acbf24de9482ff018d33def1083 ima-ng 
sha1:da39a3ee5e6b4b0d3255bfef95601890afd80709 /tmp/oUWCVeypLR
     10 9d1dc0e1e54ee2e16308a824fc5780bd21b38208 ima-ng 
sha1:da39a3ee5e6b4b0d3255bfef95601890afd80709 /tmp/etX8dy7qqy
     10 8643a5543179b86c02d7e3e01e16b3bd2f8dbb9f ima-ng 
sha1:da39a3ee5e6b4b0d3255bfef95601890afd80709 /tmp/I4zTWEuyMf
     10 56e9547a4ed39036d2e790cfad78b467aa979e32 ima-ng 
Quoted 4 lines

sha1:da39a3ee5e6b4b0d3255bfef95601890afd80709 /tmp/Lh5wDm6_Ep

I believe both the observations are consistent with the expected outcome of the patch.

These measurement records are not a result of the scenario described in the patch description, but as the result of different inodes being
measured. In the first case, the measurement is most likely coming
from the initramfs, while the second measurement is after pivot root. Without digging, I assume the second example is the file hash of an empty file.

Quoted 5 lines

Please let me know if I should test any other scenario.

I will shortly post the v3 patch with updates to description and title as you suggested.

The patch description is much better, but doesn't address the duplicate
measurements without the file changing in between. Please really
compare the before and after measurement record changes, making sure you understand the cause for all the duplicate measurement records. Based on the results, please update the patch description appropriately.

thanks,

Mimi