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>
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 79156fa..3376b1b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -678,6 +678,10 @@
} else
path = xstrdup(path);
+ if (validate_submodule_git_dir(sm_gitdir, name) < 0)
+ die(_("refusing to create/use '%s' in another submodule's "
+ "git dir"), sm_gitdir);
+
if (!file_exists(sm_gitdir)) {
if (safe_create_leading_directories_const(sm_gitdir) < 0)
die(_("could not create directory '%s'"), sm_gitdir);
diff --git a/compat/mingw.c b/compat/mingw.c
index 17b4da1..11fb2de 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -893,7 +893,7 @@
p++;
len++;
}
- if (*p == '"')
+ if (*p == '"' || !*p)
n += count*2 + 1;
continue;
}
@@ -915,16 +915,19 @@
count++;
*d++ = *arg++;
}
- if (*arg == '"') {
+ if (*arg == '"' || !*arg) {
while (count-- > 0)
*d++ = '\\';
+ /* don't escape the surrounding end quote */
+ if (!*arg)
+ break;
*d++ = '\\';
}
}
*d++ = *arg++;
}
*d++ = '"';
- *d++ = 0;
+ *d++ = '\0';
return q;
}
diff --git a/submodule.c b/submodule.c
index 36f45f5..9abc90d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1842,6 +1842,47 @@
return parallel_jobs;
}
+int validate_submodule_git_dir(char *git_dir, const char *submodule_name)
+{
+ size_t len = strlen(git_dir), suffix_len = strlen(submodule_name);
+ char *p;
+ int ret = 0;
+
+ if (len <= suffix_len || (p = git_dir + len - suffix_len)[-1] != '/' ||
+ strcmp(p, submodule_name))
+ BUG("submodule name '%s' not a suffix of git dir '%s'",
+ submodule_name, git_dir);
+
+ /*
+ * We prevent the contents of sibling submodules' git directories to
+ * clash.
+ *
+ * Example: having a submodule named `hippo` and another one named
+ * `hippo/hooks` would result in the git directories
+ * `.git/modules/hippo/` and `.git/modules/hippo/hooks/`, respectively,
+ * but the latter directory is already designated to contain the hooks
+ * of the former.
+ */
+ for (; *p; p++) {
+ if (is_dir_sep(*p)) {
+ char c = *p;
+
+ *p = '\0';
+ if (is_git_directory(git_dir))
+ ret = -1;
+ *p = c;
+
+ if (ret < 0)
+ return error(_("submodule git dir '%s' is "
+ "inside git dir '%.*s'"),
+ git_dir,
+ (int)(p - git_dir), git_dir);
+ }
+ }
+
+ return 0;
+}
+
/*
* Embeds a single submodules git directory into the superprojects git dir,
* non recursively.
@@ -1850,7 +1891,7 @@
const char *path)
{
char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;
- const char *new_git_dir;
+ char *new_git_dir;
const struct submodule *sub;
if (submodule_uses_worktrees(path))
@@ -1868,10 +1909,14 @@
if (!sub)
die(_("could not lookup name for submodule '%s'"), path);
- new_git_dir = git_path("modules/%s", sub->name);
+ new_git_dir = git_pathdup("modules/%s", sub->name);
+ if (validate_submodule_git_dir(new_git_dir, sub->name) < 0)
+ die(_("refusing to move '%s' into an existing git dir"),
+ real_old_git_dir);
if (safe_create_leading_directories_const(new_git_dir) < 0)
die(_("could not create directory '%s'"), new_git_dir);
real_new_git_dir = real_pathdup(new_git_dir, 1);
+ free(new_git_dir);
fprintf(stderr, _("Migrating git directory of '%s%s' from\n'%s' to\n'%s'\n"),
get_super_prefix_or_empty(), path,
diff --git a/submodule.h b/submodule.h
index 3c239d1..cb1ab07b 100644
--- a/submodule.h
+++ b/submodule.h
@@ -120,6 +120,11 @@
*/
int submodule_to_gitdir(struct strbuf *buf, const char *submodule);
+/*
+ * Make sure that no submodule's git dir is nested in a sibling submodule's.
+ */
+int validate_submodule_git_dir(char *git_dir, const char *submodule_name);
+
#define SUBMODULE_MOVE_HEAD_DRY_RUN (1<<0)
#define SUBMODULE_MOVE_HEAD_FORCE (1<<1)
extern int submodule_move_head(const char *path,
diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index d24d157..b622334 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -12,8 +12,8 @@
#include "run-command.h"
#include "argv-array.h"
#include "strbuf.h"
-#include <string.h>
-#include <errno.h>
+#include "gettext.h"
+#include "parse-options.h"
static int number_callbacks;
static int parallel_next(struct child_process *cp,
@@ -49,11 +49,145 @@
return 1;
}
+static uint64_t my_random_next = 1234;
+
+static uint64_t my_random(void)
+{
+ uint64_t res = my_random_next;
+ my_random_next = my_random_next * 1103515245 + 12345;
+ return res;
+}
+
+static int quote_stress_test(int argc, const char **argv)
+{
+ /*
+ * We are running a quote-stress test.
+ * spawn a subprocess that runs quote-stress with a
+ * special option that echoes back the arguments that
+ * were passed in.
+ */
+ char special[] = ".?*\\^_\"'`{}()[]<>@~&+:;$%"; // \t\r\n\a";
+ int i, j, k, trials = 100, skip = 0, msys2 = 0;
+ struct strbuf out = STRBUF_INIT;
+ struct argv_array args = ARGV_ARRAY_INIT;
+ struct option options[] = {
+ OPT_INTEGER('n', "trials", &trials, "Number of trials"),
+ OPT_INTEGER('s', "skip", &skip, "Skip <n> trials"),
+ OPT_BOOL('m', "msys2", &msys2, "Test quoting for MSYS2's sh"),
+ OPT_END()
+ };
+ const char * const usage[] = {
+ "test-run-command quote-stress-test <options>",
+ NULL
+ };
+
+ argc = parse_options(argc, argv, NULL, options, usage, 0);
+
+ setenv("MSYS_NO_PATHCONV", "1", 0);
+
+ for (i = 0; i < trials; i++) {
+ struct child_process cp = CHILD_PROCESS_INIT;
+ size_t arg_count, arg_offset;
+ int ret = 0;
+
+ argv_array_clear(&args);
+ if (msys2)
+ argv_array_pushl(&args, "sh", "-c",
+ "printf %s\\\\0 \"$@\"", "skip", NULL);
+ else
+ argv_array_pushl(&args, "test-run-command",
+ "quote-echo", NULL);
+ arg_offset = args.argc;
+
+ if (argc > 0) {
+ trials = 1;
+ arg_count = argc;
+ for (j = 0; j < arg_count; j++)
+ argv_array_push(&args, argv[j]);
+ } else {
+ arg_count = 1 + (my_random() % 5);
+ for (j = 0; j < arg_count; j++) {
+ char buf[20];
+ size_t min_len = 1;
+ size_t arg_len = min_len +
+ (my_random() % (ARRAY_SIZE(buf) - min_len));
+
+ for (k = 0; k < arg_len; k++)
+ buf[k] = special[my_random() %
+ ARRAY_SIZE(special)];
+ buf[arg_len] = '\0';
+
+ argv_array_push(&args, buf);
+ }
+ }
+
+ if (i < skip)
+ continue;
+
+ cp.argv = args.argv;
+ strbuf_reset(&out);
+ if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0) < 0)
+ return error("Failed to spawn child process");
+
+ for (j = 0, k = 0; j < arg_count; j++) {
+ const char *arg = args.argv[j + arg_offset];
+
+ if (strcmp(arg, out.buf + k))
+ ret = error("incorrectly quoted arg: '%s', "
+ "echoed back as '%s'",
+ arg, out.buf + k);
+ k += strlen(out.buf + k) + 1;
+ }
+
+ if (k != out.len)
+ ret = error("got %d bytes, but consumed only %d",
+ (int)out.len, (int)k);
+
+ if (ret) {
+ fprintf(stderr, "Trial #%d failed. Arguments:\n", i);
+ for (j = 0; j < arg_count; j++)
+ fprintf(stderr, "arg #%d: '%s'\n",
+ (int)j, args.argv[j + arg_offset]);
+
+ strbuf_release(&out);
+ argv_array_clear(&args);
+
+ return ret;
+ }
+
+ if (i && (i % 100) == 0)
+ fprintf(stderr, "Trials completed: %d\n", (int)i);
+ }
+
+ strbuf_release(&out);
+ argv_array_clear(&args);
+
+ return 0;
+}
+
+static int quote_echo(int argc, const char **argv)
+{
+ while (argc > 1) {
+ fwrite(argv[1], strlen(argv[1]), 1, stdout);
+ fputc('\0', stdout);
+ argv++;
+ argc--;
+ }
+
+ return 0;
+}
+
int cmd_main(int argc, const char **argv)
{
struct child_process proc = CHILD_PROCESS_INIT;
int jobs;
+ if (argc >= 2 && !strcmp(argv[1], "quote-stress-test"))
+ return !!quote_stress_test(argc - 1, argv + 1);
+
+ if (argc >= 2 && !strcmp(argv[1], "quote-echo"))
+ return !!quote_echo(argc - 1, argv + 1);
+
if (argc < 3)
return 1;
proc.argv = (const char **)argv + 2;
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
index 5141ff4..0338b5c 100755
--- a/t/t7415-submodule-names.sh
+++ b/t/t7415-submodule-names.sh
@@ -106,4 +106,27 @@
! grep gitdir squatting-clone/d/a/git~2
'
+test_expect_success 'git dirs of sibling submodules must not be nested' '
+ git init nested &&
+ test_commit -C nested nested &&
+ (
+ cd nested &&
+ cat >.gitmodules <<-EOF &&
+ [submodule "hippo"]
+ url = .
+ path = thing1
+ [submodule "hippo/hooks"]
+ url = .
+ path = thing2
+ EOF
+ git clone . thing1 &&
+ git clone . thing2 &&
+ git add .gitmodules thing1 thing2 &&
+ test_tick &&
+ git commit -m nested
+ ) &&
+ test_must_fail git clone --recurse-submodules nested clone 2>err &&
+ test_i18ngrep "is inside git dir" err
+'
+
test_done
diff --git a/t/t7416-submodule-dash-url.sh b/t/t7416-submodule-dash-url.sh
index 459193c..2966e93 100755
--- a/t/t7416-submodule-dash-url.sh
+++ b/t/t7416-submodule-dash-url.sh
@@ -31,4 +31,18 @@
test_i18ngrep ignoring err
'
+test_expect_success 'trailing backslash is handled correctly' '
+ git init testmodule &&
+ test_commit -C testmodule c &&
+ git submodule add ./testmodule &&
+ : ensure that the name ends in a double backslash &&
+ sed -e "s|\\(submodule \"testmodule\\)\"|\\1\\\\\\\\\"|" \
+ -e "s|url = .*|url = \" --should-not-be-an-option\"|" \
+ <.gitmodules >.new &&
+ mv .new .gitmodules &&
+ git commit -am "Add testmodule" &&
+ test_must_fail git clone --verbose --recurse-submodules . dolly 2>err &&
+ test_i18ngrep ! "unknown option" err
+'
+
test_done
