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

[Szabolcs Nagy <Szabolcs.Nagy@arm.com>]

The 02/24/2023 12:24, Wilco Dijkstra wrote:
> > 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.

note that the crypt/badsalttest test case that started
this discussion relies on strncmp stopping at the first
mismatch byte, not just at null byte:

https://sourceware.org/git/?p=glibc.git;a=blob;f=crypt/badsalttest.c;h=bc1e5c1442c731b597551919bec9174ecd2cce61;hb=HEAD#l60
https://sourceware.org/git/?p=glibc.git;a=blob;f=crypt/crypt-entry.c;h=c24c0110a66a0b6a8e885b5a3d0e942da46b1325;hb=HEAD#l84

i think it's reasonable to support stopping the read at
null byte but allow reading past the first mismatch.
(memcmp allows reading past mismatches too).

i.e. the strncmp patch and testcase patch is not needed.

then the question is if non-null-termnated input is valid
as crypt salt argument (if not the test should be fixed,
if yes, it should not use strncmp).