| Message ID | 20210713004904.8808-1-tusharsu@linux.microsoft.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | device mapper target measurements using IMA | expand |
Thank you for bringing IMA support to device mapper. The addition of dm-verity to IMA is very useful for the project I'm working on where we boot our distribution from removable USB media. One of our goals is to detect tampering of the root file system remotely. Therefore we enabled dm-verity support but implementing remote attestation for dm-verity from userland is not ideal which was our initial plan. This patch set enables us to leverage to already implemented IMA attestation infrastructure by the remote attestation service that we are using (Keylime) without trying to roll a custom solution. We tested the initial RFC patch set and will continue testing with this one to see if it fully works in our environment and with our use case. Thore Sommer -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Hello Thore, On 7/14/21 4:32 AM, Thore Sommer wrote: > Thank you for bringing IMA support to device mapper. The addition of dm-verity > to IMA is very useful for the project I'm working on where we boot > our distribution from removable USB media. Thank you for the positive ack. Appreciate it. > One of our goals is to detect tampering of the root file system remotely. > Therefore we enabled dm-verity support but implementing remote attestation for > dm-verity from userland is not ideal which was our initial plan. Yes, remote attestation from userland is not ideal. > This patch set enables us to leverage to already implemented IMA attestation > infrastructure by the remote attestation service that we are using (Keylime) > without trying to roll a custom solution. I am glad that DM-IMA functionality is useful for your scenario. > We tested the initial RFC patch set and will continue testing with > this one to see if it fully works in our environment and with our use > case. Thank you for testing the RFC patch set. Please let me know if you discover any bugs in this one, or have any other feedback. Thanks again. Regards, Tushar > Thore Sommer -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Mon, Jul 12 2021 at 8:48P -0400, Tushar Sugandhi <tusharsu@linux.microsoft.com> wrote: > For a given system, various external services/infrastructure tools > (including the attestation service) interact with it - both during the > setup and during rest of the system run-time. They share sensitive data > and/or execute critical workload on that system. The external services > may want to verify the current run-time state of the relevant kernel > subsystems before fully trusting the system with business-critical > data/workload. > > Device mapper is one such kernel subsystem that plays a critical role on > a given system by providing various important functionalities to the > block devices with various target types like crypt, verity, integrity > etc. Each of these target types’ functionalities can be configured with > various attributes. The attributes chosen to configure these target types > can significantly impact the security profile of the block device, > and in-turn, of the system itself. For instance, the type of encryption > algorithm and the key size determines the strength of encryption for a > given block device. > > Therefore, verifying the current state of various block devices as well > as their various target attributes is crucial for external services > before fully trusting the system with business-critical data/workload. > > IMA provides the necessary functionality for device mapper to measure the > state and configuration of various block devices - > - BY device mapper itself, from within the kernel, > - in a tamper resistant way, > - and re-measured - triggered on state/configuration change. > > This patch series uses this IMA functionality, by calling the function > ima_measure_critical_data(), when a block device state is changed (e.g. > on device create, resume, rename, remove etc.) It measures the device > state and configuration and stores it in IMA logs, so that it can be > used by external services for managing the system. I needed to EXPORT_SYMBOL_GPL(ima_measure_critical_data); otherwise I couldn't compile.. not sure but I can only imagine you compile DM (and all targets) to be builtin? In addition to fixing that (in first table load patch) I changed various things along the way while I reviewed each patch. Things that I recall are: - moved #ifdef CONFIG_IMA from dm-ima.c to dm-ima.h - fixed various typos and whitespace - consistently prepend "," in STATUSTYPE_IMA's DMEMIT()s as opposed to having a mix or pre and postfix throughout targets - fixed what seemed like malformed STATUSTYPE_IMA handling for dm-multipath -- it was DMEMIT(";") for each dm-mpath's pathgroup - added some fields to dm-mpath, renamed some IMA names in name=value pairs to be more clear. I've staged the result in linux-next via linux-dm.git's dm-5.15 branch, see: https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/log/?h=dm-5.15 I've compiled tested both with and without CONFIG_IMA set. But haven't actually tested the code. Please send any incremental fixes relative to the dm-5.15 branch and I'll get them folded in where appropriate. Thanks, Mike -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Hi Mike, On 7/20/21 2:27 PM, Mike Snitzer wrote: > On Mon, Jul 12 2021 at 8:48P -0400, > Tushar Sugandhi <tusharsu@linux.microsoft.com> wrote: > >> For a given system, various external services/infrastructure tools >> (including the attestation service) interact with it - both during the >> setup and during rest of the system run-time. They share sensitive data >> and/or execute critical workload on that system. The external services >> may want to verify the current run-time state of the relevant kernel >> subsystems before fully trusting the system with business-critical >> data/workload. >> >> Device mapper is one such kernel subsystem that plays a critical role on >> a given system by providing various important functionalities to the >> block devices with various target types like crypt, verity, integrity >> etc. Each of these target types’ functionalities can be configured with >> various attributes. The attributes chosen to configure these target types >> can significantly impact the security profile of the block device, >> and in-turn, of the system itself. For instance, the type of encryption >> algorithm and the key size determines the strength of encryption for a >> given block device. >> >> Therefore, verifying the current state of various block devices as well >> as their various target attributes is crucial for external services >> before fully trusting the system with business-critical data/workload. >> >> IMA provides the necessary functionality for device mapper to measure the >> state and configuration of various block devices - >> - BY device mapper itself, from within the kernel, >> - in a tamper resistant way, >> - and re-measured - triggered on state/configuration change. >> >> This patch series uses this IMA functionality, by calling the function >> ima_measure_critical_data(), when a block device state is changed (e.g. >> on device create, resume, rename, remove etc.) It measures the device >> state and configuration and stores it in IMA logs, so that it can be >> used by external services for managing the system. > I needed to EXPORT_SYMBOL_GPL(ima_measure_critical_data); otherwise I > couldn't compile.. not sure but I can only imagine you compile DM > (and all targets) to be builtin? I believe the EXPORT_SYMBOL_GPL(ima_measure_critical_data) is now needed because of the move “#ifdef CONFIG_IMA from dm-ima.c to dm-ima.h”, and the associated change in the Makefile +ifeq ($(CONFIG_IMA),y) dm-mod-objs += dm-ima.o +endif Earlier I needed to export symbol only if I was calling ima_measure_critical_data() directly from various dm-*.c files. With my earlier implementation, it wasn’t needed when it was called from the wrapper dm_ima_measure_data() in dm-ima.c. > In addition to fixing that (in first table load patch) I changed > various things along the way while I reviewed each patch. Thank you so much for making the necessary changes Mike. Really appreciate it. > Things that I recall are: > - moved #ifdef CONFIG_IMA from dm-ima.c to dm-ima.h > - fixed various typos and whitespace > - consistently prepend "," in STATUSTYPE_IMA's DMEMIT()s as opposed to > having a mix or pre and postfix throughout targets > - fixed what seemed like malformed STATUSTYPE_IMA handling for > dm-multipath -- it was DMEMIT(";") for each dm-mpath's pathgroup > - added some fields to dm-mpath, renamed some IMA names in name=value > pairs to be more clear. I went through the changes you made. They look fine. In addition to the above changes, you also fixed a warning in dm-raid.c. (initializing the recovery variable) Thanks again. :) > I've staged the result in linux-next via linux-dm.git's dm-5.15 > branch, see: > https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/log/?h=dm-5.15 > > I've compiled tested both with and without CONFIG_IMA set. But > haven't actually tested the code. No worries. I will test the changesthoroughly again. > Please send any incremental fixes relative to the dm-5.15 branch and > I'll get them folded in where appropriate. Ok. I will send incremental patches against dm-5.15 as you've suggested. Thanks again for the review and all the help so far. Really appreciate it. ~Tushar > Thanks, > Mike -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Hello Tushar, I've now tested your patches more in depth. I've used the latest changes from the dm-5.15 branch. Here are some things that I noticed during testing with dm-verity. I'm relative new to IMA and device mapper, so there are also some more general questions. No new IMA measurement is generated if dm-verity verification fails. This is unfortunate because to make the dm-verity measurements useful we need to be notified when hash_failed is set to true. We will need something like "device_update" where we remeasure the device state if it has changed. Creating a dm-verity device with mount then removing it and now if you create it again no measurement is generated. Is that the expected behavior? I would expect that new measurements for "table_load" and "device_resume" are created even if the entries are identical the other ones before. There is no way to verify if the root hash was verified against a signature. We have "root_hash_sig_key_desc SIGNATURE_DESCRIPTION" in the dm table. "SIGNATURE_DESCRIPTION" itself is not really useful because it seems that we cannot map it back to the certificate that was used for verification but the presence of "root_hash_sig_key_desc" might be enough in combination with measuring the keyring. Is there a reason that suspend is not measured? How is the measured uuid created? The format seems to be "CRYPT-VERITY-UUID-NAME" where UUID is uuid from the verity device and NAME is the device mapper name. Does this naming come from the kernel or libcryptsetup? What can happen in between a "table_load" and "device_resume" that is security relevant? For remote attestation services it would be nice if we have clear indicator from what component the "ima-buf" entry was generated. Prefixing all "n-ng" field entries with something like "dm_" would make it easier for us to add different validators for different measurements that use the "ima-buf" template. The keyring measurements already use "ima-buf" and using some kind of naming scheme to easily differentiate the entries would be nice. Not directly device mapper related, but it would be nice to also measure the mount points and a mapping to the device IDs. The reasoning is that we can measure if the target is correct but not if it was mounted correctly. In our case we can verify the trust of our initramfs that creates the dm-verity device and then mounts it and if this fails refuses to boot, but that might not always be the case. Regards, Thore -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Tue, Jul 27, 2021 at 12:18:02PM +0200, Thore Sommer wrote: > No new IMA measurement is generated if dm-verity verification fails. This is > unfortunate because to make the dm-verity measurements useful we need to be > notified when hash_failed is set to true. We will need something like > "device_update" where we remeasure the device state if it has changed. Measurements in the current patchset are only triggered by ioctl calls initiated by userspace. Having other triggering mechanisms - such as hooking into internal events when something unexpected happens - could be considered for follow-on patches. > Creating a dm-verity device with mount then removing it and now if you create it > again no measurement is generated. Is that the expected behavior? Each of the relevant dm ioctls should be logged separately each time. If that's not happening it might need fixing. > Is there a reason that suspend is not measured? A suspend doesn't change the configuration so falls outside the criteria for this patch set. (If there is some need for it, it would be covered by the need to measure internal events that I mentioned above.) > What can happen in between a "table_load" and "device_resume" that is security > relevant? The table prepared by the load can be cleared. That would change the effect of the resume. > Not directly device mapper related, but it would be nice to also measure the > mount points and a mapping to the device IDs. Again, that would be for future patches building on these ones. Alasdair -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Hi Thore, On 7/27/21 1:33 PM, Alasdair G Kergon wrote: >> Creating a dm-verity device with mount then removing it and now if you create it >> again no measurement is generated. Is that the expected behavior? > Each of the relevant dm ioctls should be logged separately each time. If that's > not happening it might need fixing. > Most likely this is because you haven't set CONFIG_IMA_DISABLE_HTABLE=y. See "IMA: support for duplicate measurement records" [1] for details. Please let us know if you still see this behavior after setting CONFIG_IMA_DISABLE_HTABLE=y. Thanks, Tushar [1] https://github.com/torvalds/linux/commit/52c208397c246f0c31d031eb8c41f9c7e9fdec0e -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Hi Tushar,
> Most likely this is because you haven't set CONFIG_IMA_DISABLE_HTABLE=y.
Yes, that was the case.
With CONFIG_IMA_DISABLE_HTABLE=y the behavior is as expected. Now a new
measurement is created if I create the same device twice.
Regards,
Thore
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Hi Alasdair, thank you for answering my questions. The answers helped a lot. > Having other triggering mechanisms - such as hooking into internal > events when something unexpected happens - could be considered for > follow-on patches. I've written [1] a proof of concept implementation that provides a hook for targets to remeasure their state. This was my first attempt at writing kernel code, so there might be some serious bugs. With something like this implemented remote attestation for state of dm-verity should be possible. The output if a target calls that hook is: device_update DEVICE_METADATA;TARGET_METADATA,TARGET_STATUS Regards, Thore [1] diff --git a/drivers/md/dm-ima.c b/drivers/md/dm-ima.c index 2d796e439bee..b743a2e9add4 100644 --- a/drivers/md/dm-ima.c +++ b/drivers/md/dm-ima.c @@ -703,3 +703,84 @@ void dm_ima_measure_on_device_rename(struct mapped_device *md) kfree(new_dev_name); kfree(new_dev_uuid); } + +/* + * Give the option for targets to remeasure on state change. + */ +void dm_ima_measure_on_target_update(struct dm_target *ti){ + char *ima_buf = NULL, *target_metadata_buf = NULL, *target_data_buf= NULL; + status_type_t type = STATUSTYPE_IMA; + size_t target_metadata_buf_len, target_data_buf_len; + unsigned int num_targets, i; + struct dm_table *table = ti->table; + struct mapped_device *md = table->md; + unsigned int target_num = -1; + bool noio = true; + int l = 0; + + ima_buf = dm_ima_alloc(DM_IMA_MEASUREMENT_BUF_LEN, GFP_KERNEL, noio); + if (!ima_buf) + return; + + target_metadata_buf = dm_ima_alloc(DM_IMA_TARGET_METADATA_BUF_LEN, GFP_KERNEL, noio); + if (!target_metadata_buf) + goto exit; + + target_data_buf = dm_ima_alloc(DM_IMA_TARGET_DATA_BUF_LEN, GFP_KERNEL, noio); + if (!target_data_buf) + goto exit; + + + num_targets = dm_table_get_num_targets(table); + + // Get target number + for (i = 0; i < num_targets; i++){ + struct dm_target *ti2 = dm_table_get_target(table, i); + if (!ti) + goto exit; + if (ti == ti2){ + target_num = i; + break; + } + } + if (target_num == -1) + goto exit; + + /* + * First retrieve the target metadata. + */ + scnprintf(target_metadata_buf, DM_IMA_TARGET_METADATA_BUF_LEN, + "target_index=%d,target_begin=%llu,target_len=%llu,", + target_num, ti->begin, ti->len); + target_metadata_buf_len = strlen(target_metadata_buf); + + /* + * Then retrieve the actual target data. + */ + if (ti->type->status) + ti->type->status(ti, type, STATUSTYPE_IMA, target_data_buf, + DM_IMA_TARGET_DATA_BUF_LEN); + else + target_data_buf[0] = '\0'; + target_data_buf_len = strlen(target_data_buf); + + // Add the device metadata + memcpy(ima_buf + l, md->ima.active_table.device_metadata, md->ima.active_table.device_metadata_len); + l += md->ima.active_table.device_metadata_len; + + // Add the target metadata + memcpy(ima_buf + l, target_metadata_buf, target_metadata_buf_len); + l += target_metadata_buf_len; + + // Add the new target state + memcpy(ima_buf + l, target_data_buf, target_data_buf_len); + + dm_ima_measure_data("device_update", ima_buf, strlen(ima_buf), noio); + +exit: + kfree(ima_buf); + kfree(target_data_buf); + kfree(target_metadata_buf); +} + +EXPORT_SYMBOL_GPL(dm_ima_measure_on_target_update); \ No newline at end of file diff --git a/drivers/md/dm-ima.h b/drivers/md/dm-ima.h index 6e6f18bf05b4..f90c4147277a 100644 --- a/drivers/md/dm-ima.h +++ b/drivers/md/dm-ima.h @@ -53,6 +53,7 @@ void dm_ima_measure_on_device_resume(struct mapped_device *md, bool swap); void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all); void dm_ima_measure_on_table_clear(struct mapped_device *md, bool new_map); void dm_ima_measure_on_device_rename(struct mapped_device *md); +void dm_ima_measure_on_target_update(struct dm_target *ti); #else @@ -62,6 +63,7 @@ static inline void dm_ima_measure_on_device_resume(struct mapped_device *md, boo static inline void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all) {} static inline void dm_ima_measure_on_table_clear(struct mapped_device *md, bool new_map) {} static inline void dm_ima_measure_on_device_rename(struct mapped_device *md) {} +static inline void dm_ima_measure_on_target_update(struct dm_target *ti) {} #endif /* CONFIG_IMA */ diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c index bfefa100c265..f0764dd8bd4d 100644 --- a/drivers/md/dm-verity-target.c +++ b/drivers/md/dm-verity-target.c @@ -16,6 +16,7 @@ #include "dm-verity.h" #include "dm-verity-fec.h" #include "dm-verity-verify-sig.h" +#include "dm-ima.h" #include <linux/module.h> #include <linux/reboot.h> @@ -217,10 +218,15 @@ static int verity_handle_err(struct dm_verity *v, enum verity_block_type type, char *envp[] = { verity_env, NULL }; const char *type_str = ""; struct mapped_device *md = dm_table_get_md(v->ti->table); + int old_hash_failed = v->hash_failed; /* Corruption should be visible in device status in all modes */ v->hash_failed = 1; + if (!old_hash_failed){ + dm_ima_measure_on_target_update(v->ti); + } + if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS) goto out;
On Tue, Jul 27, 2021 at 12:18:02PM +0200, Thore Sommer wrote: > How is the measured uuid created? The format seems to be > "CRYPT-VERITY-UUID-NAME" where UUID is uuid from the verity device and NAME is > the device mapper name. Does this naming come from the kernel or libcryptsetup? See libdevmapper.h: /* * Configure default UUID prefix string. * Conventionally this is a short capitalised prefix indicating the subsystem * that is managing the devices, e.g. "LVM-" or "MPATH-". * To support stacks of devices from different subsystems, recursive functions * stop recursing if they reach a device with a different prefix. */ int dm_set_uuid_prefix(const char *uuid_prefix); Each device-mapper device may have a uuid of up to 128 characters plus trailing NUL. Whichever piece software activates the device assigns the uuid (so userspace or kernel boot parameters). By convention each such piece of software uses a short prefix ending with a hyphen that identifies that software as the "owner" (manager) of that dm device. This means each piece of software can easily filter out the devices for which it is responsible and ignore all the others etc. It can use the remainder of the UUID to identify the device uniquely to itself. Another convention is that when one device is a 'wrapper' of some sort around another, it may create the uuid by adding its prefix to the uuid of the device it is wrapping - this might give you stacked prefixes. When there's a complex one-composed-from-many device structure, suffices may be used to identify the components. Think of the 'name' as the human-friendly device name and the uuid as a software-friendly internal name. Alasdair -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On 7/28/21 10:14 AM, Thore Sommer wrote: > Hi Tushar, > >> Most likely this is because you haven't set CONFIG_IMA_DISABLE_HTABLE=y. > Yes, that was the case. > > With CONFIG_IMA_DISABLE_HTABLE=y the behavior is as expected. Now a new > measurement is created if I create the same device twice. > > Regards, > Thore > Thanks for confirming Thore. :) Regards, Tushar -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Hi Thore, Replying to a few questions which were not already answered by me/Alasdair. On 7/27/21 3:18 AM, Thore Sommer wrote: > There is no way to verify if the root hash was verified against a signature. We > have "root_hash_sig_key_desc SIGNATURE_DESCRIPTION" in the dm table. > "SIGNATURE_DESCRIPTION" itself is not really useful because it seems that we > cannot map it back to the certificate that was used for verification but the > presence of "root_hash_sig_key_desc" might be enough in combination with > measuring the keyring. Thanks for the suggestion Thore. I can update the verity_status() to measure if v->signature_key_desc is set. Something like: DMEMIT("signature_key_desc_present=%c,", v->signature_key_desc ? 'y' : 'n'); Alasdair, Mike, Can you tell if this is needed and/or sufficient? If it is needed, should we log the full string v->signature_key_desc? I am concerned about logging the full string as it is an unbounded buffer (a char*) coming from UM. And at the same time, not sure if just logging the presence is sufficient. Thoughts? Thore, Please note – even if we measure signature_key_desc (full string or just its presence): in order to use it with the keyrings, the IMA policy also needs to be set to measure key rings (using “measure func=KEY_CHECK ...”). It is independent from measuring the device mapper data (which is measured when the policy is set to “measure func=CRITICAL_DATA label=device-mapper ...”). Therefore measuring keyrings together (i.e. in the same IMA log) with DM data is not always guaranteed, since it is dictated by how the IMA policy is configured. Just FYI. > For remote attestation services it would be nice if we have clear indicator from > what component the "ima-buf" entry was generated. Prefixing all "n-ng" field > entries with something like "dm_" would make it easier for us to add different > validators for different measurements that use the "ima-buf" template. The > keyring measurements already use "ima-buf" and using some kind of naming scheme > to easily differentiate the entries would be nice. The event names typically come from kernel components that are doing the measurement of critical data. So any duplicates should be caught in the upstream review of the kernel patch. But thanks for the suggestion. I will prefix the event names in this patch series with “dm_” to indicate they are related to device mapper. Thanks, Tushar > Regards, > Thore > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Hi Tushar, thank you for answering my questions and looking at my suggestions. > I can update the verity_status() to measure if v->signature_key_desc is > set. > > Something like: > DMEMIT("signature_key_desc_present=%c,", v->signature_key_desc ? 'y' : > 'n'); If my understanding that this entry is only set if the signature was validated is correct then this should work. > Please note – even if we measure signature_key_desc (full string or just > its presence): in order to use it with the keyrings, the IMA policy also > needs to be set to measure key rings (using "measure func=KEY_CHECK > ..."). It is independent from measuring the device mapper data (which is > measured when the policy is set to “measure func=CRITICAL_DATA > label=device-mapper ..."). > > Therefore measuring keyrings together (i.e. in the same IMA log) with DM > data is not always guaranteed, since it is dictated by how the IMA > policy is configured. Thanks for pointing that out. Currently we don't measure the keyrings but when we enable remote attestation for dm-verity we'll make sure that our IMA policy also measures the keyrings. Regards, Thore -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Hi Alasdair, thank you for the explanation and pointing me into the right direction. I was wondering how we can differentiate the different devices in the remote attestation service and now I think that I understand the format of the uuids, so that we can implement a parser/validator for that. Regards, Thore -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel