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

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

[LE Manh Cuong]

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 <cuong.manhle.vn@gmail.com>
---
 git-sh-setup.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index c48139a..85db5f1 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -160,8 +160,8 @@ git_pager() {
 	else
 		GIT_PAGER=cat
 	fi
-	: ${LESS=-FRX}
-	: ${LV=-c}
+	: "${LESS=-FRX}"
+	: "${LV=-c}"
 	export LESS LV
 
 	eval "$GIT_PAGER" '"$@"'
@@ -344,7 +344,7 @@ git_dir_init () {
 		echo >&2 "Unable to determine absolute path of git directory"
 		exit 1
 	}
-	: ${GIT_OBJECT_DIRECTORY="$(git rev-parse --git-path objects)"}
+	: "${GIT_OBJECT_DIRECTORY="$(git rev-parse --git-path objects)"}"
 }
 
 if test -z "$NONGIT_OK"
-- 
2.9.0

(no subject)

[LE Manh Cuong]

Sorry, the example in commit message is wrong, due to editor auto wrapping line.

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

[LE Manh Cuong]

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 <cuong.manhle.vn@gmail.com>
---
 git-sh-setup.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index c48139a..85db5f1 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -160,8 +160,8 @@ git_pager() {
 	else
 		GIT_PAGER=cat
 	fi
-	: ${LESS=-FRX}
-	: ${LV=-c}
+	: "${LESS=-FRX}"
+	: "${LV=-c}"
 	export LESS LV
 
 	eval "$GIT_PAGER" '"$@"'
@@ -344,7 +344,7 @@ git_dir_init () {
 		echo >&2 "Unable to determine absolute path of git directory"
 		exit 1
 	}
-	: ${GIT_OBJECT_DIRECTORY="$(git rev-parse --git-path objects)"}
+	: "${GIT_OBJECT_DIRECTORY="$(git rev-parse --git-path objects)"}"
 }
 
 if test -z "$NONGIT_OK"
-- 
2.9.0

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

[Junio C Hamano]

LE Manh Cuong <cuong.manhle.vn@gmail.com> 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 <cuong.manhle.vn@gmail.com>
> ---

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.

>  git-sh-setup.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index c48139a..85db5f1 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -160,8 +160,8 @@ git_pager() {
>  	else
>  		GIT_PAGER=cat
>  	fi
> -	: ${LESS=-FRX}
> -	: ${LV=-c}
> +	: "${LESS=-FRX}"
> +	: "${LV=-c}"
>  	export LESS LV
>  
>  	eval "$GIT_PAGER" '"$@"'
> @@ -344,7 +344,7 @@ git_dir_init () {
>  		echo >&2 "Unable to determine absolute path of git directory"
>  		exit 1
>  	}
> -	: ${GIT_OBJECT_DIRECTORY="$(git rev-parse --git-path objects)"}
> +	: "${GIT_OBJECT_DIRECTORY="$(git rev-parse --git-path objects)"}"
>  }
>  
>  if test -z "$NONGIT_OK"

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

[LE Manh Cuong]

> 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.

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

[Junio C Hamano]

LE Manh Cuong <cuong.manhle.vn@gmail.com> writes:

> 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.

Huh?  For those who run `curl url | sudo sh`, I do not think the
incoming script setting and exporting LV to an arbitrary value and
runing Git is not the top thing they need worry about.

While I think enclosing the string in dq is an improvement (as I
said already), I still do think your use of the v-word is making a
mountain out of an anthill.

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

[LE Manh Cuong]

> While I think enclosing the string in dq is an improvement (as I
> said already), I still do think your use of the v-word is making a
> mountain out of an anthill.

Well, sorry if that word is annoyed.

So, is this patch fine or I must re-phrase the title?

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> LE Manh Cuong <cuong.manhle.vn@gmail.com> writes:
>
>> 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.
>
> Huh?  For those who run `curl url | sudo sh`, I do not think the
> incoming script setting and exporting LV to an arbitrary value and
> runing Git is not the top thing they need worry about.
>
> While I think enclosing the string in dq is an improvement (as I
> said already), I still do think your use of the v-word is making a
> mountain out of an anthill.

