From 0060fd1511b94c918928fa3708f69a3f33895a4a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 12 Sep 2019 14:20:39 +0200 Subject: [PATCH 1/2] clone --recurse-submodules: prevent name squatting on Windows 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 --- 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(-) diff --git a/builtin/clone.c b/builtin/clone.c index f7e17d2295..44b716923f 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -757,7 +757,7 @@ static int checkout(int submodule_progress) if (!err && (option_recurse_submodules.nr > 0)) { 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"); diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 676cfed770..79156fac45 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -13,6 +13,7 @@ #include "remote.h" #include "refs.h" #include "connect.h" +#include "dir.h" static char *get_default_remote(void) { @@ -623,6 +624,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) char *p, *path = NULL, *sm_gitdir; struct strbuf sb = STRBUF_INIT; struct string_list reference = STRING_LIST_INIT_NODUP; + int require_init = 0; char *sm_alternate = NULL, *error_strategy = NULL; struct option module_clone_options[] = { @@ -647,6 +649,8 @@ static int module_clone(int argc, const char **argv, const char *prefix) 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() }; @@ -685,6 +689,8 @@ static int module_clone(int argc, const char **argv, const char *prefix) 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); @@ -733,6 +739,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; @@ -748,7 +755,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} @@ -850,6 +857,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, 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", sub->url, NULL); @@ -992,6 +1001,8 @@ static int update_clone(int argc, const char **argv, const char *prefix) 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() }; diff --git a/git-submodule.sh b/git-submodule.sh index 8f260fbd9c..e4843a5874 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -34,6 +34,7 @@ reference= cached= recursive= init= +require_init= files= remote= nofetch= @@ -528,6 +529,10 @@ cmd_update() -i|--init) init=1 ;; + --require-init) + init=1 + require_init=1 + ;; --remote) remote=1 ;; @@ -606,6 +611,7 @@ cmd_update() ${update:+--update "$update"} \ ${reference:+"$reference"} \ ${depth:+--depth "$depth"} \ + ${require_init:+--require-init} \ ${recommend_shallow:+"$recommend_shallow"} \ ${jobs:+$jobs} \ "$@" || echo "#unmatched" $? diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh index 75fa071c6d..e1cd0a3545 100755 --- a/t/t7415-submodule-names.sh +++ b/t/t7415-submodule-names.sh @@ -73,4 +73,35 @@ test_expect_success 'clone evil superproject' ' ! grep "RUNNING POST CHECKOUT" output ' +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 From e1d911dd4c7b76a5a8cec0f5c8de15981e34da83 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 12 Sep 2019 14:54:05 +0200 Subject: [PATCH 2/2] mingw: disallow backslash characters in tree objects' file names The backslash character is not a valid part of a file name on Windows. Hence it is dangerous to allow writing files that were unpacked from tree objects, when the stored file name contains a backslash character: it will be misinterpreted as directory separator. This not only causes ambiguity when a tree contains a blob `a\b` and a tree `a` that contains a blob `b`, but it also can be used as part of an attack vector to side-step the careful protections against writing into the `.git/` directory during a clone of a maliciously-crafted repository. Let's prevent that, addressing CVE-2019-1354. Note: we guard against backslash characters in tree objects' file names _only_ on Windows (because on other platforms, even on those where NTFS volumes can be mounted, the backslash character is _not_ a directory separator), and _only_ when `core.protectNTFS = true` (because users might need to generate tree objects for other platforms, of course without touching the worktree, e.g. using `git update-index --cacheinfo`). Signed-off-by: Johannes Schindelin --- t/t1450-fsck.sh | 1 + t/t7415-submodule-names.sh | 8 +++++--- t/t9350-fast-export.sh | 1 + tree-walk.c | 6 ++++++ 4 files changed, 13 insertions(+), 3 deletions(-) diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index cb4b66e29d..33c955f912 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -419,6 +419,7 @@ while read name path pretty; do ( git init $name-$type && cd $name-$type && + git config core.protectNTFS false && echo content >file && git add file && git commit -m base && diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh index e1cd0a3545..7c65e7a35c 100755 --- a/t/t7415-submodule-names.sh +++ b/t/t7415-submodule-names.sh @@ -89,16 +89,18 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' ' git hash-object -w --stdin)" && rev="$(git rev-parse --verify HEAD)" && hash="$(echo x | git hash-object -w --stdin)" && - git update-index --add \ + git -c core.protectNTFS=false 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" + git -c core.protectNTFS=false commit -m "module" && + test_must_fail git show HEAD: 2>err && + test_i18ngrep backslash err ) && - test_must_fail git \ + test_must_fail git -c core.protectNTFS=false \ clone --recurse-submodules squatting squatting-clone 2>err && test_i18ngrep "directory not empty" err && ! grep gitdir squatting-clone/d/a/git~2 diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh index 866ddf6058..e6062071e6 100755 --- a/t/t9350-fast-export.sh +++ b/t/t9350-fast-export.sh @@ -421,6 +421,7 @@ test_expect_success 'directory becomes symlink' ' test_expect_success 'fast-export quotes pathnames' ' git init crazy-paths && + test_config -C crazy-paths core.protectNTFS false && (cd crazy-paths && blob=$(echo foo | git hash-object -w --stdin) && git update-index --add \ diff --git a/tree-walk.c b/tree-walk.c index d459feda23..54ff959d7f 100644 --- a/tree-walk.c +++ b/tree-walk.c @@ -41,6 +41,12 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l strbuf_addstr(err, _("empty filename in tree entry")); return -1; } +#ifdef GIT_WINDOWS_NATIVE + if (protect_ntfs && strchr(path, '\\')) { + strbuf_addf(err, _("filename in tree entry contains backslash: '%s'"), path); + return -1; + } +#endif len = strlen(path) + 1; /* Initialize the descriptor entry */