[BUG] fatal: transport 'file' not allowed during submodule add

[BUG] fatal: transport 'file' not allowed during submodule add

[rsbecker]

As of 2.39.0, I am now getting fatal: transport 'file' not allowed when
performing a submodule add after a clone -l. The simple reproduce of this
is:

1. Start with an empty bare repository, src.git.
2. Create an empty non-bare repository and set the upstream remote to the
bare repo.
3. Populate the non-bare repository with:
	touch .gitignore &&
	git add .gitignore &&
	touch file1 &&
	git add file1 &&
	git commit -m initial &&
	git remote add origin ../src.git &&
	git push --set-upstream origin master
4. Create another empty bare repository to be used as the submodule,
subsrc.git.
5. Create another empty non-bare repository and set the upstream remote to
the bare repo for the submodule.
6. Populate the non-bare submodule repository with
	touch .gitignore &&
	git add .gitignore &&
	git commit -m initial &&
	git add .gitignore &&
	touch file2 &&
	git add file2 &&
	git commit -m initial &&
	git remote add origin ../subsrc.git &&
	git push --set-upstream origin master
7. Clone the main repo using -l or without it (makes no difference):
	git clone -l src.git dest
8. Attempt to add the submodule:
	cd dest &&
	git submodule add -- ../subsrc.git subsrc

This results in:
Cloning into 'dest'...
done.
Cloning into '/home/randall/dest/subsrc'...
fatal: transport 'file' not allowed
fatal: clone of '/home/randall/subsrc.git' into submodule path
'/home/randall/dest/subsrc' failed

This happens for any submodule add on the same system. Some online research
indicates that there was a security patch to git causing this, but I can't
find it. This does not seem correct to me or how this improves security.
Help please - this is causing some of my workflows to break.

Thanks,
Randall
--
Brief whoami: NonStop&UNIX developer since approximately
UNIX(421664400)
NonStop(211288444200000000)
-- In real life, I talk too much.

Re: [BUG] fatal: transport 'file' not allowed during submodule add

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

> As of 2.39.0, I am now getting fatal: transport 'file' not allowed when
> performing a submodule add after a clone -l. The simple reproduce of this
> is:
> ...
> This happens for any submodule add on the same system. Some online research
> indicates that there was a security patch to git causing this, but I can't
> find it. This does not seem correct to me or how this improves security.
> Help please - this is causing some of my workflows to break.

Thanks for reporting, Randall.

This suspiciously sounds like what a1d4f67c (transport: make
`protocol.file.allow` be "user" by default, 2022-07-29) is doing
deliberately.  Taylor, does this look like a corner case the 2.30.6
updates forgot to consider?

Thanks.

RE: [BUG] fatal: transport 'file' not allowed during submodule add

[rsbecker]

>-----Original Message-----
>From: Junio C Hamano <jch2355@gmail.com> On Behalf Of Junio C Hamano
On December 27, 2022 10:34 PM, Junio C Hamano wrote:
><rsbecker@nexbridge.com> writes:
>
>> As of 2.39.0, I am now getting fatal: transport 'file' not allowed
>> when performing a submodule add after a clone -l. The simple reproduce
>> of this
>> is:
>> ...
>> This happens for any submodule add on the same system. Some online
>> research indicates that there was a security patch to git causing
>> this, but I can't find it. This does not seem correct to me or how this
improves
>security.
>> Help please - this is causing some of my workflows to break.
>
>Thanks for reporting, Randall.
>
>This suspiciously sounds like what a1d4f67c (transport: make
`protocol.file.allow`
>be "user" by default, 2022-07-29) is doing deliberately.  Taylor, does this
look like a
>corner case the 2.30.6 updates forgot to consider?

I have tried using 'git config --local protocol.file.allow always' and/or
'git config --local protocol.allow always' to get past this, without
success. 

--Randall

Re: [BUG] fatal: transport 'file' not allowed during submodule add

[Jonathan Nieder]

