| Message ID | 20191106190116.2578-2-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KEYS: Measure keys when they are created or updated | expand |
On Wed, 2019-11-06 at 11:01 -0800, Lakshmi Ramasubramanian wrote: > Asymmetric keys used for verifying file signatures or certificates > are currently not included in the IMA measurement list. > > This patch defines a new IMA hook namely ima_post_key_create_or_update() > to measure asymmetric keys. > > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > --- > security/integrity/ima/ima_main.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index d7e987baf127..a0e233afe876 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -721,6 +721,22 @@ void ima_kexec_cmdline(const void *buf, int size) > KEXEC_CMDLINE, 0); > } > > +/** > + * ima_post_key_create_or_update - measure asymmetric keys > + * @keyring: keyring to which the key is linked to > + * @key: created or updated key > + * @flags: key flags > + * @create: flag indicating whether the key was created or updated > + * > + * Keys can only be measured, not appraised. > + */ > +void ima_post_key_create_or_update(struct key *keyring, struct key *key, > + unsigned long flags, bool create) > +{ > + if ((keyring != NULL) && (key != NULL)) > + return; I would move the patch that defines the "keyring=" policy option prior to this one. Include the call to process_buffer_measurement() in this patch. A subsequent patch would add support to defer measuring the key, by calling a function named something like ima_queue_key_measurement(). Mimi > +} > + > static int __init init_ima(void) > { > int error;
On 11/6/2019 2:43 PM, Mimi Zohar wrote: >> +void ima_post_key_create_or_update(struct key *keyring, struct key *key, >> + unsigned long flags, bool create) >> +{ >> + if ((keyring != NULL) && (key != NULL)) >> + return; > > I would move the patch that defines the "keyring=" policy option prior > to this one. Include the call to process_buffer_measurement() in this > patch. A subsequent patch would add support to defer measuring the > key, by calling a function named something like > ima_queue_key_measurement(). > > Mimi As I'd stated in the other response, I wanted to isolate all key related code in a separate C file and build it if and only if all CONFIG dependencies are met. I can do the following: => Define the IMA hook in ima_asymmetric_keys.c instead of ima_main.c => In include/linux/ima.h declare the IMA hook if CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS is enabled. Else, NOP it. #ifdef CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS extern void ima_post_key_create_or_update(struct key *keyring, struct key *key, unsigned long flags, bool create); #else static inline void ima_post_key_create_or_update(struct key *keyring, struct key *key, unsigned long flags, bool create) {} #endif Would that be acceptable? thanks, -lakshmi
On Wed, 2019-11-06 at 16:21 -0800, Lakshmi Ramasubramanian wrote: > On 11/6/2019 2:43 PM, Mimi Zohar wrote: > > >> +void ima_post_key_create_or_update(struct key *keyring, struct key *key, > >> + unsigned long flags, bool create) > >> +{ > >> + if ((keyring != NULL) && (key != NULL)) > >> + return; > > > > I would move the patch that defines the "keyring=" policy option prior > > to this one. Include the call to process_buffer_measurement() in this > > patch. A subsequent patch would add support to defer measuring the > > key, by calling a function named something like > > ima_queue_key_measurement(). > > > > Mimi > > As I'd stated in the other response, I wanted to isolate all key related > code in a separate C file and build it if and only if all CONFIG > dependencies are met. The basic measuring of keys shouldn't be any different than any other policy rule, other than it is a key and not a file. This is the reason that I keep saying start out with the basics and then add support to defer measuring keys on the trusted keyrings. Only the queueing code needed for measuring keys on the trusted keyrings would be in a separate file. Mimi
On 11/6/2019 7:40 PM, Mimi Zohar wrote: >>> I would move the patch that defines the "keyring=" policy option prior >>> to this one. Include the call to process_buffer_measurement() in this >>> patch. A subsequent patch would add support to defer measuring the >>> key, by calling a function named something like >>> ima_queue_key_measurement(). >>> >>> Mimi >> >> As I'd stated in the other response, I wanted to isolate all key related >> code in a separate C file and build it if and only if all CONFIG >> dependencies are met. > > The basic measuring of keys shouldn't be any different than any other > policy rule, other than it is a key and not a file. This is the > reason that I keep saying start out with the basics and then add > support to defer measuring keys on the trusted keyrings. I'll make the changes, rearrange the patches and send an updated set. I do have a few questions since I am still not fully understanding the requirements you are targeting. Appreciate if you could please clarify. As you already know, I am using the "public key" of the given asymmetric key as the "buffer" to measure in process_buffer_measurement(). The measurement decision is not based on whether the keyring is a trusted one or an untrusted one. As long as the IMA policy allows (through the "keyrings=" option) the key will be measured. Do you want only trusted keyrings to be allowed in the measurement? In my opinion, that decision should be deferred to whoever is setting up the IMA policy. > Only the queueing code needed for measuring keys on the trusted > keyrings would be in a separate file. > > Mimi The decision to process key immediately or defer (queue) is based on whether IMA has been initialized or not. Keyring is not used for this decision. Could you please clarify how queuing is related to keyring's trustworthiness? The check for whether the key is an asymmetric one or not, and extracting the "public key" if it is an asymmetric key needs to be in a separate file to handle the CONFIG dependencies in IMA. thanks, -lakshmi
On Thu, 2019-11-07 at 10:42 -0800, Lakshmi Ramasubramanian wrote: > On 11/6/2019 7:40 PM, Mimi Zohar wrote: > > >>> I would move the patch that defines the "keyring=" policy option prior > >>> to this one. Include the call to process_buffer_measurement() in this > >>> patch. A subsequent patch would add support to defer measuring the > >>> key, by calling a function named something like > >>> ima_queue_key_measurement(). > >>> > >> > >> As I'd stated in the other response, I wanted to isolate all key related > >> code in a separate C file and build it if and only if all CONFIG > >> dependencies are met. > > > > The basic measuring of keys shouldn't be any different than any other > > policy rule, other than it is a key and not a file. This is the > > reason that I keep saying start out with the basics and then add > > support to defer measuring keys on the trusted keyrings. > > I'll make the changes, rearrange the patches and send an updated set. > > I do have a few questions since I am still not fully understanding the > requirements you are targeting. Appreciate if you could please clarify. > > As you already know, I am using the "public key" of the given asymmetric > key as the "buffer" to measure in process_buffer_measurement(). > > The measurement decision is not based on whether the keyring is a > trusted one or an untrusted one. As long as the IMA policy allows > (through the "keyrings=" option) the key will be measured. We should be able to measure all keys being loaded onto any keyring or onto a specific "keyring=". This shouldn't be any different than any other policy rule. Once you have this basic feature working, you would address loading keys during early boot. > > Do you want only trusted keyrings to be allowed in the measurement? > In my opinion, that decision should be deferred to whoever is setting up > the IMA policy. Right, but it shouldn't be limited to just "trusted" keyrings. This way you can first test loading keys onto any keyring. > > > Only the queueing code needed for measuring keys on the trusted > > keyrings would be in a separate file. > > > > The decision to process key immediately or defer (queue) is based on > whether IMA has been initialized or not. Keyring is not used for this > decision. > > Could you please clarify how queuing is related to keyring's > trustworthiness? > > The check for whether the key is an asymmetric one or not, and > extracting the "public key" if it is an asymmetric key needs to be in a > separate file to handle the CONFIG dependencies in IMA. Queuing the keys should be independent of measuring the keys. Initially you would start with just measuring the key. From a high level it would look like: ima_post_key_create_or_update(...) { "measure key based on policy(key, keyring, ...)" } This requires the IMA "keyring=" policy option support be defined first. Subsequently you would add key queuing support, and then update ima_post_key_create_or_update(). It would look like: ima_post_key_create_or_update(...) { if (custom policy is loaded) "measure key based on policy(key, keyring, ...)" else "queue key(key, keyring)" } Mimi
On 11/7/19 12:53 PM, Mimi Zohar wrote: >> >> The measurement decision is not based on whether the keyring is a >> trusted one or an untrusted one. As long as the IMA policy allows >> (through the "keyrings=" option) the key will be measured. > > We should be able to measure all keys being loaded onto any keyring or > onto a specific "keyring=". This shouldn't be any different than any > other policy rule. Once you have this basic feature working, you > would address loading keys during early boot. Perfect - that's exactly how I have implemented it right now. Will continue to test it. >> Do you want only trusted keyrings to be allowed in the measurement? >> In my opinion, that decision should be deferred to whoever is setting up >> the IMA policy. > > Right, but it shouldn't be limited to just "trusted" keyrings. This > way you can first test loading keys onto any keyring. Thank you. > Queuing the keys should be independent of measuring the keys. > Initially you would start with just measuring the key. From a high > level it would look like: > > ima_post_key_create_or_update(...) > { > "measure key based on > policy(key, keyring, ...)" > } > > This requires the IMA "keyring=" policy option support be defined > first. > > Subsequently you would add key queuing support, and then update > ima_post_key_create_or_update(). It would look like: > > ima_post_key_create_or_update(...) > { > if (custom policy is loaded) > "measure key based on policy(key, keyring, ...)" > else > "queue key(key, keyring)" > } > > Mimi Yes - I have the above change working. Will continue testing. thanks, -lakshmi
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index d7e987baf127..a0e233afe876 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -721,6 +721,22 @@ void ima_kexec_cmdline(const void *buf, int size) KEXEC_CMDLINE, 0); } +/** + * ima_post_key_create_or_update - measure asymmetric keys + * @keyring: keyring to which the key is linked to + * @key: created or updated key + * @flags: key flags + * @create: flag indicating whether the key was created or updated + * + * Keys can only be measured, not appraised. + */ +void ima_post_key_create_or_update(struct key *keyring, struct key *key, + unsigned long flags, bool create) +{ + if ((keyring != NULL) && (key != NULL)) + return; +} + static int __init init_ima(void) { int error;
Asymmetric keys used for verifying file signatures or certificates are currently not included in the IMA measurement list. This patch defines a new IMA hook namely ima_post_key_create_or_update() to measure asymmetric keys. Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> --- security/integrity/ima/ima_main.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)