* [RFC PATCH] cygwin: disallow backslashes in file names
@ 2021-04-24 21:21 Adam Dinwoodie
2021-04-25 2:22 ` Johannes Schindelin
2021-04-29 20:11 ` [PATCH] " Adam Dinwoodie
0 siblings, 2 replies; 6+ messages in thread
From: Adam Dinwoodie @ 2021-04-24 21:21 UTC (permalink / raw)
To: git; +Cc: RyotaK, Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] cygwin: disallow backslashes in file names
2021-04-24 21:21 [RFC PATCH] cygwin: disallow backslashes in file names Adam Dinwoodie
@ 2021-04-25 2:22 ` Johannes Schindelin
[not found] ` <CA+kUOan3vk1zJezpieRhKwZ8gsYrCxDBefkXJ1fUC61O+gb12A@mail.gmail.com>
2021-04-29 20:11 ` [PATCH] " Adam Dinwoodie
1 sibling, 1 reply; 6+ messages in thread
From: Johannes Schindelin @ 2021-04-25 2:22 UTC (permalink / raw)
To: Adam Dinwoodie; +Cc: git, RyotaK
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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] cygwin: disallow backslashes in file names
2021-04-24 21:21 [RFC PATCH] cygwin: disallow backslashes in file names Adam Dinwoodie
2021-04-25 2:22 ` Johannes Schindelin
@ 2021-04-29 20:11 ` Adam Dinwoodie
2021-04-30 0:48 ` Junio C Hamano
1 sibling, 1 reply; 6+ messages in thread
From: Adam Dinwoodie @ 2021-04-29 20:11 UTC (permalink / raw)
To: git; +Cc: RyotaK, Johannes Schindelin
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(-)
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' '
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] cygwin: disallow backslashes in file names
2021-04-29 20:11 ` [PATCH] " Adam Dinwoodie
@ 2021-04-30 0:48 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2021-04-30 0:48 UTC (permalink / raw)
To: Adam Dinwoodie; +Cc: git, RyotaK, Johannes Schindelin
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' '
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-30 0:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-24 21:21 [RFC PATCH] cygwin: disallow backslashes in file names Adam Dinwoodie
2021-04-25 2:22 ` 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
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).