Hi Randall,

rsbecker@nexbridge.com wrote:
> Junio C Hamano wrote:

>> This suspiciously sounds like what a1d4f67c (transport: make `protocol.file.allow`
>> be "user" by default, 2022-07-29) is doing deliberately.
>
> I have tried using 'git config --local protocol.file.allow always' and/or
> 'git config --local protocol.allow always' to get past this, without
> success.

Does `git config --global protocol.file.allow always` do the trick?

>>                                                           Taylor, does this look like a
>> corner case the 2.30.6 updates forgot to consider?

I think it's the intended effect (preventing file:// submodules), but
I wonder if this hints that we'd want that protection to be more
targeted.  A file:// submodule (as opposed to a bare path without URL
scheme) wouldn't trigger the "git clone --local" behavior that that
commit mentions wanting to protect against, so at first glance it
would appear to be no more or less dangerous than cloning from a
remote repository.

One thing I'd be curious about is whether --local happening
automatically is actually worth it nowadays.  "git worktree" does a
better job of sharing with an existing local repository, since the
sharing continues even after the worktree has been created, after any
"git gc" operations, and so on.  Meanwhile, the distinction between
file:// and bare paths is subtle enough that I regularly encounter
people not being aware of it (for example when wanting a way to test
protocol code locally and not understanding why a bare-path clone
doesn't do that).  Would it be more in the spirit of secure defaults
to require --local when someone wants to request the hardlinking trick
of local clone?

Thanks,
Jonathan

RE: [BUG] fatal: transport 'file' not allowed during submodule add

[rsbecker]

On December 28, 2022 5:11 PM, Jonathan Nieder wrote:
>Hi Randall,
>
>rsbecker@nexbridge.com wrote:
>> Junio C Hamano wrote:
>
>>> This suspiciously sounds like what a1d4f67c (transport: make
>>> `protocol.file.allow` be "user" by default, 2022-07-29) is doing
deliberately.
>>
>> I have tried using 'git config --local protocol.file.allow always'
>> and/or 'git config --local protocol.allow always' to get past this,
>> without success.
>
>Does `git config --global protocol.file.allow always` do the trick?

I tried git config --local protocol.file.allow always after the initial
clone. This should work but does not.
I also tried git config --global protocol.file.allow always before the
initial clone.  This also did not work.

>>>                                                           Taylor,
>>> does this look like a corner case the 2.30.6 updates forgot to consider?
>
>I think it's the intended effect (preventing file:// submodules), but I
wonder if this
>hints that we'd want that protection to be more targeted.  A file://
submodule (as
>opposed to a bare path without URL
>scheme) wouldn't trigger the "git clone --local" behavior that that commit
>mentions wanting to protect against, so at first glance it would appear to
be no
>more or less dangerous than cloning from a remote repository.
>
>One thing I'd be curious about is whether --local happening automatically
is
>actually worth it nowadays.  "git worktree" does a better job of sharing
with an
>existing local repository, since the sharing continues even after the
worktree has
>been created, after any "git gc" operations, and so on.  Meanwhile, the
distinction
>between file:// and bare paths is subtle enough that I regularly encounter
people
>not being aware of it (for example when wanting a way to test protocol code
>locally and not understanding why a bare-path clone doesn't do that).
Would it be
>more in the spirit of secure defaults to require --local when someone wants
to
>request the hardlinking trick of local clone?

I think the risk of someone hacking a hardlink is less risky than someone
misdirecting a remote site not under a user's direct control.

The tests I did show the same behaviour no matter which combination of the
above. --local appears to be implied, at least there is no apparent
behavioural difference between specifying the argument and not.

--Randall

Re: [BUG] fatal: transport 'file' not allowed during submodule add

[Taylor Blau]

Hi Jonathan,

On Wed, Dec 28, 2022 at 02:10:42PM -0800, Jonathan Nieder wrote:
> Hi Randall,
>
> rsbecker@nexbridge.com wrote:
> > Junio C Hamano wrote:
>
> >> This suspiciously sounds like what a1d4f67c (transport: make `protocol.file.allow`
> >> be "user" by default, 2022-07-29) is doing deliberately.
> >
> > I have tried using 'git config --local protocol.file.allow always' and/or
> > 'git config --local protocol.allow always' to get past this, without
> > success.
>
> Does `git config --global protocol.file.allow always` do the trick?
>
> >>                                                           Taylor, does this look like a
> >> corner case the 2.30.6 updates forgot to consider?
>
> I think it's the intended effect (preventing file:// submodules), but
> I wonder if this hints that we'd want that protection to be more
> targeted.  A file:// submodule (as opposed to a bare path without URL
> scheme) wouldn't trigger the "git clone --local" behavior that that
> commit mentions wanting to protect against, so at first glance it
> would appear to be no more or less dangerous than cloning from a
> remote repository.

Changing the default value of 'protocol.file.allow' isn't solely about whether
or not we use the `file://` scheme and transport or not. Instead, it's
about preventing the user from accidentally cloning local repositories
containing sensitive data into the working copy of a malicious
repository.

One example might be that I convince you to clone my malicious
repository, which has a Dockerfile that uploads everything in the
container filesystem to some data harvesting server. Since 'docker run'
automatically puts everything in '.' into the volume mount, anything in
the working copy of my malicious repository will get exfiltrated.

The worry that I wrote about in a1d4f67c was that if I knew that you
stored, say, your SSH private key material in a repository that is at
`$HOME/.git` (as is sometimes common practice), then I could add a
submodule at /home/jrnieder/.git, and extract any sensitive data
therein.

So I think our new default is sensible here if we are concerned with
preventing such a case.

Thanks,
Taylor

RE: [BUG] fatal: transport 'file' not allowed during submodule add

[rsbecker]

On December 30, 2022 4:08 PM, Taylor Blau wrote:
>On Wed, Dec 28, 2022 at 02:10:42PM -0800, Jonathan Nieder wrote:
>> Hi Randall,
>>
>> rsbecker@nexbridge.com wrote:
>> > Junio C Hamano wrote:
>>
>> >> This suspiciously sounds like what a1d4f67c (transport: make
>> >> `protocol.file.allow` be "user" by default, 2022-07-29) is doing deliberately.
>> >
>> > I have tried using 'git config --local protocol.file.allow always'
>> > and/or 'git config --local protocol.allow always' to get past this,
>> > without success.
>>
>> Does `git config --global protocol.file.allow always` do the trick?
>>
>> >>                                                           Taylor,
>> >> does this look like a corner case the 2.30.6 updates forgot to consider?
>>
>> I think it's the intended effect (preventing file:// submodules), but
>> I wonder if this hints that we'd want that protection to be more
>> targeted.  A file:// submodule (as opposed to a bare path without URL
>> scheme) wouldn't trigger the "git clone --local" behavior that that
>> commit mentions wanting to protect against, so at first glance it
>> would appear to be no more or less dangerous than cloning from a
>> remote repository.
>
>Changing the default value of 'protocol.file.allow' isn't solely about whether or not
>we use the `file://` scheme and transport or not. Instead, it's about preventing the
>user from accidentally cloning local repositories containing sensitive data into the
>working copy of a malicious repository.
>
>One example might be that I convince you to clone my malicious repository, which
>has a Dockerfile that uploads everything in the container filesystem to some data
>harvesting server. Since 'docker run'
>automatically puts everything in '.' into the volume mount, anything in the working
>copy of my malicious repository will get exfiltrated.
>
>The worry that I wrote about in a1d4f67c was that if I knew that you stored, say,
>your SSH private key material in a repository that is at `$HOME/.git` (as is
>sometimes common practice), then I could add a submodule at
>/home/jrnieder/.git, and extract any sensitive data therein.
>
>So I think our new default is sensible here if we are concerned with preventing
>such a case.

