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

LE Manh Cuong [off-list ref] writes:

Leaving shell variables un-quotes can lead to security vulnerabilities. In:

    : ${x=.}

`$x` is always expanded, cause `glob+split` on its result. There're some
globs is too expensive to expand, like:

    x='/*/*/*/*/../../../../*/*/*/*/../../../../*/*/*/*' sh -c ':
    ${x=.}'

Run it and our machine will hang/crash (especially in Linux).

`LESS`, `LV` and `GIT_OBJECT_DIRECTORY` variables in `git-sh-setup` are
vulnerable with this case.

Fix this vulnerability  by quoting those variables.

Signed-off-by: LE Manh Cuong <redacted>
---

That is "interesting".

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.

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.

Thanks.

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)

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.