Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed

[Jeff King <peff@peff.net>]

On Thu, Dec 01, 2016 at 03:26:56PM -0800, Brandon Williams wrote:

> > I started taking a look at your http redirect series (I really should
> > have taking a look at it sooner) and I see exactly what you're talking
> > about.  We can easily move this logic into a function to make it easier
> > to generate the two whitelists.
> 
> Thinking about this some more...I was told that having http redirect to
> file:// could be scary.  The way the new protocol configuration is setup
> we have file:// as a default known-safe protocol.  Do we need to worry
> about this or can we leave this be since this can be overridden by the
> user?

Hmm. I'm not sure if file:// should actually be USER_ONLY. The submodule
code allows it, and it's certainly a convenience, but I guess you could
do tricky things by probing somebody's filesystem with submodules URLs.
On the other hand, if you are recursively cloning untrusted repos and
have sensitive contents on disk, you really _should_ be setting up a
protocol whitelist.

For HTTP redirects within curl, I think it's a non-issue; curl
automatically disallows file:// for redirects, even without us telling
it so.

For redirects via http-alternates, it's a bit more tricky, as we feed
the URL to curl ourselves, so it can't tell the difference between
trusted and untrusted input. The main protection provided by my series
is "don't follow http-alternates at all". But assuming you did want to
use them (by setting http.followRedirects to "true", at least for the
server in question), we could then feed file:// directly to curl. But I
think we are still OK, because the restricted CURLOPT_PROTOCOL setting
would prevent that from working. I.e., git _never_ wants curl to handle
file://, because it handles it without calling into remote-curl.c at
all.

So arguably file:// should be USER_ONLY, but I'm not sure how much it
matters in practice.

-Peff