Re: [PATCH] transport: add core.allowProtocol config option

From: Brandon Williams <bmwill@google.com>
To: Stefan Beller <sbeller@google.com>
Cc: Jeff King <peff@peff.net>, Jonathan Nieder <jrnieder@gmail.com>,
	"git@vger.kernel.org" <git@vger.kernel.org>,
	Blake Burkhart <bburky@bburky.com>
Subject: Re: [PATCH] transport: add core.allowProtocol config option
Date: Thu, 3 Nov 2016 10:51:31 -0700	[thread overview]
Message-ID: <20161103175131.GB182568@google.com> (raw)
In-Reply-To: <CAGZ79kbvs+ryGRjCkHYO=3iNK4tPPhBhYjRMZaH7rP0QMrULhg@mail.gmail.com>

On 11/03, Stefan Beller wrote:
> >>   protocol.X.allow = always | user | never
> >
> > It sounds like there is interest for this sort of behavior, it would
> > definitely require a larger change than what I initially proposed.  One
> > problem I see though is that with this we have support for both a
> > blacklist and a whitelist.  Which wins?
> 
> For the submodule operations we'll use a whitelist, because we want to
> provide security and for the other case we can offer a blacklist as a bandaid.
> 
> My opinion on blacklists is roughly aligned with e.g. :
> https://blog.codinghorror.com/blacklists-dont-work/
> http://blog.deepinstinct.com/2016/02/04/when-blacklists-dont-really-work/
> 
> So IMHO we could drop the "never" and substitute it with a "warn" or
> "ask-user", such that this configuration becomes a white list for both cases:
> 
>      protocol.X.allow = always | user | warn
> 
> > Or do we simply generate a
> > whitelist of allowed protocols which includes all protocols with allow
> > set to 'always' and if it is set to 'never' then it just isn't included
> > in the whitelist?
> 
> So you're suggesting that setting it to "never" doesn't have any effect
> except for cluttering the config file?
> I don't think we should do that; each setting should have an impact.
> So maybe the "never" would be there to disallow protocols of the hardcoded
> white list (e.g. http)

Thats what I meant, if a protocol is listed as 'never' then it just
removes that protocol from the whitelist.  That way we still have the
benefit of using a whitelist vs a blacklist.  Also, if we move in this
direction should we setup a default whitelist of allowed protocols?

> >
> > I don't know if I'm sold on a 'user' state just yet, perhaps that's just
> > because I view a whitelist or blacklist as well black and white and
> > having this user state adds in a gray area.
> 
> Well the "user" state is to differentiate between the
> * "I consciously typed `git clone ...` (and e.g. I know what happens as
>   I know the server admin and they are trustworthy.)
> * a repository contains a possible hostile .gitmodules file such
>   that I am not aware of the network connection.

This is still a gray area to me.  I think that if we have a whitelist of
protocols then it should be a true whitelist and not have some means of
going around it.  It just seems like something that could be exploited.

-- 
Brandon Williams