Re: [PATCH v2 3/4] parse_commit(): handle broken whitespace-only timestamp

[Phillip Wood <phillip.wood123@gmail.com>]

On 25/04/2023 17:06, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> 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