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

From: Jeff King <peff@peff.net>
Date: Feb 8, 2024, 21:41 UTC
Message-ID: <20240208214137.GB1090198@coredump.intra.peff.net>
In-Reply-To: <xmqqil2yn3ey.fsf@gitster.g>

On Thu, Feb 08, 2024 at 11:48:05AM -0800, Junio C Hamano wrote:

Show 14 quoted lines

> René Scharfe <l.s.r@web.de> writes:
> 
> > 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.
> 
> If I recall correctly, the caller that does not pass strlen() as the
> payload length gives a length that is shorter than the buffer, i.e.
> "stop the parsing here, do not get confused into thinking the
> garbage after this point contains useful payload" was the reason why
> we have a separate "len".

Yes, check_nonce() passes in a length limited by the start of the actual signature, as determined by parse_signed_buffer(). Though that generally comes after a blank line, which would also stop find_header() from parsing further.

But more interestingly: even though we pass a buf/len pair to parse_signed_buffer(), it then calls get_format_by_sig() which takes only a NUL-terminated string. So:

  1. It is not possible for the buf/len pair we pass to check_nonce() to
     contain a NUL. And thus there is no caller of find_header_mem()
     that can contain an embedded NUL. So switching from strchrnul() to
     just memchr() should be OK there.

  2. That raises the question of whether parse_signed_buffer() has a
     similar walk-too-far problem. ;) The answer is no, because we feed
     it from a strbuf. But it's not a great pattern overall.

-Peff