I think the new default is reasonable but this did catch me by surprise as it broke our workflows. I guess I need to look at the release notes in more depth - that's my bad. With the caveat that I do not think this is working as intended, which I am finding, because changing the configuration does not make any behavioural difference on any platform I can test on.

Re: [BUG] fatal: transport 'file' not allowed during submodule add

[Jeff King]

On Fri, Dec 30, 2022 at 04:08:03PM -0500, Taylor Blau wrote:

> Changing the default value of 'protocol.file.allow' isn't solely about whether
> or not we use the `file://` scheme and transport or not. Instead, it's
> about preventing the user from accidentally cloning local repositories
> containing sensitive data into the working copy of a malicious
> repository.
> 
> One example might be that I convince you to clone my malicious
> repository, which has a Dockerfile that uploads everything in the
> container filesystem to some data harvesting server. Since 'docker run'
> automatically puts everything in '.' into the volume mount, anything in
> the working copy of my malicious repository will get exfiltrated.
> 
> The worry that I wrote about in a1d4f67c was that if I knew that you
> stored, say, your SSH private key material in a repository that is at
> `$HOME/.git` (as is sometimes common practice), then I could add a
> submodule at /home/jrnieder/.git, and extract any sensitive data
> therein.
> 
> So I think our new default is sensible here if we are concerned with
> preventing such a case.

