Re: [bug #66296] rshd.c string overflow warning

Simon Josefsson via Bug reports for the GNU Internet utilities <[email protected]> Tue, 08 Oct 2024 08:33:35 +0200

Newsgroups	gmane.comp.gnu.inetutils.bugs
Message-ID	<[email protected]>

Erik Auerswald <[email protected]> writes:

> Hi,
>
> On Sat, Oct 05, 2024 at 11:48:56AM -0400, Jeffrey Cliff wrote:
>> URL:
>>   <https://savannah.gnu.org/bugs/?66296>
>> 
>>                  Summary: rshd.c string overflow warning
>
> Thanks for reporting an issue you encountered with GNU Inetutils!
>
>> [...]
>> Date: Sat 05 Oct 2024 10:48:53 AM CDT By: Jeffrey Cliff <themusicgod1>
>> inetutils: 2.5
>> gcc: (GCC) 15.0.0 20240509 (experimental)
>> 
>> rshd.c:1923:3: warning: 'strncat' specified bound 13 equals source length
>> [-Wstringop-overflow=]
>>  1923 |   strncat (path, PATH_DEFPATH, sizeof (path) - sizeof ("PATH=") - 1);
>
> I'd say this is a wrong warning, because there is nothing wrong to warn
> about.  With "src" as long as "n", strncat appends all n bytes to "dest"
> and adds a NUL byte.  This requires the "dest" buffer to be one byte longer
> than strlen(dest) + n.  This is the case here, as can be seen from your
> patch.

While I agree that the warning seems weird -- but isn't this fix the
right thing anyway?

-char path[sizeof (PATH_DEFPATH) + sizeof ("PATH=")] = "PATH=";
+char path[sizeof (PATH_DEFPATH) + sizeof ("PATH=")+1] = "PATH=";

If PATH_DEFPATH is say "/bin" then sizeof of it is 4, and
sizeof("PATH=") is 5, and we want the resulting concatenated string to
be "PATH=/bin" (with terminating NUL character) which has length 5+4 and
needs sizeof 5+4+1 for storage.

Maybe I'm confused by the strange strncat call.  I'm happy to revert
this, and I'm sorry I didn't reproduce and test things more carefully
before installing the change.

/Simon

>> if i'm reading this right, increasing the size of the "path" string by one
>> should should fix this:
>
> There is nothing to fix.  Your patch just adds an usused byte to the "path"
> buffer.
>
>> --- inetutils-2.5/src/rshd.c	2023-12-29 11:34:46.000000000 -0600
>> +++ inetutils-2.5-works/src/rshd.c	2024-10-05 09:47:18.126254725 -0600
>> @@ -411,7 +411,7 @@
>>  char logname[32 + sizeof ("LOGNAME=")] = "LOGNAME=";
>>  char homedir[256 + sizeof ("HOME=")] = "HOME=";
>>  char shell[64 + sizeof ("SHELL=")] = "SHELL=";
>> -char path[sizeof (PATH_DEFPATH) + sizeof ("PATH=")] = "PATH=";
>> +char path[sizeof (PATH_DEFPATH) + sizeof ("PATH=")+1] = "PATH=";
>>  char rhost[128 + sizeof ("RHOST=")] = "RHOST=";
>>  
>>  #ifndef WITH_PAM
>
> Thanks,
> Erik
>
>

signature.asc (application/pgp-signature, 255 B)