On Thu, Mar 13, 2014 at 10:19:06AM +0100, Michael Haggerty wrote:
> These patches are proposed for maint (but also apply cleanly to
> master).
>
> I presume that this is exploitable via Git commands, though I haven't
> verified it explicitly [1].
It's possible to overflow this buffer, like:
git init repo && cd repo &&
blob=$(git hash-object -w /dev/null) &&
big=$(perl -e 'print "a" x 250')
(for i in $(seq 1 17); do mkdir $big && cd $big; done)
printf "100644 blob $blob\t$big\n" >tree &&
tree=$(git mktree <tree) &&
git read-tree $tree &&
git checkout-index -f --all
but I'm not sure if it is easily exploitable for two reasons:
1. We never actually return from the function with the smashed stack.
Immediately after overflowing the buffer, we call lstat(), which
will fail with ENAMETOOLONG (at least on Linux), causing us to call
into die_errno and exit.
2. The overflow relies on us trying to move a very deep hierarchy out
of the way, but I could not convince git to _create_ such a
hierarchy in the first place. Here I do it using relative paths and
recursion, but git generally tries to create paths from the top of
the repo using full pathnames. So it gets ENAMETOOLONG trying to
create the paths in the first place.
Of course somebody may be more clever than I am, or perhaps some systems
have a PATH_MAX that is not enforced by the kernel. It's still a
suspicious bit of code, and I think your patches are a strict
improvement. Besides fixing this potential problem, I notice that we
currently put 4096 bytes on the stack for a recursive function. Removing
"a/a/a..." can put up to 8M on the stack, which might be too much on
some systems (besides just being silly and wasteful).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html