Re: proposal for additional search path in config

[Jordi Vilar <git.kernel.org@jordi.vilar.cat>]

Hi Johannes,

Thank you for taking the time to read the proposal and reply with your comments!

> That sounds like a Remote Code Execution vulnerability by design in the
> making. But let's read on.

I agree with you that a too simple implementation of this would pose
an unacceptable risk. But I think the details are important in order
to assess the actual issues. Let me develop those details as I reply
to your comments.

> I am really wary about the security implications of this. Most obviously
> with the idea of allowing to _override_ commands. Take for example
> `git-lfs`: the assumption is that it is safe for Git to call `git-lfs`
> after the initial check-out phase, but with this feature, it would be
> possible for Git to clone a malicious repository and _immediately_
> executing code it just cloned, _without_ giving the user a chance to even
> inspect the code.

You are again right. That's why I was suggesting the conservative
approach of not prepending to the default search path, but appending
to it, so there is no chance of overrinding existing tools. Also,
config is not versioned, so, right after cloning you wouldn't have
this option enabled, so you are always safe after cloning. Then you
should have to manually configure it, of course, only for trusted
repositories. But if you configure this search path in the global
settings, you should avoid setting a relative path (or maybe git could
ban it). But in any case, only the user can set this, it is not
automatic upon cloning at all.

> But even if you _append_ that path to the `PATH` variable, unintended
> vulnerabilities could be opened. Take for example `git difftool`, which
> looks for executables in the path until it finds one that is installed. An
> attacker could guess which executables your setup is missing, and squat on
> those, overriding your `git difftool` to execute code you did not intend
> to be executed.

Well, the apparent effect would be like appending to the PATH
variable, but I'm not suggesting to implement it in that way. I didn't
say it explicitly, but what I had in mind is that git searches for
extensions also in this additional path, but without modifying the
environment variable, so it is not inherited by any executable or any
other built-in git functionality.

> So putting a security lens before my eyes, I would be quite worried about
> such a feature.

And I appreciate your critical thinking. I just want to share an idea
I think could be useful, not to open a hole that we could later regret
to have open, so all precautions are good.

> Note that the _use case_ you present is a common one, and what I see
> projects do is to integrate the configuration into their build process
> (carefully vetting any code changes to the build process is _always_ a
> good idea, hence this is a lot safer than having Git configure what is run
> automatically). And of course, this is already possible right now, it just
> delays configuration of those included tools to the time of the first
> build, as opposed to the clone time (as suggested above).

For sure, decoupling sources/resources from tools is optimal. But not
always possible. And having to setup the environment for each worktree
makes working with multiple worktrees at a time from the same shell
session a pain and error prone.

Regards,

Jordi