| Message ID | 20241009145335.1297855-1-david.fernandez.gonzalez@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ima: Fix OOB read when violation occurs with ima template. | expand |
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);
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);
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(-)