Re: proposal for additional search path in config

[Taylor Blau <me@ttaylorr.com>]

On Wed, Jun 02, 2021 at 07:36:31PM +0200, Jordi Vilar wrote:
> > 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.

To me, this does not appear to be a conservative approach as you
suggest.

The only difference between exporting PATH=$SEARCH_PATH:$PATH and
PATH=$PATH:$SEARCH_PATH is that the former allows overwriting the
results of looking up a binary in the path, but the latter lets you
resolve locations that $PATH alone would not find.

Suppose you maliciously included a git-lfs binary with your repository.
If you include that binary in your PATH ahead of the existing system
PATH, then you'll replace system git-lfs with your malicious one, which
I think you and I both agree is bad. But if you instead append the
malicious binary onto the right-hand side of your PATH, then you can't
overwrite a git-lfs already on the path, but you *can* trick a system
which doesn't have a version of git-lfs installed into thinking that one
exists.

So your exploit would just be limited to having someone clone your
repository who doesn't already have git-lfs installed into their path,
which I would argue is just as bad.

> Also, config is not versioned, so, right after cloning you wouldn't
> have this option enabled, so you are always safe after cloning [...]

I know that this has come up in some recent-ish discussions, and I have
not been convinced that this makes things any safer.

Thanks,
Taylor