Re: [RFC PATCH] cygwin: disallow backslashes in file names

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Adam,

On Sat, 24 Apr 2021, Adam Dinwoodie wrote:

> The backslash character is not a valid part of a file name on Windows,
> so it should not be possible to write files that were unpacked from tree
> objects where the stored filename contains a backslash character, as it
> will be interpreted as a directory separator.
>
> This caused CVE-2019-1354 in mingw, which was fixed by e1d911dd4c
> ("mingw: disallow backslash characters in tree objects' file names",
> 2019-09-12), however the vulnerability also exists in Cygwin, as while
> Cygwin mostly provides a POSIX-like path system, it will also interpret
> a backslash as a directory separator in the name of compatibility.
>
> To avoid the vulnerability, extend the fix for mingw to also apply to
> Cygwin.
>
> Reported-by: RyotaK <security@ryotak.me>
> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
> ---
>
> Notes:
>     The patch to read-cache.c is the one I've applied downstream as the Cygwin Git
>     maintainer to resolve this vulnerability, and I've manually tested that it
>     resolves the vulnerability, so that's the change I'd recommend anyone who needs
>     to build Git on Cygwin themselves take until there's something officially in
>     the Git source code.
>
>     I'm much less convinced by my approach for the test script.  I definitely think
>     it's worth having a test here, but the test as written still fails, as the test
>     seems to be looking for the error message "directory not empty", but running
>     the test on Cygwin produces the error "cannot create submodule directory d\a".
>     I'm not sure why that difference exists, and whether the correct approach would
>     be to (a) ensure the error messages are consistent across platforms or (b) to
>     change the test to expect the appropriate error on the appropriate platform.

Wasn't there something in Cygwin that _allowed_ backslashes as file name
characters? I vaguely remember that the ASCII characters forbidden by
Windows were mapped into some "private page".

Maybe that is responsible for the difference here?

>     I'm also not convinced by my approach of adding a "WINDOWS" prerequisite to
>     test-lib.sh. I went with this as I couldn't immediately see a way to pass
>     prerequisites on an "any" rather than "all" basis to test_expect_success, and
>     this would allow us to simplify all the tests that currently have
>     "!MINGW,!CYGWIN" as prerequisites, but it still feels a bit clunky to me.

Right, the only way I could think of it would be

	test_lazy_prereq 'test_have_prereq MINGW || test_have_prereq CYGWIN'

Your approach looks fine to me, though.

Ciao,
Dscho

>
>  read-cache.c               | 2 +-
>  t/t7415-submodule-names.sh | 2 +-
>  t/test-lib.sh              | 2 ++
>  3 files changed, 4 insertions(+), 2 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/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
> index f70368bc2e..6505bc2085 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 &&
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 3dec266221..adaa2db601 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1459,14 +1459,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
> --
> 2.31.1
>
>