[PATCH 1/4] squashfs: Fix integer overflow in sqfs_resolve_symlink()
A carefully crafted squashfs filesystem can exhibit an inode size of 0xffffffff, as a consequence malloc() will do a zero allocation. Later in the function the inode size is again used for copying data. So an attacker can overwrite memory. Avoid the overflow by using the __builtin_add_overflow() helper.
Signed-off-by: Richard Weinberger richard@nod.at --- fs/squashfs/sqfs.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 1430e671a5..16a07c0622 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -422,8 +422,10 @@ static char *sqfs_resolve_symlink(struct squashfs_symlink_inode *sym, char *resolved, *target; u32 sz;
- sz = get_unaligned_le32(&sym->symlink_size); - target = malloc(sz + 1); + if (__builtin_add_overflow(get_unaligned_le32(&sym->symlink_size), 1, &sz)) + return NULL; + + target = malloc(sz); if (!target) return NULL;
@@ -431,9 +433,9 @@ static char *sqfs_resolve_symlink(struct squashfs_symlink_inode *sym, * There is no trailling null byte in the symlink's target path, so a * copy is made and a '\0' is added at its end. */ - target[sz] = '\0'; + target[sz - 1] = '\0'; /* Get target name (relative path) */ - strncpy(target, sym->symlink, sz); + strncpy(target, sym->symlink, sz - 1);
/* Relative -> absolute path conversion */ resolved = sqfs_get_abs_path(base_path, target);
A carefully crafted squashfs filesystem can exhibit an extremly large inode size and overflow the calculation in sqfs_inode_size(). As a consequence, the squashfs driver will read from wrong locations.
Fix by using __builtin_add_overflow() to detect the overflow.
Signed-off-by: Richard Weinberger richard@nod.at --- fs/squashfs/sqfs_inode.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/squashfs/sqfs_inode.c b/fs/squashfs/sqfs_inode.c index d25cfb53e7..bb3ccd37e3 100644 --- a/fs/squashfs/sqfs_inode.c +++ b/fs/squashfs/sqfs_inode.c @@ -78,11 +78,16 @@ int sqfs_inode_size(struct squashfs_base_inode *inode, u32 blk_size)
case SQFS_SYMLINK_TYPE: case SQFS_LSYMLINK_TYPE: { + int size; + struct squashfs_symlink_inode *symlink = (struct squashfs_symlink_inode *)inode;
- return sizeof(*symlink) + - get_unaligned_le32(&symlink->symlink_size); + if (__builtin_add_overflow(sizeof(*symlink), + get_unaligned_le32(&symlink->symlink_size), &size)) + return -EINVAL; + + return size; }
case SQFS_BLKDEV_TYPE:
Hi Richard,
richard@nod.at wrote on Fri, 12 Jul 2024 10:23:42 +0200:
A carefully crafted squashfs filesystem can exhibit an extremly large inode size and overflow the calculation in sqfs_inode_size(). As a consequence, the squashfs driver will read from wrong locations.
Fix by using __builtin_add_overflow() to detect the overflow.
Signed-off-by: Richard Weinberger richard@nod.at
Reviewed-by: Miquel Raynal miquel.raynal@bootlin.com
Thanks, Miquèl
The function can fail and return NULL.
Signed-off-by: Richard Weinberger richard@nod.at --- fs/squashfs/sqfs.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 16a07c0622..fa99d514f2 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -474,6 +474,8 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, /* Start by root inode */ table = sqfs_find_inode(dirs->inode_table, le32_to_cpu(sblk->inodes), sblk->inodes, sblk->block_size); + if (!table) + return -EINVAL;
dir = (struct squashfs_dir_inode *)table; ldir = (struct squashfs_ldir_inode *)table; @@ -529,6 +531,8 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, /* Get reference to inode in the inode table */ table = sqfs_find_inode(dirs->inode_table, new_inode_number, sblk->inodes, sblk->block_size); + if (!table) + return -EINVAL; dir = (struct squashfs_dir_inode *)table;
/* Check for symbolic link and inode type sanity */ @@ -1025,6 +1029,8 @@ int sqfs_readdir(struct fs_dir_stream *fs_dirs, struct fs_dirent **dentp) i_number = dirs->dir_header->inode_number + dirs->entry->inode_offset; ipos = sqfs_find_inode(dirs->inode_table, i_number, sblk->inodes, sblk->block_size); + if (!ipos) + return -SQFS_STOP_READDIR;
base = (struct squashfs_base_inode *)ipos;
@@ -1381,6 +1387,10 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, i_number = dirs->dir_header->inode_number + dirs->entry->inode_offset; ipos = sqfs_find_inode(dirs->inode_table, i_number, sblk->inodes, sblk->block_size); + if (!ipos) { + ret = -EINVAL; + goto out; + }
base = (struct squashfs_base_inode *)ipos; switch (get_unaligned_le16(&base->inode_type)) { @@ -1629,6 +1639,13 @@ int sqfs_size(const char *filename, loff_t *size) i_number = dirs->dir_header->inode_number + dirs->entry->inode_offset; ipos = sqfs_find_inode(dirs->inode_table, i_number, sblk->inodes, sblk->block_size); + + if (!ipos) { + *size = 0; + ret = -EINVAL; + goto free_strings; + } + free(dirs->entry); dirs->entry = NULL;
Hi Richard,
richard@nod.at wrote on Fri, 12 Jul 2024 10:23:43 +0200:
The function can fail and return NULL.
Signed-off-by: Richard Weinberger richard@nod.at
Reviewed-by: Miquel Raynal miquel.raynal@bootlin.com
Thanks, Miquèl
The squashfs driver blindly follows symlinks, and calls sqfs_size() recursively. So an attacker can create a crafted filesystem and with a deep enough nesting level a stack overflow can be achieved.
Fix by limiting the nesting level to 8.
Signed-off-by: Richard Weinberger richard@nod.at --- fs/squashfs/sqfs.c | 74 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 59 insertions(+), 15 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index fa99d514f2..3b23c5bc8f 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -25,6 +25,9 @@ #include "sqfs_utils.h"
static struct squashfs_ctxt ctxt; +static int symlinknest; + +static int sqfs_readdir_nest(struct fs_dir_stream *fs_dirs, struct fs_dirent **dentp);
static int sqfs_disk_read(__u32 block, __u32 nr_blocks, void *buf) { @@ -510,7 +513,7 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, goto out; }
- while (!sqfs_readdir(dirsp, &dent)) { + while (!sqfs_readdir_nest(dirsp, &dent)) { ret = strcmp(dent->name, token_list[j]); if (!ret) break; @@ -537,6 +540,11 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list,
/* Check for symbolic link and inode type sanity */ if (get_unaligned_le16(&dir->inode_type) == SQFS_SYMLINK_TYPE) { + if (++symlinknest == 8) { + ret = -ELOOP; + goto out; + } + sym = (struct squashfs_symlink_inode *)table; /* Get first j + 1 tokens */ path = sqfs_concat_tokens(token_list, j + 1); @@ -884,7 +892,7 @@ out: return metablks_count; }
-int sqfs_opendir(const char *filename, struct fs_dir_stream **dirsp) +static int sqfs_opendir_nest(const char *filename, struct fs_dir_stream **dirsp) { unsigned char *inode_table = NULL, *dir_table = NULL; int j, token_count = 0, ret = 0, metablks_count; @@ -979,7 +987,19 @@ out: return ret; }
+int sqfs_opendir(const char *filename, struct fs_dir_stream **dirsp) +{ + symlinknest = 0; + return sqfs_opendir_nest(filename, dirsp); +} + int sqfs_readdir(struct fs_dir_stream *fs_dirs, struct fs_dirent **dentp) +{ + symlinknest = 0; + return sqfs_readdir_nest(fs_dirs, dentp); +} + +static int sqfs_readdir_nest(struct fs_dir_stream *fs_dirs, struct fs_dirent **dentp) { struct squashfs_super_block *sblk = ctxt.sblk; struct squashfs_dir_stream *dirs; @@ -1325,8 +1345,8 @@ static int sqfs_get_lregfile_info(struct squashfs_lreg_inode *lreg, return datablk_count; }
-int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, - loff_t *actread) +static int sqfs_read_nest(const char *filename, void *buf, loff_t offset, + loff_t len, loff_t *actread) { char *dir = NULL, *fragment_block, *datablock = NULL; char *fragment = NULL, *file = NULL, *resolved, *data; @@ -1356,11 +1376,11 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, }
/* - * sqfs_opendir will uncompress inode and directory tables, and will + * sqfs_opendir_nest will uncompress inode and directory tables, and will * return a pointer to the directory that contains the requested file. */ sqfs_split_path(&file, &dir, filename); - ret = sqfs_opendir(dir, &dirsp); + ret = sqfs_opendir_nest(dir, &dirsp); if (ret) { goto out; } @@ -1368,7 +1388,7 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, dirs = (struct squashfs_dir_stream *)dirsp;
/* For now, only regular files are able to be loaded */ - while (!sqfs_readdir(dirsp, &dent)) { + while (!sqfs_readdir_nest(dirsp, &dent)) { ret = strcmp(dent->name, file); if (!ret) break; @@ -1421,9 +1441,14 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, break; case SQFS_SYMLINK_TYPE: case SQFS_LSYMLINK_TYPE: + if (++symlinknest == 8) { + ret = -ELOOP; + goto out; + } + symlink = (struct squashfs_symlink_inode *)ipos; resolved = sqfs_resolve_symlink(symlink, filename); - ret = sqfs_read(resolved, buf, offset, len, actread); + ret = sqfs_read_nest(resolved, buf, offset, len, actread); free(resolved); goto out; case SQFS_BLKDEV_TYPE: @@ -1594,7 +1619,14 @@ out: return ret; }
-int sqfs_size(const char *filename, loff_t *size) +int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, + loff_t *actread) +{ + symlinknest = 0; + return sqfs_read_nest(filename, buf, offset, len, actread); +} + +static int sqfs_size_nest(const char *filename, loff_t *size) { struct squashfs_super_block *sblk = ctxt.sblk; struct squashfs_symlink_inode *symlink; @@ -1610,10 +1642,10 @@ int sqfs_size(const char *filename, loff_t *size)
sqfs_split_path(&file, &dir, filename); /* - * sqfs_opendir will uncompress inode and directory tables, and will + * sqfs_opendir_nest will uncompress inode and directory tables, and will * return a pointer to the directory that contains the requested file. */ - ret = sqfs_opendir(dir, &dirsp); + ret = sqfs_opendir_nest(dir, &dirsp); if (ret) { ret = -EINVAL; goto free_strings; @@ -1621,7 +1653,7 @@ int sqfs_size(const char *filename, loff_t *size)
dirs = (struct squashfs_dir_stream *)dirsp;
- while (!sqfs_readdir(dirsp, &dent)) { + while (!sqfs_readdir_nest(dirsp, &dent)) { ret = strcmp(dent->name, file); if (!ret) break; @@ -1661,6 +1693,11 @@ int sqfs_size(const char *filename, loff_t *size) break; case SQFS_SYMLINK_TYPE: case SQFS_LSYMLINK_TYPE: + if (++symlinknest == 8) { + *size = 0; + return -ELOOP; + } + symlink = (struct squashfs_symlink_inode *)ipos; resolved = sqfs_resolve_symlink(symlink, filename); ret = sqfs_size(resolved, size); @@ -1700,10 +1737,11 @@ int sqfs_exists(const char *filename)
sqfs_split_path(&file, &dir, filename); /* - * sqfs_opendir will uncompress inode and directory tables, and will + * sqfs_opendir_nest will uncompress inode and directory tables, and will * return a pointer to the directory that contains the requested file. */ - ret = sqfs_opendir(dir, &dirsp); + symlinknest = 0; + ret = sqfs_opendir_nest(dir, &dirsp); if (ret) { ret = -EINVAL; goto free_strings; @@ -1711,7 +1749,7 @@ int sqfs_exists(const char *filename)
dirs = (struct squashfs_dir_stream *)dirsp;
- while (!sqfs_readdir(dirsp, &dent)) { + while (!sqfs_readdir_nest(dirsp, &dent)) { ret = strcmp(dent->name, file); if (!ret) break; @@ -1728,6 +1766,12 @@ free_strings: return ret == 0; }
+int sqfs_size(const char *filename, loff_t *size) +{ + symlinknest = 0; + return sqfs_size_nest(filename, size); +} + void sqfs_close(void) { sqfs_decompressor_cleanup(&ctxt);
Hi Richard,
richard@nod.at wrote on Fri, 12 Jul 2024 10:23:44 +0200:
The squashfs driver blindly follows symlinks, and calls sqfs_size() recursively. So an attacker can create a crafted filesystem and with a deep enough nesting level a stack overflow can be achieved.
Fix by limiting the nesting level to 8.
As this is I believe an arbitrary value, could we define this value somewhere and flag it with a comment as "arbitrary" with some details from the commit log? Right now the value '8' is hardcoded at least in 3 different places. Also, 8 seems rather small, any reason for choosing that? I believe this is easy to cross even in non-evil filesystems and could perhaps be (again, arbitrarily) increased a bit?
Signed-off-by: Richard Weinberger richard@nod.at
fs/squashfs/sqfs.c | 74 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 59 insertions(+), 15 deletions(-)
...
/* Check for symbolic link and inode type sanity */ if (get_unaligned_le16(&dir->inode_type) == SQFS_SYMLINK_TYPE) {
if (++symlinknest == 8) {ret = -ELOOP;goto out;}
...
@@ -1421,9 +1441,14 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, break; case SQFS_SYMLINK_TYPE: case SQFS_LSYMLINK_TYPE:
if (++symlinknest == 8) {ret = -ELOOP;goto out;}
...
case SQFS_LSYMLINK_TYPE:
if (++symlinknest == 8) {*size = 0;return -ELOOP;}
Thanks, Miquèl
Hi Miquel,
Am Mittwoch, 17. Juli 2024, 10:06:35 CEST schrieb 'Miquel Raynal' via upstream:
Hi Richard,
richard@nod.at wrote on Fri, 12 Jul 2024 10:23:44 +0200:
The squashfs driver blindly follows symlinks, and calls sqfs_size() recursively. So an attacker can create a crafted filesystem and with a deep enough nesting level a stack overflow can be achieved.
Fix by limiting the nesting level to 8.
As this is I believe an arbitrary value, could we define this value somewhere and flag it with a comment as "arbitrary" with some details from the commit log? Right now the value '8' is hardcoded at least in 3 different places.
I stole the value from the ext4 code. Since U-Boot lacks a common filesystem code, there will be always duplication. I can happily add a common define for the value.
Also, 8 seems rather small, any reason for choosing that? I believe this is easy to cross even in non-evil filesystems and could perhaps be (again, arbitrarily) increased a bit?
For ext4 the value seems okay. So dunno. :-)
Thanks, //richard
Hi Richard,
richard@sigma-star.at wrote on Wed, 17 Jul 2024 10:16:06 +0200:
Hi Miquel,
Am Mittwoch, 17. Juli 2024, 10:06:35 CEST schrieb 'Miquel Raynal' via upstream:
Hi Richard,
richard@nod.at wrote on Fri, 12 Jul 2024 10:23:44 +0200:
The squashfs driver blindly follows symlinks, and calls sqfs_size() recursively. So an attacker can create a crafted filesystem and with a deep enough nesting level a stack overflow can be achieved.
Fix by limiting the nesting level to 8.
As this is I believe an arbitrary value, could we define this value somewhere and flag it with a comment as "arbitrary" with some details from the commit log? Right now the value '8' is hardcoded at least in 3 different places.
I stole the value from the ext4 code.
Ah ok, interesting. So I guess it is "enough" and was probably not so random.
Since U-Boot lacks a common filesystem code, there will be always duplication. I can happily add a common define for the value.
Oh yeah, I meant a define in squashfs' code. I was not hinting to declare a global number (even though in practice it would be nice).
Also, 8 seems rather small, any reason for choosing that? I believe this is easy to cross even in non-evil filesystems and could perhaps be (again, arbitrarily) increased a bit?
For ext4 the value seems okay. So dunno. :-)
Yeah, fine I guess.
Thanks, Miquèl
Am Mittwoch, 17. Juli 2024, 10:26:29 CEST schrieb Miquel Raynal:
Since U-Boot lacks a common filesystem code, there will be always duplication. I can happily add a common define for the value.
Oh yeah, I meant a define in squashfs' code.
Ah! I thought I did so, but seems like I did this cleanup only in my head. :-S Will send a v2 soon.
Thanks, //richard
Hi Richard,
richard@nod.at wrote on Fri, 12 Jul 2024 10:23:41 +0200:
A carefully crafted squashfs filesystem can exhibit an inode size of 0xffffffff, as a consequence malloc() will do a zero allocation. Later in the function the inode size is again used for copying data. So an attacker can overwrite memory. Avoid the overflow by using the __builtin_add_overflow() helper.
Signed-off-by: Richard Weinberger richard@nod.at
Good catch.
Reviewed-by: Miquel Raynal miquel.raynal@bootlin.com
Thanks, Miquèl
participants (3)
-
Miquel Raynal -
Richard Weinberger -
Richard Weinberger