Re: [PATCH] git-sh-setup.sh: fix missing double quotes variables
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 19, 2016, 01:43 UTC
- Message-ID
- <xmqqshwax8ah.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20160618193710.32265-1-cuong.manhle.vn@gmail.com>
LE Manh Cuong <cuong.manhle.vn@gmail.com> writes:
Show 19 quoted lines
> 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.
Show 27 quoted lines
> 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"