Re: [PATCH 0/3] credential: handle partial URLs in config settings again

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: Jeff King <peff@peff.net>,
	Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	Ilya Tretyakov <it@it3xl.ru>
Subject: Re: [PATCH 0/3] credential: handle partial URLs in config settings again
Date: Fri, 24 Apr 2020 00:11:54 +0200 (CEST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2004240006510.18039@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20200422224711.GC140314@google.com>

Hi Jonathan,

On Wed, 22 Apr 2020, Jonathan Nieder wrote:

> Johannes Schindelin wrote:
> > On Wed, 22 Apr 2020, Jeff King wrote:
> >> On Wed, Apr 22, 2020 at 08:51:02PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> >>> Please note that Git v2.17.4 will not do what we would expect here: if any
> >>> host name (without protocol) is specified, e.g. -c
> >>> credential.golli.wog.username = boo, it will actually ignore the host name.
> >>> That is, this will populate the username:
> >>>
> >>>   $ echo url=https://example.com |
> >>>     git -c credential.totally.bog.us.username=foo credential fill
> >>
> >> That seems scary. What if it is not .username, but:
> >>
> >>   [credential "example.com"]
> >>   username = foo
> >>   helper = "!echo password=bar"
> >>
> >> ? (Or you can imagine a helper that is pulling from a read-only store,
> >> like "pass"). That would send the credential to the wrong host.
> >
> > It would. But I am not aware of any implications regarding `.gitmodules`
> > (for some reason I now expect every bug to open an attack vector via
> > submodules, I wonder why that is), so that's at least good.
>
> Submodules are only one of many ways that people end up cloning from
> an attacker-controlled URL.

Yet the vast majority of the vulnerabilities in Git that we fixed over the
last years has involved submodules in their attack vector.

> In submodules we're careful not to use --recurse-submodules by default
> at clone time.  So I'll mentally subsitute "attacker-controlled URLs"
> where you say "submodules". ;-)

Say what you will about this, but the practical reality seems to be that
most of the security bugs in Git affected submodule users.

Somebody even muttered the suggestion to me that submodules should be
deprecated already.

> I agree with Peff's concern about the unpatched state: since there are
> people who use `[credential "host.example.com"] helper` and there are
> credential helpers that ignore the host passed in, the combination can
> hurt people.  (Fortunately, there aren't many credential helpers in
> that category that are commonly used.)

Yes. That's why I want to make sure that the URL parser we use here is
strict by default, and lenient _only_ when parsing the config (because
then, it is more like a URL *pattern* used for matching, the parsed URL is
never used directly).

> [...]
> > Yes. For the record, I tried to be very careful here. The _only_ code path
> > that is affected by this change is the config reading.
> [...]
> > But again, I would love another pair of eyes on this, to confirm my
> > analysis.
>
> As mentioned above, the config reading is sensitive, too.  That said,
> I suspect you got it to do basically the right thing.
>
> Reading through the patches.  Thank you.

Thank you very much!
Dscho