| Message ID | 20200828015704.6629-4-tusharsu@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IMA: Infrastructure for measurement of critical kernel data | expand |
On Thu, 2020-08-27 at 18:57 -0700, Tushar Sugandhi wrote: > process_buffer_measurement() currently only measures the input buffer. > When the buffer being measured is too large, it may result in bloated > IMA logs. The subject of this sentence refers to an individual record, while "bloated" refers to the measurement list. A "bloated" measurement list would contain too many or unnecessary records. Your concern seems to be with the size of the individual record, not the number of measurement list entries. Measuring the hash of the buffer data is similar to measuring the file data. In the case of the file data, however, the attestation server may rely on a white list manifest/DB or the file signature to verify the file data hash. For buffer measurements, how will the attestation server ascertain what is a valid buffer hash? Hint: I assume, correct me if I'm wrong, the measurement list record template data is not meant to be verified, but used to detect if the "critical data" changed. Please update the patch description accordingly. > > Introduce a boolean parameter measure_buf_hash to support measuring > hash of a buffer, which would be much smaller, instead of the buffer > itself. > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > --- <snip> > +++ b/security/integrity/ima/ima_main.c > @@ -733,17 +733,21 @@ int ima_load_data(enum kernel_load_data_id id) > * @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. > */ > int 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"; > struct ima_template_entry *entry = NULL; > struct integrity_iint_cache iint = {}; > + struct integrity_iint_cache digest_iint = {}; > struct ima_event_data event_data = {.iint = &iint, > .filename = eventname, > .buf = buf, > @@ -752,7 +756,7 @@ int process_buffer_measurement(struct inode *inode, const void *buf, int size, > struct { > struct ima_digest_data hdr; > char digest[IMA_MAX_DIGEST_SIZE]; > - } hash = {}; > + } hash = {}, digest_hash = {}; > int violation = 0; > int action = 0; > u32 secid; > @@ -801,6 +805,24 @@ int process_buffer_measurement(struct inode *inode, const void *buf, int size, > goto out; > } > > + if (measure_buf_hash) { > + digest_iint.ima_hash = &digest_hash.hdr; > + digest_iint.ima_hash->algo = ima_hash_algo; > + digest_iint.ima_hash->length = hash_digest_size[ima_hash_algo]; > + > + ret = ima_calc_buffer_hash(hash.hdr.digest, > + iint.ima_hash->length, > + digest_iint.ima_hash); > + if (ret < 0) { > + audit_cause = "digest_hashing_error"; > + goto out; > + } > + > + event_data.iint = &digest_iint; > + event_data.buf = hash.hdr.digest; > + event_data.buf_len = iint.ima_hash->length; > + } > + There seems to be some code and variable duplication by doing it this way. Copying the caluclated buffer data hash to a temporary buffer might eliminate it. > ret = ima_alloc_init_template(&event_data, &entry, template); > if (ret < 0) { > audit_cause = "alloc_entry";
On 2020-08-31 10:02 a.m., Mimi Zohar wrote: > On Thu, 2020-08-27 at 18:57 -0700, Tushar Sugandhi wrote: >> process_buffer_measurement() currently only measures the input buffer. >> When the buffer being measured is too large, it may result in bloated >> IMA logs. > > The subject of this sentence refers to an individual record, while > "bloated" refers to the measurement list. A "bloated" measurement list > would contain too many or unnecessary records. Your concern seems to > be with the size of the individual record, not the number of > measurement list entries. > The intent behind that description was twofold. One, as you mentioned, the individual record size being large; and two, multiple large-sized individual records will eventually bloat the measurement list too. It can happen in SeLinux case as we discovered. The SeLinux policy (which can be a few MBs) can change from 'foo', to 'bar', back to 'foo'. And the requirement from SeLinux is that 'foo' should be measured the second time too. When 'foo' and 'bar' are large, the individual records in the ima log will be large, which will also result in measurement list being bloated. But I understand your concern with the current wording. I will update the patch description accordingly. > Measuring the hash of the buffer data is similar to measuring the file > data. In the case of the file data, however, the attestation server > may rely on a white list manifest/DB or the file signature to verify > the file data hash. For buffer measurements, how will the attestation > server ascertain what is a valid buffer hash? The client and the server implementation will go hand in hand. The client/kernel would know what data is measured as-is (e.g. KEXEC_CMDLINE), and what data has it’s hash measured (e.g. SeLinux Policy). And the attestation server would 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. It will have to maintain the hash of those buffer values. > > Hint: I assume, correct me if I'm wrong, the measurement list record > template data is not meant to be verified, but used to detect if the "critical data" changed. > As mentioned above, the use case for this feature is in-memory loaded SeLinux policy, which can be quite large (several MBs) – that's why this functionality. The data is meant to be verified by the attestation server. > Please update the patch description accordingly. I will update the patch description to clarify all this. > >> >> Introduce a boolean parameter measure_buf_hash to support measuring >> hash of a buffer, which would be much smaller, instead of the buffer >> itself. >> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> >> --- > > <snip> > >> +++ b/security/integrity/ima/ima_main.c >> @@ -733,17 +733,21 @@ int ima_load_data(enum kernel_load_data_id id) >> * @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. >> */ >> int 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"; >> struct ima_template_entry *entry = NULL; >> struct integrity_iint_cache iint = {}; >> + struct integrity_iint_cache digest_iint = {}; >> struct ima_event_data event_data = {.iint = &iint, >> .filename = eventname, >> .buf = buf, >> @@ -752,7 +756,7 @@ int process_buffer_measurement(struct inode *inode, const void *buf, int size, >> struct { >> struct ima_digest_data hdr; >> char digest[IMA_MAX_DIGEST_SIZE]; >> - } hash = {}; >> + } hash = {}, digest_hash = {}; >> int violation = 0; >> int action = 0; >> u32 secid; >> @@ -801,6 +805,24 @@ int process_buffer_measurement(struct inode *inode, const void *buf, int size, >> goto out; >> } >> >> + if (measure_buf_hash) { >> + digest_iint.ima_hash = &digest_hash.hdr; >> + digest_iint.ima_hash->algo = ima_hash_algo; >> + digest_iint.ima_hash->length = hash_digest_size[ima_hash_algo]; >> + >> + ret = ima_calc_buffer_hash(hash.hdr.digest, >> + iint.ima_hash->length, >> + digest_iint.ima_hash); >> + if (ret < 0) { >> + audit_cause = "digest_hashing_error"; >> + goto out; >> + } >> + >> + event_data.iint = &digest_iint; >> + event_data.buf = hash.hdr.digest; >> + event_data.buf_len = iint.ima_hash->length; >> + } >> + > > There seems to be some code and variable duplication by doing it this > way. Copying the caluclated buffer data hash to a temporary buffer > might eliminate it. > With the way ima_calc_buffer_hash() is implemented, I was convinced that the variable duplication was needed. I didn't want to write a helper function in order to minimize the unnecessary code churn in p_b_m(). But I will revisit this to see how I can reduce the code and variable duplication. Thanks for the feedback. >> ret = ima_alloc_init_template(&event_data, &entry, template); >> if (ret < 0) { >> audit_cause = "alloc_entry";
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 83ed57147e68..ba332de8ed0b 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); int 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 372d16382960..20adffe5bf58 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -336,7 +336,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 0979a62a9257..52cbbc1f7ea2 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -733,17 +733,21 @@ int ima_load_data(enum kernel_load_data_id id) * @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. */ int 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"; struct ima_template_entry *entry = NULL; struct integrity_iint_cache iint = {}; + struct integrity_iint_cache digest_iint = {}; struct ima_event_data event_data = {.iint = &iint, .filename = eventname, .buf = buf, @@ -752,7 +756,7 @@ int process_buffer_measurement(struct inode *inode, const void *buf, int size, struct { struct ima_digest_data hdr; char digest[IMA_MAX_DIGEST_SIZE]; - } hash = {}; + } hash = {}, digest_hash = {}; int violation = 0; int action = 0; u32 secid; @@ -801,6 +805,24 @@ int process_buffer_measurement(struct inode *inode, const void *buf, int size, goto out; } + if (measure_buf_hash) { + digest_iint.ima_hash = &digest_hash.hdr; + digest_iint.ima_hash->algo = ima_hash_algo; + digest_iint.ima_hash->length = hash_digest_size[ima_hash_algo]; + + ret = ima_calc_buffer_hash(hash.hdr.digest, + iint.ima_hash->length, + digest_iint.ima_hash); + if (ret < 0) { + audit_cause = "digest_hashing_error"; + goto out; + } + + event_data.iint = &digest_iint; + event_data.buf = hash.hdr.digest; + event_data.buf_len = iint.ima_hash->length; + } + ret = ima_alloc_init_template(&event_data, &entry, template); if (ret < 0) { audit_cause = "alloc_entry"; @@ -842,7 +864,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. When the buffer being measured is too large, it may result in bloated IMA logs. Introduce a boolean parameter measure_buf_hash to support measuring hash of a buffer, which would be much smaller, instead of the buffer itself. 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 | 29 ++++++++++++++++++-- security/integrity/ima/ima_queue_keys.c | 3 +- 5 files changed, 32 insertions(+), 7 deletions(-)