Security vulnerabilities report to Das-U-Boot

[Tom Rini]

On Fri, Feb 07, 2025 at 09:53:01AM -0800, Jonathan Bar Or wrote:

> Thank you.
> 
> So, I'm attaching my findings in a md file - see attachment.
> All of those could be avoided by using safe math, such as
> __builtin_mul_overflow and __builtin_add_overflow, which are used in some
> modules in Das-U-Boot.
> There are many cases where seemingly unsafe addition and multiplication can
> cause integer overflows, but not all are exploitable - I believe the ones I
> report here are.
> 
> Let me know your thoughts.

Adding in the eorfs and squashfs maintainers..

> 
> Best regards,
>             Jonathan
> 
> On Fri, Feb 7, 2025 at 7:50 AM Tom Rini <trini at konsulko.com> wrote:
> 
> > On Thu, Feb 06, 2025 at 07:47:54PM -0800, Jonathan Bar Or wrote:
> >
> > > Dear U-boot maintainers,
> > >
> > > What is the best way of reporting security vulnerabilities (memory
> > > corruption issues) to Das-U-Boot? Is there a PGP key I should be using?
> > > I have 4 issues that I think are worth fixing (with very easy fixes).
> > >
> > > Best regards,
> > >             Jonathan
> >
> > Hey. As per https://docs.u-boot.org/en/latest/develop/security.html
> > please post them to the list in public. If you have possible solutions
> > for them as well that's even better. Thanks!
> >
> > --
> > Tom
> >

> Filesystem-based Das-U-Boot issues.
> 
> == erofs
> 
> === Integer overflow leading to buffer overflow in symlink resolution
> In file `fs.c`, when resolving symlinks, the function `erofs_off_t` gets an `erofs_inode` argument and performs a lookup on the symlink.  
> The function blindly trusts the `i_size` member of the input as such:
> 
> ```c
>     size_t len = vi->i_size;
> 	char *target;
> 	int err;
> 
> 	target = malloc(len + 1);
> 	if (!target)
> 		return -ENOMEM;
> 	target[len] = '\0';
> 
> 	err = erofs_pread(vi, target, len, 0);
> 	if (err)
> 		goto err_out;
> ```
> 
> The `erofs_inode` structure's `i_size` member is defined with the type `erofs_off_t` (which is a 64-bit unsigned integer).  
> Thereofre, if supplied as 0xFFFFFFFFFFFFFFFF, the `len + 1` input to `malloc` would overflow to 0, allocating a chunk with 0.  
> That chunk (saved in `target`) is later written with `erofs_pread`, overriding the chunk with partial data controlled by an attacker.  
> Therefore, we will have a heap buffer overflow due to an integer overflow in `len` calculation.
> 
> == squashfs
> 
> === Integer overflow leading to buffer overflow in inode table parsing 
> In file `sqfs.c`, function `sqfs_read_inode_table` is responsible of reading an inode table.  
> It gets the superblock (attacker controlled) from the context. Then, it employs the following logic:
> 
> ```c
>     n_blks = sqfs_calc_n_blks(sblk->inode_table_start, sblk->directory_table_start, &table_offset);
> 
> 	/* Allocate a proper sized buffer (itb) to store the inode table */
> 	itb = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz);
> 	if (!itb)
> 		return -ENOMEM;
> 
> 	if (sqfs_disk_read(start, n_blks, itb) < 0) {
> 		ret = -EINVAL;
> 		goto free_itb;
> 	}
> ```
> 
> === Integer overflow leading to buffer overflow in directory table parsing
> Similarly to the previous issue in inode table parsing in `sqfs.c`, the same unsafe multiplication exists within the function `sqfs_read_directory_table` responsible for reading the directory table:
> 
> ```c
>     n_blks = sqfs_calc_n_blks(sblk->directory_table_start,
> 				  sblk->fragment_table_start, &table_offset);
> 
> 	/* Allocate a proper sized buffer (dtb) to store the directory table */
> 	dtb = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz);
> 	if (!dtb)
> 		return -ENOMEM;
> 
> 	if (sqfs_disk_read(start, n_blks, dtb) < 0)
> 		goto out;
> ```
> 
> The multiplication of `n_blks` and the block size (attacker-controlled 64-bit unsigned integer) is unsafe and might overflow, resulting in an out-of-bounds write.
> 
> === Integer overflow leading to buffer overflow in nested file reading
> Similarly to the previous issue in inode table parsing in `sqfs.c`, the same unsafe multiplication exists within the function `sqfs_read_nest` responsible for reading a file:
> 
> ```c
> 			data_buffer = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz);
> 
> 			if (!data_buffer) {
> 				ret = -ENOMEM;
> 				goto out;
> 			}
> 
> 			ret = sqfs_disk_read(start, n_blks, data_buffer);
> ```
> 
> A similar issue exists in the same function also later on:
> 
> ```c
>     fragment = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz);
> 
> 	if (!fragment) {
> 		ret = -ENOMEM;
> 		goto out;
> 	}
> 
> 	ret = sqfs_disk_read(start, n_blks, fragment);
> 	if (ret < 0)
> 		goto out;
> ```
> 


-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linux-erofs/attachments/20250210/e52a9ecb/attachment.sig>