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

From: Jeff King <peff@peff.net>
Date: Feb 8, 2024, 01:00 UTC
Message-ID: <20240208010040.GB1059751@coredump.intra.peff.net>
In-Reply-To: <ce83bd09-dbd2-4c9e-8197-6e4800935523@web.de>

On Mon, Feb 05, 2024 at 08:57:46PM +0100, René Scharfe wrote:

Show 5 quoted lines

> 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).

-Peff