Re: bw/transport-protocol-policy
On 12/01, Jeff King wrote:
> On Thu, Dec 01, 2016 at 11:35:24AM -0800, Brandon Williams wrote:
>
> > > I wouldn't expect anyone to ever set GIT_PROTOCOL_FROM_USER=1, but it
> > > does behave in a funny way here, overriding the "redirect" flag. I think
> > > we'd want something more like:
> > >
> > > if (redirect < 0)
> > > redirect = git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
> > >
> > > and then pass in "-1" from transport_check_allowed().
> >
> > I don't think I quite follow your solution but I came up with this:
> >
> > case PROTOCOL_ALLOW_USER_ONLY:
> > return redirect ? 0 : git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
> >
> > Which should address the same issue.
>
> I think mine was confused a bit by using the word "redirect". It was
> really meant to be "from_user", and could take three values: definitely
> yes, definitely no, and unknown (-1). And in the unknown case, we pull
> the value from the environment.
>
> Yours combines "definitely no" and "unknown" into a single value ("1" in
> your case, but that is because "redirect" and "from_user" have inverted
> logic from each other).
>
> I think that is OK, as there isn't any case where a caller would want to
> say "definitely no". The most they would say is "_I_ am not doing
> anything to make you think this value is not from the user", but we
> would still want to check the environment to see that nobody _else_ had
> put in such a restriction.
>
> -Peff
Oh ok, I see what you were going for now. That may be a better
(clearer) solution then what I just sent out. Mostly because we can
maintain the same logic polarity and use the same vocabulary.
--
Brandon Williams
Re: bw/transport-protocol-policy
On Thu, Dec 01, 2016 at 11:35:24AM -0800, Brandon Williams wrote:
> > I wouldn't expect anyone to ever set GIT_PROTOCOL_FROM_USER=1, but it
> > does behave in a funny way here, overriding the "redirect" flag. I think
> > we'd want something more like:
> >
> > if (redirect < 0)
> > redirect = git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
> >
> > and then pass in "-1" from transport_check_allowed().
>
> I don't think I quite follow your solution but I came up with this:
>
> case PROTOCOL_ALLOW_USER_ONLY:
> return redirect ? 0 : git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
>
> Which should address the same issue.
I think mine was confused a bit by using the word "redirect". It was
really meant to be "from_user", and could take three values: definitely
yes, definitely no, and unknown (-1). And in the unknown case, we pull
the value from the environment.
Yours combines "definitely no" and "unknown" into a single value ("1" in
your case, but that is because "redirect" and "from_user" have inverted
logic from each other).
I think that is OK, as there isn't any case where a caller would want to
say "definitely no". The most they would say is "_I_ am not doing
anything to make you think this value is not from the user", but we
would still want to check the environment to see that nobody _else_ had
put in such a restriction.
-Peff
Re: bw/transport-protocol-policy
On 12/01, Jeff King wrote:
> On Thu, Dec 01, 2016 at 10:14:15AM -0800, Brandon Williams wrote:
>
> > > 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.
>
> I wouldn't expect anyone to ever set GIT_PROTOCOL_FROM_USER=1, but it
> does behave in a funny way here, overriding the "redirect" flag. I think
> we'd want something more like:
>
> if (redirect < 0)
> redirect = git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
>
> and then pass in "-1" from transport_check_allowed().
I don't think I quite follow your solution but I came up with this:
case PROTOCOL_ALLOW_USER_ONLY:
return redirect ? 0 : git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
Which should address the same issue.
--
Brandon Williams
Re: bw/transport-protocol-policy
On Thu, Dec 01, 2016 at 10:14:15AM -0800, Brandon Williams wrote:
> > 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.
I wouldn't expect anyone to ever set GIT_PROTOCOL_FROM_USER=1, but it
does behave in a funny way here, overriding the "redirect" flag. I think
we'd want something more like:
if (redirect < 0)
redirect = git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
and then pass in "-1" from transport_check_allowed().
I think that's sufficient to fix the topic as-is. However, the http
redirect series adds an extra complication, because with http-alternates
we resolve some of the redirects ourselves. So in those cases we'd want
to restrict CURLOPT_PROTOCOLS as if they were redirects. We may need to
set up two CURLOPT values: ones from the user and ones from redirects,
and then feed them to CURLOPT_PROTOCOLS and CURLOPT_REDIR_PROTOCOLS as
appropriate depending on the request context.
> We should switch to warning all the time since this series adds in
> default whitelisted/blacklisted protocols anyways.
Yeah, good point. As a bonus it makes the code simpler.
-Peff
Re: bw/transport-protocol-policy
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
