diff mbox series

cygwin: disallow backslashes in file names

Message ID 20210429201144.8936-1-adam@dinwoodie.org (mailing list archive)
State New
Headers show
Series cygwin: disallow backslashes in file names | expand

Commit Message

Adam Dinwoodie April 29, 2021, 8:11 p.m. UTC
The backslash character is not a valid part of a file name on Windows.
If, in Windows, Git attempts to write a file that has a backslash
character in the filename, it will be incorrectly interpreted as a
directory separator.

This caused CVE-2019-1354 in MinGW, as this behaviour can be manipulated
to cause the checkout to write to files it ought not write to, such as
adding code to the .git/hooks directory.  This was fixed by e1d911dd4c
(mingw: disallow backslash characters in tree objects' file names,
2019-09-12).  However, the vulnerability also exists in Cygwin: while
Cygwin mostly provides a POSIX-like path system, it will still interpret
a backslash as a directory separator.

To avoid this vulnerability, CVE-2021-29468, extend the previous fix to
also apply to Cygwin.

Similarly, extend the test case added by the previous version of the
commit.  The test suite doesn't have an easy way to say "run this test
if in MinGW or Cygwin", so add a new test prerequisite that covers both.

As well as checking behaviour in the presence of paths containing
backslashes, the existing test also checks behaviour in the presence of
paths that differ only by the presence of a trailing ".".  MinGW follows
normal Windows application behaviour and treats them as the same path,
but Cygwin more closely emulates *nix systems (at the expense of
compatibility with native Windows applications) and will create and
distinguish between such paths.  Gate the relevant bit of that test
accordingly.

Reported-by: RyotaK <security@ryotak.me>
Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
---
 read-cache.c               |  2 +-
 t/test-lib.sh              |  2 ++
 t/t7415-submodule-names.sh | 13 ++++++++-----
 3 files changed, 11 insertions(+), 6 deletions(-)

Comments

Junio C Hamano April 30, 2021, 12:48 a.m. UTC | #1
Adam Dinwoodie <adam@dinwoodie.org> writes:

> The backslash character is not a valid part of a file name on Windows.
> If, in Windows, Git attempts to write a file that has a backslash
> character in the filename, it will be incorrectly interpreted as a
> directory separator.
>
> This caused CVE-2019-1354 in MinGW, as this behaviour can be manipulated
> to cause the checkout to write to files it ought not write to, such as
> adding code to the .git/hooks directory.  This was fixed by e1d911dd4c
> (mingw: disallow backslash characters in tree objects' file names,
> 2019-09-12).  However, the vulnerability also exists in Cygwin: while
> Cygwin mostly provides a POSIX-like path system, it will still interpret
> a backslash as a directory separator.
>
> To avoid this vulnerability, CVE-2021-29468, extend the previous fix to
> also apply to Cygwin.
>
> Similarly, extend the test case added by the previous version of the
> commit.  The test suite doesn't have an easy way to say "run this test
> if in MinGW or Cygwin", so add a new test prerequisite that covers both.
>
> As well as checking behaviour in the presence of paths containing
> backslashes, the existing test also checks behaviour in the presence of
> paths that differ only by the presence of a trailing ".".  MinGW follows
> normal Windows application behaviour and treats them as the same path,
> but Cygwin more closely emulates *nix systems (at the expense of
> compatibility with native Windows applications) and will create and
> distinguish between such paths.  Gate the relevant bit of that test
> accordingly.
>
> Reported-by: RyotaK <security@ryotak.me>
> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
> ---

Thanks, all.  Will queue.

