From 6975f297b9d85ec520e6921cbb89fa4173833b43 Mon Sep 17 00:00:00 2001 From: Arnaud Fontaine Date: Wed, 15 Apr 2020 16:56:04 +0900 Subject: [PATCH] git-2.12.2-4.hyperbola8: fix multiple CVEs --- ...dubiously-nested-submodule-git-directories.diff | 211 +++++++++++++++ git/PKGBUILD | 56 +++- ...se-submodules-prevent-name-squatting-on-Wi.diff | 235 +++++++++++++++++ ...sck-call-fsck_finish-after-fscking-objects.diff | 48 ++++ ...ule-config-verify-submodule-names-as-paths.diff | 291 +++++++++++++++++++++ 5 files changed, 831 insertions(+), 10 deletions(-) create mode 100644 git/Disallow-dubiously-nested-submodule-git-directories.diff create mode 100644 git/clone-recurse-submodules-prevent-name-squatting-on-Wi.diff create mode 100644 git/fsck-call-fsck_finish-after-fscking-objects.diff create mode 100644 git/submodule-config-verify-submodule-names-as-paths.diff diff --git a/git/Disallow-dubiously-nested-submodule-git-directories.diff b/git/Disallow-dubiously-nested-submodule-git-directories.diff new file mode 100644 index 0000000..2a83621 --- /dev/null +++ b/git/Disallow-dubiously-nested-submodule-git-directories.diff @@ -0,0 +1,211 @@ +From 392f99a5d2174e6124f829d034bac6755c33119d Mon Sep 17 00:00:00 2001 +From: Johannes Schindelin +Date: Tue, 1 Oct 2019 23:27:18 +0200 +Subject: Disallow dubiously-nested submodule git directories + +commit a8dee3ca610f5a1d403634492136c887f83b59d2 upstream. + +Currently it is technically possible to let a submodule's git +directory point right into the git dir of a sibling submodule. + +Example: the git directories of two submodules with the names `hippo` +and `hippo/hooks` would be `.git/modules/hippo/` and +`.git/modules/hippo/hooks/`, respectively, but the latter is already +intended to house the former's hooks. + +In most cases, this is just confusing, but there is also a (quite +contrived) attack vector where Git can be fooled into mistaking remote +content for file contents it wrote itself during a recursive clone. + +Let's plug this bug. + +To do so, we introduce the new function `validate_submodule_git_dir()` +which simply verifies that no git dir exists for any leading directories +of the submodule name (if there are any). + +Note: this patch specifically continues to allow sibling modules names +of the form `core/lib`, `core/doc`, etc, as long as `core` is not a +submodule name. + +This fixes CVE-2019-1387. + +[jn: backported to 2.11.y: + - port to git-submodule.sh + - use explicit chdir to emulate test_commit -C in test] + +Reported-by: Nicolas Joly +Signed-off-by: Johannes Schindelin +Signed-off-by: Jonathan Nieder +--- + builtin/submodule--helper.c | 24 ++++++++++++++++++++++ + git-submodule.sh | 5 +++++ + submodule.c | 41 +++++++++++++++++++++++++++++++++++++ + submodule.h | 5 +++++ + t/t7415-submodule-names.sh | 23 +++++++++++++++++++++ + 5 files changed, 98 insertions(+) + +Index: git-2.12.2/builtin/submodule--helper.c +=================================================================== +--- git-2.12.2.orig/builtin/submodule--helper.c ++++ git-2.12.2/builtin/submodule--helper.c +@@ -640,6 +640,10 @@ static int module_clone(int argc, const + } 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); +@@ -1166,6 +1170,25 @@ static int check_name(int argc, const ch + return 0; + } + ++/* ++ * Exit non-zero if the proposed submodule repository path is inside ++ * another submodules' git dir. ++ */ ++static int validate_git_dir(int argc, const char **argv, const char *prefix) ++{ ++ char *sm_gitdir; ++ ++ if (argc != 3) ++ usage("git submodule--helper validate-git-dir "); ++ sm_gitdir = xstrdup(argv[1]); ++ if (validate_submodule_git_dir(sm_gitdir, argv[2]) < 0) { ++ free(sm_gitdir); ++ return 1; ++ } ++ free(sm_gitdir); ++ return 0; ++} ++ + struct cmd_struct { + const char *cmd; + int (*fn)(int, const char **, const char *); +@@ -1184,6 +1207,7 @@ static struct cmd_struct commands[] = { + {"remote-branch", resolve_remote_submodule_branch, 0}, + {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX}, + {"check-name", check_name, 0}, ++ {"validate-git-dir", validate_git_dir, 0}, + }; + + int cmd_submodule__helper(int argc, const char **argv, const char *prefix) +Index: git-2.12.2/git-submodule.sh +=================================================================== +--- git-2.12.2.orig/git-submodule.sh ++++ git-2.12.2/git-submodule.sh +@@ -245,6 +245,11 @@ Use -f if you really want to add it." >& + fi + + else ++ sm_gitdir=".git/modules/$sm_name" ++ if ! git submodule--helper validate-git-dir "$sm_gitdir" "$sm_name" ++ then ++ die "$(eval_gettextln "refusing to create/use '\$sm_gitdir' in another submodule's git dir")" ++ fi + if test -d ".git/modules/$sm_name" + then + if test -z "$force" +Index: git-2.12.2/submodule.c +=================================================================== +--- git-2.12.2.orig/submodule.c ++++ git-2.12.2/submodule.c +@@ -1371,6 +1371,47 @@ int parallel_submodules(void) + 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)) ++ die("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; ++} ++ + void prepare_submodule_repo_env(struct argv_array *out) + { + const char * const *var; +Index: git-2.12.2/submodule.h +=================================================================== +--- git-2.12.2.orig/submodule.h ++++ git-2.12.2/submodule.h +@@ -83,6 +83,11 @@ extern void connect_work_tree_and_git_di + extern int parallel_submodules(void); + + /* ++ * 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); ++ ++/* + * Prepare the "env_array" parameter of a "struct child_process" for executing + * a submodule by clearing any repo-specific envirionment variables, but + * retaining any config in the environment. +Index: git-2.12.2/t/t7415-submodule-names.sh +=================================================================== +--- git-2.12.2.orig/t/t7415-submodule-names.sh ++++ git-2.12.2/t/t7415-submodule-names.sh +@@ -183,4 +183,27 @@ test_expect_success MINGW 'prevent git~1 + ! grep gitdir squatting-clone/d/a/git~2 + ' + ++test_expect_success 'git dirs of sibling submodules must not be nested' ' ++ git init nested && ++ ( ++ cd nested && ++ test_commit 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/git/PKGBUILD b/git/PKGBUILD index bfc2439..b75c969 100644 --- a/git/PKGBUILD +++ b/git/PKGBUILD @@ -5,9 +5,9 @@ pkgname=git pkgver=2.12.2 -_debver=2.12.2+next.20170416 -_debrel=1 -pkgrel=4.hyperbola7 +_debver=2.11.0 +_debrel=3+deb9u6 +pkgrel=4.hyperbola8 pkgdesc="the fast distributed version control system, with logger recommendation and with libressl and OpenRC support" arch=(i686 x86_64) url="https://git-scm.com/" @@ -31,14 +31,22 @@ backup=('etc/conf.d/git-daemon') install=git.install source=("https://www.kernel.org/pub/software/scm/$pkgname/$pkgname-$pkgver.tar.xz" "https://www.kernel.org/pub/software/scm/$pkgname/$pkgname-$pkgver.tar.sign" - "https://deb.debian.org/debian/pool/main/g/$pkgname/${pkgname}_$_debver-$_debrel.debian.tar.xz" + "http://security.debian.org/debian-security/pool/updates/main/g/git/${pkgname}_$_debver-$_debrel.debian.tar.xz" 'git-daemon.confd' - 'git-daemon.initd') + 'git-daemon.initd' + 'clone-recurse-submodules-prevent-name-squatting-on-Wi.diff' + 'Disallow-dubiously-nested-submodule-git-directories.diff' + 'fsck-call-fsck_finish-after-fscking-objects.diff' + 'submodule-config-verify-submodule-names-as-paths.diff') sha512sums=('0e980ea6932b43c87892c0628d23bce62dc8fa8be3510e6e8bbd14605526e3b13c8a32ea9fbe8a74c286c4fe0e1fa34e85922edf743a9405a31d5657804b4b8d' 'SKIP' - '709350d7e56b1dc5782e9d41913eb06fb2c5290b0d03b9d0c34c9d21b299f798f4391a04767f43ac473263f4180d38e28b5b4bb56fbe77c431308f2713e5c610' + '28540ad748d11a0592689691d8f49e984c50aefa81aaf48963e5c3c69905516f5d71af30571340230f578c599c21b87718b7c7454bcf701ece7ba3e7541ab142' 'a34720bd28f238a17e8697b3a8073d66f6b435163d8029c0a30d9c166eb07482eecb185c6661a3826b1dc18299d905e0fd4a664ca031bd4bd3507520787fa7d7' - '6fcdefec671ca9b0191e32902663a3400b76cdec82500bdcf65dec1bdecb78a40678c80c7f77b0789a2877284db3400cdf52289c2221f8daa7e3fa7620ec1bfb') + '6fcdefec671ca9b0191e32902663a3400b76cdec82500bdcf65dec1bdecb78a40678c80c7f77b0789a2877284db3400cdf52289c2221f8daa7e3fa7620ec1bfb' + 'be8ac1ebb5d431efc8019b67d327163e0801641edf48bc0575e550ef19664dd6e5ee343af478341359adf2db80f5b41b4c54b7f45265c4272e0d6ddb5d656b61' + '74d00589446f8b830535a2f3435895d32f10c3c7700287d9d273fdf1d4bdeb44b6c44abe9542394ca06d9ab3501eae4fe71fc25261c838f4640cfdf2458071b4' + 'f458e43ef6215a4274b1f9bb44c7351920f4187e153e8defbda8fbfcf92523d095e11b44eb4ef8b6bbc11195d49b695c04bfc66768ccfdc92478c6d2f4e8692a' + 'f9bfd8d6101a0d72c87e0ecbe43059c49e9b6a99b273d8dc31f82847e781963296cf4e1dc96b26c84a5d924105e8485bc93fc40c433844c0d265dd16747f9495') validpgpkeys=('96E07AF25771955980DAD10020D04E5A713660A7') # Junio C Hamano prepare() { @@ -52,9 +60,37 @@ prepare() { mv "$srcdir"/debian . - # Doesn't apply and seems unimportant - rm -v debian/patches/0002-record-version.diff || true - rm -v debian/patches/Normalize-generated-asciidoc-timestamps-with-SOURCE_D.diff || true + ## Doesn't apply + # Patches from Debian 2.11.0, updated for 2.12.2 + cp -v "${srcdir}/submodule-config-verify-submodule-names-as-paths.diff" debian/patches/ + cp -v "${srcdir}/fsck-call-fsck_finish-after-fscking-objects.diff" debian/patches/ + cp -v "${srcdir}/clone-recurse-submodules-prevent-name-squatting-on-Wi.diff" debian/patches/ + cp -v "${srcdir}/Disallow-dubiously-nested-submodule-git-directories.diff" debian/patches/ + # Reproducible build + rm -v debian/patches/xdiff-Do-not-enable-XDL_FAST_HASH-by-default.diff || true + # Already applied in 2.12 + rm -v debian/patches/sha1_file-add-read_loose_object-function.diff || true + rm -v debian/patches/fsck-parse-loose-object-paths-directly.diff || true + rm -v debian/patches/doc-mention-transfer-data-leaks-in-more-places.diff || true + rm -v debian/patches/remote-curl-don-t-hang-when-a-server-dies-before-any-.diff || true + rm -v debian/patches/merge-recursive-handle-NULL-in-add_cacheinfo-correctl.diff || true + rm -v debian/patches/http-always-update-the-base-URL-for-redirects.diff || true + rm -v debian/patches/remote-curl-rename-shadowed-options-variable.diff || true + rm -v debian/patches/http-make-redirects-more-obvious.diff || true + rm -v debian/patches/http-treat-http-alternates-like-redirects.diff || true + rm -v debian/patches/shallow.c-rename-fields-in-paint_info-to-better-expre.diff || true + rm -v debian/patches/shallow.c-stop-abusing-COMMIT_SLAB_SIZE-for-paint_inf.diff || true + rm -v debian/patches/shallow.c-make-paint_alloc-slightly-more-robust.diff || true + rm -v debian/patches/shallow.c-avoid-theoretical-pointer-wrap-around.diff || true + rm -v debian/patches/shallow.c-bit-manipulation-tweaks.diff || true + rm -v debian/patches/git-svn-allow-0-in-SVN-path-components.diff || true + rm -v debian/patches/config.c-handle-error-case-for-fstat-calls.diff || true + rm -v debian/patches/git_exec_path-avoid-Coverity-warning-about-unfree-d-r.diff || true + rm -v debian/patches/git_exec_path-do-not-return-the-result-of-getenv.diff || true + rm -v debian/patches/show-branch-drop-head_len-variable.diff || true + rm -v debian/patches/show-branch-store-resolved-head-in-heap-buffer.diff || true + rm -v debian/patches/line-log-use-COPY_ARRAY-to-fix-mis-sized-memcpy.diff || true + rm -v debian/patches/http-walker-fix-buffer-underflow-processing-remote-al.diff || true quilt push -av fi diff --git a/git/clone-recurse-submodules-prevent-name-squatting-on-Wi.diff b/git/clone-recurse-submodules-prevent-name-squatting-on-Wi.diff new file mode 100644 index 0000000..f778d4d --- /dev/null +++ b/git/clone-recurse-submodules-prevent-name-squatting-on-Wi.diff @@ -0,0 +1,235 @@ +From f4e9ab82ab3230776d92b9dd7a007fe66929d21a Mon Sep 17 00:00:00 2001 +From: Johannes Schindelin +Date: Thu, 12 Sep 2019 14:20:39 +0200 +Subject: clone --recurse-submodules: prevent name squatting on Windows + +commit 0060fd1511b94c918928fa3708f69a3f33895a4a upstream. + +In addition to preventing `.git` from being tracked by Git, on Windows +we also have to prevent `git~1` from being tracked, as the default NTFS +short name (also known as the "8.3 filename") for the file name `.git` +is `git~1`, otherwise it would be possible for malicious repositories to +write directly into the `.git/` directory, e.g. a `post-checkout` hook +that would then be executed _during_ a recursive clone. + +When we implemented appropriate protections in 2b4c6efc821 (read-cache: +optionally disallow NTFS .git variants, 2014-12-16), we had analyzed +carefully that the `.git` directory or file would be guaranteed to be +the first directory entry to be written. Otherwise it would be possible +e.g. for a file named `..git` to be assigned the short name `git~1` and +subsequently, the short name generated for `.git` would be `git~2`. Or +`git~3`. Or even `~9999999` (for a detailed explanation of the lengths +we have to go to protect `.gitmodules`, see the commit message of +e7cb0b4455c (is_ntfs_dotgit: match other .git files, 2018-05-11)). + +However, by exploiting two issues (that will be addressed in a related +patch series close by), it is currently possible to clone a submodule +into a non-empty directory: + +- On Windows, file names cannot end in a space or a period (for + historical reasons: the period separating the base name from the file + extension was not actually written to disk, and the base name/file + extension was space-padded to the full 8/3 characters, respectively). + Helpfully, when creating a directory under the name, say, `sub.`, that + trailing period is trimmed automatically and the actual name on disk + is `sub`. + + This means that while Git thinks that the submodule names `sub` and + `sub.` are different, they both access `.git/modules/sub/`. + +- While the backslash character is a valid file name character on Linux, + it is not so on Windows. As Git tries to be cross-platform, it + therefore allows backslash characters in the file names stored in tree + objects. + + Which means that it is totally possible that a submodule `c` sits next + to a file `c\..git`, and on Windows, during recursive clone a file + called `..git` will be written into `c/`, of course _before_ the + submodule is cloned. + +Note that the actual exploit is not quite as simple as having a +submodule `c` next to a file `c\..git`, as we have to make sure that the +directory `.git/modules/b` already exists when the submodule is checked +out, otherwise a different code path is taken in `module_clone()` that +does _not_ allow a non-empty submodule directory to exist already. + +Even if we will address both issues nearby (the next commit will +disallow backslash characters in tree entries' file names on Windows, +and another patch will disallow creating directories/files with trailing +spaces or periods), it is a wise idea to defend in depth against this +sort of attack vector: when submodules are cloned recursively, we now +_require_ the directory to be empty, addressing CVE-2019-1349. + +Note: the code path we patch is shared with the code path of `git +submodule update --init`, which must not expect, in general, that the +directory is empty. Hence we have to introduce the new option +`--force-init` and hand it all the way down from `git submodule` to the +actual `git submodule--helper` process that performs the initial clone. + +Reported-by: Nicolas Joly +Signed-off-by: Johannes Schindelin +Signed-off-by: Jonathan Nieder +--- + builtin/clone.c | 2 +- + builtin/submodule--helper.c | 13 ++++++++++++- + git-submodule.sh | 6 ++++++ + t/t7415-submodule-names.sh | 31 +++++++++++++++++++++++++++++++ + 4 files changed, 50 insertions(+), 2 deletions(-) + +Index: git-2.12.2/builtin/clone.c +=================================================================== +--- git-2.12.2.orig/builtin/clone.c ++++ git-2.12.2/builtin/clone.c +@@ -735,7 +735,7 @@ static int checkout(int submodule_progre + + if (!err && option_recursive) { + struct argv_array args = ARGV_ARRAY_INIT; +- argv_array_pushl(&args, "submodule", "update", "--init", "--recursive", NULL); ++ argv_array_pushl(&args, "submodule", "update", "--require-init", "--recursive", NULL); + + if (option_shallow_submodules == 1) + argv_array_push(&args, "--depth=1"); +Index: git-2.12.2/builtin/submodule--helper.c +=================================================================== +--- git-2.12.2.orig/builtin/submodule--helper.c ++++ git-2.12.2/builtin/submodule--helper.c +@@ -12,6 +12,7 @@ + #include "remote.h" + #include "refs.h" + #include "connect.h" ++#include "dir.h" + + static char *get_default_remote(void) + { +@@ -586,6 +587,7 @@ static int module_clone(int argc, const + struct strbuf sb = STRBUF_INIT; + struct string_list reference = STRING_LIST_INIT_NODUP; + char *sm_alternate = NULL, *error_strategy = NULL; ++ int require_init = 0; + + struct option module_clone_options[] = { + OPT_STRING(0, "prefix", &prefix, +@@ -609,6 +611,8 @@ static int module_clone(int argc, const + OPT__QUIET(&quiet, "Suppress output for cloning a submodule"), + OPT_BOOL(0, "progress", &progress, + N_("force cloning progress")), ++ OPT_BOOL(0, "require-init", &require_init, ++ N_("disallow cloning into non-empty directory")), + OPT_END() + }; + +@@ -647,6 +651,8 @@ static int module_clone(int argc, const + die(_("clone of '%s' into submodule path '%s' failed"), + url, path); + } else { ++ if (require_init && !access(path, X_OK) && !is_empty_dir(path)) ++ die(_("directory not empty: '%s'"), path); + if (safe_create_leading_directories_const(path) < 0) + die(_("could not create directory '%s'"), path); + strbuf_addf(&sb, "%s/index", sm_gitdir); +@@ -711,6 +717,7 @@ struct submodule_update_clone { + int quiet; + int recommend_shallow; + struct string_list references; ++ unsigned require_init; + const char *depth; + const char *recursive_prefix; + const char *prefix; +@@ -726,7 +733,7 @@ struct submodule_update_clone { + int failed_clones_nr, failed_clones_alloc; + }; + #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \ +- SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, \ ++ SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \ + NULL, NULL, NULL, \ + STRING_LIST_INIT_DUP, 0, NULL, 0, 0} + +@@ -836,6 +843,8 @@ static int prepare_to_clone_next_submodu + argv_array_pushl(&child->args, "--prefix", suc->prefix, NULL); + if (suc->recommend_shallow && sub->recommend_shallow == 1) + argv_array_push(&child->args, "--depth=1"); ++ if (suc->require_init) ++ argv_array_push(&child->args, "--require-init"); + argv_array_pushl(&child->args, "--path", sub->path, NULL); + argv_array_pushl(&child->args, "--name", sub->name, NULL); + argv_array_pushl(&child->args, "--url", url, NULL); +@@ -979,6 +988,8 @@ static int update_clone(int argc, const + OPT__QUIET(&suc.quiet, N_("don't print cloning progress")), + OPT_BOOL(0, "progress", &suc.progress, + N_("force cloning progress")), ++ OPT_BOOL(0, "require-init", &suc.require_init, ++ N_("disallow cloning into non-empty directory")), + OPT_END() + }; + +Index: git-2.12.2/git-submodule.sh +=================================================================== +--- git-2.12.2.orig/git-submodule.sh ++++ git-2.12.2/git-submodule.sh +@@ -34,6 +34,7 @@ reference= + cached= + recursive= + init= ++require_init= + files= + remote= + nofetch= +@@ -513,6 +514,10 @@ cmd_update() + -i|--init) + init=1 + ;; ++ --require-init) ++ init=1 ++ require_init=1 ++ ;; + --remote) + remote=1 + ;; +@@ -591,6 +596,7 @@ cmd_update() + ${update:+--update "$update"} \ + ${reference:+"$reference"} \ + ${depth:+--depth "$depth"} \ ++ ${require_init:+--require-init} \ + ${recommend_shallow:+"$recommend_shallow"} \ + ${jobs:+$jobs} \ + "$@" || echo "#unmatched" $? +Index: git-2.12.2/t/t7415-submodule-names.sh +=================================================================== +--- git-2.12.2.orig/t/t7415-submodule-names.sh ++++ git-2.12.2/t/t7415-submodule-names.sh +@@ -152,4 +152,35 @@ test_expect_success 'fsck detects symlin + ) + ' + ++test_expect_success MINGW 'prevent git~1 squatting on Windows' ' ++ git init squatting && ++ ( ++ cd squatting && ++ mkdir a && ++ touch a/..git && ++ git add a/..git && ++ test_tick && ++ git commit -m initial && ++ ++ modules="$(test_write_lines \ ++ "[submodule \"b.\"]" "url = ." "path = c" \ ++ "[submodule \"b\"]" "url = ." "path = d\\\\a" | ++ git hash-object -w --stdin)" && ++ rev="$(git rev-parse --verify HEAD)" && ++ hash="$(echo x | git hash-object -w --stdin)" && ++ git update-index --add \ ++ --cacheinfo 100644,$modules,.gitmodules \ ++ --cacheinfo 160000,$rev,c \ ++ --cacheinfo 160000,$rev,d\\a \ ++ --cacheinfo 100644,$hash,d./a/x \ ++ --cacheinfo 100644,$hash,d./a/..git && ++ test_tick && ++ git commit -m "module" ++ ) && ++ test_must_fail git \ ++ clone --recurse-submodules squatting squatting-clone 2>err && ++ test_i18ngrep "directory not empty" err && ++ ! grep gitdir squatting-clone/d/a/git~2 ++' ++ + test_done diff --git a/git/fsck-call-fsck_finish-after-fscking-objects.diff b/git/fsck-call-fsck_finish-after-fscking-objects.diff new file mode 100644 index 0000000..d990bdc --- /dev/null +++ b/git/fsck-call-fsck_finish-after-fscking-objects.diff @@ -0,0 +1,48 @@ +From dbac7616fb804a0f9a52d28281167d372794cbe1 Mon Sep 17 00:00:00 2001 +From: Jeff King +Date: Wed, 2 May 2018 17:20:35 -0400 +Subject: fsck: call fsck_finish() after fscking objects + +commit 1995b5e03e1cc97116be58cdc0502d4a23547856 upstream. + +Now that the internal fsck code is capable of checking +.gitmodules files, we just need to teach its callers to use +the "finish" function to check any queued objects. + +With this, we can now catch the malicious case in t7415 with +git-fsck. + +Signed-off-by: Jeff King +Signed-off-by: Jonathan Nieder +--- + builtin/fsck.c | 3 +++ + t/t7415-submodule-names.sh | 4 ++++ + 2 files changed, 7 insertions(+) + +Index: git-2.12.2/builtin/fsck.c +=================================================================== +--- git-2.12.2.orig/builtin/fsck.c ++++ git-2.12.2/builtin/fsck.c +@@ -731,6 +731,9 @@ int cmd_fsck(int argc, const char **argv + count += p->num_objects; + } + stop_progress(&progress); ++ ++ if (fsck_finish(&fsck_obj_options)) ++ errors_found |= ERROR_OBJECT; + } + } + +Index: git-2.12.2/t/t7415-submodule-names.sh +=================================================================== +--- git-2.12.2.orig/t/t7415-submodule-names.sh ++++ git-2.12.2/t/t7415-submodule-names.sh +@@ -74,4 +74,8 @@ test_expect_success 'clone evil superpro + ! grep "RUNNING POST CHECKOUT" output + ' + ++test_expect_success 'fsck detects evil superproject' ' ++ test_must_fail git fsck ++' ++ + test_done diff --git a/git/submodule-config-verify-submodule-names-as-paths.diff b/git/submodule-config-verify-submodule-names-as-paths.diff new file mode 100644 index 0000000..0f1e078 --- /dev/null +++ b/git/submodule-config-verify-submodule-names-as-paths.diff @@ -0,0 +1,291 @@ +From 34bfead2ba4d31eab35b551023fe9fa0b8a80459 Mon Sep 17 00:00:00 2001 +From: Jeff King +Date: Mon, 30 Apr 2018 03:25:25 -0400 +Subject: submodule-config: verify submodule names as paths + +commit 0383bbb9015898cbc79abd7b64316484d7713b44 upstream. + +Submodule "names" come from the untrusted .gitmodules file, +but we blindly append them to $GIT_DIR/modules to create our +on-disk repo paths. This means you can do bad things by +putting "../" into the name (among other things). + +Let's sanity-check these names to avoid building a path that +can be exploited. There are two main decisions: + + 1. What should the allowed syntax be? + + It's tempting to reuse verify_path(), since submodule + names typically come from in-repo paths. But there are + two reasons not to: + + a. It's technically more strict than what we need, as + we really care only about breaking out of the + $GIT_DIR/modules/ hierarchy. E.g., having a + submodule named "foo/.git" isn't actually + dangerous, and it's possible that somebody has + manually given such a funny name. + + b. Since we'll eventually use this checking logic in + fsck to prevent downstream repositories, it should + be consistent across platforms. Because + verify_path() relies on is_dir_sep(), it wouldn't + block "foo\..\bar" on a non-Windows machine. + + 2. Where should we enforce it? These days most of the + .gitmodules reads go through submodule-config.c, so + I've put it there in the reading step. That should + cover all of the C code. + + We also construct the name for "git submodule add" + inside the git-submodule.sh script. This is probably + not a big deal for security since the name is coming + from the user anyway, but it would be polite to remind + them if the name they pick is invalid (and we need to + expose the name-checker to the shell anyway for our + test scripts). + + This patch issues a warning when reading .gitmodules + and just ignores the related config entry completely. + This will generally end up producing a sensible error, + as it works the same as a .gitmodules file which is + missing a submodule entry (so "submodule update" will + barf, but "git clone --recurse-submodules" will print + an error but not abort the clone. + + There is one minor oddity, which is that we print the + warning once per malformed config key (since that's how + the config subsystem gives us the entries). So in the + new test, for example, the user would see three + warnings. That's OK, since the intent is that this case + should never come up outside of malicious repositories + (and then it might even benefit the user to see the + message multiple times). + +Credit for finding this vulnerability and the proof of +concept from which the test script was adapted goes to +Etienne Stalmans. + +[jn: the original patch expects 'git clone' to succeed in + the test because v2.13.0-rc0~10^2~3 (clone: teach + --recurse-submodules to optionally take a pathspec, + 2017-03-17) makes 'git clone' skip invalid submodules. + Updated the test to pass in older Git versions where the + submodule name check makes 'git clone' fail.] + +Signed-off-by: Jeff King +Signed-off-by: Jonathan Nieder +--- + builtin/submodule--helper.c | 26 ++++++++++++- + git-submodule.sh | 5 +++ + submodule-config.c | 31 +++++++++++++++ + submodule-config.h | 7 ++++ + t/t7415-submodule-names.sh | 77 +++++++++++++++++++++++++++++++++++++ + 5 files changed, 145 insertions(+), 1 deletion(-) + create mode 100755 t/t7415-submodule-names.sh + +Index: git-2.12.2/builtin/submodule--helper.c +=================================================================== +--- git-2.12.2.orig/builtin/submodule--helper.c ++++ git-2.12.2/builtin/submodule--helper.c +@@ -1131,6 +1131,29 @@ static int absorb_git_dirs(int argc, con + + #define SUPPORT_SUPER_PREFIX (1<<0) + ++/* ++ * 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; ++} ++ + struct cmd_struct { + const char *cmd; + int (*fn)(int, const char **, const char *); +@@ -1148,6 +1171,7 @@ static struct cmd_struct commands[] = { + {"init", module_init, SUPPORT_SUPER_PREFIX}, + {"remote-branch", resolve_remote_submodule_branch, 0}, + {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX}, ++ {"check-name", check_name, 0}, + }; + + int cmd_submodule__helper(int argc, const char **argv, const char *prefix) +Index: git-2.12.2/git-submodule.sh +=================================================================== +--- git-2.12.2.orig/git-submodule.sh ++++ git-2.12.2/git-submodule.sh +@@ -228,6 +228,11 @@ Use -f if you really want to add it." >& + 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 +Index: git-2.12.2/submodule-config.c +=================================================================== +--- git-2.12.2.orig/submodule-config.c ++++ git-2.12.2/submodule-config.c +@@ -163,6 +163,31 @@ static struct submodule *cache_lookup_na + 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 + 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; +Index: git-2.12.2/submodule-config.h +=================================================================== +--- git-2.12.2.orig/submodule-config.h ++++ git-2.12.2/submodule-config.h +@@ -34,4 +34,11 @@ extern int gitmodule_sha1_from_commit(co + struct strbuf *rev); + 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 */ +Index: git-2.12.2/t/t7415-submodule-names.sh +=================================================================== +--- /dev/null ++++ git-2.12.2/t/t7415-submodule-names.sh +@@ -0,0 +1,77 @@ ++#!/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' ' ++ test_might_fail git clone --recurse-submodules . victim >output 2>&1 && ++ cat output && ++ ! grep "RUNNING POST CHECKOUT" output ++' ++ ++test_done -- 2.12.2