linux-kernel - Re: [PATCH 2/4] tools/nolibc: Fix strlcat() return code and size usage

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <4de5a6a5-8f7f-4af4-a4ad-d6521b056a4f@sdfg.com.ar>
Date: Tue, 13 Feb 2024 07:20:15 +0100
From: Rodrigo Campos <rodrigo@...g.com.ar>
To: Willy Tarreau <w@....eu>
Cc: Thomas Weißschuh <linux@...ssschuh.net>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] tools/nolibc: Fix strlcat() return code and size
 usage

On 2/13/24 00:16, Rodrigo Campos wrote:
> On 2/11/24 11:48, Willy Tarreau wrote:
>> Hi Rodrigo,
>>
>> first, thanks for the series!
> 
> Thank you, for your time and review! :)
> 
>> On Mon, Jan 29, 2024 at 03:15:14PM +0100, Rodrigo Campos wrote:
>>> The return code should always be strlen(src) + strlen(dst), but dst is
>>> considered shorter if size is less than strlen(dst).
>>>
>>> While we are there, make sure to copy at most size-1 bytes and
>>> null-terminate the dst buffer.
>>>
>>> Signed-off-by: Rodrigo Campos <rodrigo@...g.com.ar>
>>> ---
>>>   tools/include/nolibc/string.h | 14 +++++++-------
>>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/tools/include/nolibc/string.h 
>>> b/tools/include/nolibc/string.h
>>> index ed15c22b1b2a..b2149e1342a8 100644
>>> --- a/tools/include/nolibc/string.h
>>> +++ b/tools/include/nolibc/string.h
>>> @@ -187,23 +187,23 @@ char *strndup(const char *str, size_t maxlen)
>>>   static __attribute__((unused))
>>>   size_t strlcat(char *dst, const char *src, size_t size)
>>>   {
>>> -    size_t len;
>>>       char c;
>>> +    size_t len = strlen(dst);
>>> +    size_t ret = strlen(src) + (size < len? size: len);
>>
>>  From what I'm reading in the man page, ret should *always* be the sum
>> of the two string lengths. I guess it helps for reallocation. It's even
>> explicitly mentioned:
>>
>>    "While this may seem somewhat confusing, it was done to make 
>> truncation
>>     detection simple."
> 
> Yes, that was my *first* understanding of the manpage too. But it 
> doesn't seem to be the correct interpretation.
> 
> Also, this is exactly what I tried to say in the cover letter, with:
> 
>      I thought the manpage was clear, but when checking against that,
>      I noted a few twists (like the manpage says the return code of
>      strlcat is strlen(src) + strlen(dst), but it was not clear it is
>      not that if size < strlen(dst). When looking at the libbsd
>      implementation and re-reading the manpage, I understood what it
>      really meant).
> 
> 
> Sorry if I wasn't clear. Let me try again.
> 
> My first interpretation of the manpage was also that, and I think it 
> would make sense to be that one. However, it is wrong, they also say 
> this, that is what made me add the ternary operator:
> 
>      Note, however, that if strlcat() traverses size characters
>      without finding a NUL, the length of the string is considered
>      to be  size and the destination string will not be NUL
>      terminated (since there was no space for the NUL)
> 
> So, my interpretation is that it is the sum of both, except when we 
> can't find the NUL in that size, in which case we conside "size" to be 
> the len of dst.
> 
> If you compare it with the output of libbsd, the return code seems to be 
> exactly that. I was surprised too, as the manpage seem so clear... :-/


I'm not convinced now if that is the right interpretation. I'll check 
the libbsd code, I don't remember it now, it's been a few days already.

My memory is that it was not something so sane.