* proposal for additional search path in config [not found] <20210601113554.52585C06174A@lindbergh.monkeyblade.net> @ 2021-06-01 14:13 ` Jordi Vilar 2021-06-02 11:08 ` Johannes Schindelin 0 siblings, 1 reply; 4+ messages in thread From: Jordi Vilar @ 2021-06-01 14:13 UTC (permalink / raw) To: git Hi folks, I would like to gather feedback about a feature I'm considering about. Basically, I would like to propose a configuration setting with a path (possibly relative to the working tree) to look for executables. In that way, a local clone could setup additional tools/scripts private for its work tree, maybe also under revision control. For instance, we could have a setting like: git config core.extensions_path "./scripts" so that whenever git has to resolve an external command also includes that path in the search, resolving relative paths to the work tree, in this case, the "/scripts" subdir. A simple use case could be a custom tool integrated with git, but local to the repository and thus versioned. For instance, let's assume that we have some metadata required by a custom build tool that is stored in commit comments or tags, so we have a custom script git-project-metadata to handle it: git project-metadata query ... Maybe the format for the metadata evolves, so the script must evolve also. Having it integrated in this way would allow that each checkout sees the correct script version, and that script is always invoked as a regular git command, without having to tweak the environment everytime. It is not clear to me whether it's a good idea for this additional path to have priority over the default search path, as in one hand it could allow to override the default tools, but in the other hand this may be a hole for bad things... For my, the conservative approach of having this additional path as a fallback is ok, so that it only adds more commands, but doesn't override commands. Also, this setting could be set either globally or locally, and in this case, in the conservative approach, the overloading rule is the different to the normal one: the system search path in the environment path variable has precedence over the global setting and this over the local setting, but all apply. Does it sound reasonable? Is there interest in such feature? Thanks, Jordi ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: proposal for additional search path in config 2021-06-01 14:13 ` proposal for additional search path in config Jordi Vilar @ 2021-06-02 11:08 ` Johannes Schindelin 2021-06-02 17:36 ` Jordi Vilar 0 siblings, 1 reply; 4+ messages in thread From: Johannes Schindelin @ 2021-06-02 11:08 UTC (permalink / raw) To: Jordi Vilar; +Cc: git Hi Jordi, On Tue, 1 Jun 2021, Jordi Vilar wrote: > I would like to gather feedback about a feature I'm considering about. > Basically, I would like to propose a configuration setting with a path > (possibly relative to the working tree) to look for executables. In > that way, a local clone could setup additional tools/scripts private > for its work tree, maybe also under revision control. That sounds like a Remote Code Execution vulnerability by design in the making. But let's read on. > For instance, we could have a setting like: > git config core.extensions_path "./scripts" > so that whenever git has to resolve an external command also includes > that path in the search, resolving relative paths to the work tree, in > this case, the "/scripts" subdir. > > A simple use case could be a custom tool integrated with git, but > local to the repository and thus versioned. > > For instance, let's assume that we have some metadata required by a > custom build tool that is stored in commit comments or tags, so we > have a custom script git-project-metadata to handle it: > git project-metadata query ... > > Maybe the format for the metadata evolves, so the script must evolve > also. Having it integrated in this way would allow that each checkout > sees the correct script version, and that script is always invoked as > a regular git command, without having to tweak the environment > everytime. > > It is not clear to me whether it's a good idea for this additional > path to have priority over the default search path, as in one hand it > could allow to override the default tools, but in the other hand this > may be a hole for bad things... For my, the conservative approach of > having this additional path as a fallback is ok, so that it only adds > more commands, but doesn't override commands. > > Also, this setting could be set either globally or locally, and in > this case, in the conservative approach, the overloading rule is the > different to the normal one: the system search path in the environment > path variable has precedence over the global setting and this over the > local setting, but all apply. > > Does it sound reasonable? Is there interest in such feature? 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. 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. So putting a security lens before my eyes, I would be quite worried about such a feature. 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). Ciao, Johannes ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: proposal for additional search path in config 2021-06-02 11:08 ` Johannes Schindelin @ 2021-06-02 17:36 ` Jordi Vilar 2021-06-03 2:45 ` Taylor Blau 0 siblings, 1 reply; 4+ messages in thread From: Jordi Vilar @ 2021-06-02 17:36 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: proposal for additional search path in config 2021-06-02 17:36 ` Jordi Vilar @ 2021-06-03 2:45 ` Taylor Blau 0 siblings, 0 replies; 4+ messages in thread From: Taylor Blau @ 2021-06-03 2:45 UTC (permalink / raw) To: Jordi Vilar; +Cc: Johannes Schindelin, git 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-06-03 2:46 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20210601113554.52585C06174A@lindbergh.monkeyblade.net> 2021-06-01 14:13 ` proposal for additional search path in config Jordi Vilar 2021-06-02 11:08 ` Johannes Schindelin 2021-06-02 17:36 ` Jordi Vilar 2021-06-03 2:45 ` Taylor Blau
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.