Merge branch 'win32-filenames-cannot-have-trailing-spaces-or-periods'

On Windows, filenames cannot have trailing spaces or periods, when
opening such paths, they are stripped automatically. Read: you can open
the file `README` via the file name `README . . .`. This ambiguity can
be used in combination with other security bugs to cause e.g. remote
code execution during recursive clones. This patch series fixes that.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

11 files changed, 127 insertions, 4 deletions

diff --git a/compat/mingw.c b/compat/mingw.c
index 459ee20..11fb2de 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 ||
@@ -2109,6 +2130,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 e03aecf..8c49c1d 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -429,6 +429,17 @@ int mingw_offset_1st_component(const char *path);
 #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.
  *
  * To support repositories with legacy-encoded file names, invalid UTF-8 bytes
diff --git a/git-compat-util.h b/git-compat-util.h
index 6cb3c2f..e587ac4 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 bde1e70..771171c 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 16d8e68..8b3ce07 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 2b8589e..1171e0b 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/t6130-pathspec-noglob.sh b/t/t6130-pathspec-noglob.sh
index 6583532..4129d9f 100755
--- a/t/t6130-pathspec-noglob.sh
+++ b/t/t6130-pathspec-noglob.sh
@@ -10,6 +10,7 @@ test_expect_success 'create commits with glob characters' '
 	# the name "f*" in the worktree, because it is not allowed
 	# on Windows (the tests below do not depend on the presence
 	# of the file in the worktree)
+	git config core.protectNTFS false &&
 	git update-index --add --cacheinfo 100644 "$(git rev-parse HEAD:foo)" "f*" &&
 	test_tick &&
 	git commit -m star &&
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
index 8bd3d09..0338b5c 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 638293f..fad9e20 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 >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
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index e606207..15b167d 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -424,7 +424,7 @@ test_expect_success 'fast-export quotes pathnames' '
 	test_config -C crazy-paths core.protectNTFS false &&
 	(cd crazy-paths &&
 	 blob=$(echo foo | git hash-object -w --stdin) &&
-	 git update-index --add \
+	 git -c core.protectNTFS=false update-index --add \
 		--cacheinfo 100644 $blob "$(printf "path with\\nnewline")" \
 		--cacheinfo 100644 $blob "path with \"quote\"" \
 		--cacheinfo 100644 $blob "path with \\backslash" \
diff --git a/unpack-trees.c b/unpack-trees.c
index 862cfce..649d118 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1821,7 +1821,8 @@ static int merged_entry(const struct cache_entry *ce,
 		invalidate_ce_path(old, o);
 	}
 
-	do_add_entry(o, merge, update, CE_STAGEMASK);
+	if (do_add_entry(o, merge, update, CE_STAGEMASK) < 0)
+		return -1;
 	return 1;
 }