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

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

quoted

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