| Message ID | 20210510190939.28279-1-tusharsu@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] IMA: support for duplicate measurement records | expand |
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>
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>
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;
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(-)