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

["René Scharfe" <l.s.r@web.de>]

Am 08.02.24 um 02:00 schrieb Jeff King:
> 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).

Good point.  A _mem function shouldn't worry about NULs.  Its callers
are responsible for that -- if necessary.

No idea what an attacker could do with nonce and push-option headers
with varying visibility.  Version detection?  Something worse?

But anyway: If NULs are of no concern and we currently end parsing when
we see one in all cases, why do we need a _mem function at all?  The
original version of the function, find_commit_header(), should suffice.
check_nonce() could be run against the NUL-terminated sigcheck.payload
and check_cert_push_options() parses an entire strbuf, so there is no
risk of out-of-bounds access.

René