diff options
| -rw-r--r-- | builtin/submodule--helper.c | 24 | ||||
| -rwxr-xr-x | git-submodule.sh | 5 | ||||
| -rw-r--r-- | submodule-config.c | 31 | ||||
| -rw-r--r-- | submodule-config.h | 7 | ||||
| -rwxr-xr-x | t/t7415-submodule-names.sh | 76 |
5 files changed, 143 insertions, 0 deletions
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index cbb17a9021..b4b4d29d82 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1195,6 +1195,29 @@ static int is_active(int argc, const char **argv, const char *prefix) return !is_submodule_initialized(argv[1]); } +/* + * Exit non-zero if any of the submodule names given on the command line is + * invalid. If no names are given, filter stdin to print only valid names + * (which is primarily intended for testing). + */ +static int check_name(int argc, const char **argv, const char *prefix) +{ + if (argc > 1) { + while (*++argv) { + if (check_submodule_name(*argv) < 0) + return 1; + } + } else { + struct strbuf buf = STRBUF_INIT; + while (strbuf_getline(&buf, stdin) != EOF) { + if (!check_submodule_name(buf.buf)) + printf("%s\n", buf.buf); + } + strbuf_release(&buf); + } + return 0; +} + #define SUPPORT_SUPER_PREFIX (1<<0) struct cmd_struct { @@ -1216,6 +1239,7 @@ static struct cmd_struct commands[] = { {"push-check", push_check, 0}, {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX}, {"is-active", is_active, 0}, + {"check-name", check_name, 0}, }; int cmd_submodule__helper(int argc, const char **argv, const char *prefix) diff --git a/git-submodule.sh b/git-submodule.sh index c0d0e9a4c6..92750b9e2f 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -228,6 +228,11 @@ Use -f if you really want to add it." >&2 sm_name="$sm_path" fi + if ! git submodule--helper check-name "$sm_name" + then + die "$(eval_gettext "'$sm_name' is not a valid submodule name")" + fi + # perhaps the path exists and is already a git repo, else clone it if test -e "$sm_path" then diff --git a/submodule-config.c b/submodule-config.c index 4f58491ddb..de54351c6f 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -163,6 +163,31 @@ static struct submodule *cache_lookup_name(struct submodule_cache *cache, return NULL; } +int check_submodule_name(const char *name) +{ + /* Disallow empty names */ + if (!*name) + return -1; + + /* + * Look for '..' as a path component. Check both '/' and '\\' as + * separators rather than is_dir_sep(), because we want the name rules + * to be consistent across platforms. + */ + goto in_component; /* always start inside component */ + while (*name) { + char c = *name++; + if (c == '/' || c == '\\') { +in_component: + if (name[0] == '.' && name[1] == '.' && + (!name[2] || name[2] == '/' || name[2] == '\\')) + return -1; + } + } + + return 0; +} + static int name_and_item_from_var(const char *var, struct strbuf *name, struct strbuf *item) { @@ -174,6 +199,12 @@ static int name_and_item_from_var(const char *var, struct strbuf *name, return 0; strbuf_add(name, subsection, subsection_len); + if (check_submodule_name(name->buf) < 0) { + warning(_("ignoring suspicious submodule name: %s"), name->buf); + strbuf_release(name); + return 0; + } + strbuf_addstr(item, key); return 1; diff --git a/submodule-config.h b/submodule-config.h index d434ecdb45..103cc79dd8 100644 --- a/submodule-config.h +++ b/submodule-config.h @@ -35,4 +35,11 @@ extern int gitmodule_sha1_from_commit(const unsigned char *commit_sha1, struct strbuf *rev); extern void submodule_free(void); +/* + * Returns 0 if the name is syntactically acceptable as a submodule "name" + * (e.g., that may be found in the subsection of a .gitmodules file) and -1 + * otherwise. + */ +int check_submodule_name(const char *name); + #endif /* SUBMODULE_CONFIG_H */ diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh new file mode 100755 index 0000000000..75fa071c6d --- /dev/null +++ b/t/t7415-submodule-names.sh @@ -0,0 +1,76 @@ +#!/bin/sh + +test_description='check handling of .. in submodule names + +Exercise the name-checking function on a variety of names, and then give a +real-world setup that confirms we catch this in practice. +' +. ./test-lib.sh + +test_expect_success 'check names' ' + cat >expect <<-\EOF && + valid + valid/with/paths + EOF + + git submodule--helper check-name >actual <<-\EOF && + valid + valid/with/paths + + ../foo + /../foo + ..\foo + \..\foo + foo/.. + foo/../ + foo\.. + foo\..\ + foo/../bar + EOF + + test_cmp expect actual +' + +test_expect_success 'create innocent subrepo' ' + git init innocent && + git -C innocent commit --allow-empty -m foo +' + +test_expect_success 'submodule add refuses invalid names' ' + test_must_fail \ + git submodule add --name ../../modules/evil "$PWD/innocent" evil +' + +test_expect_success 'add evil submodule' ' + git submodule add "$PWD/innocent" evil && + + mkdir modules && + cp -r .git/modules/evil modules && + write_script modules/evil/hooks/post-checkout <<-\EOF && + echo >&2 "RUNNING POST CHECKOUT" + EOF + + git config -f .gitmodules submodule.evil.update checkout && + git config -f .gitmodules --rename-section \ + submodule.evil submodule.../../modules/evil && + git add modules && + git commit -am evil +' + +# This step seems like it shouldn't be necessary, since the payload is +# contained entirely in the evil submodule. But due to the vagaries of the +# submodule code, checking out the evil module will fail unless ".git/modules" +# exists. Adding another submodule (with a name that sorts before "evil") is an +# easy way to make sure this is the case in the victim clone. +test_expect_success 'add other submodule' ' + git submodule add "$PWD/innocent" another-module && + git add another-module && + git commit -am another +' + +test_expect_success 'clone evil superproject' ' + git clone --recurse-submodules . victim >output 2>&1 && + ! grep "RUNNING POST CHECKOUT" output +' + +test_done |