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 diff --git a/compat/mingw.c b/compat/mingw.c index 17b4da1..3aea269 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -2134,6 +2134,8 @@ int is_valid_win32_path(const char *path) if (!protect_ntfs) return 1; + skip_dos_drive_prefix((char **)&path); + for (;;) { char c = *(path++); switch (c) { @@ -2155,6 +2157,14 @@ int is_valid_win32_path(const char *path) preceding_space_or_period = 1; i++; continue; + case ':': /* DOS drive prefix was already skipped */ + case '<': case '>': case '"': case '|': case '?': case '*': + /* illegal character */ + return 0; + default: + if (c > '\0' && c < '\x20') + /* illegal character */ + return 0; } preceding_space_or_period = 0; i++; diff --git a/compat/mingw.h b/compat/mingw.h index 8c49c1d..7482f19 100644 --- a/compat/mingw.h +++ b/compat/mingw.h @@ -431,8 +431,11 @@ int mingw_offset_1st_component(const char *path); /** * 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 `..`). + * In particular, path segments are disallowed which + * + * - end in a period or a space (except the special directories `.` and `..`). + * + * - contain any of the reserved characters, e.g. `:`, `;`, `*`, etc * * Returns 1 upon success, otherwise 0. */ diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 1171e0b..f7e2529 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 v0.10.2-6-g49f6 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 diff --git a/compat/mingw.c b/compat/mingw.c index 3aea269..27d6f4a 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1986,6 +1986,30 @@ pid_t waitpid(pid_t pid, int *status, int options) return -1; } +int mingw_has_dos_drive_prefix(const char *path) +{ + int i; + + /* + * Does it start with an ASCII letter (i.e. highest bit not set), + * followed by a colon? + */ + if (!(0x80 & (unsigned char)*path)) + return *path && path[1] == ':' ? 2 : 0; + + /* + * While drive letters must be letters of the English alphabet, it is + * possible to assign virtually _any_ Unicode character via `subst` as + * a drive letter to "virtual drives". Even `1`, or `ä`. Or fun stuff + * like this: + * + * subst ֍: %USERPROFILE%\Desktop + */ + for (i = 1; i < 4 && (0x80 & (unsigned char)path[i]); i++) + ; /* skip first UTF-8 character */ + return path[i] == ':' ? i + 1 : 0; +} + int mingw_skip_dos_drive_prefix(char **path) { int ret = has_dos_drive_prefix(*path); diff --git a/compat/mingw.h b/compat/mingw.h index 7482f19..1706466 100644 --- a/compat/mingw.h +++ b/compat/mingw.h @@ -394,8 +394,8 @@ HANDLE winansi_get_osfhandle(int fd); * git specific compatibility */ -#define has_dos_drive_prefix(path) \ - (isalpha(*(path)) && (path)[1] == ':' ? 2 : 0) +int mingw_has_dos_drive_prefix(const char *path); +#define has_dos_drive_prefix mingw_has_dos_drive_prefix int mingw_skip_dos_drive_prefix(char **path); #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix static inline int mingw_is_dir_sep(int c) diff --git a/connect.c b/connect.c index 49b28b8..a053cc2 100644 --- a/connect.c +++ b/connect.c @@ -264,7 +264,7 @@ int url_is_local_not_ssh(const char *url) const char *colon = strchr(url, ':'); const char *slash = strchr(url, '/'); return !colon || (slash && slash < colon) || - has_dos_drive_prefix(url); + (has_dos_drive_prefix(url) && is_valid_path(url)); } static const char *prot_name(enum protocol protocol) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index f7e2529..40db3e1 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 v0.10.2-6-g49f6