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

From: Jeff King <peff@peff.net>
Date: Apr 26, 2023, 11:36 UTC
Message-ID: <20230426113658.GC130148@coredump.intra.peff.net>
In-Reply-To: <xmqqttx43q08.fsf@gitster.g>

On Tue, Apr 25, 2023 at 09:06:47AM -0700, Junio C Hamano wrote:

Show 17 quoted lines

> 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.
> 
> 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?

I think the answer is no, because the worst case is that they read to the trailing NUL that we stick after any object content we read into memory. So we'd mis-parse:

  committer name <email> \v\n

  123456 in the subject line

to read "123456" as the commit timestamp (so basically the same bug my patch was trying to fix). But we'd never read out-of-bounds memory. Still, it does not give me warm fuzzies, and I think is worth fixing.

Show 5 quoted lines

> 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.

Any single non-whitespace character at all would be sufficient to avoid the problem. And that's what the current iteration of the patch is trying to do. It's just that our definition of "whitespace" has to agree with strtoumax()'s for it to work. And as Phillip notes, that may even include locale dependent characters. So I don't think we want to get into trying to match them all (i.e., a "allow known" strategy).

Instead, we should go back to what the original iteration of the series was doing, and make sure there is at least one digit (i.e., a "forbid unknown" strategy). Assuming that there is no locale where ascii "1" is considered whitespace. ;)

Note that will exclude a few cases that we do allow now, like:

  committer name <email> \v123456 +0000\n

Right now that parses as "123456", but we'd reject it as "0" after such a patch.

The alternative is to check _all_ of the characters between ">" and the newline and make sure there is some digit somewhere, which would be sufficient to prevent strtoumax() from walking past the newline.

I guess it's not even any more expensive in the normal case (since the very first non-whitespace entry should be a digit!). I'm not sure it's worth caring about too much either way. Garbage making it into name/email is an easy mistake to make (for users and implementations). Putting whitespace control codes into your timestamp is not, and marking them as "0" is an OK outcome.

-Peff