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

[Miquel Raynal <miquel.raynal@bootlin.com>]

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