Re: [PATCH 0/2] upload-archive security issues

From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Nov 15, 2011, 22:01 UTC
Message-ID: <CABPQNSZ71==pQvH9nTQ3rD6c1RJiLmkA+k_9KS=T45cQb+PTLA@mail.gmail.com>
In-Reply-To: <20111115214159.GA20457@sigill.intra.peff.net>

On Tue, Nov 15, 2011 at 10:42 PM, Jeff King <peff@peff.net> wrote:

Show 15 quoted lines

> Looking at Erik's c09cd77e again, there are some serious security
> problems, in that we are too lenient with what gets passed to
> git-archive, which is not hardened to accept random client arguments.
> That lets a client do all sorts of nasty things like running arbitrary
> code.
>
> These patches fix it by making cmd_archive handle the remote-request
> flag better. An alternative would be to pass only known-good options
> through upload-archive. That might be more future-proof, but also
> involves upload-archive knowing about the innards of write_archive and
> its options.  See also the comments in patch 2/2 for another alternative
> fix.
>
>  [1/2]: archive: don't allow negation of --remote-request
>  [2/2]: archive: limit ourselves during remote requests

Yikes! Perhaps the whole deal of rewriting the code to take explicit file descriptors (and/or dup-bonanza) would have been the better choice after all?

For the record: I would be fine with c09cd77e simply being reverted for this release, and having a better version applied in the near future. Windows support for upload-archive is not worth the risk of slipping in a remote code execution bug...

>
> And yes, I feel like a moron for not noticing these problems during my
> initial review.

Not only did you fail to spot them, you actually wrote that part of the code ;)

http://article.gmane.org/gmane.comp.version-control.git/178098

(I don't mean to shift blame over to you, I'm the one who should have spent more time thinking about this as this was "my" series)