[PATCH v4] IMA: support for duplicate measurement records

[PATCH v4] IMA: support for duplicate measurement records

[Tushar Sugandhi]

IMA measures contents of a given file/buffer/critical-data record,
and properly re-measures it on change.  However, IMA does not measure
the duplicate value for a given record, since TPM extend is a very
expensive operation.  For example, if the record changes from value
'v#1' to 'v#2', and then back to 'v#1', IMA will not measure and log
the last change to 'v#1', since the hash of 'v#1' for that record is
already present in the IMA htable.  This limits the ability of an
external attestation service to accurately determine the current state
of the system.  The service would incorrectly conclude that the latest
value of the given record on the system is 'v#2', and act accordingly.

Define and use a new Kconfig option IMA_DISABLE_HTABLE to permit
duplicate records in the IMA measurement list.

In addition to the duplicate measurement records described above,
other duplicate file measurement records may be included in the log,
when CONFIG_IMA_DISABLE_HTABLE=y.
For example,
    - i_version is not enabled,
    - i_generation changed,
    - an inode is evicted from dcache etc.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---
Change Log v4:
 - Incorporated feedback from Mimi on patch description.
 - Tested more use cases for i_version, i_generation, and dcache.

Change Log v3:
 - Incorporated feedback from Mimi on v2.
 - Updated patch title and description to make it generic.
 - Changed config description word 'data' to 'records'.
 - Tested use cases for boot param "ima_policy=tcb".

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(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 12e9250c1bec..d0ceada99243 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -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 records"
+	depends on IMA
+	default n
+	help
+	   This option disables htable to allow measurement of duplicate records.
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index c096ef8945c7..532da87ce519 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -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;
-- 
2.17.1

Re: [PATCH v4] IMA: support for duplicate measurement records

[Mimi Zohar]

Hi Tushar,

On Mon, 2021-05-10 at 12:09 -0700, Tushar Sugandhi wrote:
> IMA measures contents of a given file/buffer/critical-data record,
> and properly re-measures it on change.  However, IMA does not measure
> the duplicate value for a given record, since TPM extend is a very
> expensive operation.  For example, if the record changes from value
> 'v#1' to 'v#2', and then back to 'v#1', IMA will not measure and log
> the last change to 'v#1', since the hash of 'v#1' for that record is
> already present in the IMA htable.  This limits the ability of an
> external attestation service to accurately determine the current state
> of the system.  The service would incorrectly conclude that the latest
> value of the given record on the system is 'v#2', and act accordingly.
> 
> Define and use a new Kconfig option IMA_DISABLE_HTABLE to permit
> duplicate records in the IMA measurement list.
> 
> In addition to the duplicate measurement records described above,
> other duplicate file measurement records may be included in the log,
> when CONFIG_IMA_DISABLE_HTABLE=y.
> For example,
>     - i_version is not enabled,
>     - i_generation changed,
>     - an inode is evicted from dcache etc.

Missing from this list are the same file, perhaps on different
filesystmes, such as initramfs and real root.  These can be identified
by the different i_ino.  Is there anything else?

thanks,

Mimi

> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>

Re: [PATCH v4] IMA: support for duplicate measurement records

[Tushar Sugandhi]

Hi Mimi,
Sorry for the late response.
I wanted to spend some time thinking what other scenarios I could test.
Responses below.

On 2021-05-20 1:35 p.m., Mimi Zohar wrote:
> Hi Tushar,
> 
> On Mon, 2021-05-10 at 12:09 -0700, Tushar Sugandhi wrote:
>> IMA measures contents of a given file/buffer/critical-data record,
>> and properly re-measures it on change.  However, IMA does not measure
>> the duplicate value for a given record, since TPM extend is a very
>> expensive operation.  For example, if the record changes from value
>> 'v#1' to 'v#2', and then back to 'v#1', IMA will not measure and log
>> the last change to 'v#1', since the hash of 'v#1' for that record is
>> already present in the IMA htable.  This limits the ability of an
>> external attestation service to accurately determine the current state
>> of the system.  The service would incorrectly conclude that the latest
>> value of the given record on the system is 'v#2', and act accordingly.
>>
>> Define and use a new Kconfig option IMA_DISABLE_HTABLE to permit
>> duplicate records in the IMA measurement list.
>>
>> In addition to the duplicate measurement records described above,
>> other duplicate file measurement records may be included in the log,
>> when CONFIG_IMA_DISABLE_HTABLE=y.
>> For example,
>>      - i_version is not enabled,
>>      - i_generation changed,
>>      - an inode is evicted from dcache etc.
> 
> Missing from this list are the same file, perhaps on different
> filesystmes, such as initramfs and real root.  These can be identified
> by the different i_ino.  Is there anything else?
> 
> thanks,
> 
> Mimi
> 
Sure, I can add the 4th line to the list:

For example,
     - i_version is not enabled,
     - i_generation changed,
     - an inode is evicted from dcache,
     - same file present on different filesystems, with different i_ino
       etc.

Should I spin up v5 of this patch with the updated patch description?

I was also thinking if I should cover any other scenarios, and
soft-links/hard-links seemed like a good scenario to test - so I went
ahead and tested it. And it looks like the patch is working as expected
in this scenario.

Here are the detailed findings:

If I have a file original.txt, and create a soft-link file -
softlink.txt,

(1) only original.txt is reported in IMA log, regardless if I touch
     original.txt or softlink.txt.
     This is true in both cases - when allow_dup is turned on or off.
     (i.e. with or without this patch)
(2) duplicate entries are measured only when allow_dup is turned on.

If I have a file original.txt, and create a hard-link file -
hardlink.txt,
(1) whichever file I touch first, either original.txt or hardlink.txt,
     gets measured in IMA log, both when allow_dup is turned on or off.
     (i.e. with or without this patch)
(2) duplicate entries are measured only when allow_dup is turned on.

Since the the observed behavior is identical in both cases,
soft-links and hard-links (1), and duplicates are measured only when
allow_dup is turned on as expected (2), I don't believe we should call
out soft-links/hard-links in the list above. It is not a special case
like ones mentioned in the list.

Please let me know if you think otherwise.

Thanks,
Tushar
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>> Reviewed-by: Petr Vorel <pvorel@suse.cz>