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