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é
