Dependabot alert: Command Injection due to insecure usage of the --upload-pack feature of git

[anthonyhaffey]

Hi there, this package has been really helpful for my software! Just asking if there is there going to be an update to git-clone to deal with the following dependabot alert:

Affected versions <= 0.2.0

All versions of package git-clone are vulnerable to Command Injection due to insecure usage of the --upload-pack feature of git.

Side-note: my understanding is that this would be less of a concern for people using node-js for software on their computer, but a bigger issue for node-js on a server, is that a fair assessment?

[jaz303]

I'm struggling to see how this is any more of a security vulnerability than a stdlib function like spawn().

git-clone has an args option for passing arbitrary CLI arguments to git. Using this option with untrusted user input is obviously a recipe for security problems, and the documentation states this explicitly. Attempting to perform some sort of automatic input sanitisation is, IMO, far outside this library's scope.

One thing I've considered is a separate git-clone/unsafe import that explicitly enables the args option, but of course that would break backwards-compatibility and require a major version change.

(as an aside, I think the "vulnerability" report is rather insidiously worded - git-clone doesn't use --upload-pack in of itself)

[anthonyhaffey]

Thanks @jaz303 for the quick reply. Sorry that my posting of this issue reflects a lack of understanding about the validity of the dependabot alert; it's a shame that this is being alerted when sensible usage of git-clone isn't a liability (at least based on my understanding of your reply).

I'm happy for you to close/delete this issue.

[jaz303]

Please don't apologise, any frustration I feel is definitely not directed at yourself.

I think I'll leave this issue open as I'm keen to hear others' opinions.

[Kristories]

@jaz303 please check :

• https://gist.github.com/lirantal/9441f3a1212728476f7a6caa4acb2ccc
• https://security.snyk.io/vuln/SNYK-JS-GITCLONE-2434308
• https://github.com/advisories/GHSA-8jmw-wjr8-2x66

[jaz303]

I've read all of these.

git-clone doesn't use upload-pack; it's explicitly documented as a shell command wrapper, and exposes a method for passing arbitrary arguments to the git executable. Obviously care needs to be taken if passing dynamic/untrusted input. I fail to see how this is a vulnerability any more than a standard library function like spawn.

[Kristories]

Yes, I know. Perhaps you could provide this information in the readme, as git-promise did.

[jaz303]

I did:

NOTE: the args option allows arbitrary arguments to be passed to git; this is inherently insecure if used in combination with untrusted input. Only use the args option with static/trusted input!

[Kristories]

Thanks for the information. I didn't see that warning, because I was too focused on dependabot and npm audit.

[emorneau]

Will we eventually remove that option out of this repository to remove that potential vulnerability. It shows as a Snyk High vuln. which is very annoying even if we do not use this option. Maybe create a new repo git-clone-unload-pack which adds in that extra feature, if people desire to use this risky option.

[jaz303]

No I will not be doing that. It's not a "risky option". git-clone doesn't directly use upload-pack. Did you even read this thread?

[emorneau]

I see that it is an inherited security vuln. from git-promise. Sorry and thanks for the quick reply above.

[hakt0r]

Honestly why not create a bug with git itself?! It seems to me that some downstream users allow unfiltered arguments to be injected and they are just pointing fingers. This is not a security issue, indeed.