git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Adam Dinwoodie <adam@dinwoodie.org>
Cc: git@vger.kernel.org, RyotaK <security@ryotak.me>
Subject: Re: [RFC PATCH] cygwin: disallow backslashes in file names
Date: Sun, 25 Apr 2021 04:22:03 +0200 (CEST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2104250413320.54@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20210424212117.6165-1-adam@dinwoodie.org>

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

  reply	other threads:[~2021-04-26 14:09 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-24 21:21 Adam Dinwoodie
2021-04-25  2:22 ` Johannes Schindelin [this message]
     [not found]   ` <CA+kUOan3vk1zJezpieRhKwZ8gsYrCxDBefkXJ1fUC61O+gb12A@mail.gmail.com>
2021-04-27 19:22     ` Adam Dinwoodie
2021-04-28  0:27       ` Johannes Schindelin
2021-04-29 20:11 ` [PATCH] " Adam Dinwoodie
2021-04-30  0:48   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=nycvar.QRO.7.76.6.2104250413320.54@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=adam@dinwoodie.org \
    --cc=git@vger.kernel.org \
    --cc=security@ryotak.me \
    --subject='Re: [RFC PATCH] cygwin: disallow backslashes in file names' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox