Re: [PATCH] git-sh-setup.sh: fix missing double quotes variables

From: LE Manh Cuong <cuong.manhle.vn@gmail.com>
Date: Jun 19, 2016, 02:45 UTC
Message-ID: <20160619024554.2983-1-cuong.manhle.vn@gmail.com>
In-Reply-To: <xmqqshwax8ah.fsf@gitster.mtv.corp.google.com>

> Given that LESS, LV and GIT_OBJECT_DIRECTORY are expected to be free
> of any "expensive to expand" strings, I am not sure if this actually
> matters, though.  And more importantly, these are what the end users
> are expected to set to whatever sensible values for them.

The problem is the end users want a "string". In shell, it means you want scalar context instead of list context, which is where the vulnerability occured.

Using `$var` is actually `glob(split($var))`. While using `"$var"` means the shell interpreted $var content as string. That's also what you do with `test`, `[...]`, redirection `>"$file"` (which is mentioned in Git conding contention).

That's a mistake usage to introduce that "bug" to the end user. (I invite you to read the excelent question/answer about this problem at http://unix.stackexchange.com/q/171346/38906)

Show 7 quoted lines

> You would not be lying if you said that Git lets people who want to
> do strange things shoot their feet off; I do not think that hardly
> deserves to be called "vulnerability", though.
>
> Having said all that, I do not mind preventing people from shooting
> their foot off, and the change in this patch certainly would not
> hurt.

It's not only people shooting their foot, but also from malicious user. Given that `curl url | sudo sh/bash` is often found in many instructions, an end user may not be noticed about the environment variable injection from their side.

IMHO, it's better if  git can protect the end users in this situation.