One of the justifications for disallowing filesystem URLs in submodules
is that they're generally a bad idea anyway. If you're cloning over the
network, there's nothing to guarantee that the cloner's filesystem looks
anything like the original committer's. So it's an accident waiting to
happen.

But one case where it's not _completely_ unreasonable is when the
superproject clone itself is happening via the local filesystem. That
matches Randall's example, though I'm not sure if that's his "real"
workflow.

So one thing we could do to soften the change is to allow a submodule to
use a filesystem URL if and only if the immediate parent clone also did
(and naturally, allow "submodule add" on anything). But:

  - it's not clear to me if that just re-opens the Docker problem you
    were trying to fix (I think not, because it should be forbidding
    file:// remotes for the initial clone, too; but...)

  - as a security rule, it's complicated and confusing, which means it
    is likely to have loopholes or be mis-used. As a user I now have to
    be aware that copying an untrusted .git to my filesystem and cloning
    from it has a different trust level than cloning it over https, with
    respect to submodule protocols.

  - in a distributed system, the notion of "the parent clone" is vague
    anyway. During "git clone --recurse-submodules", sure, you can use
    the top-level URL as your basis. But later if I run "git submodule
    update", what's the "original" protocol? I can guess at it by
    looking at remote.origin.url, but of course it could change, there
    could be multiple remotes, etc.

So I think it's probably not worth doing. People with that workflow
should set protocol.file.allow as appropriate.

I also wondered if you could get around this with url.*.insteadOf. That
is, it would seem reasonable to use a unique URL in .gitmodules (even if
it's not a real functional URL!), and then let local developers override
it to point to their filesystem with the insteadOf mechanism. That gives
tighter permissions, and provides more options for local redirection.

Unfortunately, it doesn't work here. We check the protocol permissions
as we're about to use them (which is good, because we catch all paths
that get there). But at that point we have no idea about the rewrite.
Looks like there was some discussion in this thread:

  https://lore.kernel.org/git/CAPZ477MCsBsfbqKzp69MT_brwz-0aes6twJofQrhizUBV7ZoeA@mail.gmail.com/

about having a rewrite make the URL "from the user". But ultimately it
seems scary. And anyway, it doesn't help this case much anyway (people
still need to set up config, so they might as well just set the protocol
config).

-Peff

Re: [BUG] fatal: transport 'file' not allowed during submodule add

[Taylor Blau]

