Re: [PATCH v9 5/5] transport: add from_user parameter to is_transport_allowed

[Jeff King <peff@peff.net>]

On Wed, Dec 14, 2016 at 12:13:23PM -0800, Brandon Williams wrote:

> > I find it unlikely that anybody would ever care, but at least we'd do
> > the safe thing. I dunno. Maybe I am just being lazy.
> 
> Well, that's unfortunate!  It does sound like a more full-proof solution
> to these dumb http alternates could be involved.  I don't think your
> simple "lazy" solution may be enough to not just die by default.
> 
> By default ftp/ftps will have a policy of "user only" which means they
> will be set by the call to get_curl_allowed_protocol(-1) but not set by
> get_curl_allowed_protocol(0).  This would result in the call to
> check_alternates_protocol_restrictions failing all the time unless the
> user explicitly sets ftp/ftps to "always" or "never".  If that is the
> desired behavior then your proposed solution would be fine, otherwise we
> may have to do the more involved approach.

Oh, hrm, you're right. I was definitely meaning for this to kick in only
when you had explicitly configured a protocol to "user" (they'd still
need to enable redirects, but that's much more likely).

Arguably ftp should be on the "safe" list, since it's implemented via
the exact same code as https. That list comes originally from 33cfccbbf
(submodule: allow only certain protocols for submodule fetches,
2015-09-16), but that was just based on what I thought was reasonable at
the time.

I find it hard to believe anybody actually uses git-over-ftp these days,
let alone as a protocol redirect via http-alternates. But AFAIK it does
work.

One other "simple" fix is at the moment we parse http-alternates, to
parse the URL ourself and check the policy. Like:

  const char *protocols[] = {
	"http", "https", "ftp", "ftps"
  };

  for (i = 0; i < ARRAY_SIZE(protocols); i++) {
	const char *end;
	if (skip_prefix(url, protocols[i], end) && starts_with(end, "://"))
		break;
  }

  if (i >= ARRAY_SIZE(protocols))
	warning("ignoring alternate with unknown protocol: %s", url);
  else if (!is_transport_allowed(protocols[i], 0))
	warning("ignoring alternate with restricted protocol: %s", url);
  else {
	/* actually set up the alt struct */
  }

That keeps the logic nicely contained. The downside is that if our
interpretation of the URL is ever different than curl's, it could create
a security problem. The bit above seems fairly foolproof, though,
because it functions as a whitelist. So it doesn't matter if you can
trigger an http request via curl with exotic syntax; it matters whether
curl will trigger anything _besides_ an http syntax if the string starts
with "http://". Which seems unlikely.

I may have convinced myself this is a reasonable approach.

-Peff