Re: [PATCH v2] string: Fix OOB read on generic strncmp

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 24/02/23 09:24, Wilco Dijkstra wrote:
> Hi,
> 
>> It's common to use strncmp as a starts-with predicate, like this:
>>
>>   strncmp (s, "prefix", 5)
>>
>> This requires that reading stops at the first null byte.  C11 wording
>> makes this kind of usage inadvisable because it treats the inputs as
>> arrays, and this means that mean that implementations could read past
>> the null byte.  But that doesn't match current programming practice.
> 
> C11 says you can't read past the end of the array, but it doesn't say that
> you *must* read past a NUL-byte. My interpretation is that this is a valid
> strncmp implementation:
> 
> int
> simple_strncmp (const char *s1, const char *s2, size_t size)
> {
>    size_t len1 = strnlen (s1, size);
>    size_t len2 = strnlen (s2, size);
>    if (len1 < len2)
>      len1++;
>    else if (len1 > len2)
>      len1 = len2 + 1;
>    return memcmp (s1, s2, len1);
> }
> 
> Ie. both strings must be valid up until the given size or NUL-terminated if
> smaller. This works on the example above even if the first string is only 1 byte.

That was my understanding as well when I wrote the generic strncmp, and
multiple strncmp implementation also follows this same idea. 

So another option is to keep the generic strncmp as is and just remove the 
crypt/badsalttest.c, since it passing invalid inputs on crypt (non null 
terminated strings).

> 
>> The strnlen function has the same problem if you want to use it to limit
>> readhead, e.g. in sscanf to fix bug 17577.  (POSIX also speaks of an
>> array argument.)  In stdio-common/Xprintf_buffer_puts_1.c, we already
>> use it to avoid a similar performance glitch.  It's not the first such
>> uses, there is already a similar call (with similar rationale) in
>> string/strcasestr.c, and for wcsnlen in
>> stdio-common/vfprintf-process-arg.c.
>>
>> I think we should support all these as extensions.  The alternative
>> would be to add new functions that stop reading after the first null
>> byte (particularly for the strnlen optimization).
> 
> Existing functions already do stop after the first NUL byte. Even if the C
> standard doesn't explicitly disallow it, it doesn't seem valid to read beyond
> it (a lot of code could fail if we did).
> 
> Cheers,
> Wilco