>  read-cache.c               |  2 +-
>  t/test-lib.sh              |  2 ++
>  t/t7415-submodule-names.sh | 13 ++++++++-----
>  3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 5a907af2fb..b6c13bc04e 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -985,7 +985,7 @@ int verify_path(const char *path, unsigned mode)
>  				}
>  			}
>  			if (protect_ntfs) {
> -#ifdef GIT_WINDOWS_NATIVE
> +#if defined GIT_WINDOWS_NATIVE || defined __CYGWIN__
>  				if (c == '\\')
>  					return 0;
>  #endif
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index d3f6af6a65..e84b8c87f9 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1457,14 +1457,16 @@ case $uname_s in
>  	test_set_prereq NATIVE_CRLF
>  	test_set_prereq SED_STRIPS_CR
>  	test_set_prereq GREP_STRIPS_CR
> +	test_set_prereq WINDOWS
>  	GIT_TEST_CMP=mingw_test_cmp
>  	;;
>  *CYGWIN*)
>  	test_set_prereq POSIXPERM
>  	test_set_prereq EXECKEEPSPID
>  	test_set_prereq CYGWIN
>  	test_set_prereq SED_STRIPS_CR
>  	test_set_prereq GREP_STRIPS_CR
> +	test_set_prereq WINDOWS
>  	;;
>  *)
>  	test_set_prereq POSIXPERM
> diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
> index f70368bc2e..6bf098a6be 100755
> --- a/t/t7415-submodule-names.sh
> +++ b/t/t7415-submodule-names.sh
> @@ -191,7 +191,7 @@ test_expect_success 'fsck detects corrupt .gitmodules' '
>  	)
>  '
>  
> -test_expect_success MINGW 'prevent git~1 squatting on Windows' '
> +test_expect_success WINDOWS 'prevent git~1 squatting on Windows' '
>  	git init squatting &&
>  	(
>  		cd squatting &&
> @@ -219,10 +219,13 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '
>  		test_tick &&
>  		git -c core.protectNTFS=false commit -m "module"
>  	) &&
> -	test_must_fail git -c core.protectNTFS=false \
> -		clone --recurse-submodules squatting squatting-clone 2>err &&
> -	test_i18ngrep -e "directory not empty" -e "not an empty directory" err &&
> -	! grep gitdir squatting-clone/d/a/git~2
> +	if test_have_prereq MINGW
> +	then
> +		test_must_fail git -c core.protectNTFS=false \
> +			clone --recurse-submodules squatting squatting-clone 2>err &&
> +		test_i18ngrep -e "directory not empty" -e "not an empty directory" err &&
> +		! grep gitdir squatting-clone/d/a/git~2
> +	fi
>  '
>  
>  test_expect_success 'git dirs of sibling submodules must not be nested' '
diff mbox series

Patch

diff --git a/read-cache.c b/read-cache.c
index 5a907af2fb..b6c13bc04e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -985,7 +985,7 @@  int verify_path(const char *path, unsigned mode)
 				}
 			}
 			if (protect_ntfs) {
-#ifdef GIT_WINDOWS_NATIVE
+#if defined GIT_WINDOWS_NATIVE || defined __CYGWIN__
 				if (c == '\\')
 					return 0;
 #endif
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d3f6af6a65..e84b8c87f9 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1457,14 +1457,16 @@  case $uname_s in
 	test_set_prereq NATIVE_CRLF
 	test_set_prereq SED_STRIPS_CR
 	test_set_prereq GREP_STRIPS_CR
+	test_set_prereq WINDOWS
 	GIT_TEST_CMP=mingw_test_cmp
 	;;
 *CYGWIN*)
 	test_set_prereq POSIXPERM
 	test_set_prereq EXECKEEPSPID
 	test_set_prereq CYGWIN
 	test_set_prereq SED_STRIPS_CR
 	test_set_prereq GREP_STRIPS_CR
+	test_set_prereq WINDOWS
 	;;
 *)
 	test_set_prereq POSIXPERM
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
index f70368bc2e..6bf098a6be 100755
--- a/t/t7415-submodule-names.sh
+++ b/t/t7415-submodule-names.sh
@@ -191,7 +191,7 @@  test_expect_success 'fsck detects corrupt .gitmodules' '
 	)
 '
 
-test_expect_success MINGW 'prevent git~1 squatting on Windows' '
+test_expect_success WINDOWS 'prevent git~1 squatting on Windows' '
 	git init squatting &&
 	(
 		cd squatting &&
@@ -219,10 +219,13 @@  test_expect_success MINGW 'prevent git~1 squatting on Windows' '
 		test_tick &&
 		git -c core.protectNTFS=false commit -m "module"
 	) &&
-	test_must_fail git -c core.protectNTFS=false \
-		clone --recurse-submodules squatting squatting-clone 2>err &&
-	test_i18ngrep -e "directory not empty" -e "not an empty directory" err &&
-	! grep gitdir squatting-clone/d/a/git~2
+	if test_have_prereq MINGW
+	then
+		test_must_fail git -c core.protectNTFS=false \
+			clone --recurse-submodules squatting squatting-clone 2>err &&
+		test_i18ngrep -e "directory not empty" -e "not an empty directory" err &&
+		! grep gitdir squatting-clone/d/a/git~2
+	fi
 '
 
 test_expect_success 'git dirs of sibling submodules must not be nested' '