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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 23, 2020, 22:11 UTC
Message-ID: <nycvar.QRO.7.76.6.2004240006510.18039@tvgsbejvaqbjf.bet>
In-Reply-To: <20200422224711.GC140314@google.com>

Hi Jonathan,

On Wed, 22 Apr 2020, Jonathan Nieder wrote:

Show 27 quoted lines

> 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.

Show 5 quoted lines

> 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).

Show 11 quoted lines

> [...]
> > 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