| Message ID | 20201101222626.6111-3-tusharsu@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | IMA: Infrastructure for measurement of critical kernel data | expand |
Hi Tushar, Please don't include the filename in the Subject line[1]. The Subject line should be a summary phrase describing the patch. In this case, it is adding support for measuring the buffer data hash. On Sun, 2020-11-01 at 14:26 -0800, Tushar Sugandhi wrote: > process_buffer_measurement() currently only measures the input buffer. > In case of SeLinux policy measurement, the policy being measured could > be large (several MB). This may result in a large entry in IMA > measurement log. SELinux is an example of measuring large buffer data. Please rewrite this patch description (and the other patch descriptions in this patch set) without using the example to describe its purpose [1]. In this case, you might say, The original IMA buffer data measurement sizes were small (e.g. boot command line), but new buffer data measurement use cases are a lot larger. Just as IMA measures the file data hash, not the file data, IMA should similarly support measuring the buffer data hash. > > Introduce a boolean parameter measure_buf_hash to support measuring > hash of a buffer, which would be much smaller, instead of the buffer > itself. > To use the functionality introduced in this patch, the attestation > client and the server changes need to go hand in hand. The > client/kernel would know what data is being measured as-is > (e.g. KEXEC_CMDLINE), and what data has it’s hash measured (e.g. SeLinux > Policy). And the attestation server should verify data/hash accordingly. > > Just like the data being measured in other cases, the attestation server > will know what are possible values of the large buffers being measured. > e.g. the possible valid SeLinux policy values that are being pushed to > the client. The attestation server will have to maintain the hash of > those buffer values. Each patch in the patch set builds upon the previous one. (Think of it as a story, where each chapter builds upon the previous ones.) With rare exceptions, should patches reference subsequent patches. [2] [1] Refer to Documentation/process/submitting-patches.rst [2] Refer to the section "8) Commenting" in Documentation/process/coding-style.rst thanks, Mimi
Hi Tushar, Below inline are a few additional comments. > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index ae5da9f3339d..4485d87c0aa5 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -787,12 +787,15 @@ int ima_post_load_data(char *buf, loff_t size, > * @func: IMA hook > * @pcr: pcr to extend the measurement > * @func_data: private data specific to @func, can be NULL. > + * @measure_buf_hash: if set to true - will measure hash of the buf, > + * instead of buf > * > * Based on policy, the buffer is measured into the ima log. Both the brief and longer function descriptions need to be updated, as well as the last argument description. The last argument should be limited to "measure buffer hash". How it is used could be included in the longer function description. The longer function description would include adding the buffer data or the buffer data hash to the IMA measurement list and extending the PCR. For example, process_buffer_measurement - measure the buffer data or the buffer data hash > */ > void process_buffer_measurement(struct inode *inode, const void *buf, int size, > const char *eventname, enum ima_hooks func, > - int pcr, const char *func_data) > + int pcr, const char *func_data, > + bool measure_buf_hash) > { > int ret = 0; > const char *audit_cause = "ENOMEM"; > @@ -807,6 +810,8 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, > struct ima_digest_data hdr; > char digest[IMA_MAX_DIGEST_SIZE]; > } hash = {}; > + char digest_hash[IMA_MAX_DIGEST_SIZE]; > + int hash_len = hash_digest_size[ima_hash_algo]; > int violation = 0; > int action = 0; > u32 secid; > @@ -855,6 +860,21 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, > goto out; > } > > + if (measure_buf_hash) { > + memcpy(digest_hash, hash.hdr.digest, hash_len); Instead of digest_hash and hash_len, consider naming the variables buf_hash and buf_hashlen. > + > + ret = ima_calc_buffer_hash(digest_hash, > + hash_len, > + iint.ima_hash); There's no need for each variable to be on a separate line. thanks, Mimi > + if (ret < 0) { > + audit_cause = "measure_buf_hash_error"; > + goto out; > + } > + > + event_data.buf = digest_hash; > + event_data.buf_len = hash_len; > + } > + > ret = ima_alloc_init_template(&event_data, &entry, template); > if (ret < 0) { > audit_cause = "alloc_entry";
Hello Mimi, On 2020-11-05 6:30 a.m., Mimi Zohar wrote: > Hi Tushar, > > Please don't include the filename in the Subject line[1]. The Subject > line should be a summary phrase describing the patch. In this case, > it is adding support for measuring the buffer data hash. > Thanks. Will update the subject line accordingly. > On Sun, 2020-11-01 at 14:26 -0800, Tushar Sugandhi wrote: >> process_buffer_measurement() currently only measures the input buffer. >> In case of SeLinux policy measurement, the policy being measured could >> be large (several MB). This may result in a large entry in IMA >> measurement log. > > SELinux is an example of measuring large buffer data. Please rewrite > this patch description (and the other patch descriptions in this patch > set) without using the example to describe its purpose [1]. > > In this case, you might say, > > The original IMA buffer data measurement sizes were small (e.g. boot > command line), but new buffer data measurement use cases are a lot > larger. Just as IMA measures the file data hash, not the file data, > IMA should similarly support measuring the buffer data hash. > Sure. Thanks a lot for giving an example wording for us. Will update. >> >> Introduce a boolean parameter measure_buf_hash to support measuring >> hash of a buffer, which would be much smaller, instead of the buffer >> itself. > >> To use the functionality introduced in this patch, the attestation >> client and the server changes need to go hand in hand. The >> client/kernel would know what data is being measured as-is >> (e.g. KEXEC_CMDLINE), and what data has it’s hash measured (e.g. SeLinux >> Policy). And the attestation server should verify data/hash accordingly. >> >> Just like the data being measured in other cases, the attestation server >> will know what are possible values of the large buffers being measured. >> e.g. the possible valid SeLinux policy values that are being pushed to >> the client. The attestation server will have to maintain the hash of >> those buffer values. > > Each patch in the patch set builds upon the previous one. (Think of > it as a story, where each chapter builds upon the previous ones.) > With rare exceptions, should patches reference subsequent patches. [2] > > [1] Refer to Documentation/process/submitting-patches.rst > [2] Refer to the section "8) Commenting" in > Documentation/process/coding-style.rst > > thanks, > > Mimi > I am not sure if you have any concerns about the last two paragraphs. The description about the attestation client and server (the last two paragraphs) was added for information/clarification purpose only, as per your feedback on previous iterations. The subsequent patches don’t have any code pertaining to attestation client/server. *Question* Maybe the last two paragraphs are confusing/redundant. Could you please let me know if I should remove the above two paragraphs altogether? (starting with “To use the functionality introduced in this patch ...”) If we decide to keep the paragraphs, I will remove the specific examples (KEXEC_CMDLINE, SeLinux etc.) as you mentioned elsewhere.
On 2020-11-06 4:11 a.m., Mimi Zohar wrote: > Hi Tushar, > > Below inline are a few additional comments. > >> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c >> index ae5da9f3339d..4485d87c0aa5 100644 >> --- a/security/integrity/ima/ima_main.c >> +++ b/security/integrity/ima/ima_main.c >> @@ -787,12 +787,15 @@ int ima_post_load_data(char *buf, loff_t size, >> * @func: IMA hook >> * @pcr: pcr to extend the measurement >> * @func_data: private data specific to @func, can be NULL. >> + * @measure_buf_hash: if set to true - will measure hash of the buf, >> + * instead of buf >> * >> * Based on policy, the buffer is measured into the ima log. > > Both the brief and longer function descriptions need to be updated, as > well as the last argument description. The last argument should be > limited to "measure buffer hash". How it is used could be included in > the longer function description. The longer function description would > include adding the buffer data or the buffer data hash to the IMA > measurement list and extending the PCR. > > For example, > process_buffer_measurement - measure the buffer data or the buffer data > hash > Thanks Mimi. Will update the brief and longer descriptions accordingly. > >> */ >> void process_buffer_measurement(struct inode *inode, const void *buf, int size, >> const char *eventname, enum ima_hooks func, >> - int pcr, const char *func_data) >> + int pcr, const char *func_data, >> + bool measure_buf_hash) >> { >> int ret = 0; >> const char *audit_cause = "ENOMEM"; >> @@ -807,6 +810,8 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, >> struct ima_digest_data hdr; >> char digest[IMA_MAX_DIGEST_SIZE]; >> } hash = {}; >> + char digest_hash[IMA_MAX_DIGEST_SIZE]; >> + int hash_len = hash_digest_size[ima_hash_algo]; >> int violation = 0; >> int action = 0; >> u32 secid; >> @@ -855,6 +860,21 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, >> goto out; >> } >> >> + if (measure_buf_hash) { >> + memcpy(digest_hash, hash.hdr.digest, hash_len); > > Instead of digest_hash and hash_len, consider naming the variables > buf_hash and buf_hashlen. > Thanks. Will do. >> + >> + ret = ima_calc_buffer_hash(digest_hash, >> + hash_len, >> + iint.ima_hash); > > There's no need for each variable to be on a separate line. > Thanks, will fix. ~Tushar > thanks, > > Mimi > >> + if (ret < 0) { >> + audit_cause = "measure_buf_hash_error"; >> + goto out; >> + } >> + >> + event_data.buf = digest_hash; >> + event_data.buf_len = hash_len; >> + } >> + >> ret = ima_alloc_init_template(&event_data, &entry, template); >> if (ret < 0) { >> audit_cause = "alloc_entry";
On Thu, 2020-11-12 at 13:47 -0800, Tushar Sugandhi wrote: > > On Sun, 2020-11-01 at 14:26 -0800, Tushar Sugandhi wrote: > >> process_buffer_measurement() currently only measures the input buffer. > >> In case of SeLinux policy measurement, the policy being measured could > >> be large (several MB). This may result in a large entry in IMA > >> measurement log. > > > > SELinux is an example of measuring large buffer data. Please rewrite > > this patch description (and the other patch descriptions in this patch > > set) without using the example to describe its purpose [1]. > > > > In this case, you might say, > > > > The original IMA buffer data measurement sizes were small (e.g. boot > > command line), but new buffer data measurement use cases are a lot > > larger. Just as IMA measures the file data hash, not the file data, > > IMA should similarly support measuring the buffer data hash. > > > Sure. Thanks a lot for giving an example wording for us. Will update. > >> > >> Introduce a boolean parameter measure_buf_hash to support measuring > >> hash of a buffer, which would be much smaller, instead of the buffer > >> itself. > > > >> To use the functionality introduced in this patch, the attestation > >> client and the server changes need to go hand in hand. The > >> client/kernel would know what data is being measured as-is > >> (e.g. KEXEC_CMDLINE), and what data has it’s hash measured (e.g. SeLinux > >> Policy). And the attestation server should verify data/hash accordingly. > >> > >> Just like the data being measured in other cases, the attestation server > >> will know what are possible values of the large buffers being measured. > >> e.g. the possible valid SeLinux policy values that are being pushed to > >> the client. The attestation server will have to maintain the hash of > >> those buffer values. > > > > Each patch in the patch set builds upon the previous one. (Think of > > it as a story, where each chapter builds upon the previous ones.) > > With rare exceptions, should patches reference subsequent patches. [2] > > > > [1] Refer to Documentation/process/submitting-patches.rst > > [2] Refer to the section "8) Commenting" in > > Documentation/process/coding-style.rst > > > I am not sure if you have any concerns about the last two paragraphs. > The description about the attestation client and server (the last two > paragraphs) was added for information/clarification purpose only, as per > your feedback on previous iterations. The subsequent patches don’t have > any code pertaining to attestation client/server. > > *Question* > Maybe the last two paragraphs are confusing/redundant. Could you please > let me know if I should remove the above two paragraphs altogether? > (starting with “To use the functionality introduced in this patch ...”) > > If we decide to keep the paragraphs, I will remove the specific examples > (KEXEC_CMDLINE, SeLinux etc.) as you mentioned elsewhere. Instead of the above two paragraphs, perhaps explain how measuring the file data hash differs from measuring the buffer data hash. Keep the explanation generic, short and simple. Mimi
On 2020-11-12 2:19 p.m., Mimi Zohar wrote: > On Thu, 2020-11-12 at 13:47 -0800, Tushar Sugandhi wrote: >>> On Sun, 2020-11-01 at 14:26 -0800, Tushar Sugandhi wrote: >>>> process_buffer_measurement() currently only measures the input buffer. >>>> In case of SeLinux policy measurement, the policy being measured could >>>> be large (several MB). This may result in a large entry in IMA >>>> measurement log. >>> >>> SELinux is an example of measuring large buffer data. Please rewrite >>> this patch description (and the other patch descriptions in this patch >>> set) without using the example to describe its purpose [1]. >>> >>> In this case, you might say, >>> >>> The original IMA buffer data measurement sizes were small (e.g. boot >>> command line), but new buffer data measurement use cases are a lot >>> larger. Just as IMA measures the file data hash, not the file data, >>> IMA should similarly support measuring the buffer data hash. >>> >> Sure. Thanks a lot for giving an example wording for us. Will update. >>>> >>>> Introduce a boolean parameter measure_buf_hash to support measuring >>>> hash of a buffer, which would be much smaller, instead of the buffer >>>> itself. >>> >>>> To use the functionality introduced in this patch, the attestation >>>> client and the server changes need to go hand in hand. The >>>> client/kernel would know what data is being measured as-is >>>> (e.g. KEXEC_CMDLINE), and what data has it’s hash measured (e.g. SeLinux >>>> Policy). And the attestation server should verify data/hash accordingly. >>>> >>>> Just like the data being measured in other cases, the attestation server >>>> will know what are possible values of the large buffers being measured. >>>> e.g. the possible valid SeLinux policy values that are being pushed to >>>> the client. The attestation server will have to maintain the hash of >>>> those buffer values. >>> >>> Each patch in the patch set builds upon the previous one. (Think of >>> it as a story, where each chapter builds upon the previous ones.) >>> With rare exceptions, should patches reference subsequent patches. [2] >>> >>> [1] Refer to Documentation/process/submitting-patches.rst >>> [2] Refer to the section "8) Commenting" in >>> Documentation/process/coding-style.rst >>> >> I am not sure if you have any concerns about the last two paragraphs. >> The description about the attestation client and server (the last two >> paragraphs) was added for information/clarification purpose only, as per >> your feedback on previous iterations. The subsequent patches don’t have >> any code pertaining to attestation client/server. >> >> *Question* >> Maybe the last two paragraphs are confusing/redundant. Could you please >> let me know if I should remove the above two paragraphs altogether? >> (starting with “To use the functionality introduced in this patch ...”) >> >> If we decide to keep the paragraphs, I will remove the specific examples >> (KEXEC_CMDLINE, SeLinux etc.) as you mentioned elsewhere. > > Instead of the above two paragraphs, perhaps explain how measuring the > file data hash differs from measuring the buffer data hash. Keep the > explanation generic, short and simple. > > Mimi > Will do. Thanks for the quick response Mimi. I also have some clarification questions on the other patches in this series as well. Would really appreciate if you could help us get clarification on those. Thanks a lot again. ~Tushar
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 8875085db689..0f77e0b697a3 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -267,7 +267,8 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file, struct ima_template_desc *template_desc); void process_buffer_measurement(struct inode *inode, const void *buf, int size, const char *eventname, enum ima_hooks func, - int pcr, const char *func_data); + int pcr, const char *func_data, + bool measure_buf_hash); void ima_audit_measurement(struct integrity_iint_cache *iint, const unsigned char *filename); int ima_alloc_init_template(struct ima_event_data *event_data, diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 3dd8c2e4314e..be64c0bf62a7 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -347,7 +347,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint, if ((rc == -EPERM) && (iint->flags & IMA_MEASURE)) process_buffer_measurement(NULL, digest, digestsize, "blacklisted-hash", NONE, - pcr, NULL); + pcr, NULL, false); } return rc; diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c index 1c68c500c26f..a74095793936 100644 --- a/security/integrity/ima/ima_asymmetric_keys.c +++ b/security/integrity/ima/ima_asymmetric_keys.c @@ -60,5 +60,5 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key, */ process_buffer_measurement(NULL, payload, payload_len, keyring->description, KEY_CHECK, 0, - keyring->description); + keyring->description, false); } diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index ae5da9f3339d..4485d87c0aa5 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -787,12 +787,15 @@ int ima_post_load_data(char *buf, loff_t size, * @func: IMA hook * @pcr: pcr to extend the measurement * @func_data: private data specific to @func, can be NULL. + * @measure_buf_hash: if set to true - will measure hash of the buf, + * instead of buf * * Based on policy, the buffer is measured into the ima log. */ void process_buffer_measurement(struct inode *inode, const void *buf, int size, const char *eventname, enum ima_hooks func, - int pcr, const char *func_data) + int pcr, const char *func_data, + bool measure_buf_hash) { int ret = 0; const char *audit_cause = "ENOMEM"; @@ -807,6 +810,8 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, struct ima_digest_data hdr; char digest[IMA_MAX_DIGEST_SIZE]; } hash = {}; + char digest_hash[IMA_MAX_DIGEST_SIZE]; + int hash_len = hash_digest_size[ima_hash_algo]; int violation = 0; int action = 0; u32 secid; @@ -855,6 +860,21 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, goto out; } + if (measure_buf_hash) { + memcpy(digest_hash, hash.hdr.digest, hash_len); + + ret = ima_calc_buffer_hash(digest_hash, + hash_len, + iint.ima_hash); + if (ret < 0) { + audit_cause = "measure_buf_hash_error"; + goto out; + } + + event_data.buf = digest_hash; + event_data.buf_len = hash_len; + } + ret = ima_alloc_init_template(&event_data, &entry, template); if (ret < 0) { audit_cause = "alloc_entry"; @@ -896,7 +916,8 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) return; process_buffer_measurement(file_inode(f.file), buf, size, - "kexec-cmdline", KEXEC_CMDLINE, 0, NULL); + "kexec-cmdline", KEXEC_CMDLINE, 0, NULL, + false); fdput(f); } diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c index 69a8626a35c0..c2f2ad34f9b7 100644 --- a/security/integrity/ima/ima_queue_keys.c +++ b/security/integrity/ima/ima_queue_keys.c @@ -162,7 +162,8 @@ void ima_process_queued_keys(void) entry->payload_len, entry->keyring_name, KEY_CHECK, 0, - entry->keyring_name); + entry->keyring_name, + false); list_del(&entry->list); ima_free_key_entry(entry); }
process_buffer_measurement() currently only measures the input buffer. In case of SeLinux policy measurement, the policy being measured could be large (several MB). This may result in a large entry in IMA measurement log. Introduce a boolean parameter measure_buf_hash to support measuring hash of a buffer, which would be much smaller, instead of the buffer itself. To use the functionality introduced in this patch, the attestation client and the server changes need to go hand in hand. The client/kernel would know what data is being measured as-is (e.g. KEXEC_CMDLINE), and what data has it’s hash measured (e.g. SeLinux Policy). And the attestation server should verify data/hash accordingly. Just like the data being measured in other cases, the attestation server will know what are possible values of the large buffers being measured. e.g. the possible valid SeLinux policy values that are being pushed to the client. The attestation server will have to maintain the hash of those buffer values. Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> --- security/integrity/ima/ima.h | 3 ++- security/integrity/ima/ima_appraise.c | 2 +- security/integrity/ima/ima_asymmetric_keys.c | 2 +- security/integrity/ima/ima_main.c | 25 ++++++++++++++++++-- security/integrity/ima/ima_queue_keys.c | 3 ++- 5 files changed, 29 insertions(+), 6 deletions(-)