Re: [PATCH 4/9] cache-tree: avoid strtol() on non-string buffer

> A cache-tree extension entry in the index looks like this:
> 
>   <name> NUL <entry_nr> SPACE <subtree_nr> NEWLINE <binary_oid>
> 
> where the "_nr" items are human-readable base-10 ASCII. We parse them
> with strtol(), even though we do not have a NUL-terminated string (we'd
> generally have an mmap() of the on-disk index file). For a well-formed
> entry, this is not a problem; strtol() will stop when it sees the
> newline. But there are two problems:
> 
>   1. A corrupted entry could omit the newline, causing us to read
>      further. You'd mostly get stopped by seeing non-digits in the oid
>      field (and if it is likewise truncated, there will still be 20 or
>      more bytes of the index checksum). So it's possible, though
>      unlikely, to see read off the end of the mmap'd buffer. Of course a

>      malicious index file can fake the oid and the index checksum to all
>      (ASCII) 0's.
> 
>      This is further complicated by the fact that mmap'd buffers tend to
>      be zero-padded up to the page boundary. So to run off the end, the
>      index size also has to be a multiple of the page size. This is also
>      unlikely, though you can construct a malicious index file that
>      matches this.
> 
>      The security implications aren't too interesting. The index file is
>      a local file anyway (so you can't attack somebody by cloning, but
>      only if you convince them to operate in a .git directory you made,
>      at which point attacking .git/config is much easier). And it's just
>      a read overflow via strtol(), which is unlikely to buy you much
>      beyond a crash.

> diff --git a/cache-tree.c b/cache-tree.c
> index 2aba47060e..ab20ffe863 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -548,12 +548,36 @@ void cache_tree_write(struct strbuf *sb, struct cache_tree *root)
>  	trace2_region_leave("cache_tree", "write", the_repository);
>  }
>  
> +static long parse_long(const char **ptr, unsigned long *len_p)
> +{
> +	const char *s = *ptr;
> +	unsigned long len = *len_p;
> +	long ret = 0;
> +	int sign = 1;
> +
> +	while (len && *s == '-') {
> +		sign *= -1;
> +		s++;
> +		len--;
> +	}
> +
> +	while (len) {
> +		if (!isdigit(*s))
> +			break;
> +		ret *= 10;
> +		ret += *s - '0';
> +		s++;
> +		len--;
> +	}
> +	*ptr = s;
> +	*len_p = len;
> +	return sign * ret;
> +}