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 <[email protected]> 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
signature.asc
Description: PGP signature
