Commit e6c757a7f134b121d1fcad3a3a693e042d41ed34
2018-04-30T13:47:15
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.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154
diff --git a/src/submodule.c b/src/submodule.c
index 3ec0307..9548134 100644
--- a/src/submodule.c
+++ b/src/submodule.c
@@ -169,7 +169,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;
@@ -187,6 +187,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");
@@ -314,6 +317,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);
@@ -359,7 +371,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)
@@ -411,7 +423,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)
@@ -1509,6 +1521,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 */
mods = gitmodules_snapshot(sm->repo);
@@ -1865,6 +1882,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
diff --git a/src/submodule.h b/src/submodule.h
index 72867a3..e188dbb 100644
--- a/src/submodule.h
+++ b/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
diff --git a/tests/submodule/escape.c b/tests/submodule/escape.c
index 18d238f..576d788 100644
--- a/tests/submodule/escape.c
+++ b/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);
}