submodule: also validate Windows-separated paths for validity

Otherwise we would also admit `..\..\foo\bar` as a valid path and fail to
protect Windows users.

Ideally we would check for both separators without the need for the copied
string, but this'll get us over the RCE.

src/submodule.c

@@ -175,7 +175,7 @@ static int load_submodule_names(git_strmap *out, git_repository *repo, git_confi
 	git_config_iterator *iter;
 	git_config_entry *entry;
 	git_buf buf = GIT_BUF_INIT;
-	int rval;
+	int rval, isvalid;
 	int error = 0;
 
 	if ((error = git_config_iterator_glob_new(&iter, cfg, key)) < 0)
@@ -187,7 +187,10 @@ static int load_submodule_names(git_strmap *out, git_repository *repo, git_confi
 		ldot = strrchr(entry->name, '.');
 
 		git_buf_put(&buf, fdot + 1, ldot - fdot - 1);
-		if (!git_submodule_name_is_valid(repo, buf.ptr, 0))
+		isvalid = git_submodule_name_is_valid(repo, buf.ptr, 0);
+		if (isvalid < 0)
+			return isvalid;
+		if (!isvalid)
 			continue;
 
 		git_strmap_insert(out, entry->value, git_buf_detach(&buf), &rval);
@@ -314,11 +317,25 @@ int git_submodule_lookup(
 
 int git_submodule_name_is_valid(const git_repository *repo, const char *name, int flags)
 {
+	git_buf buf = GIT_BUF_INIT;
+	int error, isvalid;
+
 	if (flags == 0)
 		flags = GIT_PATH_REJECT_FILESYSTEM_DEFAULTS;
 
+	/* Avoid allocating a new string if we can avoid it */
+	if (index(name, '\\')) {
+		if ((error = git_path_normalize_slashes(&buf, name)) < 0)
+			return error;
+	} else {
+		git_buf_attach_notowned(&buf, name, strlen(name));
+	}
+
 	/* FIXME: Un-consting it to reduce the amount of diff */
-	return git_path_isvalid((git_repository *)repo, name, flags);
+	isvalid =  git_path_isvalid((git_repository *)repo, buf.ptr, flags);
+	git_buf_free(&buf);
+
+	return isvalid;
 }
 
 static void submodule_free_dup(void *sm)
@@ -1504,16 +1521,17 @@ static int submodule_update_head(git_submodule *submodule)
 
 int git_submodule_reload(git_submodule *sm, int force)
 {
-	int error = 0;
+	int error = 0, isvalid;
 	git_config *mods;
 
 	GIT_UNUSED(force);
 
 	assert(sm);
 
-	if (!git_submodule_name_is_valid(sm->repo, sm->name, 0)) {
+	isvalid = git_submodule_name_is_valid(sm->repo, sm->name, 0);
+	if (isvalid <= 0) {
 		/* This should come with a warning, but we've no API for that */
-		return 0;
+		return isvalid;
 	}
 
 	if (!git_repository_is_bare(sm->repo)) {
@@ -1856,7 +1874,7 @@ static int submodule_load_each(const git_config_entry *entry, void *payload)
 	git_strmap *map = data->map;
 	git_buf name = GIT_BUF_INIT;
 	git_submodule *sm;
-	int error;
+	int error, isvalid;
 
 	if (git__prefixcmp(entry->name, "submodule.") != 0)
 		return 0;
@@ -1872,8 +1890,9 @@ static int submodule_load_each(const git_config_entry *entry, void *payload)
 	if ((error = git_buf_set(&name, namestart, property - namestart -1)) < 0)
 		return error;
 
-	if (!git_path_isvalid(data->repo, name.ptr, GIT_PATH_REJECT_TRAVERSAL)) {
-		error = 0;
+	isvalid = git_submodule_name_is_valid(data->repo, name.ptr, 0);
+	if (isvalid <= 0) {
+		error = isvalid;
 		goto done;
 	}
 

tests/submodule/escape.c

@@ -13,6 +13,8 @@ void test_submodule_escape__cleanup(void)
 }
 
 #define EVIL_SM_NAME "../../modules/evil"
+#define EVIL_SM_NAME_WINDOWS "..\\\\..\\\\modules\\\\evil"
+#define EVIL_SM_NAME_WINDOWS_UNESC "..\\..\\modules\\evil"
 
 static int find_evil(git_submodule *sm, const char *name, void *payload)
 {
@@ -20,7 +22,8 @@ static int find_evil(git_submodule *sm, const char *name, void *payload)
 
 	GIT_UNUSED(sm);
 
-	if (!git__strcmp(EVIL_SM_NAME, name))
+	if (!git__strcmp(EVIL_SM_NAME, name) ||
+	    !git__strcmp(EVIL_SM_NAME_WINDOWS_UNESC, name))
 		*foundit = true;
 
 	return 0;
@@ -29,7 +32,6 @@ static int find_evil(git_submodule *sm, const char *name, void *payload)
 void test_submodule_escape__from_gitdir(void)
 {
 	int foundit;
-	git_config *cfg;
 	git_submodule *sm;
 	git_buf buf = GIT_BUF_INIT;
 	unsigned int sm_location;
@@ -42,10 +44,6 @@ void test_submodule_escape__from_gitdir(void)
 			   "    path = testrepo\n"
 			   "    url = ../testrepo.git\n");
 
-	/* We also need to update the value in the config */
-	cl_git_pass(git_repository_config__weakptr(&cfg, g_repo));
-	cfg = NULL;
-
 	/* Find it all the different ways we know about it */
 	foundit = 0;
 	cl_git_pass(git_submodule_foreach(g_repo, find_evil, &foundit));
@@ -63,3 +61,36 @@ void test_submodule_escape__from_gitdir(void)
 	cl_assert_equal_i(GIT_SUBMODULE_STATUS_IN_INDEX | GIT_SUBMODULE_STATUS_IN_HEAD, sm_location);
 	git_submodule_free(sm);
 }
+
+void test_submodule_escape__from_gitdir_windows(void)
+{
+	int foundit;
+	git_submodule *sm;
+	git_buf buf = GIT_BUF_INIT;
+	unsigned int sm_location;
+
+	g_repo = setup_fixture_submodule_simple();
+
+	cl_git_pass(git_buf_joinpath(&buf, git_repository_workdir(g_repo), ".gitmodules"));
+	cl_git_rewritefile(buf.ptr,
+			   "[submodule \"" EVIL_SM_NAME_WINDOWS "\"]\n"
+			   "    path = testrepo\n"
+			   "    url = ../testrepo.git\n");
+
+	/* Find it all the different ways we know about it */
+	foundit = 0;
+	cl_git_pass(git_submodule_foreach(g_repo, find_evil, &foundit));
+	cl_assert_equal_i(0, foundit);
+	cl_git_fail_with(GIT_ENOTFOUND, git_submodule_lookup(&sm, g_repo, EVIL_SM_NAME_WINDOWS_UNESC));
+	/*
+	 * We do know about this as it's in the index and HEAD, but the data is
+	 * incomplete as there is no configured data for it (we pretend it
+	 * doesn't exist). This leaves us with an odd situation but it's
+	 * consistent with what we would do if we did add a submodule with no
+	 * configuration.
+	 */
+	cl_git_pass(git_submodule_lookup(&sm, g_repo, "testrepo"));
+	cl_git_pass(git_submodule_location(&sm_location, sm));
+	cl_assert_equal_i(GIT_SUBMODULE_STATUS_IN_INDEX | GIT_SUBMODULE_STATUS_IN_HEAD, sm_location);
+	git_submodule_free(sm);
+}