linux-hardening - Re: [PATCH] smb: client: replace deprecated strncpy with strscpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <202403282041.6A4E12EA@keescook>
Date: Thu, 28 Mar 2024 20:44:08 -0700
From: Kees Cook <keescook@...omium.org>
To: Justin Stitt <justinstitt@...gle.com>
Cc: Steve French <sfrench@...ba.org>, Paulo Alcantara <pc@...guebit.com>,
	Ronnie Sahlberg <ronniesahlberg@...il.com>,
	Shyam Prasad N <sprasad@...rosoft.com>, Tom Talpey <tom@...pey.com>,
	Bharath SM <bharathsm@...rosoft.com>, linux-cifs@...r.kernel.org,
	samba-technical@...ts.samba.org, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH] smb: client: replace deprecated strncpy with strscpy

On Thu, Mar 28, 2024 at 09:44:48PM +0000, Justin Stitt wrote:
> strncpy() is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
> 
> In cifssmb.c:
> Using strncpy with a length argument equal to strlen(src) is generally
> dangerous because it can cause string buffers to not be NUL-terminated.
> In this case, however, there was extra effort made to ensure the buffer
> was NUL-terminated via a manual NUL-byte assignment. In an effort to rid
> the kernel of strncpy() use, let's swap over to using strscpy() which
> guarantees NUL-termination on the destination buffer.
> 
> To handle the case where ea_name is NULL, let's use the ?: operator to
> substitute in an empty string, thereby allowing strscpy to still
> NUL-terminate the destintation string.

Yeah. And for the non-NULL case, namelen is 0-255. And 255 comes from
strnlen() as the limit of characters, so namelen + 1 will include the NUL
terminator.

> Interesting note: this flex array buffer may go on to also have some
> value encoded after the NUL-termination:
> |	if (ea_value_len)
> |		memcpy(parm_data->list.name + name_len + 1,
> |			ea_value, ea_value_len);
> 
> Now for smb2ops.c and smb2transport.c:
> Both of these cases are simple, strncpy() is used to copy string
> literals which have a length less than the destination buffer's size. We
> can simply swap in the new 2-argument version of strscpy() introduced in
> Commit e6584c3964f2f ("string: Allow 2-argument strscpy()").
> 
> 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 [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@...r.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@...gle.com>

Reviewed-by: Kees Cook <keescook@...omium.org>

-- 
Kees Cook