linux-kernel - Re: [PATCH v5 2/7] IMA: update process_buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <702e7d17-27f0-30e7-b5ce-affecb0c8de7@linux.microsoft.com>
Date:   Thu, 12 Nov 2020 13:48:53 -0800
From:   Tushar Sugandhi <tusharsu@...ux.microsoft.com>
To:     Mimi Zohar <zohar@...ux.ibm.com>, stephen.smalley.work@...il.com,
        casey@...aufler-ca.com, agk@...hat.com, snitzer@...hat.com,
        gmazyland@...il.com, paul@...l-moore.com
Cc:     tyhicks@...ux.microsoft.com, sashal@...nel.org, jmorris@...ei.org,
        nramas@...ux.microsoft.com, linux-integrity@...r.kernel.org,
        selinux@...r.kernel.org, linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, dm-devel@...hat.com
Subject: Re: [PATCH v5 2/7] IMA: update process_buffer_measurement to measure
 buffer hash



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";