Re: [PATCH 0/2] Fix possible buffer overflow in remove_subtree()

[Jeff King <peff@peff.net>]

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