On Wed, Dec 28, 2022 at 09:42:39AM -0500, rsbecker@nexbridge.com wrote:
>
>
> >-----Original Message-----
> >From: Junio C Hamano <jch2355@gmail.com> On Behalf Of Junio C Hamano
> On December 27, 2022 10:34 PM, Junio C Hamano wrote:
> ><rsbecker@nexbridge.com> writes:
> >
> >> As of 2.39.0, I am now getting fatal: transport 'file' not allowed
> >> when performing a submodule add after a clone -l. The simple reproduce
> >> of this
> >> is:
> >> ...
> >> This happens for any submodule add on the same system. Some online
> >> research indicates that there was a security patch to git causing
> >> this, but I can't find it. This does not seem correct to me or how this
> improves
> >security.
> >> Help please - this is causing some of my workflows to break.
> >
> >Thanks for reporting, Randall.
> >
> >This suspiciously sounds like what a1d4f67c (transport: make
> `protocol.file.allow`
> >be "user" by default, 2022-07-29) is doing deliberately.  Taylor, does this
> look like a
> >corner case the 2.30.6 updates forgot to consider?
>
> I have tried using 'git config --local protocol.file.allow always' and/or
> 'git config --local protocol.allow always' to get past this, without
> success.

I couldn't reproduce the symptom you described. Indeed, the behavior of
not allowing local-submodules to be cloned without explicitly opting in
via the `protocol.file.allow` configuration is intentional.

The patch Junio mentioned, a1d4f67c12 (transport: make
`protocol.file.allow` be "user" by default, 2022-07-29) has some
examples of why this behavior was changed in the 2.30.6 update.

If you run either `git config --global protocol.file.allow always`, or
replace your last submodule add with:

  $ git -c protocol.file.allow=always submodule add /path/to/subsrc.git

it should work as expected.

Thanks,
Taylor

RE: [BUG] fatal: transport 'file' not allowed during submodule add

[rsbecker]

On December 30, 2022 4:04 PM, Taylor Blau wrote:
>On Wed, Dec 28, 2022 at 09:42:39AM -0500, rsbecker@nexbridge.com wrote:
>> >-----Original Message-----
>> >From: Junio C Hamano <jch2355@gmail.com> On Behalf Of Junio C Hamano
>> On December 27, 2022 10:34 PM, Junio C Hamano wrote:
>> ><rsbecker@nexbridge.com> writes:
>> >
>> >> As of 2.39.0, I am now getting fatal: transport 'file' not allowed
>> >> when performing a submodule add after a clone -l. The simple
>> >> reproduce of this
>> >> is:
>> >> ...
>> >> This happens for any submodule add on the same system. Some online
>> >> research indicates that there was a security patch to git causing
>> >> this, but I can't find it. This does not seem correct to me or how
>> >> this
>> improves
>> >security.
>> >> Help please - this is causing some of my workflows to break.
>> >
>> >Thanks for reporting, Randall.
>> >
>> >This suspiciously sounds like what a1d4f67c (transport: make
>> `protocol.file.allow`
>> >be "user" by default, 2022-07-29) is doing deliberately.  Taylor,
>> >does this
>> look like a
>> >corner case the 2.30.6 updates forgot to consider?
>>
>> I have tried using 'git config --local protocol.file.allow always'
>> and/or 'git config --local protocol.allow always' to get past this,
>> without success.
>
>I couldn't reproduce the symptom you described. Indeed, the behavior of not
>allowing local-submodules to be cloned without explicitly opting in via the
>`protocol.file.allow` configuration is intentional.
>
>The patch Junio mentioned, a1d4f67c12 (transport: make `protocol.file.allow` be
>"user" by default, 2022-07-29) has some examples of why this behavior was
>changed in the 2.30.6 update.
>
>If you run either `git config --global protocol.file.allow always`, or replace your last
>submodule add with:
>
>  $ git -c protocol.file.allow=always submodule add /path/to/subsrc.git
>
>it should work as expected.