I failed to say why I found the dq is an improvement, but that
should be in the log message of this commit.  Off the top of my
head, something like:

	We often make sure an environment variable is set to
	something, either set by the user (in which case we do not
	molest it) or set it to our default value (otherwise), with

		: ${VAR=default value}

	i.e. running the no-op command ":" with ${VAR} as its
	parameters (or the default value we supply), relying on that
	":" is a no-op.

	This pattern, even though it is no-op from correctness point
	of view, still can be expensive if the existing value in VAR
	has shell glob (because they will be expanded against
	filesystem entities) and IFS whitespaces (because the value
	need to be split into multiple parameters).  Our invocation
	of ":" command does not care if the parameter given to it is
	after the value in VAR goes through these processing.

	Enclosing the whole thing in double-quote, i.e.

		: "${VAR=default value}"

	avoids paying the unnecessary cost, so let's do so.

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

[LE Manh Cuong]

It's really a good commit message, better than mine.

So must I make another patch or you will update the commit message?

PS: This is the first time I submit a patch to Git, so forgive me
if I made any silly questions.

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

[Junio C Hamano]

LE Manh Cuong <cuong.manhle.vn@gmail.com> writes:

> It's really a good commit message, better than mine.
>
> So must I make another patch or you will update the commit message?
>
> PS: This is the first time I submit a patch to Git, so forgive me
> if I made any silly questions.

I can locally amend by replacing the log message when I queue your
original patch, now we seem to have agreed how it should read.

Thanks.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> LE Manh Cuong <cuong.manhle.vn@gmail.com> writes:
>
>> It's really a good commit message, better than mine.
>>
>> So must I make another patch or you will update the commit message?
>>
>> PS: This is the first time I submit a patch to Git, so forgive me
>> if I made any silly questions.
>
> I can locally amend by replacing the log message when I queue your
> original patch, now we seem to have agreed how it should read.
>
> Thanks.

By the way, here is the hits from

	$ git grep '^[    ]*: ${' master

All of them in t/t4100/* and contrib/examples do not need touching.
But others are in the same boat as what you updated in this patch,
i.e. "wasteful -- the no-op invocation do not need its $VAR
expanded".

contrib/examples/git-reset.sh:: ${rev=HEAD}
contrib/hooks/post-receive-email:: ${diffopts:="--stat --summary --find-copies-harder"}
git-mergetool--lib.sh:: ${MERGE_TOOLS_DIR=$(git --exec-path)/mergetools}
git-quiltimport.sh:: ${QUILT_PATCHES:=patches}
git-quiltimport.sh:: ${QUILT_SERIES:=$QUILT_PATCHES/series}
git-rebase--interactive.sh:: ${comment_char:=#}
git-sh-setup.sh:	: ${LESS=-FRX}
git-sh-setup.sh:	: ${LV=-c}
git-sh-setup.sh:	: ${GIT_OBJECT_DIRECTORY="$(git rev-parse --git-path objects)"}
git-submodule.sh:: ${GIT_ALLOW_PROTOCOL=file:git:http:https:ssh}
git-web--browse.sh:	: ${browser_path:="$1"}
t/t1014-read-tree-confusing.sh:	: ${pretty:=$path}
t/t1450-fsck.sh:		: ${pretty:=$path}
t/t4100/t-apply-2.patch: : ${GIT_DIR=.git}
t/t4100/t-apply-2.patch: : ${GIT_OBJECT_DIRECTORY="${SHA1_FILE_DIRECTORY-"$GIT_DIR/objects"}"}
t/t4100/t-apply-6.patch: : ${GIT_DIR=.git}
t/t4100/t-apply-6.patch: : ${GIT_OBJECT_DIRECTORY="${SHA1_FILE_DIRECTORY-"$GIT_DIR/objects"}"}
t/test-lib.sh:: ${ASAN_OPTIONS=detect_leaks=0}
t/valgrind/analyze.sh:: ${TEST_OUTPUT_DIRECTORY=$(dirname "$0")/..}