Re: [PATCH] ima: Fix OOB read when violation occurs with ima template.

[Roberto Sassu]

On Wed, 2024-10-09 at 14:53 +0000, David Fernandez Gonzalez wrote:
> When processing a violation inside ima_eventdigest_init,
> ima_eventdigest_init_common will be called with cur_digest
> being NULL. hash_algo is always set to HASH_ALGO__LAST.
> 
> Inside ima_eventdigest_init_common, since digest is NULL,
> offset will be calculated by accessing hash_digest_size
> with HASH_ALGO__LAST, one element OOB.
> 
> This will be used to calculate the amount of bytes
> to be copied as file content hash. Depending on the memory,
> this could lead to the 0 hash not being recorded if offset is 0,
> the violation not being recorded at all if offset is too big
> (as it will be used to allocate the buffer in
> ima_write_template_field_data), or potentially leaking
> memory values into the measurements file, if offset is big
> enough but can still be used to allocate the buffer.

Hi David

thanks a lot for the patch! We currently have another similar in our
queue:

https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git/commit/?h=next-integrity&id=fa8a4ce432e82cc138e61fab7f44d60f9e720d47


Will be sent to Linus soon.

Thanks

Roberto

> UBSAN: array-index-out-of-bounds in security/integrity/ima/ima_template_lib.c:329:29
> index 23 is out of range for type 'int [23]'
> CPU: 0 UID: 0 PID: 383 Comm: journal-offline Not tainted 6.12.0-rc2 #14
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x64/0x80
>  __ubsan_handle_out_of_bounds+0xc6/0x100
>  ima_eventdigest_init_common+0x297/0x2c0
>  ? ima_add_violation+0x10b/0x260
>  ? __pfx_ima_eventdigest_init_common+0x10/0x10
>  ? path_openat+0x739/0x1ba0
>  ? do_filp_open+0x168/0x290
>  ? do_sys_openat2+0x126/0x160
>  ima_eventdigest_init+0xba/0x280
>  ? __pfx_ima_eventdigest_init+0x10/0x10
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? __kmalloc_noprof+0x1cd/0x490
>  ? ima_alloc_init_template+0xd8/0x2f0
>  ima_alloc_init_template+0x1d1/0x2f0
>  ima_add_violation+0x10b/0x260
>  ...
> 
> HASH_ALGO__LAST is only passed to ima_eventdigest_init_common
> for ima template. This change ensures to set an appropriate hash_algo
> value before calculating the offset.
> 
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 9fab303a2cb3 ("ima: fix violation measurement list record")
> Signed-off-by: David Fernandez Gonzalez <david.fernandez.gonzalez@xxxxxxxxxx>
> ---
>  security/integrity/ima/ima_template_lib.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index 4183956c53af..7a46d720303b 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -318,15 +318,19 @@ static int ima_eventdigest_init_common(const u8 *digest, u32 digestsize,
>  				      hash_algo_name[hash_algo]);
>  	}
>  
> -	if (digest)
> +	if (digest) {
>  		memcpy(buffer + offset, digest, digestsize);
> -	else
> +	} else {
>  		/*
>  		 * If digest is NULL, the event being recorded is a violation.
>  		 * Make room for the digest by increasing the offset by the
>  		 * hash algorithm digest size.
>  		 */
> +		if (hash_algo == HASH_ALGO__LAST) /* To handle ima template case */
> +			hash_algo = ima_template_hash_algo_allowed(ima_hash_algo) ?
> +				ima_hash_algo : HASH_ALGO_SHA1;
>  		offset += hash_digest_size[hash_algo];
> +	}
>  
>  	return ima_write_template_field_data(buffer, offset + digestsize,
>  					     fmt, field_data);