Received-SPF: pass (google.com: domain of linux-kernel+bounces-124103-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 28 Mar 2024 21:03:23 -0700
From: Kees Cook <keescook@chromium.org>
To: Justin Stitt <justinstitt@google.com>
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-hardening@vger.kernel.org
Subject: Re: [PATCH] hfsplus: refactor copy_name to not use strncpy
Message-ID: <202403282059.37BA0B8E20@keescook>
References: <20240321-strncpy-fs-hfsplus-xattr-c-v1-1-0c6385a10251@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240321-strncpy-fs-hfsplus-xattr-c-v1-1-0c6385a10251@google.com>

On Thu, Mar 21, 2024 at 11:46:27PM +0000, Justin Stitt wrote:
> strncpy() is deprecated with NUL-terminated destination strings [1].
> 
> The copy_name() method does a lot of manual buffer manipulation to
> eventually arrive with its desired string. If we don't know the
> namespace this attr has or belongs to we want to prepend "osx." to our
> final string. Following this, we're copying xattr_name and doing a
> bizarre manual NUL-byte assignment with a memset where n=1.
> 
> Really, we can use some more obvious string APIs to acomplish this,
> improving readability and security. Following the same control flow as
> before: if we don't know the namespace let's use scnprintf() to form our
> prefix + xattr_name pairing (while NUL-terminating too!). Otherwise, use
> strscpy() to return the number of bytes copied into our buffer.
> 
> Note that strscpy() _can_ return -E2BIG but this is already handled by
> all callsites:
> 
> In both hfsplus_listxattr_finder_info() and hfsplus_listxattr(), ret is
> already type ssize_t so we can change the return type of copy_name() to
> match (understanding that scnprintf()'s return type is different yet
> fully representable by ssize_t). Furthermore, listxattr() in fs/xattr.c
> is well-equipped to handle a potential -E2BIG return result from
> vfs_listxattr():
> |	ssize_t error;
> ...
> |	error = vfs_listxattr(d, klist, size);
> |	if (error > 0) {
> |		if (size && copy_to_user(list, klist, error))
> |			error = -EFAULT;
> |	} else if (error == -ERANGE && size >= XATTR_LIST_MAX) {
> |		/* The file system tried to returned a list bigger
> |			than XATTR_LIST_MAX bytes. Not possible. */
> |		error = -E2BIG;
> |	}
> ... our error can potentially already be -E2BIG, skipping this else-if
> and ending up at the same state as other errors.
> 
> This whole copy_name() function could really be a one-line with some
> ternary statements embedded into a scnprintf() arg-list but I've opted
> to maintain some semblance of readability.
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> ---
>  fs/hfsplus/xattr.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 9c9ff6b8c6f7..00351f566e9f 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -400,22 +400,13 @@ static int name_len(const char *xattr_name, int xattr_name_len)
>  	return len;
>  }
>  
> -static int copy_name(char *buffer, const char *xattr_name, int name_len)
> +static ssize_t copy_name(char *buffer, const char *xattr_name, int name_len)
>  {
> -	int len = name_len;
> -	int offset = 0;
> -
> -	if (!is_known_namespace(xattr_name)) {
> -		memcpy(buffer, XATTR_MAC_OSX_PREFIX, XATTR_MAC_OSX_PREFIX_LEN);
> -		offset += XATTR_MAC_OSX_PREFIX_LEN;
> -		len += XATTR_MAC_OSX_PREFIX_LEN;
> -	}
> -
> -	strncpy(buffer + offset, xattr_name, name_len);
> -	memset(buffer + offset + name_len, 0, 1);
> -	len += 1;

The old code appears to include the NUL in the count of bytes written.

> +	if (!is_known_namespace(xattr_name))
> +		return scnprintf(buffer, name_len + XATTR_MAC_OSX_PREFIX_LEN,
> +				 "%s%s", XATTR_MAC_OSX_PREFIX, xattr_name);
>  
> -	return len;
> +	return strscpy(buffer, xattr_name, name_len + 1);

Neither scnprintf nor strscpy include the NUL in their return value, so
I think special handling is needed here. Perhaps:

	ssize_t len;
	...
	if (!is_known_namespace(xattr_name))
		len = scnprintf(buffer, name_len + XATTR_MAC_OSX_PREFIX_LEN,
				 "%s%s", XATTR_MAC_OSX_PREFIX, xattr_name);
	else
		len = strscpy(buffer, xattr_name, name_len + 1);

	if (len >= 0)
		len++;
	return len;

-Kees

>  }
>  
>  int hfsplus_setxattr(struct inode *inode, const char *name,
> 
> ---
> base-commit: 241590e5a1d1b6219c8d3045c167f2fbcc076cbb
> change-id: 20240321-strncpy-fs-hfsplus-xattr-c-4ebfe67f4c6d
> 
> Best regards,
> --
> Justin Stitt <justinstitt@google.com>
> 

-- 
Kees Cook