From 91bd46588e6959e6903e275f78b10bd07830d547 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 28 Aug 2019 12:22:17 +0200 Subject: path: also guard `.gitmodules` against NTFS Alternate Data Streams We just safe-guarded `.git` against NTFS Alternate Data Stream-related attack vectors, and now it is time to do the same for `.gitmodules`. Note: In the added regression test, we refrain from verifying all kinds of variations between short names and NTFS Alternate Data Streams: as the new code disallows _all_ Alternate Data Streams of `.gitmodules`, it is enough to test one in order to know that all of them are guarded against. Signed-off-by: Johannes Schindelin --- t/t0060-path-utils.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 't/t0060-path-utils.sh') diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 3f3357ed9f..2b8589e921 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -408,6 +408,9 @@ test_expect_success 'match .gitmodules' ' ~1000000 \ ~9999999 \ \ + .gitmodules:\$DATA \ + "gitmod~4 . :\$DATA" \ + \ --not \ ".gitmodules x" \ ".gitmodules .x" \ @@ -432,7 +435,9 @@ test_expect_success 'match .gitmodules' ' \ GI7EB~1 \ GI7EB~01 \ - GI7EB~1X + GI7EB~1X \ + \ + .gitmodules,:\$DATA ' test_done -- cgit v1.2.3 From d2c84dad1c88f40906799bc879f70b965efd8ba6 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 5 Sep 2019 13:27:53 +0200 Subject: mingw: refuse to access paths with trailing spaces or periods When creating a directory on Windows whose path ends in a space or a period (or chains thereof), the Win32 API "helpfully" trims those. For example, `mkdir("abc ");` will return success, but actually create a directory called `abc` instead. This stems back to the DOS days, when all file names had exactly 8 characters plus exactly 3 characters for the file extension, and the only way to have shorter names was by padding with spaces. Sadly, this "helpful" behavior is a bit inconsistent: after a successful `mkdir("abc ");`, a `mkdir("abc /def")` will actually _fail_ (because the directory `abc ` does not actually exist). Even if it would work, we now have a serious problem because a Git repository could contain directories `abc` and `abc `, and on Windows, they would be "merged" unintentionally. As these paths are illegal on Windows, anyway, let's disallow any accesses to such paths on that Operating System. For practical reasons, this behavior is still guarded by the config setting `core.protectNTFS`: it is possible (and at least two regression tests make use of it) to create commits without involving the worktree. In such a scenario, it is of course possible -- even on Windows -- to create such file names. Among other consequences, this patch disallows submodules' paths to end in spaces on Windows (which would formerly have confused Git enough to try to write into incorrect paths, anyway). While this patch does not fix a vulnerability on its own, it prevents an attack vector that was exploited in demonstrations of a number of recently-fixed security bugs. The regression test added to `t/t7417-submodule-path-url.sh` reflects that attack vector. Note that we have to adjust the test case "prevent git~1 squatting on Windows" in `t/t7415-submodule-names.sh` because of a very subtle issue. It tries to clone two submodules whose names differ only in a trailing period character, and as a consequence their git directories differ in the same way. Previously, when Git tried to clone the second submodule, it thought that the git directory already existed (because on Windows, when you create a directory with the name `b.` it actually creates `b`), but with this patch, the first submodule's clone will fail because of the illegal name of the git directory. Therefore, when cloning the second submodule, Git will take a different code path: a fresh clone (without an existing git directory). Both code paths fail to clone the second submodule, both because the the corresponding worktree directory exists and is not empty, but the error messages are worded differently. Signed-off-by: Johannes Schindelin --- t/t0060-path-utils.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 't/t0060-path-utils.sh') diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 2b8589e921..1171e0bb88 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -440,4 +440,18 @@ test_expect_success 'match .gitmodules' ' .gitmodules,:\$DATA ' +test_expect_success MINGW 'is_valid_path() on Windows' ' + test-path-utils is_valid_path \ + win32 \ + "win32 x" \ + ../hello.txt \ + \ + --not \ + "win32 " \ + "win32 /x " \ + "win32." \ + "win32 . ." \ + .../hello.txt +' + test_done -- cgit v1.2.3 From 817ddd64c20b29b2d86b3a0589f7ff88d1279109 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 5 Sep 2019 13:44:21 +0200 Subject: mingw: refuse to access paths with illegal characters Certain characters are not admissible in file names on Windows, even if Cygwin/MSYS2 (and therefore, Git for Windows' Bash) pretend that they are, e.g. `:`, `<`, `>`, etc Let's disallow those characters explicitly in Windows builds of Git. Note: just like trailing spaces or periods, it _is_ possible on Windows to create commits adding files with such illegal characters, as long as the operation leaves the worktree untouched. To allow for that, we continue to guard `is_valid_win32_path()` behind the config setting `core.protectNTFS`, so that users _can_ continue to do that, as long as they turn the protections off via that config setting. Among other problems, this prevents Git from trying to write to an "NTFS Alternate Data Stream" (which refers to metadata stored alongside a file, under a special name: ":"). This fix therefore also prevents an attack vector that was exploited in demonstrations of a number of recently-fixed security bugs. Further reading on illegal characters in Win32 filenames: https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file Signed-off-by: Johannes Schindelin --- t/t0060-path-utils.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 't/t0060-path-utils.sh') diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 1171e0bb88..f7e2529bff 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -445,13 +445,15 @@ test_expect_success MINGW 'is_valid_path() on Windows' ' win32 \ "win32 x" \ ../hello.txt \ + C:\\git \ \ --not \ "win32 " \ "win32 /x " \ "win32." \ "win32 . ." \ - .../hello.txt + .../hello.txt \ + colon:test ' test_done -- cgit v1.2.3 From f82a97eb9197c1e3768e72648f37ce0ca3233734 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 6 Sep 2019 00:09:10 +0200 Subject: mingw: handle `subst`-ed "DOS drives" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Over a decade ago, in 25fe217b86c (Windows: Treat Windows style path names., 2008-03-05), Git was taught to handle absolute Windows paths, i.e. paths that start with a drive letter and a colon. Unbeknownst to us, while drive letters of physical drives are limited to letters of the English alphabet, there is a way to assign virtual drive letters to arbitrary directories, via the `subst` command, which is _not_ limited to English letters. It is therefore possible to have absolute Windows paths of the form `1:\what\the\hex.txt`. Even "better": pretty much arbitrary Unicode letters can also be used, e.g. `ä:\tschibät.sch`. While it can be sensibly argued that users who set up such funny drive letters really seek adverse consequences, the Windows Operating System is known to be a platform where many users are at the mercy of administrators who have their very own idea of what constitutes a reasonable setup. Therefore, let's just make sure that such funny paths are still considered absolute paths by Git, on Windows. In addition to Unicode characters, pretty much any character is a valid drive letter, as far as `subst` is concerned, even `:` and `"` or even a space character. While it is probably the opposite of smart to use them, let's safeguard `is_dos_drive_prefix()` against all of them. Note: `[::1]:repo` is a valid URL, but not a valid path on Windows. As `[` is now considered a valid drive letter, we need to be very careful to avoid misinterpreting such a string as valid local path in `url_is_local_not_ssh()`. To do that, we use the just-introduced function `is_valid_path()` (which will label the string as invalid file name because of the colon characters). This fixes CVE-2019-1351. Reported-by: Nicolas Joly Signed-off-by: Johannes Schindelin --- t/t0060-path-utils.sh | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 't/t0060-path-utils.sh') diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index f7e2529bff..40db3e1e1a 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -165,6 +165,15 @@ test_expect_success 'absolute path rejects the empty string' ' test_must_fail test-path-utils absolute_path "" ' +test_expect_success MINGW ':\\abc is an absolute path' ' + for letter in : \" C Z 1 ä + do + path=$letter:\\abc && + absolute="$(test-path-utils absolute_path "$path")" && + test "$path" = "$absolute" || return 1 + done +' + test_expect_success 'real path rejects the empty string' ' test_must_fail test-path-utils real_path "" ' -- cgit v1.2.3