ima: Fix OOB read when violation occurs with ima template.

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

Posted by David Fernandez Gonzalez 1 month, 2 weeks ago

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.

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@vger.kernel.org
Fixes: 9fab303a2cb3 ("ima: fix violation measurement list record")
Signed-off-by: David Fernandez Gonzalez <david.fernandez.gonzalez@oracle.com>
---
 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);
-- 
2.43.0

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

Posted by Roberto Sassu 1 month, 2 weeks ago

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@vger.kernel.org
> Fixes: 9fab303a2cb3 ("ima: fix violation measurement list record")
> Signed-off-by: David Fernandez Gonzalez <david.fernandez.gonzalez@oracle.com>
> ---
>  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);