From 288a74bcd28229a00c3632f18cba92dbfdf73ee9 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 23 Sep 2019 08:58:11 +0200 Subject: is_ntfs_dotgit(): only verify the leading segment The config setting `core.protectNTFS` is specifically designed to work not only on Windows, but anywhere, to allow for repositories hosted on, say, Linux servers to be protected against NTFS-specific attack vectors. As a consequence, `is_ntfs_dotgit()` manually splits backslash-separated paths (but does not do the same for paths separated by forward slashes), under the assumption that the backslash might not be a valid directory separator on the _current_ Operating System. However, the two callers, `verify_path()` and `fsck_tree()`, are supposed to feed only individual path segments to the `is_ntfs_dotgit()` function. This causes a lot of duplicate scanning (and very inefficient scanning, too, as the inner loop of `is_ntfs_dotgit()` was optimized for readability rather than for speed. Let's simplify the design of `is_ntfs_dotgit()` by putting the burden of splitting the paths by backslashes as directory separators on the callers of said function. Consequently, the `verify_path()` function, which already splits the path by directory separators, now treats backslashes as directory separators _explicitly_ when `core.protectNTFS` is turned on, even on platforms where the backslash is _not_ a directory separator. Note that we have to repeat some code in `verify_path()`: if the backslash is not a directory separator on the current Operating System, we want to allow file names like `\`, but we _do_ want to disallow paths that are clearly intended to cause harm when the repository is cloned on Windows. The `fsck_tree()` function (the other caller of `is_ntfs_dotgit()`) now needs to look for backslashes in tree entries' names specifically when `core.protectNTFS` is turned on. While it would be tempting to completely disallow backslashes in that case (much like `fsck` reports names containing forward slashes as "full paths"), this would be overzealous: when `core.protectNTFS` is turned on in a non-Windows setup, backslashes are perfectly valid characters in file names while we _still_ want to disallow tree entries that are clearly designed to exploit NTFS-specific behavior. This simplification will make subsequent changes easier to implement, such as turning `core.protectNTFS` on by default (not only on Windows) or protecting against attack vectors involving NTFS Alternate Data Streams. Incidentally, this change allows for catching malicious repositories that contain tree entries of the form `dir\.gitmodules` already on the server side rather than only on the client side (and previously only on Windows): in contrast to `is_ntfs_dotgit()`, the `is_ntfs_dotgitmodules()` function already expects the caller to split the paths by directory separators. Signed-off-by: Johannes Schindelin --- read-cache.c | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'read-cache.c') diff --git a/read-cache.c b/read-cache.c index 5b57b369e8..bde1e70c51 100644 --- a/read-cache.c +++ b/read-cache.c @@ -874,7 +874,15 @@ inside: if ((c == '.' && !verify_dotfile(path, mode)) || is_dir_sep(c) || c == '\0') return 0; + } else if (c == '\\' && protect_ntfs) { + if (is_ntfs_dotgit(path)) + return 0; + if (S_ISLNK(mode)) { + if (is_ntfs_dotgitmodules(path)) + return 0; + } } + c = *path++; } } -- 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 --- 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(-) (limited to 'read-cache.c') diff --git a/compat/mingw.c b/compat/mingw.c index 8b6fa0db44..17b4da16e8 100644 --- 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 e03aecfe2e..8c49c1d09b 100644 --- a/compat/mingw.h +++ b/compat/mingw.h @@ -428,6 +428,17 @@ int mingw_offset_1st_component(const char *path); #include #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 6cb3c2f19e..e587ac4e23 100644 --- 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 bde1e70c51..771171c402 100644 --- 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 16d8e689c8..8b3ce07860 100644 --- 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 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 diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh index 7c65e7a35c..5141ff45c3 100755 --- 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 638293f0da..fad9e20dc4 100755 --- 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.new && + tree=$(git -C super mktree err && + test_i18ngrep "sub " err +' + test_done -- cgit v1.2.3