Hi, 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. 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". ;-) 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. 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. Jonathan