I have reproduced this on multiple platforms including NonStop and Cygwin64 on Windows with the same results as earlier. The protocol.file.allowed=always does not appear to even get considered. With some fprintfs in the code, the code in is_transport_allowed falls through to the PROTOCOL_ALLOW_USER_ONLY case and only considers environment variable GIT_PROTOCOL_FROM_USER, which is not passed into the child doing the submodule add. The is_transport_allowed("file",-1) always returns 0 no matter what and 0 is what gets used upwards. There is no difference in the behaviour regardless of the protocol.file.allowed value either in -c, .gitconfig, or on the user environment variable.

RE: [BUG] fatal: transport 'file' not allowed during submodule add

[rsbecker]

On December 30, 2022 4:04 PM, Taylor Blau wrote:
>On Wed, Dec 28, 2022 at 09:42:39AM -0500, rsbecker@nexbridge.com wrote:
>> >From: Junio C Hamano <jch2355@gmail.com> On Behalf Of Junio C Hamano
>> On December 27, 2022 10:34 PM, Junio C Hamano wrote:
>> ><rsbecker@nexbridge.com> writes:
>> >
>> >> As of 2.39.0, I am now getting fatal: transport 'file' not allowed
>> >> when performing a submodule add after a clone -l. The simple
>> >> reproduce of this
>> >> is:
>> >> ...
>> >> This happens for any submodule add on the same system. Some online
>> >> research indicates that there was a security patch to git causing
>> >> this, but I can't find it. This does not seem correct to me or how
>> >> this
>> improves
>> >security.
>> >> Help please - this is causing some of my workflows to break.
>> >
>> >Thanks for reporting, Randall.
>> >
>> >This suspiciously sounds like what a1d4f67c (transport: make
>> `protocol.file.allow`
>> >be "user" by default, 2022-07-29) is doing deliberately.  Taylor,
>> >does this
>> look like a
>> >corner case the 2.30.6 updates forgot to consider?
>>
>> I have tried using 'git config --local protocol.file.allow always'
>> and/or 'git config --local protocol.allow always' to get past this,
>> without success.
>
>I couldn't reproduce the symptom you described. Indeed, the behavior of not
>allowing local-submodules to be cloned without explicitly opting in via the
>`protocol.file.allow` configuration is intentional.
>
>The patch Junio mentioned, a1d4f67c12 (transport: make `protocol.file.allow` be
>"user" by default, 2022-07-29) has some examples of why this behavior was
>changed in the 2.30.6 update.
>
>If you run either `git config --global protocol.file.allow always`, or replace your last
>submodule add with:
>
>  $ git -c protocol.file.allow=always submodule add /path/to/subsrc.git
>
>it should work as expected.

Ok, operator error. This does work as expected if you run the test slightly different. If the repositories are all cloned, the upstream remotes are set up properly and the submodule add works. In my test case, I used git remote add with a relative path. This seems to be an edge condition that triggers the situation. When avoiding the upstream remote command (implied by clone), the submodule add works if the protocol.file.allow = always, no matter how it is set.

--Randall

RE: [BUG] fatal: transport 'file' not allowed during submodule add

[rsbecker]

On December 27, 2022 10:34 PM, Junio C Hamano wrote:
><rsbecker@nexbridge.com> writes:
>
>> As of 2.39.0, I am now getting fatal: transport 'file' not allowed
>> when performing a submodule add after a clone -l. The simple reproduce
>> of this
>> is:
>> ...
>> This happens for any submodule add on the same system. Some online
>> research indicates that there was a security patch to git causing
>> this, but I can't find it. This does not seem correct to me or how this
improves
>security.
>> Help please - this is causing some of my workflows to break.
>
>Thanks for reporting, Randall.
>
>This suspiciously sounds like what a1d4f67c (transport: make
`protocol.file.allow`
>be "user" by default, 2022-07-29) is doing deliberately.  Taylor, does this
look like a
>corner case the 2.30.6 updates forgot to consider?

Any updates on this? Neither protocol.file.allow=always nor
protocol.allow=always gets past the error condition.

Thanks,
Randall