On 12/01, Jeff King wrote:
> On Mon, Nov 28, 2016 at 04:15:08PM -0800, Junio C Hamano wrote:
>
> > * bw/transport-protocol-policy (2016-11-09) 2 commits
> > (merged to 'next' on 2016-11-16 at 1391d3eeed)
> > + transport: add protocol policy config option
> > + lib-proto-disable: variable name fix
> >
> > Finer-grained control of what protocols are allowed for transports
> > during clone/fetch/push have been enabled via a new configuration
> > mechanism.
> >
> > Will cook in 'next'.
>
> I was looking at the way the http code feeds protocol restrictions to
> CURLOPT_REDIR_PROTOCOLS, and I think this topic is missing two elements:
>
> 1. The new policy config lets you say "only allow this protocol when
> the user specifies it". But when http.c calls is_transport_allowed(),
> the latter has no idea that we are asking it about potential
> redirects (which obviously do _not_ come from the user), and would
> erroneously allow them.
>
> I think this needs fixed before the topic is merged. It's not a
> regression, as it only comes into play if you use the new policy
> config. But it is a minor security hole in the new feature.
I agree and it should be an easy fix. We can just add a parameter like
so:
diff --git a/transport.c b/transport.c
index 2c0ec76..d38d50f 100644
--- a/transport.c
+++ b/transport.c
@@ -723,7 +723,7 @@ static enum protocol_allow_config get_protocol_config(const char *type)
return PROTOCOL_ALLOW_USER_ONLY;
}
-int is_transport_allowed(const char *type)
+int is_transport_allowed(const char *type, int redirect)
{
const struct string_list *whitelist = protocol_whitelist();
if (whitelist)
@@ -735,7 +735,7 @@ int is_transport_allowed(const char *type)
case PROTOCOL_ALLOW_NEVER:
return 0;
case PROTOCOL_ALLOW_USER_ONLY:
- return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+ return git_env_bool("GIT_PROTOCOL_FROM_USER", !redirect);
}
die("BUG: invalid protocol_allow_config type");
That way the libcurl code can say it is asking if it is ok to redirect
to that protocol.
>
> 2. If your curl is too old to support CURLOPT_REDIR_PROTOCOLS, we will
> warn if there is a protocol whitelist in effect. But that check
> only covers the environment whitelist, and we do not warn if you
> restrict other protocols.
>
> I actually think this should probably just warn indiscriminately.
> Even without a Git protocol whitelist specified, the code serves to
> prevent curl from redirecting to bizarre protocols like smtp. The
> affected curl versions are from 2009 and prior, so I kind of doubt
> it matters much either way (I'm actually tempted to suggest we bump
> the minimum curl version there; there's a ton of #ifdef cruft going
> back to 2002-era versions of libcurl).
We should switch to warning all the time since this series adds in
default whitelisted/blacklisted protocols anyways.
--
Brandon Williams