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.
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 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169 170 171 172 173
diff --git a/src/submodule.c b/src/submodule.c
index a7f76d3..233957e 100644
--- a/src/submodule.c
+++ b/src/submodule.c
@@ -220,7 +220,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)
@@ -232,7 +232,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);
@@ -364,11 +367,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)
@@ -1571,16 +1588,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)) {
@@ -1924,7 +1942,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;
@@ -1940,8 +1958,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;
}
diff --git a/tests/submodule/escape.c b/tests/submodule/escape.c
index 576d788..0a3c4a3 100644
--- a/tests/submodule/escape.c
+++ b/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);
+}