submodule: ignore submodules which include path traversal in their name

If the we decide that the "name" of the submodule (i.e. its path inside
`.git/modules/`) is trying to escape that directory or otherwise trick us, we
ignore the configuration for that submodule.

This leaves us with a half-configured submodule when looking it up by path, but
it's the same result as if the configuration really were missing.

The name check is potentially more strict than it needs to be, but it lets us
re-use the check we're doing for the checkout. The function that encapsulates
this logic is ready to be exported but we don't want to do that in a security
release so it remains internal for now.

Author: carlosmn

src/submodule.c

@@ -214,7 +214,7 @@ static void free_submodule_names(git_strmap *names)
  * TODO: for some use-cases, this might need case-folding on a
  * case-insensitive filesystem
  */
-static int load_submodule_names(git_strmap *out, git_config *cfg)
+static int load_submodule_names(git_strmap *out, git_repository *repo, git_config *cfg)
 {
 	const char *key = "submodule\\..*\\.path";
 	git_config_iterator *iter;
@@ -232,6 +232,9 @@ static int load_submodule_names(git_strmap *out, git_config *cfg)
 		ldot = strrchr(entry->name, '.');
 
 		git_buf_put(&buf, fdot + 1, ldot - fdot - 1);
+		if (!git_submodule_name_is_valid(repo, buf.ptr, 0))
+			continue;
+
 		git_strmap_insert(out, entry->value, git_buf_detach(&buf), &rval);
 		if (rval < 0) {
 			giterr_set(GITERR_NOMEMORY, "error inserting submodule into hash table");
@@ -359,6 +362,15 @@ int git_submodule_lookup(
 	return 0;
 }
 
+int git_submodule_name_is_valid(const git_repository *repo, const char *name, int flags)
+{
+	if (flags == 0)
+		flags = GIT_PATH_REJECT_FILESYSTEM_DEFAULTS;
+
+	/* FIXME: Un-consting it to reduce the amount of diff */
+	return git_path_isvalid((git_repository *)repo, name, flags);
+}
+
 static void submodule_free_dup(void *sm)
 {
 	git_submodule_free(sm);
@@ -404,7 +416,7 @@ static int submodules_from_index(git_strmap *map, git_index *idx, git_config *cf
 	git_strmap *names = 0;
 
 	git_strmap_alloc(&names);
-	if ((error = load_submodule_names(names, cfg)))
+	if ((error = load_submodule_names(names, git_index_owner(idx), cfg)))
 		goto done;
 
 	if ((error = git_iterator_for_index(&i, git_index_owner(idx), idx, NULL)) < 0)
@@ -456,7 +468,7 @@ static int submodules_from_head(git_strmap *map, git_tree *head, git_config *cfg
 	const git_index_entry *entry;
 	git_strmap *names = 0;
 	git_strmap_alloc(&names);
-	if ((error = load_submodule_names(names, cfg)))
+	if ((error = load_submodule_names(names, git_tree_owner(head), cfg)))
 		goto done;
 
 	if ((error = git_iterator_for_tree(&i, head, NULL)) < 0)
@@ -1566,6 +1578,11 @@ int git_submodule_reload(git_submodule *sm, int force)
 
 	assert(sm);
 
+	if (!git_submodule_name_is_valid(sm->repo, sm->name, 0)) {
+		/* This should come with a warning, but we've no API for that */
+		return 0;
+	}
+
 	if (!git_repository_is_bare(sm->repo)) {
 		/* refresh config data */
 		if ((error = gitmodules_snapshot(&mods, sm->repo)) < 0 && error != GIT_ENOTFOUND)
@@ -1923,6 +1940,11 @@ 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;
+		goto done;
+	}
+
 	/*
 	 * Now that we have the submodule's name, we can use that to
 	 * figure out whether it's in the map. If it's not, we create

src/submodule.h

@@ -148,4 +148,17 @@ extern int git_submodule_parse_update(
 extern int git_submodule__map(
 	git_repository *repo,
 	git_strmap *map);
+
+/**
+ * Check whether a submodule's name is valid.
+ *
+ * Check the path against the path validity rules, either the filesystem
+ * defaults (like checkout does) or whichever you want to compare against.
+ *
+ * @param repo the repository which contains the submodule
+ * @param name the name to check
+ * @param flags the `GIT_PATH` flags to use for the check (0 to use filesystem defaults)
+ */
+extern int git_submodule_name_is_valid(const git_repository *repo, const char *name, int flags);
+
 #endif

tests/submodule/escape.c

@@ -32,29 +32,34 @@ void test_submodule_escape__from_gitdir(void)
 	git_config *cfg;
 	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_pass(git_config_open_ondisk(&cfg, git_buf_cstr(&buf)));
-
-	/* We don't have a function to rename a subsection so we do it manually */
-	cl_git_pass(git_submodule_lookup(&sm, g_repo, "testrepo"));
-	cl_git_pass(git_config_set_string(cfg, "submodule." EVIL_SM_NAME ".path", git_submodule_path(sm)));
-	cl_git_pass(git_config_set_string(cfg, "submodule." EVIL_SM_NAME ".url", git_submodule_url(sm)));
-	cl_git_pass(git_config_delete_entry(cfg, "submodule.testrepo.path"));
-	cl_git_pass(git_config_delete_entry(cfg, "submodule.testrepo.url"));
-	git_config_free(cfg);
+	cl_git_rewritefile(buf.ptr,
+			   "[submodule \"" EVIL_SM_NAME "\"]\n"
+			   "    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));
-	cl_git_pass(git_config_set_string(cfg, "submodule." EVIL_SM_NAME ".url", git_submodule_url(sm)));
 	cfg = NULL;
 
 	/* Find it all the different ways we know about it */
-	cl_git_fail_with(GIT_ENOTFOUND, git_submodule_lookup(&sm, g_repo, EVIL_SM_NAME));
-	cl_git_fail_with(GIT_ENOTFOUND, git_submodule_lookup(&sm, g_repo, "testrepo"));
 	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));
+	/*
+	 * 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);
 }