From: Stefan Sperling <stsp@stsp.name>
Subject: Re: fuse: readlink shouldn't silently truncate
To: Helg <helg-openbsd@gmx.de>
Cc: tech@openbsd.org
Date: Tue, 9 Sep 2025 10:44:53 +0200

On Mon, Sep 08, 2025 at 08:25:39PM +0200, Helg wrote:
> The fuse vfs syscall readlink doesn't check the size of the buffer
> passed to it. This patch now uses checks the buffer size without making
> any assumptions. It also adds a few other related checks.
> 
> Note that Linux libfuse uses a buffer of PATH_MAX + 1 but in my testing
> this can sometimes be too small. For example, if I pass a larger buffer
> to readlink(2) then the syscall uses this size. I think that libfuse
> should not make any assumptions and use fb_io_len as passed by the
> kernel. It's up to the file system if it wants to use the POSIX PATH_MAX
> limit.
> 
> I've added a sanity check in fuse_devic.c to check that the buffer
> returned from userland is not larger than fb_io_len. The buffer would
> simply be truncated by uiomove in fuse_vnops.c later but silently
> truncating is not ideal. This will now also protect read and readdir.
> 
> The NUL check in fusefs_readlink was inspired by FreeBSD.
> 
> Lastly, I noticed that readdir was not mentioned in the man page so
> added it.
> 
> OK?

ok by me, and I have two suggestions below. Feel free to take them or
ignore them as you wish:

> +	if (!fbuf->fb_err) {
> +		/* file system should return a NUL-terminated string */
> +		fbuf->fb_len = strnlen(name, bufsize);
>  
> -	fbuf->fb_err = ret;
> -	if (!ret) {
> -		len = strnlen(name, PATH_MAX);
> -		fbuf->fb_len = len;
> -		memcpy(fbuf->fb_dat, name, len);
> +		/* string was not NUL-terminated, assume it was too long */
> +		if (fbuf->fb_len == bufsize)
> +			fbuf->fb_err = -ENAMETOOLONG;
> +		else
> +			memcpy(fbuf->fb_dat, name, fbuf->fb_len);

The above strnlen() + memcpy() dance seems like a suitable use case
for strlcpy(). Which will likewise tell us whether the string was
truncated while avoiding a second pass over the string.

	fbuf->fb_len = strlcpy(fbuf->fb_dat, name, bufsize);
	if (fbuf->fb_len >= bufsize)
		fbuf->fb_err = -ENAMETOOLONG;

Though perhaps compiler optimizations achieve the same effect anyway.

> @@ -934,6 +941,14 @@ fusefs_readlink(void *v)
>  		return (error);
>  	}
>  
> +	if (strnlen(fbuf->fb_dat, fbuf->fb_len) != fbuf->fb_len) {
> +		printf("fusefs: symbolic link contains embedded NUL: %s\n",
> +		   fbuf->fb_dat);
> +
> +		fb_delete(fbuf);
> +		return (EIO);
> +	}
> +

The above printf should either be dropped, written as a code comment,
or delegated to a debug-printf. Else, I'm pretty sure someone will
complain about dmesg spam at some point.