From 98afac7a7cefdca0d2c4917dd8066a59f7088265 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 24 Sep 2018 04:32:15 -0400 Subject: submodule--helper: use "--" to signal end of clone options When we clone a submodule, we call "git clone $url $path". But there's nothing to say that those components can't begin with a dash themselves, confusing git-clone into thinking they're options. Let's pass "--" to make it clear what we expect. There's no test here, because it's actually quite hard to make these names work, even with "git clone" parsing them correctly. And we're going to restrict these cases even further in future commits. So we'll leave off testing until then; this is just the minimal fix to prevent us from doing something stupid with a badly formed entry. Reported-by: joernchen Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index e8ccddd..676cfed 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -510,6 +510,7 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url if (gitdir && *gitdir) argv_array_pushl(&cp.args, "--separate-git-dir", gitdir, NULL); + argv_array_push(&cp.args, "--"); argv_array_push(&cp.args, url); argv_array_push(&cp.args, path); -- cgit v0.10.2-6-g49f6 From f6adec4e329ef0e25e14c63b735a5956dc67b8bc Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 24 Sep 2018 04:36:30 -0400 Subject: submodule-config: ban submodule urls that start with dash The previous commit taught the submodule code to invoke our "git clone $url $path" with a "--" separator so that we aren't confused by urls or paths that start with dashes. However, that's just one code path. It's not clear if there are others, and it would be an easy mistake to add one in the future. Moreover, even with the fix in the previous commit, it's quite hard to actually do anything useful with such an entry. Any url starting with a dash must fall into one of three categories: - it's meant as a file url, like "-path". But then any clone is not going to have the matching path, since it's by definition relative inside the newly created clone. If you spell it as "./-path", the submodule code sees the "/" and translates this to an absolute path, so it at least works (assuming the receiver has the same filesystem layout as you). But that trick does not apply for a bare "-path". - it's meant as an ssh url, like "-host:path". But this already doesn't work, as we explicitly disallow ssh hostnames that begin with a dash (to avoid option injection against ssh). - it's a remote-helper scheme, like "-scheme::data". This _could_ work if the receiver bends over backwards and creates a funny-named helper like "git-remote--scheme". But normally there would not be any helper that matches. Since such a url does not work today and is not likely to do anything useful in the future, let's simply disallow them entirely. That protects the existing "git clone" path (in a belt-and-suspenders way), along with any others that might exist. Our tests cover two cases: 1. A file url with "./" continues to work, showing that there's an escape hatch for people with truly silly repo names. 2. A url starting with "-" is rejected. Note that we expect case (2) to fail, but it would have done so even without this commit, for the reasons given above. So instead of just expecting failure, let's also check for the magic word "ignoring" on stderr. That lets us know that we failed for the right reason. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/submodule-config.c b/submodule-config.c index acb7767..6eb13a5 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -367,6 +367,12 @@ static void warn_multiple_config(const unsigned char *treeish_name, commit_string, name, option); } +static void warn_command_line_option(const char *var, const char *value) +{ + warning(_("ignoring '%s' which may be interpreted as" + " a command-line option: %s"), var, value); +} + struct parse_config_parameter { struct submodule_cache *cache; const unsigned char *treeish_name; @@ -432,6 +438,8 @@ static int parse_config(const char *var, const char *value, void *data) } else if (!strcmp(item.buf, "url")) { if (!value) { ret = config_error_nonbool(var); + } else if (looks_like_command_line_option(value)) { + warn_command_line_option(var, value); } else if (!me->overwrite && submodule->url) { warn_multiple_config(me->treeish_name, submodule->name, "url"); diff --git a/t/t7416-submodule-dash-url.sh b/t/t7416-submodule-dash-url.sh new file mode 100755 index 0000000..459193c --- /dev/null +++ b/t/t7416-submodule-dash-url.sh @@ -0,0 +1,34 @@ +#!/bin/sh + +test_description='check handling of .gitmodule url with dash' +. ./test-lib.sh + +test_expect_success 'create submodule with protected dash in url' ' + git init upstream && + git -C upstream commit --allow-empty -m base && + mv upstream ./-upstream && + git submodule add ./-upstream sub && + git add sub .gitmodules && + git commit -m submodule +' + +test_expect_success 'clone can recurse submodule' ' + test_when_finished "rm -rf dst" && + git clone --recurse-submodules . dst && + echo base >expect && + git -C dst/sub log -1 --format=%s >actual && + test_cmp expect actual +' + +test_expect_success 'remove ./ protection from .gitmodules url' ' + perl -i -pe "s{\./}{}" .gitmodules && + git commit -am "drop protection" +' + +test_expect_success 'clone rejects unprotected dash' ' + test_when_finished "rm -rf dst" && + test_must_fail git clone --recurse-submodules . dst 2>err && + test_i18ngrep ignoring err +' + +test_done -- cgit v0.10.2-6-g49f6 From 273c61496f88c6495b886acb1041fe57965151da Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 24 Sep 2018 04:39:55 -0400 Subject: submodule-config: ban submodule paths that start with a dash We recently banned submodule urls that look like command-line options. This is the matching change to ban leading-dash paths. As with the urls, this should not break any use cases that currently work. Even with our "--" separator passed to git-clone, git-submodule.sh gets confused. Without the code portion of this patch, the clone of "-sub" added in t7417 would yield results like: /path/to/git-submodule: 410: cd: Illegal option -s /path/to/git-submodule: 417: cd: Illegal option -s /path/to/git-submodule: 410: cd: Illegal option -s /path/to/git-submodule: 417: cd: Illegal option -s Fetched in submodule path '-sub', but it did not contain b56243f8f4eb91b2f1f8109452e659f14dd3fbe4. Direct fetching of that commit failed. Moreover, naively adding such a submodule doesn't work: $ git submodule add $url -sub The following path is ignored by one of your .gitignore files: -sub even though there is no such ignore pattern (the test script hacks around this with a well-placed "git mv"). Unlike leading-dash urls, though, it's possible that such a path _could_ be useful if we eventually made it work. So this commit should be seen not as recommending a particular policy, but rather temporarily closing off a broken and possibly dangerous code-path. We may revisit this decision later. There are two minor differences to the tests in t7416 (that covered urls): 1. We don't have a "./-sub" escape hatch to make this work, since the submodule code expects to be able to match canonical index names to the path field (so you are free to add submodule config with that path, but we would never actually use it, since an index entry would never start with "./"). 2. After this patch, cloning actually succeeds. Since we ignore the submodule.*.path value, we fail to find a config stanza for our submodule at all, and simply treat it as inactive. We still check for the "ignoring" message. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/submodule-config.c b/submodule-config.c index 6eb13a5..774fb10 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -398,6 +398,8 @@ static int parse_config(const char *var, const char *value, void *data) if (!strcmp(item.buf, "path")) { if (!value) ret = config_error_nonbool(var); + else if (looks_like_command_line_option(value)) + warn_command_line_option(var, value); else if (!me->overwrite && submodule->path) warn_multiple_config(me->treeish_name, submodule->name, "path"); diff --git a/t/t7417-submodule-path-url.sh b/t/t7417-submodule-path-url.sh new file mode 100755 index 0000000..638293f --- /dev/null +++ b/t/t7417-submodule-path-url.sh @@ -0,0 +1,20 @@ +#!/bin/sh + +test_description='check handling of .gitmodule path with dash' +. ./test-lib.sh + +test_expect_success 'create submodule with dash in path' ' + git init upstream && + git -C upstream commit --allow-empty -m base && + git submodule add ./upstream sub && + git mv sub ./-sub && + git commit -m submodule +' + +test_expect_success 'clone rejects unprotected dash' ' + test_when_finished "rm -rf dst" && + git clone --recurse-submodules . dst 2>err && + test_i18ngrep ignoring err +' + +test_done -- cgit v0.10.2-6-g49f6 From d0832b2847aa9669c09397c5639d7fe56abaf9fc Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 27 Sep 2018 11:19:11 -0700 Subject: Git 2.14.5 Signed-off-by: Junio C Hamano diff --git a/Documentation/RelNotes/2.14.5.txt b/Documentation/RelNotes/2.14.5.txt new file mode 100644 index 0000000..130645f --- /dev/null +++ b/Documentation/RelNotes/2.14.5.txt @@ -0,0 +1,16 @@ +Git v2.14.5 Release Notes +========================= + +This release is to address the recently reported CVE-2018-17456. + +Fixes since v2.14.4 +------------------- + + * Submodules' "URL"s come from the untrusted .gitmodules file, but + we blindly gave it to "git clone" to clone submodules when "git + clone --recurse-submodules" was used to clone a project that has + such a submodule. The code has been hardened to reject such + malformed URLs (e.g. one that begins with a dash). + +Credit for finding and fixing this vulnerability goes to joernchen +and Jeff King, respectively. diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index 918b6c2..4068048 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.14.4 +DEF_VER=v2.14.5 LF=' ' diff --git a/RelNotes b/RelNotes index 1b1ac35..a127ce6 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.14.4.txt \ No newline at end of file +Documentation/RelNotes/2.14.5.txt \ No newline at end of file -- cgit v0.10.2-6-g49f6 From 924c623e1c71b98da608f980a97f9730c021ba44 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 27 Sep 2018 11:33:47 -0700 Subject: Git 2.15.3 Signed-off-by: Junio C Hamano diff --git a/Documentation/RelNotes/2.15.3.txt b/Documentation/RelNotes/2.15.3.txt new file mode 100644 index 0000000..fd2e6f8 --- /dev/null +++ b/Documentation/RelNotes/2.15.3.txt @@ -0,0 +1,6 @@ +Git v2.15.3 Release Notes +========================= + +This release merges up the fixes that appear in v2.14.5 to address +the recently reported CVE-2018-17456; see the release notes for that +version for details. diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index 824649f..4a63ce3 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.15.2 +DEF_VER=v2.15.3 LF=' ' diff --git a/RelNotes b/RelNotes index 6162eb4..e7fe59f 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.15.2.txt \ No newline at end of file +Documentation/RelNotes/2.15.3.txt \ No newline at end of file -- cgit v0.10.2-6-g49f6 From 27d05d1a1a62273aa3749f4d0ab8a126ef11ff66 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 27 Sep 2018 11:38:32 -0700 Subject: Git 2.16.5 Signed-off-by: Junio C Hamano diff --git a/Documentation/RelNotes/2.16.5.txt b/Documentation/RelNotes/2.16.5.txt new file mode 100644 index 0000000..cb8ee02 --- /dev/null +++ b/Documentation/RelNotes/2.16.5.txt @@ -0,0 +1,6 @@ +Git v2.16.5 Release Notes +========================= + +This release merges up the fixes that appear in v2.14.5 to address +the recently reported CVE-2018-17456; see the release notes for that +version for details. diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index f6c7db0..64f5097 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.16.4 +DEF_VER=v2.16.5 LF=' ' diff --git a/RelNotes b/RelNotes index d93c6ee..7b0f25d 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.16.4.txt \ No newline at end of file +Documentation/RelNotes/2.16.5.txt \ No newline at end of file -- cgit v0.10.2-6-g49f6 From a124133e1e6ab5c7a9fef6d0e6bcb084e3455b46 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 24 Sep 2018 04:37:17 -0400 Subject: fsck: detect submodule urls starting with dash Urls with leading dashes can cause mischief on older versions of Git. We should detect them so that they can be rejected by receive.fsckObjects, preventing modern versions of git from being a vector by which attacks can spread. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/fsck.c b/fsck.c index 9339f31..c472856 100644 --- a/fsck.c +++ b/fsck.c @@ -64,6 +64,7 @@ static struct oidset gitmodules_done = OIDSET_INIT; FUNC(GITMODULES_PARSE, ERROR) \ FUNC(GITMODULES_NAME, ERROR) \ FUNC(GITMODULES_SYMLINK, ERROR) \ + FUNC(GITMODULES_URL, ERROR) \ /* warnings */ \ FUNC(BAD_FILEMODE, WARN) \ FUNC(EMPTY_NAME, WARN) \ @@ -945,6 +946,12 @@ static int fsck_gitmodules_fn(const char *var, const char *value, void *vdata) FSCK_MSG_GITMODULES_NAME, "disallowed submodule name: %s", name); + if (!strcmp(key, "url") && value && + looks_like_command_line_option(value)) + data->ret |= report(data->options, data->obj, + FSCK_MSG_GITMODULES_URL, + "disallowed submodule url: %s", + value); free(name); return 0; diff --git a/t/t7416-submodule-dash-url.sh b/t/t7416-submodule-dash-url.sh index 459193c..1cd2c1c 100755 --- a/t/t7416-submodule-dash-url.sh +++ b/t/t7416-submodule-dash-url.sh @@ -20,6 +20,13 @@ test_expect_success 'clone can recurse submodule' ' test_cmp expect actual ' +test_expect_success 'fsck accepts protected dash' ' + test_when_finished "rm -rf dst" && + git init --bare dst && + git -C dst config transfer.fsckObjects true && + git push dst HEAD +' + test_expect_success 'remove ./ protection from .gitmodules url' ' perl -i -pe "s{\./}{}" .gitmodules && git commit -am "drop protection" @@ -31,4 +38,12 @@ test_expect_success 'clone rejects unprotected dash' ' test_i18ngrep ignoring err ' +test_expect_success 'fsck rejects unprotected dash' ' + test_when_finished "rm -rf dst" && + git init --bare dst && + git -C dst config transfer.fsckObjects true && + test_must_fail git push dst HEAD 2>err && + grep gitmodulesUrl err +' + test_done -- cgit v0.10.2-6-g49f6 From 1a7fd1fb2998002da6e9ff2ee46e1bdd25ee8404 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 24 Sep 2018 04:42:19 -0400 Subject: fsck: detect submodule paths starting with dash As with urls, submodule paths with dashes are ignored by git, but may end up confusing older versions. Detecting them via fsck lets us prevent modern versions of git from being a vector to spread broken .gitmodules to older versions. Compared to blocking leading-dash urls, though, this detection may be less of a good idea: 1. While such paths provide confusing and broken results, they don't seem to actually work as option injections against anything except "cd". In particular, the submodule code seems to canonicalize to an absolute path before running "git clone" (so it passes /your/clone/-sub). 2. It's more likely that we may one day make such names actually work correctly. Even after we revert this fsck check, it will continue to be a hassle until hosting servers are all updated. On the other hand, it's not entirely clear that the behavior in older versions is safe. And if we do want to eventually allow this, we may end up doing so with a special syntax anyway (e.g., writing "./-sub" in the .gitmodules file, and teaching the submodule code to canonicalize it when comparing). So on balance, this is probably a good protection. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/fsck.c b/fsck.c index c472856..5d9b25f 100644 --- a/fsck.c +++ b/fsck.c @@ -65,6 +65,7 @@ static struct oidset gitmodules_done = OIDSET_INIT; FUNC(GITMODULES_NAME, ERROR) \ FUNC(GITMODULES_SYMLINK, ERROR) \ FUNC(GITMODULES_URL, ERROR) \ + FUNC(GITMODULES_PATH, ERROR) \ /* warnings */ \ FUNC(BAD_FILEMODE, WARN) \ FUNC(EMPTY_NAME, WARN) \ @@ -952,6 +953,12 @@ static int fsck_gitmodules_fn(const char *var, const char *value, void *vdata) FSCK_MSG_GITMODULES_URL, "disallowed submodule url: %s", value); + if (!strcmp(key, "path") && value && + looks_like_command_line_option(value)) + data->ret |= report(data->options, data->obj, + FSCK_MSG_GITMODULES_PATH, + "disallowed submodule path: %s", + value); free(name); return 0; diff --git a/t/t7417-submodule-path-url.sh b/t/t7417-submodule-path-url.sh index 638293f..756af8c 100755 --- a/t/t7417-submodule-path-url.sh +++ b/t/t7417-submodule-path-url.sh @@ -17,4 +17,12 @@ test_expect_success 'clone rejects unprotected dash' ' test_i18ngrep ignoring err ' +test_expect_success 'fsck rejects unprotected dash' ' + test_when_finished "rm -rf dst" && + git init --bare dst && + git -C dst config transfer.fsckObjects true && + test_must_fail git push dst HEAD 2>err && + grep gitmodulesPath err +' + test_done -- cgit v0.10.2-6-g49f6 From 6e9e91e9cae74cd7feb9300563d40361b2b17dd2 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 27 Sep 2018 11:44:07 -0700 Subject: Git 2.17.2 Signed-off-by: Junio C Hamano diff --git a/Documentation/RelNotes/2.17.2.txt b/Documentation/RelNotes/2.17.2.txt new file mode 100644 index 0000000..ef021be --- /dev/null +++ b/Documentation/RelNotes/2.17.2.txt @@ -0,0 +1,12 @@ +Git v2.17.2 Release Notes +========================= + +This release merges up the fixes that appear in v2.14.5 to address +the recently reported CVE-2018-17456; see the release notes for that +version for details. + +In addition, this release also teaches "fsck" and the server side +logic to reject pushes to repositories that attempt to create such a +problematic ".gitmodules" file as tracked contents, to help hosting +sites protect their customers by preventing malicious contents from +spreading. diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index 4baa413..bc54879 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.17.1 +DEF_VER=v2.17.2 LF=' ' diff --git a/RelNotes b/RelNotes index cde891d..733d174 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.17.1.txt \ No newline at end of file +Documentation/RelNotes/2.17.2.txt \ No newline at end of file -- cgit v0.10.2-6-g49f6 From 268fbcd172cdb306e8a3e7143cc16677c963d6cd Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 27 Sep 2018 11:48:19 -0700 Subject: Git 2.18.1 Signed-off-by: Junio C Hamano diff --git a/Documentation/RelNotes/2.18.1.txt b/Documentation/RelNotes/2.18.1.txt new file mode 100644 index 0000000..2098cdd --- /dev/null +++ b/Documentation/RelNotes/2.18.1.txt @@ -0,0 +1,6 @@ +Git v2.18.1 Release Notes +========================= + +This release merges up the fixes that appear in v2.14.5 and in +v2.17.2 to address the recently reported CVE-2018-17456; see the +release notes for those versions for details. diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index ed9d9e4..5998893 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.18.0 +DEF_VER=v2.18.1 LF=' ' diff --git a/RelNotes b/RelNotes index f6c58b3..392b0b3 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.18.0.txt \ No newline at end of file +Documentation/RelNotes/2.18.1.txt \ No newline at end of file -- cgit v0.10.2-6-g49f6