From: Phillip Wood Date: Wed, 26 Apr 2023 14:06:16 GMT Subject: Re: [PATCH v2 3/4] parse_commit(): handle broken whitespace-only timestamp Message-ID: In-Reply-To: On 25/04/2023 17:06, Junio C Hamano wrote: > Phillip Wood writes: > >> This probably doesn't matter in practice but we define our own >> isspace() that does not treat '\v' and '\f' as whitespace. However >> parse_timestamp() (which is just strtoumax()) uses the standard >> library's isspace() which does treat those characters as whitespace >> and is locale dependent. This means we can potentially stop at a >> character that parse_timestamp() treats as whitespace and if there are >> no digits after it we'll still walk past the end of the line. Using >> Rene's suggestion of testing the character with isdigit() would fix >> that. It would also avoid parsing negative timestamps as positive >> numbers >> and reject any timestamps that begin with a locale dependent >> digit. Sorry, that bit is not correct, I've since checked the C standard and I think strtoul() and friends expect ascii digits (isdigit() and isxdigit() are also locale independent unlike isspace(), isalpha() etc.) > A very interesting observation. I wonder if a curious person can > craft a malformed timestamp with "hash-object --literally" to do > more than DoS themselves? > > We are not going to put anything other than [ 0-9+-] after the '>' > we scan for, and making sure '>' is followed by SP and then [0-9] > would be sufficient to ensure strtoumax() to stop before the '\n' > but does not ensure that the "signal a bad timestamp with 0" > happens. Perhaps that would be sufficient. I dunno. > >> I'm not familiar with this code, but would it be worth changing >> parse_timestamp() to stop parsing if it sees a newline? > > Meaning replace or write our own strtoumax() equivalent? I was thinking of a wrapper around strtoumax() that skipped the leading whitespace itself and returned 0 if it saw '\n' or the first non-whitespace character was not a digit. It would help other callers avoid the problem with missing timestamps that is being fixed in this series. I was surprised to see that callers are expected to pass a base to parse_timestamp(). All of them seem to pass "10" apart from a caller in upload-pack.c that passes "0" when parsing the argument to "deepen-since" - do we really want to support octal and hexadecimal timestamps there?. Best Wishes Phillip