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