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 <johannes.schindelin@gmx.de>

diff --git a/compat/mingw.c b/compat/mingw.c
index 8b6fa0db446aee9888ff6484560131143c7e22e5..17b4da16e85cef21a26a3674165e67b78ea95ebe 100644 (file)
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -333,6 +333,12 @@ int mingw_mkdir(const char *path, int mode)
 {
        int ret;
        wchar_t wpath[MAX_PATH];
+
+       if (!is_valid_win32_path(path)) {
+               errno = EINVAL;
+               return -1;
+       }
+
        if (xutftowcs_path(wpath, path) < 0)
                return -1;
        ret = _wmkdir(wpath);
@@ -345,13 +351,18 @@ int mingw_open (const char *filename, int oflags, ...)
 {
        va_list args;
        unsigned mode;
-       int fd;
+       int fd, create = (oflags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL);
        wchar_t wfilename[MAX_PATH];
 
        va_start(args, oflags);
        mode = va_arg(args, int);
        va_end(args);
 
+       if (!is_valid_win32_path(filename)) {
+               errno = create ? EINVAL : ENOENT;
+               return -1;
+       }
+
        if (filename && !strcmp(filename, "/dev/null"))
                filename = "nul";
 
@@ -413,6 +424,11 @@ FILE *mingw_fopen (const char *filename, const char *otype)
        int hide = needs_hiding(filename);
        FILE *file;
        wchar_t wfilename[MAX_PATH], wotype[4];
+       if (!is_valid_win32_path(filename)) {
+               int create = otype && strchr(otype, 'w');
+               errno = create ? EINVAL : ENOENT;
+               return NULL;
+       }
        if (filename && !strcmp(filename, "/dev/null"))
                filename = "nul";
        if (xutftowcs_path(wfilename, filename) < 0 ||
@@ -435,6 +451,11 @@ FILE *mingw_freopen (const char *filename, const char *otype, FILE *stream)
        int hide = needs_hiding(filename);
        FILE *file;
        wchar_t wfilename[MAX_PATH], wotype[4];
+       if (!is_valid_win32_path(filename)) {
+               int create = otype && strchr(otype, 'w');
+               errno = create ? EINVAL : ENOENT;
+               return NULL;
+       }
        if (filename && !strcmp(filename, "/dev/null"))
                filename = "nul";
        if (xutftowcs_path(wfilename, filename) < 0 ||
@@ -2106,6 +2127,40 @@ static void setup_windows_environment(void)
                setenv("TERM", "cygwin", 1);
 }
 
+int is_valid_win32_path(const char *path)
+{
+       int preceding_space_or_period = 0, i = 0, periods = 0;
+
+       if (!protect_ntfs)
+               return 1;
+
+       for (;;) {
+               char c = *(path++);
+               switch (c) {
+               case '\0':
+               case '/': case '\\':
+                       /* cannot end in ` ` or `.`, except for `.` and `..` */
+                       if (preceding_space_or_period &&
+                           (i != periods || periods > 2))
+                               return 0;
+                       if (!c)
+                               return 1;
+
+                       i = periods = preceding_space_or_period = 0;
+                       continue;
+               case '.':
+                       periods++;
+                       /* fallthru */
+               case ' ':
+                       preceding_space_or_period = 1;
+                       i++;
+                       continue;
+               }
+               preceding_space_or_period = 0;
+               i++;
+       }
+}
+
 /*
  * Disable MSVCRT command line wildcard expansion (__getmainargs called from
  * mingw startup code, see init.c in mingw runtime).

diff --git a/compat/mingw.h b/compat/mingw.h
index e03aecfe2e6556e1ef513922104557373eaa9260..8c49c1d09b697d009e4dc964bc34cfc5377334d7 100644 (file)
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -428,6 +428,17 @@ int mingw_offset_1st_component(const char *path);
 #include <inttypes.h>
 #endif
 
+/**
+ * Verifies that the given path is a valid one on Windows.
+ *
+ * In particular, path segments are disallowed which end in a period or a
+ * space (except the special directories `.` and `..`).
+ *
+ * Returns 1 upon success, otherwise 0.
+ */
+int is_valid_win32_path(const char *path);
+#define is_valid_path(path) is_valid_win32_path(path)
+
 /**
  * Converts UTF-8 encoded string to UTF-16LE.
  *

diff --git a/git-compat-util.h b/git-compat-util.h
index 6cb3c2f19eb5a0822ae17a1b1e1abc08d3de25e4..e587ac4e2365bb790a8c74dd7646b349e1f69c5e 100644 (file)
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -370,6 +370,10 @@ static inline int git_offset_1st_component(const char *path)
 #define offset_1st_component git_offset_1st_component
 #endif
 
+#ifndef is_valid_path
+#define is_valid_path(path) 1
+#endif
+
 #ifndef find_last_dir_sep
 static inline char *git_find_last_dir_sep(const char *path)
 {

diff --git a/read-cache.c b/read-cache.c
index bde1e70c5142f905c3f98514ec19370412348572..771171c4028d0630405fffe3f129fcef55cad20b 100644 (file)
--- a/read-cache.c
+++ b/read-cache.c
@@ -847,6 +847,9 @@ int verify_path(const char *path, unsigned mode)
        if (has_dos_drive_prefix(path))
                return 0;
 
+       if (!is_valid_path(path))
+               return 0;
+
        goto inside;
        for (;;) {
                if (!c)

diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
index 16d8e689c80029d996d4902e527db736e0559bfc..8b3ce07860d4d5dfeca06041b811282a59ef40bb 100644 (file)
--- a/t/helper/test-path-utils.c
+++ b/t/helper/test-path-utils.c
@@ -386,6 +386,23 @@ int cmd_main(int argc, const char **argv)
        if (argc > 1 && !strcmp(argv[1], "protect_ntfs_hfs"))
                return !!protect_ntfs_hfs_benchmark(argc - 1, argv + 1);
 
+       if (argc > 1 && !strcmp(argv[1], "is_valid_path")) {
+               int res = 0, expect = 1, i;
+
+               for (i = 2; i < argc; i++)
+                       if (!strcmp("--not", argv[i]))
+                               expect = 0;
+                       else if (expect != is_valid_path(argv[i]))
+                               res = error("'%s' is%s a valid path",
+                                           argv[i], expect ? " not" : "");
+                       else
+                               fprintf(stderr,
+                                       "'%s' is%s a valid path\n",
+                                       argv[i], expect ? "" : " not");
+
+               return !!res;
+       }
+
        fprintf(stderr, "%s: unknown function name: %s\n", argv[0],
                argv[1] ? argv[1] : "(there was none)");
        return 1;

diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 2b8589e921cefc09d18c6701c1b4db2c833c8591..1171e0bb8850b97bd3ad0b6e70ce178467e28f65 100755 (executable)
--- 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

diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
index 7c65e7a35c9819411eb1eeb0d8a0e157fae58015..5141ff45c300c05075b9bd00aeb5c98f2aadca0f 100755 (executable)
--- a/t/t7415-submodule-names.sh
+++ b/t/t7415-submodule-names.sh
@@ -102,7 +102,7 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '
        ) &&
        test_must_fail git -c core.protectNTFS=false \
                clone --recurse-submodules squatting squatting-clone 2>err &&
-       test_i18ngrep "directory not empty" err &&
+       test_i18ngrep -e "directory not empty" -e "not an empty directory" err &&
        ! grep gitdir squatting-clone/d/a/git~2
 '
 

diff --git a/t/t7417-submodule-path-url.sh b/t/t7417-submodule-path-url.sh
index 638293f0dab1c69b2f984b69a8bf96a9c2c98de8..fad9e20dc4cb17bfcfc3b514840bba15c0e83f50 100755 (executable)
--- a/t/t7417-submodule-path-url.sh
+++ b/t/t7417-submodule-path-url.sh
@@ -17,4 +17,21 @@ test_expect_success 'clone rejects unprotected dash' '
        test_i18ngrep ignoring err
 '
 
+test_expect_success MINGW 'submodule paths disallows trailing spaces' '
+       git init super &&
+       test_must_fail git -C super submodule add ../upstream "sub " &&
+
+       : add "sub", then rename "sub" to "sub ", the hard way &&
+       git -C super submodule add ../upstream sub &&
+       tree=$(git -C super write-tree) &&
+       git -C super ls-tree $tree >tree &&
+       sed "s/sub/sub /" <tree >tree.new &&
+       tree=$(git -C super mktree <tree.new) &&
+       commit=$(echo with space | git -C super commit-tree $tree) &&
+       git -C super update-ref refs/heads/master $commit &&
+
+       test_must_fail git clone --recurse-submodules super dst 2>err &&
+       test_i18ngrep "sub " err
+'
+
 test_done