Backport fsck tests for CVE-2018-17456, arbitrary code execution via submodule URL and path
Apply the changes v2.19.0..v2.19.1 from upstream.
This provides defense in depth by adding a second check for anyone who
has transfer.fsckobjects enabled.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
diff --git a/debian/changelog b/debian/changelog
index 382d897..d61aaad 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -5,6 +5,8 @@
- submodule: ban submodule urls that start with a dash
- submodule: ban submodule paths that start with a dash
- submodule: use "--" to signal end of clone options
+ - fsck: detect submodule urls that start with a dash
+ - fsck: detect submodule paths that start with a dash
Thanks to joernchen of Phenoelit for discovering and reporting this
vulnerability and to Jeff King for fixing it.
diff --git a/debian/patches/fsck-detect-submodule-paths-starting-with-dash.diff b/debian/patches/fsck-detect-submodule-paths-starting-with-dash.diff
new file mode 100644
index 0000000..01c2a20
--- /dev/null
+++ b/debian/patches/fsck-detect-submodule-paths-starting-with-dash.diff
@@ -0,0 +1,89 @@
+From 374d157a8e6a539b56c93ed3d8cbcb09d99380d4 Mon Sep 17 00:00:00 2001
+From: Jeff King <peff@peff.net>
+Date: Mon, 24 Sep 2018 04:42:19 -0400
+Subject: fsck: detect submodule paths starting with dash
+
+commit 1a7fd1fb2998002da6e9ff2ee46e1bdd25ee8404 upstream.
+
+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 <peff@peff.net>
+Signed-off-by: Junio C Hamano <gitster@pobox.com>
+Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
+---
+ fsck.c | 7 +++++++
+ t/t7417-submodule-path-url.sh | 8 ++++++++
+ 2 files changed, 15 insertions(+)
+
+diff --git a/fsck.c b/fsck.c
+index eebc89df61..def446a3e3 100644
+--- a/fsck.c
++++ b/fsck.c
+@@ -94,6 +94,7 @@ static int oidhash_contains(struct hashmap *h, const struct object_id *oid)
+ 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) \
+@@ -975,6 +976,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 894ed51685..53b09e5aa4 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
+--
+2.19.0.605.g01d371f741
+
diff --git a/debian/patches/fsck-detect-submodule-urls-starting-with-dash.diff b/debian/patches/fsck-detect-submodule-urls-starting-with-dash.diff
new file mode 100644
index 0000000..ec02279
--- /dev/null
+++ b/debian/patches/fsck-detect-submodule-urls-starting-with-dash.diff
@@ -0,0 +1,79 @@
+From 7fc71c564889486b09bf88e588c88cf51be58dd1 Mon Sep 17 00:00:00 2001
+From: Jeff King <peff@peff.net>
+Date: Mon, 24 Sep 2018 04:37:17 -0400
+Subject: fsck: detect submodule urls starting with dash
+
+commit a124133e1e6ab5c7a9fef6d0e6bcb084e3455b46 upstream.
+
+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 <peff@peff.net>
+Signed-off-by: Junio C Hamano <gitster@pobox.com>
+Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
+---
+ fsck.c | 7 +++++++
+ t/t7416-submodule-dash-url.sh | 15 +++++++++++++++
+ 2 files changed, 22 insertions(+)
+
+diff --git a/fsck.c b/fsck.c
+index 921cd6a409..eebc89df61 100644
+--- a/fsck.c
++++ b/fsck.c
+@@ -93,6 +93,7 @@ static int oidhash_contains(struct hashmap *h, const struct object_id *oid)
+ 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) \
+@@ -968,6 +969,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 459193c976..1cd2c1c1ea 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
+--
+2.19.0.605.g01d371f741
+
diff --git a/debian/patches/series b/debian/patches/series
index 96fcb4a..1968fe9 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -39,3 +39,5 @@
submodule-helper-use-to-signal-end-of-clone-options.diff
submodule-config-ban-submodule-urls-that-start-with-d.diff
submodule-config-ban-submodule-paths-that-start-with-.diff
+fsck-detect-submodule-urls-starting-with-dash.diff
+fsck-detect-submodule-paths-starting-with-dash.diff
