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@xxxxxxxxx>
> Helped-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
> Signed-off-by: Adam Dinwoodie <adam@xxxxxxxxxxxxx>
> ---
>
> 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
>
>
