Re: [PATCH] commit.c: ensure strchrnul() doesn't scan beyond range

> On Mon, Feb 05, 2024 at 08:57:46PM +0100, René Scharfe wrote:
>
>> If you want to make the code work with buffers that lack a terminating
>> NUL then you need to replace the strchrnul() call with something that
>> respects buffer lengths.  You could e.g. call memchr().  Don't forget
>> to check for NUL to preserve the original behavior.  Or you could roll
>> your own custom replacement, perhaps like this:
>
> I'm not sure it is worth retaining the check for NUL. The original
> function added by me in fe6eb7f2c5 (commit: provide a function to find a
> header in a buffer, 2014-08-27) just took a NUL-terminated string, so
> we certainly were not expecting embedded NULs.
>
> In cfc5cf428b (receive-pack.c: consolidate find header logic,
> 2022-01-06) we switched to taking the "len" parameter, but the new
> caller just passes strlen(msg) anyway.
>
> I guess you could argue that before that commit, receive-pack.c's
> find_header() which took a length was buggy to use strchrnul(). It gets
> fed with a push-cert buffer. I guess it's possible for there to be an
> embedded NUL there, but in practice there shouldn't be. If we are
> thinking of malformed or malicious input, it's not clear which behavior
> (finding or not finding a header past a NUL) is more harmful. So all
> things being equal, I would try to reduce the number of special cases
> here by not worrying about NULs.
>
> (Though if somebody really wants to dig, it's possible there's a clever
> dual-parser attack here where "\nfoo\0bar baz" finds the header "bar
> baz" in one parser but not in another).