| From d47ef968cb7f7d752ef8bda65f73e16b163f96a4 Mon Sep 17 00:00:00 2001 |
| From: Johannes Schindelin <johannes.schindelin@gmx.de> |
| 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 <johannes.schindelin@gmx.de> |
| (cherry picked from commit d2c84dad1c88f40906799bc879f70b965efd8ba6) |
| Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> |
| --- |
| compat/mingw.c | 57 ++++++++++++++++++++++++++++++++++- |
| compat/mingw.h | 11 +++++++ |
| git-compat-util.h | 4 +++ |
| read-cache.c | 3 ++ |
| t/helper/test-path-utils.c | 17 +++++++++++ |
| t/t0060-path-utils.sh | 14 +++++++++ |
| t/t7415-submodule-names.sh | 2 +- |
| t/t7417-submodule-path-url.sh | 17 +++++++++++ |
| 8 files changed, 123 insertions(+), 2 deletions(-) |
| |
| diff --git a/compat/mingw.c b/compat/mingw.c |
| index 94b0746f2f..386aa94acb 100644 |
| --- a/compat/mingw.c |
| +++ b/compat/mingw.c |
| @@ -389,6 +389,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); |
| @@ -462,7 +468,7 @@ int mingw_open (const char *filename, int oflags, ...) |
| typedef int (*open_fn_t)(wchar_t const *wfilename, 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]; |
| open_fn_t open_fn; |
| |
| @@ -470,6 +476,11 @@ int mingw_open (const char *filename, int 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"; |
| |
| @@ -536,6 +547,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 || |
| @@ -558,6 +574,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 || |
| @@ -2419,6 +2440,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 8c24ddaa3e..1d7e9d7c9d 100644 |
| --- a/compat/mingw.h |
| +++ b/compat/mingw.h |
| @@ -479,6 +479,17 @@ extern char *mingw_query_user_email(void); |
| #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 09b0102cae..d17360e897 100644 |
| --- a/git-compat-util.h |
| +++ b/git-compat-util.h |
| @@ -385,6 +385,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 1a4e184478..1d82dbdd65 100644 |
| --- a/read-cache.c |
| +++ b/read-cache.c |
| @@ -955,6 +955,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 d9411032d2..e737a941d3 100644 |
| --- a/t/helper/test-path-utils.c |
| +++ b/t/helper/test-path-utils.c |
| @@ -387,6 +387,23 @@ int cmd__path_utils(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 85cccc655a..dc245c7dfe 100755 |
| --- a/t/t0060-path-utils.sh |
| +++ b/t/t0060-path-utils.sh |
| @@ -455,4 +455,18 @@ test_expect_success 'match .gitmodules' ' |
| .gitmodules,:\$DATA |
| ' |
| |
| +test_expect_success MINGW 'is_valid_path() on Windows' ' |
| + test-tool 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 71391fe161..33a9126ee0 100755 |
| --- a/t/t7415-submodule-names.sh |
| +++ b/t/t7415-submodule-names.sh |
| @@ -220,7 +220,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 756af8c4d6..f7e7e94d7b 100755 |
| --- a/t/t7417-submodule-path-url.sh |
| +++ b/t/t7417-submodule-path-url.sh |
| @@ -25,4 +25,21 @@ test_expect_success 'fsck rejects unprotected dash' ' |
| grep gitmodulesPath 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 |
| -- |
| 2.24.0.393.g34dc348eaf |
| |