[PATCH 1/4] squashfs: Fix integer overflow in sqfs_resolve_symlink()

[PATCH 1/4] squashfs: Fix integer overflow in sqfs_resolve_symlink()

[Richard Weinberger]

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

[PATCH 2/4] squashfs: Fix integer overflow in sqfs_inode_size()

[Richard Weinberger]

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:
-- 
2.35.3

[PATCH 3/4] squashfs: Check sqfs_find_inode() return value

[Richard Weinberger]

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

[PATCH 4/4] squashfs: Fix stack overflow while symlink resolving

[Richard Weinberger]

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

Re: [PATCH 1/4] squashfs: Fix integer overflow in sqfs_resolve_symlink()

[Miquel Raynal]

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

Re: [PATCH 2/4] squashfs: Fix integer overflow in sqfs_inode_size()

[Miquel Raynal]

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

Re: [PATCH 3/4] squashfs: Check sqfs_find_inode() return value

[Miquel Raynal]

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

Re: [PATCH 4/4] squashfs: Fix stack overflow while symlink resolving

[Miquel Raynal]

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

Re: [PATCH 4/4] squashfs: Fix stack overflow while symlink resolving

[Richard Weinberger]

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

-- 
​​​​​sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT
UID/VAT Nr: ATU 66964118 | FN: 374287y

Re: [PATCH 4/4] squashfs: Fix stack overflow while symlink resolving

[Miquel Raynal]

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

Re: [PATCH 4/4] squashfs: Fix stack overflow while symlink resolving

[Richard Weinberger]

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

-- 
​​​​​sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT
UID/VAT Nr: ATU 66964118 | FN: 374287y