git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Adam Dinwoodie <adam@dinwoodie.org>
To: git@vger.kernel.org
Cc: RyotaK <security@ryotak.me>,
	Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: [RFC PATCH] cygwin: disallow backslashes in file names
Date: Sat, 24 Apr 2021 22:21:17 +0100	[thread overview]
Message-ID: <20210424212117.6165-1-adam@dinwoodie.org> (raw)

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

 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-24 21:21 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-24 21:21 Adam Dinwoodie [this message]
2021-04-25  2:22 ` [RFC PATCH] cygwin: disallow backslashes in file names Johannes Schindelin
     [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=20210424212117.6165-1-adam@dinwoodie.org \
    --to=adam@dinwoodie.org \
    --cc=git@vger.kernel.org \
    --cc=johannes.schindelin@gmx.de \
    --cc=security@ryotak.me \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).