{"thread":{"id":"57685","subject":"Bare repositories in the working tree are a security risk","startedAt":"2022-04-06T22:43:25Z","lastAt":"2022-05-02T19:39:15Z","messageCount":50,"participants":["Glen Choo","Johannes Schindelin","Ævar Arnfjörð Bjarmason","Derrick Stolee","Junio C Hamano","John Cai","brian m. carlson","Justin Steven","rsbecker@nexbridge.com","Taylor Blau","Emily Shaffer","Philip Oakley"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"453235","messageId":"kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57685","inReplyTo":null,"subject":"Bare repositories in the working tree are a security risk","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-06T22:43:08Z","receivedAt":"2022-04-06T22:43:25Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\nHi all,\n\nMy Google colleagues and I would like to address what we think is a security\nrisk - namely that Git repositories can contain valid bare repositories in their\nworking trees. This is based on an excellent article by Justin Steven [1] (CC-ed\nhere).\n\nBelow is a discussion of:\n\n* The risky behavior - what Git does and why it is risky\n* Possible responses to this risk\n* A proposed approach\n\nThe proposed changes are nontrivial, so I’d really appreciate any feedback here.\nUnfortunately, I will be out of the office and won’t respond to emails for the\nnext 7 days or so, but there will still be at least one Google Git team member\nkeeping tabs on the discussion :)\n\n= TL;DR\n\nGit repositories should not be allowed to contain bare repositories in their\nworking trees because:\n\n* Such bare repositories can have maliciously crafted config files that cause\n `git` to execute arbitrary code.\n* Git itself can distribute the malicious repo via `git clone`; no need to\n distribute repos out of band e.g. via tarballs [2].\n* Many `git` commands can be affected by malicious config files, and many users\n have tools that will run `git` in the current directory or the subdirectories\n of a repo. Once the malicious repo has been cloned, very little social\n engineering is needed; the user might only need to open the repo in an editor\n or `cd` into the correct subdirectory.\n\n= Background\n\n(This section is primarily a summary of [1]. I highly, highly recommend reading\nthat as it describes the issue in much more detail and is extremely readable\nregardless of Git experience.)\n\nCertain Git configuration options are particularly enticing targets for\nattackers, e.g. core.fsmonitor can execute arbitrary commands and is invoked\non many innocuous-looking `git` commands (like `git status`). This is even more\nrisky when one considers that many tools (like shell prompts and IDEs) will run\n`git` opportunistically inside a repository - so many users won't even need to\nexplicitly run `git` to be affected [3].\n\nSince config files are such an enticing target for attackers, Git intentionally\navoids distributing config files with repos - a user shouldn't be able to `git\nclone` a repo with a config file (or really, any files inside .git). However,\none can 'trick' Git into doing this by embedding a bare repository inside of\nanother, containing repository: a repository can contain subdirectories that are\nvalid bare repositories, and any `git` operations run in such a subdirectory\nwill then use the bare repository’s config file instead of the \"main\"\nrepository’s.\n\nAn attack might look like this:\n\n* Attacker creates a repository where subdirectory \"Documentation/\" is a bare\n repository i.e. it contains \"HEAD\", \"refs/\" and \"objects/\" [4]. Attacker\n also adds \"config\" with a malicious setting for core.fsmonitor.\n* Attacker convinces User to read their project's documentation by `git\n clone`-ing their repository and inspecting the \"Documentation/\" directory.\n* User cd-s into \"Documentation/\" and their shell prompt runs `git status`,\n executing the core.fsmonitor command defined by Attacker.\n\n= What can we do about it?\n\nEach subsection is an alternative and an analysis (+/- are pros/cons).\n\n== 1. Prevent users from checking out bare repos\n\nThis is similar to an existing mitigation where we prevent repository entries\nthat can be confused for \".git\" (like \".GIT\"). but it requires checking multiple\nentries instead of a single entry. I suspect that we could accomplish this in\none of two ways:\n\na. Prevent bare repos from entering the index.\nb. Prevent writing bare repos to the working tree.\n\n+ Relatively robust protection - because the malicious repo never enters the\n working tree, we even protect other tools (e.g. JGit) from doing dangerous\n things in the embedded repo (provided the checkout is done with `git`, of\n course).\n- This breaks some 'valid' workflows (e.g. someone embedding a bare repo as a\n more convenient alternative to submodules), but it seems reasonable to let\n users opt out of this behavior.\n- (1a) is difficult to do in practice because many code paths add entries to\n the index, and checking a combination of new entry and existing entries is\n much trickier than checking just the new entry.\n- (1b) might also be difficult, though not as difficult as 1a because we\n already have a complete list of entries we will write. I don’t think there\n are existing facilities that do this sort of checking of multiple entries.\n \n== 2. Detect and reject bare repos using `git fsck` and `transfer.fsckObjects`.\n\nThis entails checking for the markers of a bare repository (HEAD, refs/,\nobjects/) in tree objects. This shares a precedent with (1), since `git fsck`\nwill also detect \".GIT\".\n\n+ Most reputable hosting sites set `transfer.fsckObjects`, which allows them to\n detect and prevent this kind of transmission.\n+ Confers some protection to users even without them doing anything.\n+ Easy to visualize and to write.\n- This won’t directly protect most users because they don’t typically set\n `transfer.fsckObjects` (in fact, `transfer.fsckObjects` will render many\n projects, like linux.git, uncloneable without additional config)\n- Won’t guard against malicious/poorly configured hosts.\n\n== 3. Detect that we are in an embedded bare repo and ignore the embedded bare\n repository in favor of the containing repo.\n\nFor example, if setup.c detects that we are in a bare repo, it could then walk\nup the directory hierarchy to see if there is a containing repo that tracks the\nbare one. If so, setup.c chooses to use the containing repo instead.\n\n+ Extremely robust; this even protects against a checkout by an earlier Git\n version.\n+ Users who don’t use bare repos won’t even notice the difference.\n- The change in rules breaks some legitimate workflows e.g. a user with a repo\n at HOME and bare repos underneath.\n- Potentially very expensive for bare repo users because setup.c will likely\n walk up to the root directory; we’d be essentially *forcing* those users to\n set GIT_CEILING_DIRECTORIES.\n- Doesn’t protect users who use other tools e.g. JGit.\n \n== 4. Educate users about this risk without making code changes.\n\nSome risks fall into this category e.g. \"Avoid unarchiving repositories because\n.git/config might be poisoned.\" [2].\n\n+ We don’t break any users.\n- Breaks the trust that users have in `git clone`.\n- IMO \"Inspect every repo in its entirety before cloning it\" is too much of a\n burden to put on users.\n\n= Next steps\n\nI propose that we prevent repositories from containing bare repositories by\ndoing the following (in order):\n\n* Implement (2) by adding a new fsck message \"embeddedBareRepo\".\n * When this is done, hosting sites can hopefully use this capability to\n prevent transmission, and help us understand the prevalence of such attacks.\n* Implement (1b) by teaching unpack_trees.c to check whether the tree contains\n an entire bare repo, and die() if so. This will be guarded by a\n defaults-to-true config value.\n * This would only block a bare repo from being written in a single operation.\n It wouldn’t stop a user from writing a bare repo entry-by-entry using \"git\n checkout \", but the amount of social engineering required probably\n renders this attack infeasible.\n * As I noted earlier, I foresee some difficulty actually implementing this\n because I don’t think we have facilities for checking multiple tree entries\n at once.\n\nI am particularly interested in hearing feedback about (1b), namely:\n\n* How to actually teach unpack_trees.c to detect bare repos.\n* Whether preventing bare repos in unpack_trees.c is a good enough mitigation\n (e.g. Are there other code paths that should block bare repos? Should we be\n checking the index instead of the tree?).\n\nI have a patch that does (2); I will send that out and we can leave feedback on\nthat separately.\n\n= Demonstration\n\nThis is based on a script by Taylor Blau (thanks!). [1] also contains a\ndemonstration that runs in Docker.\n\n #!/bin/sh\n\n rm -fr malicious cloned &&git init malicious &&\n\n (\n cd malicious &&\n\n mkdir -p bare &&\n cd bare &&\n\n echo 'ref: refs/heads/main' >HEAD &&\n cat >config <<-EOF\n [core]\n repositoryformatversion = 0\n filemode = true\n bare = false\n worktree = \"worktree\"\n fsmonitor = \"echo pwned >&2; false\"\n EOF\n\n mkdir objects refs worktree &&\n touch worktree/.gitkeep &&\n\n git add . &&\n git commit -m \".gitkeep\" &&\n\n cd .. &&\n git add bare &&\n git commit -m 'initial commit'\n ) &&\n\n git clone --no-local malicious cloned &&\n cd cloned/bare &&\n git status # pwned\n\n= Footnotes\n\n[1] https://github.com/justinsteven/advisories/blob/main/2022_git_buried_bare_repos_and_fsmonitor_various_abuses.md. \n[2] Archived repositories with malicious .git directories are a known risk. See\n https://lore.kernel.org/git/20171003123239.lisk43a2goxtxkro@sigill.intra.peff.net/\n for an on-list discussion. This is also described in\n https://blog.sonarsource.com/securing-developer-tools-git-integrations\n (referenced in [1]).\n[3] We even ship such a tool - contrib/completion/git-prompt.sh. A user can pwn\n themselves with tab completion even if they don’t have a prompt configured\n[4] Any directory containing these entries will be recognized as a bare repo, so\n an attacker could add arbitrary entries to the directory to obfuscate the fact\n that it is a bare repo.\n[*] https://offensi.com/2019/12/16/4-google-cloud-shell-bugs-explained-bug-3/ is\n similar to [1], but uses hooks instead of core.fsmonitor.\n"},{"id":"453236","messageId":"20220406232231.47714-1-chooglen@google.com","threadId":"57685","inReplyTo":"kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"[PATCH] fsck: detect bare repos in trees and warn","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-06T23:22:31Z","receivedAt":"2022-04-06T23:22:45Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Git tries not to distribute configs in-repo because they are a security\nrisk. However, an attacker can do exactly this if they embed a bare\nrepo inside of another repo.\n\nTeach fsck to detect whether a tree object contains a bare repo (as\ndetermined by setup.c) and warn. This will help hosting sites detect and\nprevent transmission of such malicious repos.\n\nSee [1] for a more in-depth discussion, including future steps and\nalternatives.\n\n[1] https://lore.kernel.org/git/kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com/\n\nSigned-off-by: Glen Choo \n---\n fsck.c | 19 +++++++++++++++++++\n fsck.h | 1 +\n setup.c | 4 ++++\n t/t1450-fsck.sh | 36 ++++++++++++++++++++++++++++++++++++\n 4 files changed, 60 insertions(+)\n\ndiff --git a/fsck.c b/fsck.c\nindex 3ec500d707..11c11c348a 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -573,6 +573,9 @@ static int fsck_tree(const struct object_id *tree_oid,\n \tint has_bad_modes = 0;\n \tint has_dup_entries = 0;\n \tint not_properly_sorted = 0;\n+\tint has_head = 0;\n+\tint has_refs_entry = 0;\n+\tint has_objects_entry = 0;\n \tstruct tree_desc desc;\n \tunsigned o_mode;\n \tconst char *o_name;\n@@ -602,6 +605,12 @@ static int fsck_tree(const struct object_id *tree_oid,\n \t\thas_dotdot |= !strcmp(name, \"..\");\n \t\thas_dotgit |= is_hfs_dotgit(name) || is_ntfs_dotgit(name);\n \t\thas_zero_pad |= *(char *)desc.buffer == '0';\n+\t\thas_head |= !strcasecmp(name, \"HEAD\")\n+\t\t\t&& (S_ISLNK(mode) || S_ISREG(mode));\n+\t\thas_refs_entry |= !strcasecmp(name, \"refs\")\n+\t\t\t&& (S_ISLNK(mode) || S_ISDIR(mode));\n+\t\thas_objects_entry |= !strcasecmp(name, \"objects\")\n+\t\t\t&& (S_ISLNK(mode) || S_ISDIR(mode));\n \n \t\tif (is_hfs_dotgitmodules(name) || is_ntfs_dotgitmodules(name)) {\n \t\t\tif (!S_ISLNK(mode))\n@@ -739,6 +748,16 @@ static int fsck_tree(const struct object_id *tree_oid,\n \t\tretval += report(options, tree_oid, OBJ_TREE,\n \t\t\t\t FSCK_MSG_TREE_NOT_SORTED,\n \t\t\t\t \"not properly sorted\");\n+\t/*\n+\t * Determine if this tree looks like a bare repository according\n+\t * to the rules of setup.c. If those are changed, this should be\n+\t * changed too.\n+\t */\n+\tif (has_head && has_refs_entry && has_objects_entry)\n+\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t FSCK_MSG_EMBEDDED_BARE_REPO,\n+\t\t\t\t \"contains bare repository\");\n+\n \treturn retval;\n }\n \ndiff --git a/fsck.h b/fsck.h\nindex d07f7a2459..3f0f73b0f3 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -65,6 +65,7 @@ enum fsck_msg_type {\n \tFUNC(NULL_SHA1, WARN) \\\n \tFUNC(ZERO_PADDED_FILEMODE, WARN) \\\n \tFUNC(NUL_IN_COMMIT, WARN) \\\n+\tFUNC(EMBEDDED_BARE_REPO, WARN) \\\n \t/* infos (reported as warnings, but ignored by default) */ \\\n \tFUNC(GITMODULES_PARSE, INFO) \\\n \tFUNC(GITIGNORE_SYMLINK, INFO) \\\ndiff --git a/setup.c b/setup.c\nindex 04ce33cdcd..2600548776 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -336,6 +336,10 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n * - either a HEAD symlink or a HEAD file that is formatted as\n * a proper \"ref:\", or a regular file HEAD that has a properly\n * formatted sha1 object name.\n+ *\n+ * fsck.c checks for bare repositories in trees using similar rules, but a\n+ * duplicated implementation. If these are changed, the correspnding code in\n+ * fsck.c should change too.\n */\n int is_git_directory(const char *suspect)\n {\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex de50c0ea01..a65827bc03 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -563,6 +563,42 @@ dot-backslash-case .\\\\\\\\.GIT\\\\\\\\foobar\n dotgit-case-backslash .git\\\\\\\\foobar\n EOF\n \n+test_expect_success \"fsck notices bare repo\" '\n+(\n+\tmkdir -p embedded-bare-repo/bare &&\n+\tgit init embedded-bare-repo &&\n+\t(\n+\t\tcd embedded-bare-repo/bare &&\n+\t\techo content >HEAD &&\n+\t\tmkdir refs/ objects/ &&\n+\t\techo content >refs/foo &&\n+\t\techo content >objects/foo &&\n+\t\tgit add . &&\n+\t\tgit commit -m base &&\n+\t\tbad_tree=$(git rev-parse HEAD:bare) &&\n+\t\tgit fsck 2>out &&\n+\t\ttest_i18ngrep \"warning.*tree $bad_tree: embeddedBareRepo: contains bare repository\" out\n+\t)\n+)'\n+\n+test_expect_success \"fsck notices bare repo with odd casing\" '\n+(\n+\tmkdir -p embedded-bare-repo-case/bare &&\n+\tgit init embedded-bare-repo-case &&\n+\t(\n+\t\tcd embedded-bare-repo-case/bare &&\n+\t\techo content >heAD &&\n+\t\tmkdir Refs/ objectS/ &&\n+\t\techo content >Refs/foo &&\n+\t\techo content >objectS/foo &&\n+\t\tgit add . &&\n+\t\tgit commit -m base &&\n+\t\tbad_tree=$(git rev-parse HEAD:bare) &&\n+\t\tgit fsck 2>out &&\n+\t\ttest_i18ngrep \"warning.*tree $bad_tree: embeddedBareRepo: contains bare repository\" out\n+\t)\n+)'\n+\n test_expect_success 'fsck allows .Ňit' '\n \t(\n \t\tgit init not-dotgit &&\n\nbase-commit: 805e0a68082a217f0112db9ee86a022227a9c81b\n-- \n2.33.GIT\n\n"},{"id":"453255","messageId":"nycvar.QRO.7.76.6.2204071440520.347@tvgsbejvaqbjf.bet","threadId":"57685","inReplyTo":"20220406232231.47714-1-chooglen@google.com","subject":"Re: [PATCH] fsck: detect bare repos in trees and warn","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-04-07T12:42:39Z","receivedAt":"2022-04-07T12:42:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Glen,\n\nOn Wed, 6 Apr 2022, Glen Choo wrote:\n\n> Git tries not to distribute configs in-repo because they are a security\n> risk. However, an attacker can do exactly this if they embed a bare\n> repo inside of another repo.\n>\n> Teach fsck to detect whether a tree object contains a bare repo (as\n> determined by setup.c) and warn. This will help hosting sites detect and\n> prevent transmission of such malicious repos.\n>\n> See [1] for a more in-depth discussion, including future steps and\n> alternatives.\n>\n> [1] https://lore.kernel.org/git/kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com/\n\nOut of curiosity: does this new check trigger with\nhttps://github.com/libgit2/libgit2? AFAIR it has embedded repositories\nthat are used in its test suite. In other words, libgit2 has a legitimate\nuse case for embedded bare repositories, I believe.\n\nThank you,\nJohannes\n\n>\n> Signed-off-by: Glen Choo \n> ---\n> fsck.c | 19 +++++++++++++++++++\n> fsck.h | 1 +\n> setup.c | 4 ++++\n> t/t1450-fsck.sh | 36 ++++++++++++++++++++++++++++++++++++\n> 4 files changed, 60 insertions(+)\n>\n> diff --git a/fsck.c b/fsck.c\n> index 3ec500d707..11c11c348a 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -573,6 +573,9 @@ static int fsck_tree(const struct object_id *tree_oid,\n> \tint has_bad_modes = 0;\n> \tint has_dup_entries = 0;\n> \tint not_properly_sorted = 0;\n> +\tint has_head = 0;\n> +\tint has_refs_entry = 0;\n> +\tint has_objects_entry = 0;\n> \tstruct tree_desc desc;\n> \tunsigned o_mode;\n> \tconst char *o_name;\n> @@ -602,6 +605,12 @@ static int fsck_tree(const struct object_id *tree_oid,\n> \t\thas_dotdot |= !strcmp(name, \"..\");\n> \t\thas_dotgit |= is_hfs_dotgit(name) || is_ntfs_dotgit(name);\n> \t\thas_zero_pad |= *(char *)desc.buffer == '0';\n> +\t\thas_head |= !strcasecmp(name, \"HEAD\")\n> +\t\t\t&& (S_ISLNK(mode) || S_ISREG(mode));\n> +\t\thas_refs_entry |= !strcasecmp(name, \"refs\")\n> +\t\t\t&& (S_ISLNK(mode) || S_ISDIR(mode));\n> +\t\thas_objects_entry |= !strcasecmp(name, \"objects\")\n> +\t\t\t&& (S_ISLNK(mode) || S_ISDIR(mode));\n>\n> \t\tif (is_hfs_dotgitmodules(name) || is_ntfs_dotgitmodules(name)) {\n> \t\t\tif (!S_ISLNK(mode))\n> @@ -739,6 +748,16 @@ static int fsck_tree(const struct object_id *tree_oid,\n> \t\tretval += report(options, tree_oid, OBJ_TREE,\n> \t\t\t\t FSCK_MSG_TREE_NOT_SORTED,\n> \t\t\t\t \"not properly sorted\");\n> +\t/*\n> +\t * Determine if this tree looks like a bare repository according\n> +\t * to the rules of setup.c. If those are changed, this should be\n> +\t * changed too.\n> +\t */\n> +\tif (has_head && has_refs_entry && has_objects_entry)\n> +\t\tretval += report(options, tree_oid, OBJ_TREE,\n> +\t\t\t\t FSCK_MSG_EMBEDDED_BARE_REPO,\n> +\t\t\t\t \"contains bare repository\");\n> +\n> \treturn retval;\n> }\n>\n> diff --git a/fsck.h b/fsck.h\n> index d07f7a2459..3f0f73b0f3 100644\n> --- a/fsck.h\n> +++ b/fsck.h\n> @@ -65,6 +65,7 @@ enum fsck_msg_type {\n> \tFUNC(NULL_SHA1, WARN) \\\n> \tFUNC(ZERO_PADDED_FILEMODE, WARN) \\\n> \tFUNC(NUL_IN_COMMIT, WARN) \\\n> +\tFUNC(EMBEDDED_BARE_REPO, WARN) \\\n> \t/* infos (reported as warnings, but ignored by default) */ \\\n> \tFUNC(GITMODULES_PARSE, INFO) \\\n> \tFUNC(GITIGNORE_SYMLINK, INFO) \\\n> diff --git a/setup.c b/setup.c\n> index 04ce33cdcd..2600548776 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -336,6 +336,10 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n> * - either a HEAD symlink or a HEAD file that is formatted as\n> * a proper \"ref:\", or a regular file HEAD that has a properly\n> * formatted sha1 object name.\n> + *\n> + * fsck.c checks for bare repositories in trees using similar rules, but a\n> + * duplicated implementation. If these are changed, the correspnding code in\n> + * fsck.c should change too.\n> */\n> int is_git_directory(const char *suspect)\n> {\n> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n> index de50c0ea01..a65827bc03 100755\n> --- a/t/t1450-fsck.sh\n> +++ b/t/t1450-fsck.sh\n> @@ -563,6 +563,42 @@ dot-backslash-case .\\\\\\\\.GIT\\\\\\\\foobar\n> dotgit-case-backslash .git\\\\\\\\foobar\n> EOF\n>\n> +test_expect_success \"fsck notices bare repo\" '\n> +(\n> +\tmkdir -p embedded-bare-repo/bare &&\n> +\tgit init embedded-bare-repo &&\n> +\t(\n> +\t\tcd embedded-bare-repo/bare &&\n> +\t\techo content >HEAD &&\n> +\t\tmkdir refs/ objects/ &&\n> +\t\techo content >refs/foo &&\n> +\t\techo content >objects/foo &&\n> +\t\tgit add . &&\n> +\t\tgit commit -m base &&\n> +\t\tbad_tree=$(git rev-parse HEAD:bare) &&\n> +\t\tgit fsck 2>out &&\n> +\t\ttest_i18ngrep \"warning.*tree $bad_tree: embeddedBareRepo: contains bare repository\" out\n> +\t)\n> +)'\n> +\n> +test_expect_success \"fsck notices bare repo with odd casing\" '\n> +(\n> +\tmkdir -p embedded-bare-repo-case/bare &&\n> +\tgit init embedded-bare-repo-case &&\n> +\t(\n> +\t\tcd embedded-bare-repo-case/bare &&\n> +\t\techo content >heAD &&\n> +\t\tmkdir Refs/ objectS/ &&\n> +\t\techo content >Refs/foo &&\n> +\t\techo content >objectS/foo &&\n> +\t\tgit add . &&\n> +\t\tgit commit -m base &&\n> +\t\tbad_tree=$(git rev-parse HEAD:bare) &&\n> +\t\tgit fsck 2>out &&\n> +\t\ttest_i18ngrep \"warning.*tree $bad_tree: embeddedBareRepo: contains bare repository\" out\n> +\t)\n> +)'\n> +\n> test_expect_success 'fsck allows .??it' '\n> \t(\n> \t\tgit init not-dotgit &&\n>\n> base-commit: 805e0a68082a217f0112db9ee86a022227a9c81b\n> --\n> 2.33.GIT\n>\n>\n"},{"id":"453257","messageId":"220407.86pmltc9w3.gmgdl@evledraar.gmail.com","threadId":"57685","inReplyTo":"20220406232231.47714-1-chooglen@google.com","subject":"Re: [PATCH] fsck: detect bare repos in trees and warn","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-07T13:12:24Z","receivedAt":"2022-04-07T13:15:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 06 2022, Glen Choo wrote:\n\n> @@ -602,6 +605,12 @@ static int fsck_tree(const struct object_id *tree_oid,\n> \t\thas_dotdot |= !strcmp(name, \"..\");\n> \t\thas_dotgit |= is_hfs_dotgit(name) || is_ntfs_dotgit(name);\n> \t\thas_zero_pad |= *(char *)desc.buffer == '0';\n> +\t\thas_head |= !strcasecmp(name, \"HEAD\")\n> +\t\t\t&& (S_ISLNK(mode) || S_ISREG(mode));\n> +\t\thas_refs_entry |= !strcasecmp(name, \"refs\")\n> +\t\t\t&& (S_ISLNK(mode) || S_ISDIR(mode));\n> +\t\thas_objects_entry |= !strcasecmp(name, \"objects\")\n> +\t\t\t&& (S_ISLNK(mode) || S_ISDIR(mode));\n\nDoesn't this code need to use is_hfs_dot_str() instead of strcasecmp()\nlike the other similar checks?\n\n> @@ -336,6 +336,10 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n> * - either a HEAD symlink or a HEAD file that is formatted as\n> * a proper \"ref:\", or a regular file HEAD that has a properly\n> * formatted sha1 object name.\n> + *\n> + * fsck.c checks for bare repositories in trees using similar rules, but a\n> + * duplicated implementation. If these are changed, the correspnding code in\n> + * fsck.c should change too.\n> */\n\nProbably took much hassle to factor these so it can be re-used. Typo:\ncorrespnding.\n\n> +\t\ttest_i18ngrep \"warning.*tree $bad_tree: embeddedBareRepo: contains bare repository\" out\n\ns/test_i18ngrep/grep/\n"},{"id":"453258","messageId":"e81cdc6e-da42-d1d1-5d66-7d5e2a8aebbe@github.com","threadId":"57685","inReplyTo":"nycvar.QRO.7.76.6.2204071440520.347@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] fsck: detect bare repos in trees and warn","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-07T13:21:35Z","receivedAt":"2022-04-07T13:21:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/7/2022 8:42 AM, Johannes Schindelin wrote:\n> Hi Glen,\n> \n> On Wed, 6 Apr 2022, Glen Choo wrote:\n> \n>> Git tries not to distribute configs in-repo because they are a security\n>> risk. However, an attacker can do exactly this if they embed a bare\n>> repo inside of another repo.\n>>\n>> Teach fsck to detect whether a tree object contains a bare repo (as\n>> determined by setup.c) and warn. This will help hosting sites detect and\n>> prevent transmission of such malicious repos.\n>>\n>> See [1] for a more in-depth discussion, including future steps and\n>> alternatives.\n>>\n>> [1] https://lore.kernel.org/git/kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com/\n> \n> Out of curiosity: does this new check trigger with\n> https://github.com/libgit2/libgit2? AFAIR it has embedded repositories\n> that are used in its test suite. In other words, libgit2 has a legitimate\n> use case for embedded bare repositories, I believe.\n\nIt is definitely good to keep in mind that other repositories have\nincluded bare repositories for convenience. I'm not sure that the behavior\nof some good actors should outweigh the benefits of protecting against\nthis attack vector.\n\nThe trouble here is: how could the libgit2 repo change their project to\nnot trigger this warning? These bare repos are in their history forever if\nthey don't do go through significant work and pain to remove them from\ntheir history. We would want to have a way to make the warnings less\nsevere for special cases like this.\n\nSimultaneously, we wouldn't want to bless all _forks_ of libgit2.\n\nFinally, the real thing we want to avoid is having the Git client write\nthese trees to disk, for example during a 'git checkout', unless the user\ngives an override. (We would want 'git bisect' to still work on the\nlibgit2 repo, for example.)\n\nA more complete protection here would be:\n\n 1. Warn when finding a bare repo as a tree (this patch).\n\n 2. Suppress warnings on trusted repos, scoped to a specific set of known\n trees _or_ based on some set of known commits (in case the known trees\n are too large).\n\n 3. Prevent writing a bare repo to the worktree, unless the user provided\n an opt-in to that behavior.\n\nSince your patch is moving in the right direction here, I don't think\nsteps (2) and (3) are required to move forward with your patch. However,\nit is a good opportunity to discuss the full repercussions of this issue.\n\nThanks,\n-Stolee\n"},{"id":"453264","messageId":"220407.86lewhc6bz.gmgdl@evledraar.gmail.com","threadId":"57685","inReplyTo":"e81cdc6e-da42-d1d1-5d66-7d5e2a8aebbe@github.com","subject":"Re: [PATCH] fsck: detect bare repos in trees and warn","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-07T14:14:31Z","receivedAt":"2022-04-07T14:34:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 07 2022, Derrick Stolee wrote:\n\n> A more complete protection here would be:\n>\n> 1. Warn when finding a bare repo as a tree (this patch).\n>\n> 2. Suppress warnings on trusted repos, scoped to a specific set of known\n> trees _or_ based on some set of known commits (in case the known trees\n> are too large).\n>\n> 3. Prevent writing a bare repo to the worktree, unless the user provided\n> an opt-in to that behavior.\n>\n> Since your patch is moving in the right direction here, I don't think\n> steps (2) and (3) are required to move forward with your patch. However,\n> it is a good opportunity to discuss the full repercussions of this issue.\n\nIsn't a gentler solution here to:\n\n 1. In setup.c, we detect a repo\n 2. Walk up a directory\n 3. Do we find a repo?\n 4. Does that repo \"contain\" the first one?\n If yes: die on setup\n If no: it's OK\n\nIt also seems to me that there's pretty much perfect overlap between\nthis and the long-discussed topic of marking a submodule with config\nv.s. detecting it on the fly.\n\nSince we're already discussing breaking long-standing repos in the wild\nhere I think it's good to take a step back and think of a potential more\nnarrow solution.\n\nThere's nothing wrong with having tracked content per-se, it's just that\nwe ourselves have other code that'll shoot itself in the foot in certain\nscenarios, like this one.\n\nBut for a newer git that needs to run this fsck check it'll already need\nto detect that something \"looks like a repo\", but if it can do that and\nsetup.c can walk up from the parent directory & find a repository we can\ncombine the two.\n\nOf course we'll probably still want an opt-in fsck check for hosting\nproviders who'll want to protect older clients, but as long as the two\nare separated that'll only need to last as long as such clients are\npotentially there in the wild.\n"},{"id":"453266","messageId":"xmqqbkxdudvv.fsf@gitster.g","threadId":"57685","inReplyTo":"e81cdc6e-da42-d1d1-5d66-7d5e2a8aebbe@github.com","subject":"Re: [PATCH] fsck: detect bare repos in trees and warn","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-07T15:11:48Z","receivedAt":"2022-04-07T15:12:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> It is definitely good to keep in mind that other repositories have\n> included bare repositories for convenience. I'm not sure that the behavior\n> of some good actors should outweigh the benefits of protecting against\n> this attack vector.\n\nGood line of thinking.\n\n> 2. Suppress warnings on trusted repos, scoped to a specific set of known\n> trees _or_ based on some set of known commits (in case the known trees\n> are too large).\n\nIs \"It is OK to check out an embedded repository from this commit or\nany of its ancestors\" the kind of suppression you meant by \"known\ncommits\"?\n\n> 3. Prevent writing a bare repo to the worktree, unless the user provided\n> an opt-in to that behavior.\n>\n> Since your patch is moving in the right direction here, I don't think\n> steps (2) and (3) are required to move forward with your patch. However,\n> it is a good opportunity to discuss the full repercussions of this issue.\n\nWe can definitely start without (3). Unleashing (1) before (2) is\nready would mean folks cannot clone projects like libgit2 until\nlater, which takes us back to the first point you made above.\n\nThose who use embedded bare repositories as test vector can easily\nswitch to storing an equivalent of \"a tarball that is expanded while\nbuilding and/or testing\", and as long as the user trusts the project\nenough to run its build procedure or tests, we are not adding much\nto the attack surface, I would guess.\n"},{"id":"453267","messageId":"xmqqzgkwudh6.fsf@gitster.g","threadId":"57685","inReplyTo":"20220406232231.47714-1-chooglen@google.com","subject":"Re: [PATCH] fsck: detect bare repos in trees and warn","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-07T15:20:37Z","receivedAt":"2022-04-07T15:20:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> @@ -602,6 +605,12 @@ static int fsck_tree(const struct object_id *tree_oid,\n> \t\thas_dotdot |= !strcmp(name, \"..\");\n> \t\thas_dotgit |= is_hfs_dotgit(name) || is_ntfs_dotgit(name);\n> \t\thas_zero_pad |= *(char *)desc.buffer == '0';\n> +\t\thas_head |= !strcasecmp(name, \"HEAD\")\n> +\t\t\t&& (S_ISLNK(mode) || S_ISREG(mode));\n> +\t\thas_refs_entry |= !strcasecmp(name, \"refs\")\n> +\t\t\t&& (S_ISLNK(mode) || S_ISDIR(mode));\n\nSorry, I know I am to blame, but I think it is wrong to insist\n\"refs\" (and others) to be a directory. setup.c::is_git_directory()\nonly checks with access(X_OK). Also, if I am not mistaken, we plan\nto take advantage of it and may make refs a regular file, for\nexample, to signal that the ref-files backend is unwelcome in the\nrepository. HEAD can be a symbolic link, not necessarily a regular\nfile. We do not create \"refs\" and \"objects\" as symbolic links\nourselves, but it is good to see that the code protects us against\nsuch a directory with these entries being one.\n\n> +\t\thas_objects_entry |= !strcasecmp(name, \"objects\")\n> +\t\t\t&& (S_ISLNK(mode) || S_ISDIR(mode));\n> \n> \t\tif (is_hfs_dotgitmodules(name) || is_ntfs_dotgitmodules(name)) {\n> \t\t\tif (!S_ISLNK(mode))\n> @@ -739,6 +748,16 @@ static int fsck_tree(const struct object_id *tree_oid,\n> \t\tretval += report(options, tree_oid, OBJ_TREE,\n> \t\t\t\t FSCK_MSG_TREE_NOT_SORTED,\n> \t\t\t\t \"not properly sorted\");\n> +\t/*\n> +\t * Determine if this tree looks like a bare repository according\n> +\t * to the rules of setup.c. If those are changed, this should be\n> +\t * changed too.\n> +\t */\n> +\tif (has_head && has_refs_entry && has_objects_entry)\n> +\t\tretval += report(options, tree_oid, OBJ_TREE,\n> +\t\t\t\t FSCK_MSG_EMBEDDED_BARE_REPO,\n> +\t\t\t\t \"contains bare repository\");\n> +\n> \treturn retval;\n> }\n"},{"id":"453284","messageId":"FF4B21A6-A8B6-4142-B325-B9C9193BD885@gmail.com","threadId":"57685","inReplyTo":"kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-04-07T18:38:41Z","receivedAt":"2022-04-07T18:38:48Z","isPatch":false,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Glen,\n\nThis is an interesting find. Thanks for writing it up.\n\nOn 6 Apr 2022, at 18:43, Glen Choo wrote:\n\n> Hi all,\n>\n> My Google colleagues and I would like to address what we think is a security\n> risk - namely that Git repositories can contain valid bare repositories in their\n> working trees. This is based on an excellent article by Justin Steven [1] (CC-ed\n> here).\n>\n> Below is a discussion of:\n>\n> * The risky behavior - what Git does and why it is risky\n> * Possible responses to this risk\n> * A proposed approach\n>\n> The proposed changes are nontrivial, so I’d really appreciate any feedback here.\n> Unfortunately, I will be out of the office and won’t respond to emails for the\n> next 7 days or so, but there will still be at least one Google Git team member\n> keeping tabs on the discussion :)\n>\n> = TL;DR\n>\n> Git repositories should not be allowed to contain bare repositories in their\n> working trees because:\n>\n> * Such bare repositories can have maliciously crafted config files that cause\n> `git` to execute arbitrary code.\n> * Git itself can distribute the malicious repo via `git clone`; no need to\n> distribute repos out of band e.g. via tarballs [2].\n> * Many `git` commands can be affected by malicious config files, and many users\n> have tools that will run `git` in the current directory or the subdirectories\n> of a repo. Once the malicious repo has been cloned, very little social\n> engineering is needed; the user might only need to open the repo in an editor\n> or `cd` into the correct subdirectory.\n>\n> = Background\n>\n> (This section is primarily a summary of [1]. I highly, highly recommend reading\n> that as it describes the issue in much more detail and is extremely readable\n> regardless of Git experience.)\n>\n> Certain Git configuration options are particularly enticing targets for\n> attackers, e.g. core.fsmonitor can execute arbitrary commands and is invoked\n> on many innocuous-looking `git` commands (like `git status`). This is even more\n> risky when one considers that many tools (like shell prompts and IDEs) will run\n> `git` opportunistically inside a repository - so many users won't even need to\n> explicitly run `git` to be affected [3].\n>\n> Since config files are such an enticing target for attackers, Git intentionally\n> avoids distributing config files with repos - a user shouldn't be able to `git\n> clone` a repo with a config file (or really, any files inside .git). However,\n> one can 'trick' Git into doing this by embedding a bare repository inside of\n> another, containing repository: a repository can contain subdirectories that are\n> valid bare repositories, and any `git` operations run in such a subdirectory\n> will then use the bare repository’s config file instead of the \"main\"\n> repository’s.\n>\n> An attack might look like this:\n>\n> * Attacker creates a repository where subdirectory \"Documentation/\" is a bare\n> repository i.e. it contains \"HEAD\", \"refs/\" and \"objects/\" [4]. Attacker\n> also adds \"config\" with a malicious setting for core.fsmonitor.\n> * Attacker convinces User to read their project's documentation by `git\n> clone`-ing their repository and inspecting the \"Documentation/\" directory.\n> * User cd-s into \"Documentation/\" and their shell prompt runs `git status`,\n\nThis might be obvious to most reading this, but for those trying this out at home,\nyou'll need to create a worktree that points to the bare repository and then run\n`git status` in there.\n\n> executing the core.fsmonitor command defined by Attacker.\n\nThis is easily reproduceable.\n\n>\n> = What can we do about it?\n>\n> Each subsection is an alternative and an analysis (+/- are pros/cons).\n>\n> == 1. Prevent users from checking out bare repos\n>\n> This is similar to an existing mitigation where we prevent repository entries\n> that can be confused for \".git\" (like \".GIT\"). but it requires checking multiple\n> entries instead of a single entry. I suspect that we could accomplish this in\n> one of two ways:\n>\n> a. Prevent bare repos from entering the index.\n> b. Prevent writing bare repos to the working tree.\n>\n> + Relatively robust protection - because the malicious repo never enters the\n> working tree, we even protect other tools (e.g. JGit) from doing dangerous\n> things in the embedded repo (provided the checkout is done with `git`, of\n> course).\n> - This breaks some 'valid' workflows (e.g. someone embedding a bare repo as a\n> more convenient alternative to submodules), but it seems reasonable to let\n> users opt out of this behavior.\n> - (1a) is difficult to do in practice because many code paths add entries to\n> the index, and checking a combination of new entry and existing entries is\n> much trickier than checking just the new entry.\n> - (1b) might also be difficult, though not as difficult as 1a because we\n> already have a complete list of entries we will write. I don’t think there\n> are existing facilities that do this sort of checking of multiple entries.\n>\n> == 2. Detect and reject bare repos using `git fsck` and `transfer.fsckObjects`.\n>\n> This entails checking for the markers of a bare repository (HEAD, refs/,\n> objects/) in tree objects. This shares a precedent with (1), since `git fsck`\n> will also detect \".GIT\".\n>\n> + Most reputable hosting sites set `transfer.fsckObjects`, which allows them to\n> detect and prevent this kind of transmission.\n> + Confers some protection to users even without them doing anything.\n> + Easy to visualize and to write.\n> - This won’t directly protect most users because they don’t typically set\n> `transfer.fsckObjects` (in fact, `transfer.fsckObjects` will render many\n> projects, like linux.git, uncloneable without additional config)\n> - Won’t guard against malicious/poorly configured hosts.\n>\n> == 3. Detect that we are in an embedded bare repo and ignore the embedded bare\n> repository in favor of the containing repo.\n>\n> For example, if setup.c detects that we are in a bare repo, it could then walk\n> up the directory hierarchy to see if there is a containing repo that tracks the\n> bare one. If so, setup.c chooses to use the containing repo instead.\n>\n> + Extremely robust; this even protects against a checkout by an earlier Git\n> version.\n> + Users who don’t use bare repos won’t even notice the difference.\n> - The change in rules breaks some legitimate workflows e.g. a user with a repo\n> at HOME and bare repos underneath.\n> - Potentially very expensive for bare repo users because setup.c will likely\n> walk up to the root directory; we’d be essentially *forcing* those users to\n> set GIT_CEILING_DIRECTORIES.\n> - Doesn’t protect users who use other tools e.g. JGit.\n>\n> == 4. Educate users about this risk without making code changes.\n>\n> Some risks fall into this category e.g. \"Avoid unarchiving repositories because\n> .git/config might be poisoned.\" [2].\n>\n> + We don’t break any users.\n> - Breaks the trust that users have in `git clone`.\n> - IMO \"Inspect every repo in its entirety before cloning it\" is too much of a\n> burden to put on users.\n>\n> = Next steps\n>\n> I propose that we prevent repositories from containing bare repositories by\n> doing the following (in order):\n>\n> * Implement (2) by adding a new fsck message \"embeddedBareRepo\".\n> * When this is done, hosting sites can hopefully use this capability to\n> prevent transmission, and help us understand the prevalence of such attacks.\n> * Implement (1b) by teaching unpack_trees.c to check whether the tree contains\n> an entire bare repo, and die() if so. This will be guarded by a\n> defaults-to-true config value.\n> * This would only block a bare repo from being written in a single operation.\n> It wouldn’t stop a user from writing a bare repo entry-by-entry using \"git\n> checkout \", but the amount of social engineering required probably\n> renders this attack infeasible.\n> * As I noted earlier, I foresee some difficulty actually implementing this\n> because I don’t think we have facilities for checking multiple tree entries\n> at once.\n>\n> I am particularly interested in hearing feedback about (1b), namely:\n>\n> * How to actually teach unpack_trees.c to detect bare repos.\n> * Whether preventing bare repos in unpack_trees.c is a good enough mitigation\n> (e.g. Are there other code paths that should block bare repos? Should we be\n> checking the index instead of the tree?).\n>\n> I have a patch that does (2); I will send that out and we can leave feedback on\n> that separately.\n>\n> = Demonstration\n>\n> This is based on a script by Taylor Blau (thanks!). [1] also contains a\n> demonstration that runs in Docker.\n>\n> #!/bin/sh\n>\n> rm -fr malicious cloned &&git init malicious &&\n>\n> (\n> cd malicious &&\n>\n> mkdir -p bare &&\n> cd bare &&\n>\n> echo 'ref: refs/heads/main' >HEAD &&\n> cat >config <<-EOF\n> [core]\n> repositoryformatversion = 0\n> filemode = true\n> bare = false\n> worktree = \"worktree\"\n> fsmonitor = \"echo pwned >&2; false\"\n> EOF\n>\n> mkdir objects refs worktree &&\n> touch worktree/.gitkeep &&\n>\n> git add . &&\n> git commit -m \".gitkeep\" &&\n>\n> cd .. &&\n> git add bare &&\n> git commit -m 'initial commit'\n> ) &&\n>\n> git clone --no-local malicious cloned &&\n> cd cloned/bare &&\n> git status # pwned\n>\n> = Footnotes\n>\n> [1] https://github.com/justinsteven/advisories/blob/main/2022_git_buried_bare_repos_and_fsmonitor_various_abuses.md.\n> [2] Archived repositories with malicious .git directories are a known risk. See\n> https://lore.kernel.org/git/20171003123239.lisk43a2goxtxkro@sigill.intra.peff.net/\n> for an on-list discussion. This is also described in\n> https://blog.sonarsource.com/securing-developer-tools-git-integrations\n> (referenced in [1]).\n> [3] We even ship such a tool - contrib/completion/git-prompt.sh. A user can pwn\n> themselves with tab completion even if they don’t have a prompt configured\n> [4] Any directory containing these entries will be recognized as a bare repo, so\n> an attacker could add arbitrary entries to the directory to obfuscate the fact\n> that it is a bare repo.\n> [*] https://offensi.com/2019/12/16/4-google-cloud-shell-bugs-explained-bug-3/ is\n> similar to [1], but uses hooks instead of core.fsmonitor.\n"},{"id":"453292","messageId":"Yk9Wcr74gvhtyOi7@camp.crustytoothpaste.net","threadId":"57685","inReplyTo":"kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-04-07T21:24:02Z","receivedAt":"2022-04-07T21:24:12Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-04-06 at 22:43:08, Glen Choo wrote:\n> An attack might look like this:\n> \n> * Attacker creates a repository where subdirectory \"Documentation/\" is a bare\n> repository i.e. it contains \"HEAD\", \"refs/\" and \"objects/\" [4]. Attacker\n> also adds \"config\" with a malicious setting for core.fsmonitor.\n> * Attacker convinces User to read their project's documentation by `git\n> clone`-ing their repository and inspecting the \"Documentation/\" directory.\n> * User cd-s into \"Documentation/\" and their shell prompt runs `git status`,\n> executing the core.fsmonitor command defined by Attacker.\n\nAs mentioned elsewhere, git status doesn't work without a working tree.\n\n> = Next steps\n> \n> I propose that we prevent repositories from containing bare repositories by\n> doing the following (in order):\n> \n> * Implement (2) by adding a new fsck message \"embeddedBareRepo\".\n> * When this is done, hosting sites can hopefully use this capability to\n> prevent transmission, and help us understand the prevalence of such attacks.\n> * Implement (1b) by teaching unpack_trees.c to check whether the tree contains\n> an entire bare repo, and die() if so. This will be guarded by a\n> defaults-to-true config value.\n> * This would only block a bare repo from being written in a single operation.\n> It wouldn’t stop a user from writing a bare repo entry-by-entry using \"git\n> checkout \", but the amount of social engineering required probably\n> renders this attack infeasible.\n> * As I noted earlier, I foresee some difficulty actually implementing this\n> because I don’t think we have facilities for checking multiple tree entries\n> at once.\n\nI'm aware of repositories that happen to break in this case. It's not\nuncommon to embed bare repositories when working with tools that involve\nGit, and this will definitely break them. git fast-import isn't always\na valid option because the test data may involve specific structures or\ntooling that can't be replicated that way, or it involves things like\ncommit signatures which aren't round-tripped.\n\nInstead, I'd rather see us avoid executing any program from the config\nor any hooks in a bare repository without a working tree (except for\npushes). I think that would avoid breaking things while still improving\nsecurity.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"453296","messageId":"CAHZU0ySHqc7f9qB0+ZrMWHHJiWsS-_hsUzomwNrGNMTF6qwcOw@mail.gmail.com","threadId":"57685","inReplyTo":"Yk9Wcr74gvhtyOi7@camp.crustytoothpaste.net","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Justin Steven","fromEmail":"justin@justinsteven.com","sentAt":"2022-04-07T21:53:26Z","receivedAt":"2022-04-07T21:53:43Z","isPatch":false,"sender":{"key":"justin@justinsteven.com","avatar":null},"body":"Hi all,\n\nI'm the author of one of the articles linked in Glen's mail. Thank you\nGlen for summarising the problem beautifully and pushing this forward.\n\nBrian said:\n> As mentioned elsewhere, git status doesn't work without a working tree.\n\nThis is correct. However, it is possible to embed a bare repo that has\nits own core.worktree which points to a directory within the\ncontaining repo, satisfying the requirement of having a working tree.\nThis is covered in the article [1] and looks to be accounted for in\nTaylor's reproducer script which admittedly I haven't run.\n\n> Instead, I'd rather see us avoid executing any program from the config\n> or any hooks in a bare repository without a working tree (except for\n> pushes). I think that would avoid breaking things while still improving\n> security.\n\nDue to the fact that the embedded bare repo can be made to have a\nworking tree, this won't be an effective fix.\n\nI'm not dismissing your examples of uses of Git which would break\nunder Glen's suggestions. Thank you for describing these.\n\n[1] https://github.com/justinsteven/advisories/blob/main/2022_git_buried_bare_repos_and_fsmonitor_various_abuses.md#poc---regular-vs-bare-repos-and-adding-a-corefsmonitor-payload-to-a-bare-repo\n\n--\nJustin\n"},{"id":"453303","messageId":"Yk9hONuCIVIq6ieV@camp.crustytoothpaste.net","threadId":"57685","inReplyTo":"CAHZU0ySHqc7f9qB0+ZrMWHHJiWsS-_hsUzomwNrGNMTF6qwcOw@mail.gmail.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-04-07T22:10:00Z","receivedAt":"2022-04-07T22:10:11Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-04-07 at 21:53:26, Justin Steven wrote:\n> Hi all,\n> \n> I'm the author of one of the articles linked in Glen's mail. Thank you\n> Glen for summarising the problem beautifully and pushing this forward.\n> \n> Brian said:\n> > As mentioned elsewhere, git status doesn't work without a working tree.\n> \n> This is correct. However, it is possible to embed a bare repo that has\n> its own core.worktree which points to a directory within the\n> containing repo, satisfying the requirement of having a working tree.\n> This is covered in the article [1] and looks to be accounted for in\n> Taylor's reproducer script which admittedly I haven't run.\n> \n> > Instead, I'd rather see us avoid executing any program from the config\n> > or any hooks in a bare repository without a working tree (except for\n> > pushes). I think that would avoid breaking things while still improving\n> > security.\n> \n> Due to the fact that the embedded bare repo can be made to have a\n> working tree, this won't be an effective fix.\n\nThen we'd probably be better off just walking up the entire hierarchy\nand excluding worktrees from embedded bare repositories, or otherwise\nrestricting the config we read. That will probably mean we'll need to\nwalk the entire directory hierarchy to see if it's embedded (or at least\nto the root of the device) in such a case, but that should be relatively\nuncommon.\n\nI'd definitely like to see us make a security improvement here, but I\nalso would like to avoid us breaking a lot of repositories, especially\nsince we lack alternatives.\n\nIf git fast-import could 100% correctly round-trip all commits and\nrepositories, I would be much more open to blocking this in fsck after a\ndeprecation period, but as it stands that's not possible. Perhaps\nimproving that would be a suitable way forward.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"453304","messageId":"005d01d84ad0$782e8fc0$688baf40$@nexbridge.com","threadId":"57685","inReplyTo":"Yk9hONuCIVIq6ieV@camp.crustytoothpaste.net","subject":"RE: Bare repositories in the working tree are a security risk","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2022-04-07T22:40:18Z","receivedAt":"2022-04-07T22:40:45Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On April 7, 2022 6:10 PM, brian m. carlson wrote:\n>On 2022-04-07 at 21:53:26, Justin Steven wrote:\n>> Hi all,\n>>\n>> I'm the author of one of the articles linked in Glen's mail. Thank you\n>> Glen for summarising the problem beautifully and pushing this forward.\n>>\n>> Brian said:\n>> > As mentioned elsewhere, git status doesn't work without a working tree.\n>>\n>> This is correct. However, it is possible to embed a bare repo that has\n>> its own core.worktree which points to a directory within the\n>> containing repo, satisfying the requirement of having a working tree.\n>> This is covered in the article [1] and looks to be accounted for in\n>> Taylor's reproducer script which admittedly I haven't run.\n>>\n>> > Instead, I'd rather see us avoid executing any program from the\n>> > config or any hooks in a bare repository without a working tree\n>> > (except for pushes). I think that would avoid breaking things while\n>> > still improving security.\n>>\n>> Due to the fact that the embedded bare repo can be made to have a\n>> working tree, this won't be an effective fix.\n>\n>Then we'd probably be better off just walking up the entire hierarchy and\n>excluding worktrees from embedded bare repositories, or otherwise restricting\n>the config we read. That will probably mean we'll need to walk the entire\n>directory hierarchy to see if it's embedded (or at least to the root of the device) in\n>such a case, but that should be relatively uncommon.\n>\n>I'd definitely like to see us make a security improvement here, but I also would like\n>to avoid us breaking a lot of repositories, especially since we lack alternatives.\n>\n>If git fast-import could 100% correctly round-trip all commits and repositories, I\n>would be much more open to blocking this in fsck after a deprecation period, but\n>as it stands that's not possible. Perhaps improving that would be a suitable way\n>forward.\n\nOne option relating to enable/disable this is to set up a config option that, by default is false, to allow embedded bare repositories. At least with enough warning that this option is required, it might be acceptable. I would prefer never to receive a bare repo through any means (including through a more worrying submodule). From an attack vector standpoint, I would be more concerned about this in an automation setting, say GitHub workflows or Jenkins GitSCM. At least with GitHub workflows, this is \"somewhat\" contained within VMs under GitHub's control - although not entirely. Jenkins is probably more vulnerable as the clones done through that path do not get the same scrutiny, although in my world, I use a dedicated non-root UID and sandbox the whole thing. This topic makes me nervous and wonder whether we should be self-reporting a CVE.\n\nShuddering,\nRandall\n\n"},{"id":"453312","messageId":"xmqqmtgwp1aq.fsf@gitster.g","threadId":"57685","inReplyTo":"Yk9hONuCIVIq6ieV@camp.crustytoothpaste.net","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-08T05:54:53Z","receivedAt":"2022-04-08T05:55:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> Then we'd probably be better off just walking up the entire hierarchy\n> and excluding worktrees from embedded bare repositories, or otherwise\n> restricting the config we read. That will probably mean we'll need to\n> walk the entire directory hierarchy to see if it's embedded (or at least\n> to the root of the device) in such a case, but that should be relatively\n> uncommon.\n\nI find this direction to notice iffy \"user data\" and disable it\nquite reasonable. A configuration file can define alias, and it\nwould be yet another attack vector to overload common ones users\nlikely use (\"git co\", \"git st\", ...). There may also be a hooks/\ndirectory.\n\nI wonder if it is an acceptable defence to deliberately \"corrupt\"\nsuch user data when we notice that they smell fishy, perhaps by\nrenaming \"config\" and \"hooks\", when they are found next to \"HEAD\"\nand \"objects\" and \"refs\", to \"config.disabled\" and \"hooks.disabled\"?\nI am just thinking aloud without assessing if it is sensible or\nfeasible at ths point.\n\nI am not sure if \"walking the hierarchy up\" is an effective enough\ndefence offhand. Do we consider it too much social engineering to\nmake the user follow cloning instruction of the malicious project to\nprepare a repository, with core.worktree set to elsewhere, and pull\ninto it? Since walking up from any subdirectory of the directory\nthe core.worktree points at will never see a directory, with \".git/\"\nsubdirectory that is the malicious project, \"git status\" run in the\n\"embedded\" place in such a scenario will not notice that it is a\nrepository lookalike that came from outside. But we can write it\noff as an approach needing too much social engineering, that's OK.\n"},{"id":"453595","messageId":"kl6lv8vc90ts.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57685","inReplyTo":"kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-13T20:37:35Z","receivedAt":"2022-04-13T20:37:42Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Thanks for the thoughtful replies, everyone. I'm still catching up with\nthe discussion but I thought I'd share a suggestion/observation from\na colleague (cc-ed, thanks!) who was following the discussion\nseparately.\n\nGlen Choo writes:\n\n> = What can we do about it?\n>\n> Each subsection is an alternative and an analysis (+/- are pros/cons).\n>\n> == 1. Prevent users from checking out bare repos\n> ...\n> == 2. Detect and reject bare repos using `git fsck` and `transfer.fsckObjects`.\n> ...\n> == 3. Detect that we are in an embedded bare repo and ignore the embedded bare\n> repository in favor of the containing repo.\n> ... \n> == 4. Educate users about this risk without making code changes.\n\nMartin observed that, viability aside, there's another approach that I\nhaven't discussed:\n\n == 5 Disable bare repo discovery\n\n We could introduce a config value that disables bare repo discovery\n altogether. This would only disable _discovery_; a user can still use\n the bare repo by specifying the gitdir (e.g. via `--git-dir=.` or\n GIT_DIR).\n\n + Extremely robust (for the majority of users who don't need bare\n repos at least).\n + Easy to understand.\n - We need to read config _before_ discovering the gitdir; although Git\n _can_ do this (e.g. via read_early_config()), I don't think there is\n precedent for doing this during repository setup.\n - Making this behavior opt-out would break every bare repo user\n (they'd need to either start passing --git-dir or change their\n config).\n - Making this behavior opt-in won't help the users who need it the\n most.\n\nI'm not sure if specifically \"disable bare repo discovery for all users\nby default\" is viable, but the line of thinking seems like it might\nyield some good ideas.\n\nThoughts?\n"},{"id":"453607","messageId":"kl6lsfqg8vv9.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57685","inReplyTo":"e81cdc6e-da42-d1d1-5d66-7d5e2a8aebbe@github.com","subject":"Re: [PATCH] fsck: detect bare repos in trees and warn","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-13T22:24:42Z","receivedAt":"2022-04-13T22:24:48Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\nDerrick Stolee writes:\n\n> On 4/7/2022 8:42 AM, Johannes Schindelin wrote:\n>> Hi Glen,\n>> \n>> On Wed, 6 Apr 2022, Glen Choo wrote:\n>> \n>>> Git tries not to distribute configs in-repo because they are a security\n>>> risk. However, an attacker can do exactly this if they embed a bare\n>>> repo inside of another repo.\n>>>\n>>> Teach fsck to detect whether a tree object contains a bare repo (as\n>>> determined by setup.c) and warn. This will help hosting sites detect and\n>>> prevent transmission of such malicious repos.\n>>>\n>>> See [1] for a more in-depth discussion, including future steps and\n>>> alternatives.\n>>>\n>>> [1] https://lore.kernel.org/git/kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com/\n>> \n>> Out of curiosity: does this new check trigger with\n>> https://github.com/libgit2/libgit2? AFAIR it has embedded repositories\n>> that are used in its test suite. In other words, libgit2 has a legitimate\n>> use case for embedded bare repositories, I believe.\n>\n> It is definitely good to keep in mind that other repositories have\n> included bare repositories for convenience. I'm not sure that the behavior\n> of some good actors should outweigh the benefits of protecting against\n> this attack vector.\n>\n> The trouble here is: how could the libgit2 repo change their project to\n> not trigger this warning? These bare repos are in their history forever if\n> they don't do go through significant work and pain to remove them from\n> their history. We would want to have a way to make the warnings less\n> severe for special cases like this.\n>\n> Simultaneously, we wouldn't want to bless all _forks_ of libgit2.\n\nYes, that makes sense. Thanks for the thoughtful reply.\n\n> 2. Suppress warnings on trusted repos, scoped to a specific set of known\n> trees _or_ based on some set of known commits (in case the known trees\n> are too large).\n\nSince Junio mentioned downthread that we'd need (2), I'll focus on this.\nI'm not sure I follow, though, so let me try to verbalize my thought\nprocess to see what I'm not understanding...\n\nBy \"Suppress warnings on trusted repos\", I assume this is done on the\nhosting side? (Since I can't imagine a built-in Git feature that could\nselectively trust repos.)\n\n\"scoped to a specific set of known trees\" sounds like fsck.skipList\ni.e. as a host, I can configure a list of \"good\" libgit2 trees that I\nwill trust and those will be skipped by fsck.\n\nSo from my _very_ naive reading of (2), it seems like we already have\nall of the pieces in place for hosts to do (2) on their own, _unless_\nwe think that fsck.skipList is inadequate for this use case. e.g. I\npersonally can't imagine any way to list every \"good\" tree and still\nhave a cloneable fork of libgit2, so we might to teach fsck to do\nsomething smarter like \"skip any objects reachable by these commits\".\n\n"},{"id":"453614","messageId":"xmqqwnfs4kud.fsf@gitster.g","threadId":"57685","inReplyTo":"kl6lv8vc90ts.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-13T23:36:26Z","receivedAt":"2022-04-13T23:36:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> Martin observed that, viability aside, there's another approach that I\n> haven't discussed:\n>\n> == 5 Disable bare repo discovery\n>\n> We could introduce a config value that disables bare repo discovery\n> altogether. This would only disable _discovery_; a user can still use\n> the bare repo by specifying the gitdir (e.g. via `--git-dir=.` or\n> GIT_DIR).\n\nDoes it or does it not count as \"allowing discovery to do its job\"\nif you go to the directory, knowing that the directory is a bare\none, and expect Git to work in it?\n\nI am guessing that your definition of \"discovery\" is not even\nconsider if the current directory is a repository and always force\nthe user to tell us with --git-dir or GIT_DIR. I am not sure that\nis realistically feasible (I am thinking of cases like \"git fetch\"\ngoing to the remote repository on the local disk that is bare to run\n\"git upload-pack\"), but if the fallout is not too bad, it may be a\ngood heuristics.\n\n"},{"id":"453615","messageId":"kl6lmtgo8s5w.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57685","inReplyTo":"Yk9hONuCIVIq6ieV@camp.crustytoothpaste.net","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-13T23:44:43Z","receivedAt":"2022-04-13T23:44:51Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\nThanks a lot for chiming in, especially for advocating on behalf of\nusers of embedded bare repos :)\n\n\"brian m. carlson\" writes:\n\n> On 2022-04-07 at 21:53:26, Justin Steven wrote:\n>> Hi all,\n>> \n>> I'm the author of one of the articles linked in Glen's mail. Thank you\n>> Glen for summarising the problem beautifully and pushing this forward.\n>> \n>> Brian said:\n>> > As mentioned elsewhere, git status doesn't work without a working tree.\n>> \n>> This is correct. However, it is possible to embed a bare repo that has\n>> its own core.worktree which points to a directory within the\n>> containing repo, satisfying the requirement of having a working tree.\n>> This is covered in the article [1] and looks to be accounted for in\n>> Taylor's reproducer script which admittedly I haven't run.\n>> \n>> > Instead, I'd rather see us avoid executing any program from the config\n>> > or any hooks in a bare repository without a working tree (except for\n>> > pushes). I think that would avoid breaking things while still improving\n>> > security.\n>> \n>> Due to the fact that the embedded bare repo can be made to have a\n>> working tree, this won't be an effective fix.\n>\n> Then we'd probably be better off just walking up the entire hierarchy\n> and excluding worktrees from embedded bare repositories, or otherwise\n> restricting the config we read. That will probably mean we'll need to\n> walk the entire directory hierarchy to see if it's embedded (or at least\n> to the root of the device) in such a case, but that should be relatively\n> uncommon.\n\nIIUC, the difference between your suggestion and (3) in the original\nemail:\n\n == 3. Detect that we are in an embedded bare repo and ignore the embedded bare\n repository in favor of the containing repo.\n\nis that we only walk the hierarchy in the event that the bare repo has a\nworktree? If so, then yes that seems relatively uncommon and wouldn't\nbreak most bare repo users.\n\nWhat I'm unsure about is whether or not this reduces the attack surface\nenough; it protects the highest value target, `git status`, but I'm not\nsure about the following:\n\n* How well does this protect other, existing commands (e.g. `log`,\n `branch` and `remote` fall into the same category of commands that\n appear informative and harmless)?\n* How well does this protect other, abusable configuration directives\n (`core.pager` perhaps)?\n* How future-proof is this? (if someone removes a dependency on\n core.worktree, is the command vulnerable again?)\n\nThere's a similar argument to be made about restricting the config we\nread - we'd be running the risk of either missing some abusable config\nthat already exists or will exist.\n\nAnd if my understanding is correct (and it very well might be wrong),\nthen this seems too piecemeal to be worth the effort.\n\n> I'd definitely like to see us make a security improvement here, but I\n> also would like to avoid us breaking a lot of repositories, especially\n> since we lack alternatives.\n\nAgreed, leaving existing users high and dry is an awful outcome.\n\n>\n> If git fast-import could 100% correctly round-trip all commits and\n> repositories, I would be much more open to blocking this in fsck after a\n> deprecation period, but as it stands that's not possible. Perhaps\n> improving that would be a suitable way forward.\n\n"},{"id":"453617","messageId":"xmqqfsmg4jki.fsf@gitster.g","threadId":"57685","inReplyTo":"xmqqmtgwp1aq.fsf@gitster.g","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-14T00:03:57Z","receivedAt":"2022-04-14T00:04:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I am not sure if \"walking the hierarchy up\" is an effective enough\n> defence offhand. Do we consider it too much social engineering to\n> make the user follow cloning instruction of the malicious project to\n> prepare a repository, with core.worktree set to elsewhere, and pull\n> into it? Since walking up from any subdirectory of the directory\n> the core.worktree points at will never see a directory, with \".git/\"\n> subdirectory that is the malicious project, \"git status\" run in the\n> \"embedded\" place in such a scenario will not notice that it is a\n> repository lookalike that came from outside. But we can write it\n> off as an approach needing too much social engineering, that's OK.\n\nIn other words, an attacker could lead the victim to clone their\nmalicious repository at path $R with core.worktree in its\nconfiguration file $R/config pointing at some directory $D that is\nentirely unrelated to $R. In a subdirectory of $D (i.e. the working\ntree of that malicious repository) there may be a directory $G with\nHEAD, refs and config in it.\n\nWhen the victim visits such a directory $G, going up from there to\nthe root of the filesystem would not find any directory with \".git/\"\nsubdirectory in it, so your protection may not even notice that $G\ncame from a checkout of $R.\n"},{"id":"453618","messageId":"kl6lk0bs8r8o.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57685","inReplyTo":"xmqqmtgwp1aq.fsf@gitster.g","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-14T00:04:39Z","receivedAt":"2022-04-14T00:04:50Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> \"brian m. carlson\" writes:\n>\n>> Then we'd probably be better off just walking up the entire hierarchy\n>> and excluding worktrees from embedded bare repositories, or otherwise\n>> restricting the config we read. That will probably mean we'll need to\n>> walk the entire directory hierarchy to see if it's embedded (or at least\n>> to the root of the device) in such a case, but that should be relatively\n>> uncommon.\n>\n> I find this direction to notice iffy \"user data\" and disable it\n> quite reasonable. A configuration file can define alias, and it\n> would be yet another attack vector to overload common ones users\n> likely use (\"git co\", \"git st\", ...). There may also be a hooks/\n> directory.\n>\n> I wonder if it is an acceptable defence to deliberately \"corrupt\"\n> such user data when we notice that they smell fishy, perhaps by\n> renaming \"config\" and \"hooks\", when they are found next to \"HEAD\"\n> and \"objects\" and \"refs\", to \"config.disabled\" and \"hooks.disabled\"?\n> I am just thinking aloud without assessing if it is sensible or\n> feasible at ths point.\n\nInteresting idea - so the final result for the user is that they can\ncheck out a valid bare repository, but it will not have anything\n\"sensitive\".\n\nI suspect that it will be difficult to define \"will not have anything\nsensitive\" in practice. e.g. Justin's original article [1] shows how\n.git/index can be used in surprising, malicious ways. And if we're going\nto corrupt the repo anyway, it just be easier block the entire bare repo\nfrom entering the worktree by default (but with an escape hatch for\nusers who know what they are doing).\n\n> I am not sure if \"walking the hierarchy up\" is an effective enough\n> defence offhand. Do we consider it too much social engineering to\n> make the user follow cloning instruction of the malicious project to\n> prepare a repository, with core.worktree set to elsewhere, and pull\n> into it? Since walking up from any subdirectory of the directory\n> the core.worktree points at will never see a directory, with \".git/\"\n> subdirectory that is the malicious project, \"git status\" run in the\n> \"embedded\" place in such a scenario will not notice that it is a\n> repository lookalike that came from outside. But we can write it\n> off as an approach needing too much social engineering, that's OK.\n\nIMO that sounds like too much social engineering :)\n\n[1] https://github.com/justinsteven/advisories/blob/main/2022_git_buried_bare_repos_and_fsmonitor_various_abuses.md\n"},{"id":"453656","messageId":"kl6lh76v8vnj.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57685","inReplyTo":"xmqqwnfs4kud.fsf@gitster.g","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-14T16:41:36Z","receivedAt":"2022-04-14T17:02:00Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> Glen Choo writes:\n>\n>> Martin observed that, viability aside, there's another approach that I\n>> haven't discussed:\n>>\n>> == 5 Disable bare repo discovery\n>>\n>> We could introduce a config value that disables bare repo discovery\n>> altogether. This would only disable _discovery_; a user can still use\n>> the bare repo by specifying the gitdir (e.g. via `--git-dir=.` or\n>> GIT_DIR).\n>\n> Does it or does it not count as \"allowing discovery to do its job\"\n> if you go to the directory, knowing that the directory is a bare\n> one, and expect Git to work in it?\n>\n> I am guessing that your definition of \"discovery\" is not even\n> consider if the current directory is a repository and always force\n> the user to tell us with --git-dir or GIT_DIR.\n\nYes, I mean that even the current directory will be ignored when\ndiscovery is disabled.\n\n> I am not sure that\n> is realistically feasible (I am thinking of cases like \"git fetch\"\n> going to the remote repository on the local disk that is bare to run\n> \"git upload-pack\"), but if the fallout is not too bad, it may be a\n> good heuristics.\n\nGood detail - I hadn't considered the impact on our own child processes.\nI suspect this might be a huge undertaking. Unless there is significant\ninterest in this option, I probably won't pursue it further.\n"},{"id":"453661","messageId":"xmqqbkx31sc0.fsf@gitster.g","threadId":"57685","inReplyTo":"kl6lh76v8vnj.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-14T17:35:11Z","receivedAt":"2022-04-14T17:35:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> Yes, I mean that even the current directory will be ignored when\n> discovery is disabled.\n\nOK.\n\n>> I am not sure that\n>> is realistically feasible (I am thinking of cases like \"git fetch\"\n>> going to the remote repository on the local disk that is bare to run\n>> \"git upload-pack\"), but if the fallout is not too bad, it may be a\n>> good heuristics.\n>\n> Good detail - I hadn't considered the impact on our own child processes.\n> I suspect this might be a huge undertaking. Unless there is significant\n> interest in this option, I probably won't pursue it further.\n\nI do not necessarily think so. The entry points to transport on the\nserver side are quite limited (and the client side is dealing with\nyour own repositories anyway), and they already know which directory\nin the server filesystem to hand to the upload-pack and friends, so\nit would be a matter of passing GIT_DIR=$there when they call into the\nrun_command() API, if they are not already doing so.\n"},{"id":"453662","messageId":"xmqq7d7r1qa4.fsf@gitster.g","threadId":"57685","inReplyTo":"xmqqbkx31sc0.fsf@gitster.g","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-14T18:19:31Z","receivedAt":"2022-04-14T18:19:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Glen Choo writes:\n>\n>> Yes, I mean that even the current directory will be ignored when\n>> discovery is disabled.\n>\n> OK.\n>\n>>> I am not sure that\n>>> is realistically feasible (I am thinking of cases like \"git fetch\"\n>>> going to the remote repository on the local disk that is bare to run\n>>> \"git upload-pack\"), but if the fallout is not too bad, it may be a\n>>> good heuristics.\n>>\n>> Good detail - I hadn't considered the impact on our own child processes.\n>> I suspect this might be a huge undertaking. Unless there is significant\n>> interest in this option, I probably won't pursue it further.\n\n> I do not necessarily think so. The entry points to transport on the\n\nBy \"not\" I meant \"this might be huge? It may not be\". Sorry for\nbeing unclear.\n\n> server side are quite limited (and the client side is dealing with\n> your own repositories anyway), and they already know which directory\n> in the server filesystem to hand to the upload-pack and friends, so\n> it would be a matter of passing GIT_DIR=$there when they call into the\n> run_command() API, if they are not already doing so.\n"},{"id":"453670","messageId":"kl6lee1z8mcm.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57685","inReplyTo":"220407.86lewhc6bz.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] fsck: detect bare repos in trees and warn","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-14T20:02:33Z","receivedAt":"2022-04-14T20:02:46Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Thu, Apr 07 2022, Derrick Stolee wrote:\n>\n>> A more complete protection here would be:\n>>\n>> 1. Warn when finding a bare repo as a tree (this patch).\n>>\n>> 2. Suppress warnings on trusted repos, scoped to a specific set of known\n>> trees _or_ based on some set of known commits (in case the known trees\n>> are too large).\n>>\n>> 3. Prevent writing a bare repo to the worktree, unless the user provided\n>> an opt-in to that behavior.\n>>\n>> Since your patch is moving in the right direction here, I don't think\n>> steps (2) and (3) are required to move forward with your patch. However,\n>> it is a good opportunity to discuss the full repercussions of this issue.\n>\n> Isn't a gentler solution here to:\n>\n> 1. In setup.c, we detect a repo\n> 2. Walk up a directory\n> 3. Do we find a repo?\n> 4. Does that repo \"contain\" the first one?\n> If yes: die on setup\n> If no: it's OK\n>\n> It also seems to me that there's pretty much perfect overlap between\n> this and the long-discussed topic of marking a submodule with config\n> v.s. detecting it on the fly.\n\nYour suggestion seems similar to:\n\n == 3. Detect that we are in an embedded bare repo and ignore the embedded bare\n repository in favor of the containing repo.\n\nwhich I also think is a simple, robust mitigation if we put aside the\nproblem of walking up to the root in too many situations. I seem to\nrecall that this problem has come up before in [1] (and possibly other\ntopics? I wasn't really able to locate them through a cursory search..),\nso I assume that's what you're referring to by \"long-discussed topic\".\n\n(Forgive me if I'm asking you to repeat yourself yet another time) I\nseem to recall that we weren't able to reach consensus on whether it's\nokay for Git to opportunistically walk up the directory hierarchy during\nsetup, especially since There are some situations where this is\nextremely expensive (VFS, network mount).\n\nI actually like this option quite a lot, but I don't see how we could\nimplement this without imposing a big penalty to all bare repo users -\nthey'd either be forced to set GIT_DIR or GIT_CEILING_DIRECTORIES, or\ntake a (potentially big) performance hit. Hopefully I'm just framing\nthis too narrowly and you're approaching this differently.\n\nPS: As an aside, wouldn't this also break libgit2? We could make this\nopt-out behavior, though that requires us to read system config _before_\ndiscovering the gitdir (as I discussed in [2]).\n\n[1] https://lore.kernel.org/git/211109.86v912dtfw.gmgdl@evledraar.gmail.com/\n[2] https://lore.kernel.org/git/kl6lv8vc90ts.fsf@chooglen-macbookpro.roam.corp.google.com\n"},{"id":"453697","messageId":"220415.861qxycx03.gmgdl@evledraar.gmail.com","threadId":"57685","inReplyTo":"kl6lee1z8mcm.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH] fsck: detect bare repos in trees and warn","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-15T12:46:15Z","receivedAt":"2022-04-15T13:11:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 14 2022, Glen Choo wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> On Thu, Apr 07 2022, Derrick Stolee wrote:\n>>\n>>> A more complete protection here would be:\n>>>\n>>> 1. Warn when finding a bare repo as a tree (this patch).\n>>>\n>>> 2. Suppress warnings on trusted repos, scoped to a specific set of known\n>>> trees _or_ based on some set of known commits (in case the known trees\n>>> are too large).\n>>>\n>>> 3. Prevent writing a bare repo to the worktree, unless the user provided\n>>> an opt-in to that behavior.\n>>>\n>>> Since your patch is moving in the right direction here, I don't think\n>>> steps (2) and (3) are required to move forward with your patch. However,\n>>> it is a good opportunity to discuss the full repercussions of this issue.\n>>\n>> Isn't a gentler solution here to:\n>>\n>> 1. In setup.c, we detect a repo\n>> 2. Walk up a directory\n>> 3. Do we find a repo?\n>> 4. Does that repo \"contain\" the first one?\n>> If yes: die on setup\n>> If no: it's OK\n>>\n>> It also seems to me that there's pretty much perfect overlap between\n>> this and the long-discussed topic of marking a submodule with config\n>> v.s. detecting it on the fly.\n>\n> Your suggestion seems similar to:\n>\n> == 3. Detect that we are in an embedded bare repo and ignore the embedded bare\n> repository in favor of the containing repo.\n>\n> which I also think is a simple, robust mitigation if we put aside the\n> problem of walking up to the root in too many situations. I seem to\n> recall that this problem has come up before in [1] (and possibly other\n> topics? I wasn't really able to locate them through a cursory search..),\n> so I assume that's what you're referring to by \"long-discussed topic\".\n\nYes, I mean the submodule.superprojectGitDir topic.\n\n> (Forgive me if I'm asking you to repeat yourself yet another time) I\n> seem to recall that we weren't able to reach consensus on whether it's\n> okay for Git to opportunistically walk up the directory hierarchy during\n> setup, especially since There are some situations where this is\n> extremely expensive (VFS, network mount).\n\nI'm not sure, but I think per the later\nhttps://lore.kernel.org/git/220204.86pmo34d2m.gmgdl@evledraar.gmail.com/\nand\nhttps://lore.kernel.org/git/220311.8635joj0lf.gmgdl@evledraar.gmail.com/\nthat any optimization concerns were likely just \"this is slow in\nshellscript\" and not at the FS level.\n\nThere were also passing references to some internal Google-specific\nNFS-ish implementation that I know nothing about (but you might),\ni.e. what I asked about in:\nhttps://lore.kernel.org/git/220212.864k53yfws.gmgdl@evledraar.gmail.com/\n\nBut given the v9 superprojectGitDir becoming a boolean instead of a path\nin v9 I'm not sure/have no idea.\n\nThe only thing I'm sure of is if past iterations of the series were\naddressing such a problem as an optimization that doesn't seem to be a\ncurrent goal.\n\nAs noted in those past exchanges I have tested this method on e.g. AIX\nwhose FS is unbelievably slow, and I couldn't even tell the differenc.\n\nThat's because if you look at the total FS syscalls even for an\nuninitialized repo just traversing .git, getting config etc. is going to\ndwarf \"walking up\" in terms of number of calls.\n\nOf course not all calls are going to be equal, and there's that\npotential \"I'm not NFS-y, but a parent is\" case etc.\n\nIn any case, I think even *if* we had such a case somewhere that this\nplan would still make sense. Such users could simply set\nGIT_CEILING_DIRECTORIES or something similar if they cared about the\nperformance.\n\nBut for everyone else we'd do the right thing, and not prematurely\noptimize. I.e. we actually *are* concerned not with \"does it look like a\nbare repo?\" but \"is this thing that looks like a bare repo within our\ncurrent actual repo or not?\".\n\n> I actually like this option quite a lot, but I don't see how we could\n> implement this without imposing a big penalty to all bare repo users -\n> they'd either be forced to set GIT_DIR or GIT_CEILING_DIRECTORIES, or\n> take a (potentially big) performance hit. Hopefully I'm just framing\n> this too narrowly and you're approaching this differently.\n\nAs noted in the [1] you quoted (link below) I tried to quantify that\npotential penalty, and it seems to be a complete non-issue.\n\nOf course there may be other scenarios where it matters, but I haven't\nseen any concrete data to support that.\n\nDoesn't pretty everyone who cares about the performance of bare in any\ncapacity do so because they're running a server that's using\ngit-upload-pack and the like? Those require you to specify the exact\n.git directory you want.\n\nI.e. wouldn't this *only* apply to those doing the equivalent of \"git -C\nsome-dir\" to \"cd\" to a bare repo?\n\n> PS: As an aside, wouldn't this also break libgit2? We could make this\n> opt-out behavior, though that requires us to read system config _before_\n> discovering the gitdir (as I discussed in [2]).\n\nNo it wouldn't? I don't use libgit2, but upthread there's concern that\nbanning things that look-like-a-repo from being tracked would break it.\n\nWhereas I'm pointing out that we don't need to do that, we can just keep\nsearching upwards.\n\nBut yes, it would \"break\" anything that assumed you could cd to that\ntracked-looks-like-or-is--a-gitdir and have e.g. \"git config\" pick up\nits config instead of our \"real repo\" config, but that's exactly what we\nwant in this case isn't it?\n\nI'm just pointing out that we can do it on the fly in setup.c, instead\nof forbidding such content from ever being tracked within the\nrepository, which we'd be doing because we know we're doing the wrong\nthing in that setup.c codepath.\n\nLet's just fix that bit in setup.c instead.\n\n> [1] https://lore.kernel.org/git/211109.86v912dtfw.gmgdl@evledraar.gmail.com/\n> [2] https://lore.kernel.org/git/kl6lv8vc90ts.fsf@chooglen-macbookpro.roam.corp.google.com\n\n"},{"id":"453720","messageId":"xmqqh76ucdg6.fsf@gitster.g","threadId":"57685","inReplyTo":"kl6lh76v8vnj.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-15T20:13:29Z","receivedAt":"2022-04-15T20:13:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> Yes, I mean that even the current directory will be ignored when\n> discovery is disabled.\n>\n>> I am not sure that\n>> is realistically feasible (I am thinking of cases like \"git fetch\"\n>> going to the remote repository on the local disk that is bare to run\n>> \"git upload-pack\"), but if the fallout is not too bad, it may be a\n>> good heuristics.\n>\n> Good detail - I hadn't considered the impact on our own child processes.\n> I suspect this might be a huge undertaking. Unless there is significant\n> interest in this option, I probably won't pursue it further.\n\nSo, I was doing a bit of experiment. The change to the set-up\nessentially means that working in either a bare repository or in the\n.git/ subdirectory of a non-bare repository that you used to be able\nto do with\n\n cd bare.git && git cmd\n cd .git/ && git cmd\n\nis now forbidden, unless you explicitly say which directory you want\nto use as the GIT_DIR, i.e.\n\n cd bare.git && GIT_DIR=. git cmd\n cd .git/ && GIT_DIR=. git cmd\n\nThe required change to the code is surprisingly small, but the\nfallout is very big. Partial patches to two tests are attached with\nsome commentary at the end.\n\n setup.c | 9 +++++++--\n\ndiff --git c/setup.c w/setup.c\nindex a7b36f3ffb..52304ae57e 100644\n--- c/setup.c\n+++ w/setup.c\n@@ -1203,12 +1203,15 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t * Test in the following order (relative to the dir):\n \t * - .git (file containing \"gitdir: \")\n \t * - .git/\n-\t * - ./ (bare)\n+\t * - ./ (bare) --- not\n \t * - ../.git\n \t * - ../.git/\n-\t * - ../ (bare)\n+\t * - ../ (bare) --- not\n \t * - ../../.git\n \t * etc.\n+\t * we used to test the directory itself as a bare one\n+\t * after checking if the directory has .git/ that is a repository\n+\t * and before moving up the hierarchy, but no longer do so.\n \t */\n \tone_filesystem = !git_env_bool(\"GIT_DISCOVERY_ACROSS_FILESYSTEM\", 0);\n \tif (one_filesystem)\n@@ -1238,12 +1241,14 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t\treturn GIT_DIR_DISCOVERED;\n \t\t}\n \n+#if 0\n \t\tif (is_git_directory(dir->buf)) {\n \t\t\tif (!ensure_valid_ownership(dir->buf))\n \t\t\t\treturn GIT_DIR_INVALID_OWNERSHIP;\n \t\t\tstrbuf_addstr(gitdir, \".\");\n \t\t\treturn GIT_DIR_BARE;\n \t\t}\n+#endif\n \n \t\tif (offset <= min_offset)\n \t\t\treturn GIT_DIR_HIT_CEILING;\n\n\nTHe above is all it takes to disable the recognition of a bare\nrepository during the repositiry discovery. The code to handle\nexplicitly specified GIT_DIR is way before the loop being touched\nhere and works OK as before.\n\nInterestingly, many breakages in tests are not due to \"clone\"\nfailing to operate in \"the other side\". For example, this one tries\nto inspect a bare repository that was created by a clone operation,\nand we need to tell \"git\" where the GIT_DIR is, even after we chdir\nto it. I do not think this is so common but with our popularity,\nnot-so-common population is still an absolute large number of users\nthat we do not want to hurt.\n\ndiff --git c/t/t5601-clone.sh w/t/t5601-clone.sh\nindex 4a61f2c901..822ee3164a 100755\n--- c/t/t5601-clone.sh\n+++ w/t/t5601-clone.sh\n@@ -111,9 +111,9 @@ test_expect_success 'clone --mirror' '\n \tgit clone --mirror src mirror &&\n \ttest -f mirror/HEAD &&\n \ttest ! -f mirror/file &&\n-\tFETCH=\"$(cd mirror && git config remote.origin.fetch)\" &&\n+\tFETCH=\"$(cd mirror && GIT_DIR=. git config remote.origin.fetch)\" &&\n \ttest \"+refs/*:refs/*\" = \"$FETCH\" &&\n-\tMIRROR=\"$(cd mirror && git config --bool remote.origin.mirror)\" &&\n+\tMIRROR=\"$(cd mirror && GIT_DIR=. git config --bool remote.origin.mirror)\" &&\n \ttest \"$MIRROR\" = true\n \n '\n@@ -151,7 +151,7 @@ test_expect_success 'clone --mirror does not repeat tags' '\n \t git tag some-tag HEAD) &&\n \tgit clone --mirror src mirror2 &&\n \t(cd mirror2 &&\n-\t git show-ref 2> clone.err > clone.out) &&\n+\t GIT_DIR=. git show-ref 2>clone.err >clone.out) &&\n \t! grep Duplicate mirror2/clone.err &&\n \tgrep some-tag mirror2/clone.out\n \n\n\nThe next example is \"init\" test, which seems to reveal that refusing\nto discover a bare repository is not exactly an approach that is\nworkable.\n\nThe first two hunks just show the same pattern as above. The\ncheck_config helper is given a directory, under which 'config' and\n'refs' should exist (in other words, the first part of the helper\nemulates setup.c::is_git_directory()). Then it tries to read from\nthe configuration file found in that directory. Be it a bare\nrepository or .git/ subdirectory of a non-bare repository the caller\nhas given us, we need to do the same GIT_DIR=. dance as the above.\n\nThe last hunk does not really fix. The scenario is this:\n\n * A bare repository bare-ancestor-aliased.git is prepared\n\n * Its config file says \"[alias] aliasedinit=init\" in it\n\n * In that bare repository (next to 'refs', 'config'), we create\n a subdirectory 'plain-nested'.\n\n * We go to that bare-ancestor-aliased.git/plain-nested/\n directory and say \"git aliasedinit\".\n\nThis wants to be able to create a subdirectory in this bare\nrepository and make that subdirectory an independent repository (it\nhappens to be a non-bare repository, but a bare repository should\nalso work---think of .git/modules/name/ bare repository that is\ndesigned to be pointed at with gitfile: link from a submodule\nworking tree).\n\nNow, in order for the last step to be able to even _use_ the alias,\nit has to treat the bare-ancestor-aliased.git directory as a bare\nrepository and read its configuration. But by explicitly setting\nGIT_DIR to that location, the behaviour of \"git init\" itself is\nchanged. It no longer initializes the current directory,\ni.e. plain-nested subdirectory of the bare-ancestor-aliased.git;\nGIT_DIR tells us to (re)initialize it instead.\n\nThis fundamentally does not work.\n\nIt is a separate matter if it makes sense to allow creating a new\nrepository inside a bare repository (it does---that is what the\nmodern submodule layout uses), or to allow an alias to help doing so\ndefined in the top-level bare repository (it probably does---the\nend-user may want to have a handy way to customize how submodules\nare configured). But it seems to tell us that with today's external\ninterface we have for \"git init\", we cannot take configuration from\na shallower level and use it to drive \"git init\" to create a new\nrepository at a deeper level.\n\ndiff --git c/t/t0001-init.sh w/t/t0001-init.sh\nindex d479303efa..57f2eedac0 100755\n--- c/t/t0001-init.sh\n+++ w/t/t0001-init.sh\n@@ -6,6 +6,7 @@ TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n check_config () {\n+\t# NEEDSWORK: refs could be a file in post reftable world.\n \tif test_path_is_dir \"$1\" &&\n \t test_path_is_file \"$1/config\" && test_path_is_dir \"$1/refs\"\n \tthen\n@@ -21,8 +22,8 @@ check_config () {\n \t\treturn 1\n \tfi\n \n-\tbare=$(cd \"$1\" && git config --bool core.bare)\n-\tworktree=$(cd \"$1\" && git config core.worktree) ||\n+\tbare=$(cd \"$1\" && GIT_DIR=. git config --bool core.bare)\n+\tworktree=$(cd \"$1\" && GIT_DIR=. git config core.worktree) ||\n \tworktree=unset\n \n \ttest \"$bare\" = \"$2\" && test \"$worktree\" = \"$3\" || {\n@@ -84,7 +85,7 @@ test_expect_success 'plain nested in bare through aliased command' '\n \t\techo \"[alias] aliasedinit = init\" >>config &&\n \t\tmkdir plain-nested &&\n \t\tcd plain-nested &&\n-\t\tgit aliasedinit\n+\t\tGIT_DIR=.. git aliasedinit ;# does not work\n \t) &&\n \tcheck_config bare-ancestor-aliased.git/plain-nested/.git false unset\n '\n"},{"id":"453726","messageId":"kl6lbkx2821q.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57685","inReplyTo":"xmqqbkx31sc0.fsf@gitster.g","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-15T21:33:21Z","receivedAt":"2022-04-15T21:33:33Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> Glen Choo writes:\n>\n>> Yes, I mean that even the current directory will be ignored when\n>> discovery is disabled.\n>\n> OK.\n>\n>>> I am not sure that\n>>> is realistically feasible (I am thinking of cases like \"git fetch\"\n>>> going to the remote repository on the local disk that is bare to run\n>>> \"git upload-pack\"), but if the fallout is not too bad, it may be a\n>>> good heuristics.\n>>\n>> Good detail - I hadn't considered the impact on our own child processes.\n>> I suspect this might be a huge undertaking. Unless there is significant\n>> interest in this option, I probably won't pursue it further.\n>\n> I do not necessarily think so. The entry points to transport on the\n> server side are quite limited (and the client side is dealing with\n> your own repositories anyway), and they already know which directory\n> in the server filesystem to hand to the upload-pack and friends, so\n> it would be a matter of passing GIT_DIR=$there when they call into the\n> run_command() API, if they are not already doing so.\n\nFWIW I experimented with turning off bare repo recognition altogether\nand seeing what breaks.\n\nCI Run: https://github.com/chooglen/git/runs/6042600953?check_suite_focus=true\n\nAFAICT, most of the test failures are what we'd expect if we stopped\nrecognizing bare repos altogether. One stands out to me in particular\nthough - in t5550-http-fetch-dumb.sh, \n\n git clone $HTTPD_URL/dumb/repo.git clone-tmpl\n\nyields \n\n ++ git clone http://127.0.0.1:5550/dumb/repo.git clone-tmpl\n Cloning into 'clone-tmpl'...\n fatal: repository 'http://127.0.0.1:5550/dumb/repo.git/' not found\n\nThis sounds to me like Git isn't recognizing the static http files as a\nremote Git repo, and if so, --git-dir doesn't sound like it'll save us.\nBut I'm not that familiar with the transport code so I don't know if\nthis is a big deal or just a quirk with our httpd tests.\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n\ndiff --git a/setup.c b/setup.c\nindex 2600548776..794d9624d5 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1196,10 +1196,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t\treturn GIT_DIR_DISCOVERED;\n \t\t}\n \n-\t\tif (is_git_directory(dir->buf)) {\n-\t\t\tstrbuf_addstr(gitdir, \".\");\n-\t\t\treturn GIT_DIR_BARE;\n-\t\t}\n+\t\t/* if (is_git_directory(dir->buf)) { */\n+\t\t/* \tstrbuf_addstr(gitdir, \".\"); */\n+\t\t/* \treturn GIT_DIR_BARE; */\n+\t\t/* } */\n \n \t\tif (offset <= min_offset)\n \t\t\treturn GIT_DIR_HIT_CEILING;\n\n"},{"id":"453727","messageId":"xmqqk0bqat4d.fsf@gitster.g","threadId":"57685","inReplyTo":"kl6lbkx2821q.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-15T22:17:54Z","receivedAt":"2022-04-15T22:18:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> FWIW I experimented with turning off bare repo recognition altogether\n> and seeing what breaks.\n\nI guess our mails crossed ;-) I've done a similar one locally and\nsent a report out earlier, and I think the part of the function in\nquestion that we disabled is identical.\n\n> ++ git clone http://127.0.0.1:5550/dumb/repo.git clone-tmpl\n> Cloning into 'clone-tmpl'...\n> fatal: repository 'http://127.0.0.1:5550/dumb/repo.git/' not found\n>\n> This sounds to me like Git isn't recognizing the static http files as a\n> remote Git repo, and if so, --git-dir doesn't sound like it'll save us.\n\nIf the http server side we ship _depends_ on the fact that we\nhistorically consider that it is enough to chdir into a directory to\nuse that directory as a bare repository, it is not all that\nsurprising that the server side infrastructure needs to do an\nequivalent of \"export GIT_DIR=.\" in addition to chdir it already\ndoes into the directory. There may be other things that the http\nresponder in the affected test needs to do before it can recognize\nthat the dumb/repo.git URL refers to a valid bare repository, and\nuntil that happens, the above experiment may not start working.\n\nI am not worried about that kind for breakage all that much, because\nit is entirely under _our_ control how an HTTP request received\nresults in a \"git\" invocation, how \"git clone/fetch ssh://\" invokes\nthe process that runs \"git upload-pack\" on the other side in the\ndirectory requested, etc.\n\nWhat worries me more is the effect on _other_ people's server\nimplementations (\"server\", because that is one major class of use\ncase of bare repositories). Essentially they need to identify the\nplaces where they depend on the current behaviour (i.e. going to the\nbare repository is enough to use it) and export GIT_DIR=. when they\ninvoke \"git\" there. The actual change that is necessary might be\nsmall, but identifying the places that need such changes may be\nadded burden to them.\n\nThanks.\n"},{"id":"453731","messageId":"kl6l8rs67yt8.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57685","inReplyTo":"kl6lbkx2821q.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-15T22:43:15Z","receivedAt":"2022-04-15T22:43:27Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Glen Choo writes:\n\n> Junio C Hamano writes:\n>\n>> Glen Choo writes:\n>>\n>>> Yes, I mean that even the current directory will be ignored when\n>>> discovery is disabled.\n>>\n>> OK.\n>>\n>>>> I am not sure that\n>>>> is realistically feasible (I am thinking of cases like \"git fetch\"\n>>>> going to the remote repository on the local disk that is bare to run\n>>>> \"git upload-pack\"), but if the fallout is not too bad, it may be a\n>>>> good heuristics.\n>>>\n>>> Good detail - I hadn't considered the impact on our own child processes.\n>>> I suspect this might be a huge undertaking. Unless there is significant\n>>> interest in this option, I probably won't pursue it further.\n>>\n>> I do not necessarily think so. The entry points to transport on the\n>> server side are quite limited (and the client side is dealing with\n>> your own repositories anyway), and they already know which directory\n>> in the server filesystem to hand to the upload-pack and friends, so\n>> it would be a matter of passing GIT_DIR=$there when they call into the\n>> run_command() API, if they are not already doing so.\n>\n> FWIW I experimented with turning off bare repo recognition altogether\n> and seeing what breaks.\n>\n> CI Run: https://github.com/chooglen/git/runs/6042600953?check_suite_focus=true\n>\n> AFAICT, most of the test failures are what we'd expect if we stopped\n> recognizing bare repos altogether. One stands out to me in particular\n> though - in t5550-http-fetch-dumb.sh, \n>\n> git clone $HTTPD_URL/dumb/repo.git clone-tmpl\n>\n> yields \n>\n> ++ git clone http://127.0.0.1:5550/dumb/repo.git clone-tmpl\n> Cloning into 'clone-tmpl'...\n> fatal: repository 'http://127.0.0.1:5550/dumb/repo.git/' not found\n>\n> This sounds to me like Git isn't recognizing the static http files as a\n> remote Git repo, and if so, --git-dir doesn't sound like it'll save us.\n> But I'm not that familiar with the transport code so I don't know if\n> this is a big deal or just a quirk with our httpd tests.\n\nOk I think this is a false alarm - the previous test does some setup on\nthe server, which is a bare repo. It was the _setup_ that was broken,\nnot the actual `git clone`.\n"},{"id":"453739","messageId":"kl6l5yn99ahv.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57685","inReplyTo":"xmqqh76ucdg6.fsf@gitster.g","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-15T23:45:32Z","receivedAt":"2022-04-15T23:45:43Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> Glen Choo writes:\n>\n>> Yes, I mean that even the current directory will be ignored when\n>> discovery is disabled.\n>>\n>>> I am not sure that\n>>> is realistically feasible (I am thinking of cases like \"git fetch\"\n>>> going to the remote repository on the local disk that is bare to run\n>>> \"git upload-pack\"), but if the fallout is not too bad, it may be a\n>>> good heuristics.\n>>\n>> Good detail - I hadn't considered the impact on our own child processes.\n>> I suspect this might be a huge undertaking. Unless there is significant\n>> interest in this option, I probably won't pursue it further.\n>\n> So, I was doing a bit of experiment. The change to the set-up\n> essentially means that working in either a bare repository or in the\n> .git/ subdirectory of a non-bare repository that you used to be able\n> to do with\n>\n> cd bare.git && git cmd\n> cd .git/ && git cmd\n>\n> is now forbidden, unless you explicitly say which directory you want\n> to use as the GIT_DIR, i.e.\n>\n> cd bare.git && GIT_DIR=. git cmd\n> cd .git/ && GIT_DIR=. git cmd\n>\n> The required change to the code is surprisingly small, but the\n> fallout is very big. Partial patches to two tests are attached with\n> some commentary at the end.\n\nThanks for running the experiment and the thorough writeup! Within the\nGoogle Git team, there was quite a bit of interest in this approach\nbecause of how simple it is, but the projected impact on users is\npotentially very high; this experiment is a great starting heuristic.\n\n> Interestingly, many breakages in tests are not due to \"clone\"\n> failing to operate in \"the other side\". For example, this one tries\n> to inspect a bare repository that was created by a clone operation,\n> and we need to tell \"git\" where the GIT_DIR is, even after we chdir\n> to it. I do not think this is so common but with our popularity,\n> not-so-common population is still an absolute large number of users\n> that we do not want to hurt.\n>\n> diff --git c/t/t5601-clone.sh w/t/t5601-clone.sh\n> index 4a61f2c901..822ee3164a 100755\n> --- c/t/t5601-clone.sh\n> +++ w/t/t5601-clone.sh\n> @@ -111,9 +111,9 @@ test_expect_success 'clone --mirror' '\n> \tgit clone --mirror src mirror &&\n> \ttest -f mirror/HEAD &&\n> \ttest ! -f mirror/file &&\n> -\tFETCH=\"$(cd mirror && git config remote.origin.fetch)\" &&\n> +\tFETCH=\"$(cd mirror && GIT_DIR=. git config remote.origin.fetch)\" &&\n> \ttest \"+refs/*:refs/*\" = \"$FETCH\" &&\n> -\tMIRROR=\"$(cd mirror && git config --bool remote.origin.mirror)\" &&\n> +\tMIRROR=\"$(cd mirror && GIT_DIR=. git config --bool remote.origin.mirror)\" &&\n> \ttest \"$MIRROR\" = true\n> \n> '\n> @@ -151,7 +151,7 @@ test_expect_success 'clone --mirror does not repeat tags' '\n> \t git tag some-tag HEAD) &&\n> \tgit clone --mirror src mirror2 &&\n> \t(cd mirror2 &&\n> -\t git show-ref 2> clone.err > clone.out) &&\n> +\t GIT_DIR=. git show-ref 2>clone.err >clone.out) &&\n> \t! grep Duplicate mirror2/clone.err &&\n> \tgrep some-tag mirror2/clone.out\n\nThe assumption when I made this proposal is that --git-dir is a\nsufficient workaround when we don't detect bare repos. As such, I'm of\nthe opinion that this is the \"expected\" result of such a change - we\nreally do expect that `git clone --bare` no longer works without\n--git-dir.\n\nI don't think this case (by itself) means that we can ignore bare repos.\n\n> The next example is \"init\" test, which seems to reveal that refusing\n> to discover a bare repository is not exactly an approach that is\n> workable.\n>\n> The first two hunks just show the same pattern as above. The\n> check_config helper is given a directory, under which 'config' and\n> 'refs' should exist (in other words, the first part of the helper\n> emulates setup.c::is_git_directory()). Then it tries to read from\n> the configuration file found in that directory. Be it a bare\n> repository or .git/ subdirectory of a non-bare repository the caller\n> has given us, we need to do the same GIT_DIR=. dance as the above.\n>\n> The last hunk does not really fix. The scenario is this:\n>\n> * A bare repository bare-ancestor-aliased.git is prepared\n>\n> * Its config file says \"[alias] aliasedinit=init\" in it\n>\n> * In that bare repository (next to 'refs', 'config'), we create\n> a subdirectory 'plain-nested'.\n>\n> * We go to that bare-ancestor-aliased.git/plain-nested/\n> directory and say \"git aliasedinit\".\n>\n> This wants to be able to create a subdirectory in this bare\n> repository and make that subdirectory an independent repository (it\n> happens to be a non-bare repository, but a bare repository should\n> also work---think of .git/modules/name/ bare repository that is\n> designed to be pointed at with gitfile: link from a submodule\n> working tree).\n>\n> Now, in order for the last step to be able to even _use_ the alias,\n> it has to treat the bare-ancestor-aliased.git directory as a bare\n> repository and read its configuration. But by explicitly setting\n> GIT_DIR to that location, the behaviour of \"git init\" itself is\n> changed. It no longer initializes the current directory,\n> i.e. plain-nested subdirectory of the bare-ancestor-aliased.git;\n> GIT_DIR tells us to (re)initialize it instead.\n\nHowever, this case shows that --git-dir won't work at all with \"git\ninit\", and I wouldn't be surprised if there are other breakages hiding\njust out of plain sight. This worries me a lot more, and I think\ndisabling bare repo detection wholesale might be a nonstarter.\n\nI wonder if all we need to do is make setup.c a little smarter - we\nrecognize bare repos, but *only* if they are contained in a directory\nnamed `.git/`. This should fix \"git init\" I think, because even though\nwe'd ignore \"./ (bare)\", we'd recognize `../.git/` and we get the right\nbehavior again.\n\nI haven't tested it yet, but this proposal sounds like it has quite a\nfew merits to me:\n\n- Easy to explain.\n- Easy rationalization (\"We used to be quite cavalier about detecting\n bare repos, but now we're being more strict by default\").\n- Fixes the embedded bare repo problem (since we don't allow .git).\n- No-op and neglible performance hit for non-bare repo users, even if\n they occasionally run \"git\" inside \".git\".\n\nAs with the original proposal of \"ignore bare repos altogether\", this\nwill still be a headache for bare repo users (unless they don't mind\nrenaming their bare repo directory to \".git\"), so this behavior needs to\nbe configurable, but perhaps the fallout is small enough that this\ncould be a safe default.\n\n> This fundamentally does not work.\n>\n> It is a separate matter if it makes sense to allow creating a new\n> repository inside a bare repository (it does---that is what the\n> modern submodule layout uses), or to allow an alias to help doing so\n> defined in the top-level bare repository (it probably does---the\n> end-user may want to have a handy way to customize how submodules\n> are configured). But it seems to tell us that with today's external\n> interface we have for \"git init\", we cannot take configuration from\n> a shallower level and use it to drive \"git init\" to create a new\n> repository at a deeper level.\n> '\n"},{"id":"453742","messageId":"kl6l35id99ue.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57685","inReplyTo":"kl6l5yn99ahv.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-15T23:59:37Z","receivedAt":"2022-04-15T23:59:44Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\n> I wonder if all we need to do is make setup.c a little smarter - we\n> recognize bare repos, but *only* if they are contained in a directory\n> named `.git/`. This should fix \"git init\" I think, because even though\n> we'd ignore \"./ (bare)\", we'd recognize `../.git/` and we get the right\n> behavior again.\n> \n> > It is a separate matter if it makes sense to allow creating a new\n> > repository inside a bare repository (it does---that is what the\n> > modern submodule layout uses), or to allow an alias to help doing so\n> > defined in the top-level bare repository (it probably does---the\n> > end-user may want to have a handy way to customize how submodules\n> > are configured). But it seems to tell us that with today's external\n> > interface we have for \"git init\", we cannot take configuration from\n> > a shallower level and use it to drive \"git init\" to create a new\n> > repository at a deeper level.\n\nHah, I forgot that submodules are stored as `.git/modules/foo` and not\n`.git/modules/foo/.git`, so this doesn't preserve the existing behavior\nfor submodules nested > 1 level.\n"},{"id":"453748","messageId":"kl6lwnfp7tbc.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57685","inReplyTo":"kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-16T00:41:59Z","receivedAt":"2022-04-16T02:05:46Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\n(resending this because I think the original got blocked, and I messed\nup the CC list anyway.)\n\nHi all! Thanks for chiming in.\n\nI'm sorry I wasn't able to participate in a timely manner, and unfortunately\n(once again..), I will be out of the office (returning 27 Apr, PDT) and my\ncolleagues will be keeping tabs on the discussion in my stead (once again..).\n\nNevertheless, I think the conversation has been pretty fruitful, so I'm\noptimistic about next steps. Here's my understanding of the conversation thus\nfar - do chime in if you feel I'm off base or if I've missed something:\n\n* We all agree that something needs to be done about embedded bare repos. This\n is a pretty good starting point IMO, because we agree that 'do nothing' isn't\n a good response.\n\n* There are use cases for embedded bare repos that don't have great alternatives\n (e.g. libgit2 uses bare repos in its tests). Even if this workflow is frowned\n upon (I personally don't think we should support it), I don't think we're\n ready to categorically declare that Git should ban embedded bare repos\n altogether (e.g. the way we ban .GiT).\n\n* We want additional protection on the client besides `git fsck`; there are \n a few ways to do this:\n\n 1. Prevent checking out an embedded bare repo.\n 2. Detect if the bare repo is embedded and refuse to work with it.\n 3. Detect if the bare repo is embedded and do not read its config/hooks, but\n everything else still 'works'.\n 4. Don't detect bare repos.\n 5. Only detect bare repos that are named `.git` [1].\n\n (I've responded with my thoughts on each of these approaches in-thread).\n\nWith that in mind, here's what I propose we do next:\n\n* Merge the `git fsck` patch [2] if we think that it is useful despite the\n potentially huge number of false positives. That patch needs some fixing; I'll\n make the changes when I'm back.\n\n* I'll experiment with (5), and if it seems promising, I'll propose this as an\n opt-in feature, with the intent of making it opt-out in the future. We'll\n opt-into this at Google to help figure out if this is a good default or not.\n\n* If that direction turns out not to be promising, I'll pursue (1), since that\n is the only option that can be configured per-repo, which should hopefully\n minimize the fallout.\n\nGiven that this embedded bare repo problem has been known for a long time, I\ndon't think we need to rush out a fix, but (especially since I'll be OOO) I'm\nmore than happy for someone to take my ideas (or any ideas) and run with them.\n\n[1] https://lore.kernel.org/git/kl6l5yn99ahv.fsf@chooglen-macbookpro.roam.corp.google.com/\n[2] https://lore.kernel.org/git/20220406232231.47714-1-chooglen@google.com\n"},{"id":"453749","messageId":"xmqqy2057rmx.fsf@gitster.g","threadId":"57685","inReplyTo":"YloVFn0SC/SF33b3@nand.local","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-16T01:18:14Z","receivedAt":"2022-04-16T02:05:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Is the proposal to only detect bare repositories that are called `.git`?\n> I think that's what you're suggesting, though can't we just as easily\n> embed a bare repository named \".git\" in a clone as long as its not in\n> the root directory?\n\nI do not think \"you can use your bare repository as before ONLY if\nthe directory is named .git; otherwise you must use GIT_DIR to point\nat it\" would fly; the Glen's exception may help many uses of \".git\nsubdirectory of a non-bare repository as if it were a bare\" you can\nfind in tests, but does not help real-world use cases where there\nmay be bunch of bare repositories named \"$project.git\" at all.\n\nBut I have to point out that your attack above would not work, as we\ndo not allow \".git\" directory in the index to begin with. IOW, you\nas an attacker may be able to prepare such a tree with nonstandard\ntools, but the victim won't be able to check it out (and\nfsck-during-transfer would probably block the cloning).\n"},{"id":"453750","messageId":"YloTQH35r2xVdPm1@nand.local","threadId":"57685","inReplyTo":"xmqqk0bqat4d.fsf@gitster.g","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-04-16T00:52:16Z","receivedAt":"2022-04-16T02:06:05Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 15, 2022 at 03:17:54PM -0700, Junio C Hamano wrote:\n> What worries me more is the effect on _other_ people's server\n> implementations (\"server\", because that is one major class of use\n> case of bare repositories). Essentially they need to identify the\n> places where they depend on the current behaviour (i.e. going to the\n> bare repository is enough to use it) and export GIT_DIR=. when they\n> invoke \"git\" there. The actual change that is necessary might be\n> small, but identifying the places that need such changes may be\n> added burden to them.\n\nI'm definitely biased here, but I think that this is probably too big of\na burden to place on forge administrators.\n\nEvery repository on GitHub (and I suspect GitLab, too) is written as a\nbare repository on disk, and so having to inject `GIT_DIR=.` into every\ngit invocation, while doable, seems like an unnecessary pain to impose.\n\nI suppose you could hide this behavior behind an opt-in configuration\nsetting, e.g., `core.bareDiscovery` which determines whether or not we\nwill try to discover bare repositories without `GIT_DIR` in the\nenvironment. That setting could default to true to avoid breaking\nexisting use-cases.\n\nBut I think that's just masking the overall pain that this approach\nwould incur, without providing much protection, so I wouldn't be unhappy\nto see us pursue a different approach here.\n\nThanks,\nTaylor\n"},{"id":"453753","messageId":"YlocN5bbvPuN3Dn9@nand.local","threadId":"57685","inReplyTo":"xmqqy2057rmx.fsf@gitster.g","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-04-16T01:30:31Z","receivedAt":"2022-04-16T02:06:33Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 15, 2022 at 06:18:14PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > Is the proposal to only detect bare repositories that are called `.git`?\n> > I think that's what you're suggesting, though can't we just as easily\n> > embed a bare repository named \".git\" in a clone as long as its not in\n> > the root directory?\n>\n> I do not think \"you can use your bare repository as before ONLY if\n> the directory is named .git; otherwise you must use GIT_DIR to point\n> at it\" would fly; the Glen's exception may help many uses of \".git\n> subdirectory of a non-bare repository as if it were a bare\" you can\n> find in tests, but does not help real-world use cases where there\n> may be bunch of bare repositories named \"$project.git\" at all.\n\nAgreed.\n\n> But I have to point out that your attack above would not work, as we\n> do not allow \".git\" directory in the index to begin with. IOW, you\n> as an attacker may be able to prepare such a tree with nonstandard\n> tools, but the victim won't be able to check it out (and\n> fsck-during-transfer would probably block the cloning).\n\nMakes sense, and thanks for the reminder; I agree.\n\nThanks,\nTaylor\n"},{"id":"453755","messageId":"Ylobp7sntKeWTLDX@nand.local","threadId":"57685","inReplyTo":"kl6lwnfp7tbc.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-04-16T01:28:07Z","receivedAt":"2022-04-16T02:07:46Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 15, 2022 at 05:41:59PM -0700, Glen Choo wrote:\n> * We all agree that something needs to be done about embedded bare repos. This\n> is a pretty good starting point IMO, because we agree that 'do nothing' isn't\n> a good response.\n\nTo be totally honest, I am not absolutely convinced.\n\nI agree that it's sub-optimal that Git is an attack vector for remote\ncode execution, but I think there is significant social engineering\nrequired in order to meaningfully exploit this. Particularly because an\nattacker must convince their victim to:\n\n - clone the repository,\n - cd into the embedded bare repository, and\n - run a git command\n\nScripting around the output of git commands in your PS1 makes the latter\nmore likely, so I think it's worthwhile to explore how to either prevent\nthis type of attack or make it substantially less likely to have a user\nrun git commands that execute parts of the config opportunistically.\n\nThat said, I think there are other approaches that we could take that\nwould hopefully disrupt fewer existing workflows.\n\n> * There are use cases for embedded bare repos that don't have great alternatives\n> (e.g. libgit2 uses bare repos in its tests). Even if this workflow is frowned\n> upon (I personally don't think we should support it), I don't think we're\n> ready to categorically declare that Git should ban embedded bare repos\n> altogether (e.g. the way we ban .GiT).\n\nI think there are a handful of legitimate reasons that we might want to\ncontinue supporting this; for projects like libgit2 and Git LFS, it's\nuseful to have repositories in a known state to execute tests in. Having\nbare Git repositories contained in some \"test fixture\" directory is a\nreally easy way to do just that.\n\n> * We want additional protection on the client besides `git fsck`; there are\n> a few ways to do this:\n\nI'm a little late to chime into the thread, but I appreciate you\nsummarizing some of the approaches discussed so far. Let me add my\nthoughts on each of these in order:\n\n> 1. Prevent checking out an embedded bare repo.\n> 2. Detect if the bare repo is embedded and refuse to work with it.\n> 3. Detect if the bare repo is embedded and do not read its config/hooks, but\n> everything else still 'works'.\n> 4. Don't detect bare repos.\n> 5. Only detect bare repos that are named `.git` [1].\n>\n> (I've responded with my thoughts on each of these approaches in-thread).\n\n 1. Likely disrupts too many legitimate workflows for us to adopt\n without designing some way to declare an embedded bare repository\n is \"safe\".\n 2. Ditto.\n 3. This seems the most promising approach so far. Similar to (1), I\n would also want to make sure we provide an easy way to declare a\n bare repository as \"safe\" in order to avoid permanently disrupting\n valid workflows that have accumulated over the past >15 years.\n 4. Seems like this approach is too heavy-handed.\n 5. Ditto.\n\n> With that in mind, here's what I propose we do next:\n>\n> * Merge the `git fsck` patch [2] if we think that it is useful despite the\n> potentially huge number of false positives. That patch needs some fixing; I'll\n> make the changes when I'm back.\n\nIf there are lots of false positives, we should consider downgrading the\nseverity of the proposed `EMBEDDED_BARE_REPO` fsck check to \"info\". I'm\nnot clear if there is another reason why this patch would have a\nsignificant number of false positives (i.e., if the detection mechanism\nis over-zealous).\n\nBut if not, and this does detect only legitimate embedded bare\nrepositories, we should use it as a reminder to consider how many\nuse-cases and workflows will be affected by whatever approach we take\nhere, if any.\n\n> * I'll experiment with (5), and if it seems promising, I'll propose this as an\n> opt-in feature, with the intent of making it opt-out in the future. We'll\n> opt-into this at Google to help figure out if this is a good default or not.\n>\n> * If that direction turns out not to be promising, I'll pursue (1), since that\n> is the only option that can be configured per-repo, which should hopefully\n> minimize the fallout.\n\nHere's an alternative approach, which I haven't seen discussed thus far:\n\nWhen a bare repository is embedded in another repository, avoid reading\nits config by default. This means that most Git commands will still\nwork, but without the possibility of running any \"executable\" portions\nof the config. To opt-out (i.e., to allow legitimate use-cases to start\nreading embedded bare repository config again), the embedding repository\nwould have to set a multi-valued `safe.embeddedRepo` configuration. This\nwould specify a list of paths relative to the embedding repository's\nroot of known-safe bare repositories.\n\nI think there are a couple of desirable attributes of this approach:\n\n - It minimally disrupts bare repositories, restricting the change to\n only embedded repositories.\n - It allows most Git commands to continue working as expected (modulo\n reading the config), hopefully making the population of users whose\n workflows will suddenly break pretty small.\n - It requires the user to explicitly opt-in to the unsafe behavior,\n because an attacker could not influence the embedding repository's\n `safe.embeddedRepo` config.\n\nIf we were going to do something about this, I would strongly advocate\nfor something that resembles the above. Or at the very least, some\nsolution that captures the attributes I outlined there.\n\nI would be happy to work together with you (or anybody!) on developing\npatches in that direction, so let me know if you are interested in\ncoordinating our efforts.\n\n> Given that this embedded bare repo problem has been known for a long time, I\n> don't think we need to rush out a fix, but (especially since I'll be OOO) I'm\n> more than happy for someone to take my ideas (or any ideas) and run with them.\n\nNo rush. Regardless of your time out-of-office, we should take advantage\nof the fact that this is a long-known wart to carefully craft a solution\nthat provides some additional safety while disrupting as few existing\nworkflows as possible.\n\nThanks,\nTaylor\n"},{"id":"453756","messageId":"YloVFn0SC/SF33b3@nand.local","threadId":"57685","inReplyTo":"kl6l5yn99ahv.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-04-16T01:00:06Z","receivedAt":"2022-04-16T02:08:09Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 15, 2022 at 04:45:32PM -0700, Glen Choo wrote:\n> However, this case shows that --git-dir won't work at all with \"git\n> init\", and I wouldn't be surprised if there are other breakages hiding\n> just out of plain sight. This worries me a lot more, and I think\n> disabling bare repo detection wholesale might be a nonstarter.\n>\n> I wonder if all we need to do is make setup.c a little smarter - we\n> recognize bare repos, but *only* if they are contained in a directory\n> named `.git/`. This should fix \"git init\" I think, because even though\n> we'd ignore \"./ (bare)\", we'd recognize `../.git/` and we get the right\n> behavior again.\n>\n> I haven't tested it yet, but this proposal sounds like it has quite a\n> few merits to me:\n>\n> - Easy to explain.\n> - Easy rationalization (\"We used to be quite cavalier about detecting\n> bare repos, but now we're being more strict by default\").\n> - Fixes the embedded bare repo problem (since we don't allow .git).\n> - No-op and neglible performance hit for non-bare repo users, even if\n> they occasionally run \"git\" inside \".git\".\n>\n> As with the original proposal of \"ignore bare repos altogether\", this\n> will still be a headache for bare repo users (unless they don't mind\n> renaming their bare repo directory to \".git\"), so this behavior needs to\n> be configurable, but perhaps the fallout is small enough that this\n> could be a safe default.\n\nI think you are underestimating the fallout, at least if I'm\nunderstanding your proposal correctly.\n\nIs the proposal to only detect bare repositories that are called `.git`?\nI think that's what you're suggesting, though can't we just as easily\nembed a bare repository named \".git\" in a clone as long as its not in\nthe root directory?\n\nBut like I said in an earlier message, I think that this is vastly\nunderestimating the number of legitimate repositories that are stored as\nbare repos on disk (not embedded in a non-bare repo, nor named \".git\").\n\nThanks,\nTaylor\n"},{"id":"453758","messageId":"kl6lzgkl7to3.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57685","inReplyTo":"kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-16T00:34:20Z","receivedAt":"2022-04-16T02:49:00Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\nHi all! Thanks for chiming in.\n\nI'm sorry I wasn't able to participate in a timely manner, and unfortunately\n(once again..), I will be out of the office (returning 27 Apr, PDT) and my\ncolleagues will be keeping tabs on the discussion in my stead (once again..).\n\nNevertheless, I think the conversation has been pretty fruitful, so I'm\noptimistic about next steps. Here's my understanding of the conversation thus\nfar - do chime in if you feel I'm off base or if I've missed something:\n\n* We all agree that something needs to be done about embedded bare repos. This\n is a pretty good starting point IMO, because we agree that 'do nothing' isn't\n a good response.\n\n* There are use cases for embedded bare repos that don't have great alternatives\n (e.g. libgit2 uses bare repos in its tests). Even if this workflow is frowned\n upon (I personally don't think we should support it), I don't think we're\n ready to categorically declare that Git should ban embedded bare repos\n altogether (e.g. the way we ban .GiT).\n\n* We want additional protection on the client besides `git fsck`; there are \n a few ways to do this:\n\n 1. Prevent checking out an embedded bare repo.\n 2. Detect if the bare repo is embedded and refuse to work with it.\n 3. Detect if the bare repo is embedded and do not read its config/hooks, but\n everything else still 'works'.\n 4. Don't detect bare repos.\n 5. Only detect bare repos that are named `.git` [1].\n\n (I've responded with my thoughts on each of these approaches in-thread).\n\nWith that in mind, here's what I propose we do next:\n\n* Merge the `git fsck` patch [2] if we think that it is useful despite the\n potentially huge number of false positives. That patch needs some fixing; I'll\n make the changes when I'm back.\n\n* I'll experiment with (5), and if it seems promising, I'll propose this as an\n opt-in feature, with the intent of making it opt-out in the future. We'll\n opt-into this at Google to help figure out if this is a good default or not.\n\n* If that direction turns out not to be promising, I'll pursue (1), since that\n is the only option that can be configured per-repo, which should hopefully\n minimize the fallout.\n\nGiven that this embedded bare repo problem has been known for a long time, I\ndon't think we need to rush out a fix, but (especially since I'll be OOO) I'm\nmore than happy for someone to take my ideas (or any ideas) and run with them.\n\n[1] https://lore.kernel.org/git/kl6l5yn99ahv.fsf@chooglen-macbookpro.roam.corp.google.com/\n[2] https://lore.kernel.org/git/20220406232231.47714-1-chooglen@google.com\n"},{"id":"454130","messageId":"CAJoAoZkgnnvdymuBsM9Ja3+eYSnyohr=FQZMVX_uzZ_pkQhgaw@mail.gmail.com","threadId":"57685","inReplyTo":"Ylobp7sntKeWTLDX@nand.local","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2022-04-21T18:25:39Z","receivedAt":"2022-04-21T18:26:48Z","isPatch":false,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, Apr 15, 2022 at 6:28 PM Taylor Blau wrote:\n>\n> On Fri, Apr 15, 2022 at 05:41:59PM -0700, Glen Choo wrote:\n> > * We want additional protection on the client besides `git fsck`; there are\n> > a few ways to do this:\n>\n> I'm a little late to chime into the thread, but I appreciate you\n> summarizing some of the approaches discussed so far. Let me add my\n> thoughts on each of these in order:\n>\n> > 1. Prevent checking out an embedded bare repo.\n> > 2. Detect if the bare repo is embedded and refuse to work with it.\n> > 3. Detect if the bare repo is embedded and do not read its config/hooks, but\n> > everything else still 'works'.\n> > 4. Don't detect bare repos.\n> > 5. Only detect bare repos that are named `.git` [1].\n> >\n> > (I've responded with my thoughts on each of these approaches in-thread).\n>\n> 1. Likely disrupts too many legitimate workflows for us to adopt\n> without designing some way to declare an embedded bare repository\n> is \"safe\".\n> 2. Ditto.\n> 3. This seems the most promising approach so far. Similar to (1), I\n> would also want to make sure we provide an easy way to declare a\n> bare repository as \"safe\" in order to avoid permanently disrupting\n> valid workflows that have accumulated over the past >15 years.\n\nI'd like to think a little more about how we want to determine that a\nbare repo isn't embedded - wantonly scanning up the filesystem for any\ngitdir above the current one is really expensive. When I tried that\napproach for the purposes of including some shared config between\nsuperproject and submodules, it slowed down the Git test suite by\nsomething like 3-5x. So, I suppose that \"we think this is bare\" means\nrefs/ and objects/ present in a directory that isn't named '.git/',\nand then we hunt anywhere above us for another .git/? Of course being\ncareful not to accept another bare repo as the \"parent\" gitdir.\n\nDoes it work for submodules? I suppose it works for non-bare\nsubmodules - and for bare submodules, e.g.\n'submodule-having-project.git/modules/some-submodule/' we can rely on\nthe user to turn that flag on if they want their submodule's config\nand hooks to be noticed from the gitdir. (From\n'worktree-for-submodule-having-project/some-submodule' there is a\n'.git' pointer, so I'd expect things to work normally that way,\nright?)\n\nAs long as we are careful to avoid searching the filesystem in *every*\ncase, this seems like a pretty reasonable approach to me.\n\n> 4. Seems like this approach is too heavy-handed.\n> 5. Ditto.\n>\n> > With that in mind, here's what I propose we do next:\n> >\n> > * Merge the `git fsck` patch [2] if we think that it is useful despite the\n> > potentially huge number of false positives. That patch needs some fixing; I'll\n> > make the changes when I'm back.\n>\n> If there are lots of false positives, we should consider downgrading the\n> severity of the proposed `EMBEDDED_BARE_REPO` fsck check to \"info\". I'm\n> not clear if there is another reason why this patch would have a\n> significant number of false positives (i.e., if the detection mechanism\n> is over-zealous).\n>\n> But if not, and this does detect only legitimate embedded bare\n> repositories, we should use it as a reminder to consider how many\n> use-cases and workflows will be affected by whatever approach we take\n> here, if any.\n>\n> > * I'll experiment with (5), and if it seems promising, I'll propose this as an\n> > opt-in feature, with the intent of making it opt-out in the future. We'll\n> > opt-into this at Google to help figure out if this is a good default or not.\n> >\n> > * If that direction turns out not to be promising, I'll pursue (1), since that\n> > is the only option that can be configured per-repo, which should hopefully\n> > minimize the fallout.\n>\n> Here's an alternative approach, which I haven't seen discussed thus far:\n>\n> When a bare repository is embedded in another repository, avoid reading\n> its config by default. This means that most Git commands will still\n> work, but without the possibility of running any \"executable\" portions\n> of the config. To opt-out (i.e., to allow legitimate use-cases to start\n> reading embedded bare repository config again), the embedding repository\n> would have to set a multi-valued `safe.embeddedRepo` configuration. This\n> would specify a list of paths relative to the embedding repository's\n> root of known-safe bare repositories.\n>\n> I think there are a couple of desirable attributes of this approach:\n>\n> - It minimally disrupts bare repositories, restricting the change to\n> only embedded repositories.\n> - It allows most Git commands to continue working as expected (modulo\n> reading the config), hopefully making the population of users whose\n> workflows will suddenly break pretty small.\n> - It requires the user to explicitly opt-in to the unsafe behavior,\n> because an attacker could not influence the embedding repository's\n> `safe.embeddedRepo` config.\n>\n> If we were going to do something about this, I would strongly advocate\n> for something that resembles the above. Or at the very least, some\n> solution that captures the attributes I outlined there.\n\nNice - a more strict spin on proposal 3 from above, if I understand it\nright. Rather than allowing any and all bare repos, avoid someone\nsneaking in a malicious one next to legitimate ones by using an\nallowlist. Seems reasonable to me.\n\n - Emily\n"},{"id":"454131","messageId":"CAJoAoZkf6VuAOwX9j8Zc0x4HqJRJ5zQgqfmu+8Zs1kVx88dGpg@mail.gmail.com","threadId":"57685","inReplyTo":"CAJoAoZkgnnvdymuBsM9Ja3+eYSnyohr=FQZMVX_uzZ_pkQhgaw@mail.gmail.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2022-04-21T18:29:15Z","receivedAt":"2022-04-21T18:29:32Z","isPatch":false,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Apr 21, 2022 at 11:25 AM Emily Shaffer wrote:\n>\n> On Fri, Apr 15, 2022 at 6:28 PM Taylor Blau wrote:\n> >\n> > On Fri, Apr 15, 2022 at 05:41:59PM -0700, Glen Choo wrote:\n> > > * We want additional protection on the client besides `git fsck`; there are\n> > > a few ways to do this:\n> >\n> > I'm a little late to chime into the thread, but I appreciate you\n> > summarizing some of the approaches discussed so far. Let me add my\n> > thoughts on each of these in order:\n> >\n> > > 1. Prevent checking out an embedded bare repo.\n> > > 2. Detect if the bare repo is embedded and refuse to work with it.\n> > > 3. Detect if the bare repo is embedded and do not read its config/hooks, but\n> > > everything else still 'works'.\n> > > 4. Don't detect bare repos.\n> > > 5. Only detect bare repos that are named `.git` [1].\n> > >\n> > > (I've responded with my thoughts on each of these approaches in-thread).\n> >\n> > 1. Likely disrupts too many legitimate workflows for us to adopt\n> > without designing some way to declare an embedded bare repository\n> > is \"safe\".\n> > 2. Ditto.\n> > 3. This seems the most promising approach so far. Similar to (1), I\n> > would also want to make sure we provide an easy way to declare a\n> > bare repository as \"safe\" in order to avoid permanently disrupting\n> > valid workflows that have accumulated over the past >15 years.\n>\n> I'd like to think a little more about how we want to determine that a\n> bare repo isn't embedded - wantonly scanning up the filesystem for any\n> gitdir above the current one is really expensive. When I tried that\n> approach for the purposes of including some shared config between\n> superproject and submodules, it slowed down the Git test suite by\n> something like 3-5x. So, I suppose that \"we think this is bare\" means\n> refs/ and objects/ present in a directory that isn't named '.git/',\n> and then we hunt anywhere above us for another .git/? Of course being\n> careful not to accept another bare repo as the \"parent\" gitdir.\n>\n> Does it work for submodules? I suppose it works for non-bare\n> submodules - and for bare submodules, e.g.\n> 'submodule-having-project.git/modules/some-submodule/' we can rely on\n> the user to turn that flag on if they want their submodule's config\n> and hooks to be noticed from the gitdir. (From\n> 'worktree-for-submodule-having-project/some-submodule' there is a\n> '.git' pointer, so I'd expect things to work normally that way,\n> right?)\n>\n> As long as we are careful to avoid searching the filesystem in *every*\n> case, this seems like a pretty reasonable approach to me.\n>\n> > 4. Seems like this approach is too heavy-handed.\n> > 5. Ditto.\n> >\n> > > With that in mind, here's what I propose we do next:\n> > >\n> > > * Merge the `git fsck` patch [2] if we think that it is useful despite the\n> > > potentially huge number of false positives. That patch needs some fixing; I'll\n> > > make the changes when I'm back.\n> >\n> > If there are lots of false positives, we should consider downgrading the\n> > severity of the proposed `EMBEDDED_BARE_REPO` fsck check to \"info\". I'm\n> > not clear if there is another reason why this patch would have a\n> > significant number of false positives (i.e., if the detection mechanism\n> > is over-zealous).\n> >\n> > But if not, and this does detect only legitimate embedded bare\n> > repositories, we should use it as a reminder to consider how many\n> > use-cases and workflows will be affected by whatever approach we take\n> > here, if any.\n> >\n> > > * I'll experiment with (5), and if it seems promising, I'll propose this as an\n> > > opt-in feature, with the intent of making it opt-out in the future. We'll\n> > > opt-into this at Google to help figure out if this is a good default or not.\n> > >\n> > > * If that direction turns out not to be promising, I'll pursue (1), since that\n> > > is the only option that can be configured per-repo, which should hopefully\n> > > minimize the fallout.\n> >\n> > Here's an alternative approach, which I haven't seen discussed thus far:\n> >\n> > When a bare repository is embedded in another repository, avoid reading\n> > its config by default. This means that most Git commands will still\n> > work, but without the possibility of running any \"executable\" portions\n> > of the config. To opt-out (i.e., to allow legitimate use-cases to start\n> > reading embedded bare repository config again), the embedding repository\n> > would have to set a multi-valued `safe.embeddedRepo` configuration. This\n> > would specify a list of paths relative to the embedding repository's\n> > root of known-safe bare repositories.\n> >\n> > I think there are a couple of desirable attributes of this approach:\n> >\n> > - It minimally disrupts bare repositories, restricting the change to\n> > only embedded repositories.\n> > - It allows most Git commands to continue working as expected (modulo\n> > reading the config), hopefully making the population of users whose\n> > workflows will suddenly break pretty small.\n> > - It requires the user to explicitly opt-in to the unsafe behavior,\n> > because an attacker could not influence the embedding repository's\n> > `safe.embeddedRepo` config.\n> >\n> > If we were going to do something about this, I would strongly advocate\n> > for something that resembles the above. Or at the very least, some\n> > solution that captures the attributes I outlined there.\n>\n> Nice - a more strict spin on proposal 3 from above, if I understand it\n> right. Rather than allowing any and all bare repos, avoid someone\n> sneaking in a malicious one next to legitimate ones by using an\n> allowlist. Seems reasonable to me.\n\nAh, another thing I forgot to mention. There has been a little\ndiscussion in the past about isolating \"safe\" parts of config (and\ngitdir) from \"unsafe\" parts, e.g. \"which configs and embedded scripts\nare executables\", to help better protect from zipfile-type attacks,\nwhich are very similar to this kind of attack. I wonder if it makes\nsense to consider that sort of thing as a needswork for this bugfix?\ne.g. \"/* NEEDSWORK: Only ignore unsafe configs and hooks instead of\nignoring the entire embedded config and hooks in the future */\"?\n\n>\n> - Emily\n"},{"id":"454145","messageId":"xmqq4k2mi88p.fsf@gitster.g","threadId":"57685","inReplyTo":"CAJoAoZkf6VuAOwX9j8Zc0x4HqJRJ5zQgqfmu+8Zs1kVx88dGpg@mail.gmail.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-21T18:47:34Z","receivedAt":"2022-04-21T18:47:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n>> Nice - a more strict spin on proposal 3 from above, if I understand it\n>> right. Rather than allowing any and all bare repos, avoid someone\n>> sneaking in a malicious one next to legitimate ones by using an\n>> allowlist. Seems reasonable to me.\n>\n> Ah, another thing I forgot to mention. There has been a little\n> discussion in the past about isolating \"safe\" parts of config (and\n> gitdir) from \"unsafe\" parts, e.g. \"which configs and embedded scripts\n> are executables\", to help better protect from zipfile-type attacks,\n> which are very similar to this kind of attack. I wonder if it makes\n> sense to consider that sort of thing as a needswork for this bugfix?\n> e.g. \"/* NEEDSWORK: Only ignore unsafe configs and hooks instead of\n> ignoring the entire embedded config and hooks in the future */\"?\n\nThere have been such discussions in the past and they all went\nnowhere because such safe-listing fundamentally does not work. What\nyou consider \"safe\" today may turn out to be \"unsafe\" and in a later\nversion of Git will stop honoring it, and for those who depended on\nit being listed as \"safe\", such a security fix will be a regression.\n\nDisabling the whole thing if the file can be tainted is the only\nsensible way forward without promising users that they will be hurt\nby such changes/regressions in the future, I would think.\n\nSo, no, I do not think such a NEEDSWORK comment is welcome.\n\nThanks.\n\n\n"},{"id":"454147","messageId":"YmGoc8gwB3W/cfHM@nand.local","threadId":"57685","inReplyTo":"xmqq4k2mi88p.fsf@gitster.g","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-04-21T18:54:43Z","receivedAt":"2022-04-21T18:55:11Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 21, 2022 at 11:47:34AM -0700, Junio C Hamano wrote:\n> Emily Shaffer writes:\n>\n> > Ah, another thing I forgot to mention. There has been a little\n> > discussion in the past about isolating \"safe\" parts of config (and\n> > gitdir) from \"unsafe\" parts, e.g. \"which configs and embedded scripts\n> > are executables\", to help better protect from zipfile-type attacks,\n> > which are very similar to this kind of attack. I wonder if it makes\n> > sense to consider that sort of thing as a needswork for this bugfix?\n> > e.g. \"/* NEEDSWORK: Only ignore unsafe configs and hooks instead of\n> > ignoring the entire embedded config and hooks in the future */\"?\n>\n> There have been such discussions in the past and they all went\n> nowhere because such safe-listing fundamentally does not work. What\n> you consider \"safe\" today may turn out to be \"unsafe\" and in a later\n> version of Git will stop honoring it, and for those who depended on\n> it being listed as \"safe\", such a security fix will be a regression.\n>\n> Disabling the whole thing if the file can be tainted is the only\n> sensible way forward without promising users that they will be hurt\n> by such changes/regressions in the future, I would think.\n\nI assume when Junio says \"safe-listing\" he is talking about your \"which\nconfigs and embedded scripts are executables\". I have tossed that idea\naround in my own head for a little while, and in addition to the points\nthat Junio refers to, I think that this could be confusing for users.\n\nI worry about forcing the user to consider which parts of their\nconfig and hooks are being read/ignored, and then re-interpret their\nmeaning in light of that.\n\nThat seems like it would be an unnecessarily tricky position to put\nusers in, and I think we could get around it by either reading the\nconfig/hooks, or ignoring them entirely.\n\n(It also seems error-prone to me: just trying to list which parts of our\nconfig could lead to command-execution is challenging for me at least.\nIn addition to the ongoing maintenance cost, a clever attacker would\nalmost certainly be able to spot some obscure piece of config that we\ndidn't consider and then use it in their attack.)\n\nThanks,\nTaylor\n"},{"id":"454151","messageId":"YmGr0upicQJML+9B@nand.local","threadId":"57685","inReplyTo":"CAJoAoZkgnnvdymuBsM9Ja3+eYSnyohr=FQZMVX_uzZ_pkQhgaw@mail.gmail.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-04-21T19:09:06Z","receivedAt":"2022-04-21T19:09:11Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 21, 2022 at 11:25:39AM -0700, Emily Shaffer wrote:\n> On Fri, Apr 15, 2022 at 6:28 PM Taylor Blau wrote:\n> >\n> > On Fri, Apr 15, 2022 at 05:41:59PM -0700, Glen Choo wrote:\n> > > * We want additional protection on the client besides `git fsck`; there are\n> > > a few ways to do this:\n> >\n> > I'm a little late to chime into the thread, but I appreciate you\n> > summarizing some of the approaches discussed so far. Let me add my\n> > thoughts on each of these in order:\n> >\n> > > 1. Prevent checking out an embedded bare repo.\n> > > 2. Detect if the bare repo is embedded and refuse to work with it.\n> > > 3. Detect if the bare repo is embedded and do not read its config/hooks, but\n> > > everything else still 'works'.\n> > > 4. Don't detect bare repos.\n> > > 5. Only detect bare repos that are named `.git` [1].\n> > >\n> > > (I've responded with my thoughts on each of these approaches in-thread).\n> >\n> > 1. Likely disrupts too many legitimate workflows for us to adopt\n> > without designing some way to declare an embedded bare repository\n> > is \"safe\".\n> > 2. Ditto.\n> > 3. This seems the most promising approach so far. Similar to (1), I\n> > would also want to make sure we provide an easy way to declare a\n> > bare repository as \"safe\" in order to avoid permanently disrupting\n> > valid workflows that have accumulated over the past >15 years.\n>\n> I'd like to think a little more about how we want to determine that a\n> bare repo isn't embedded - wantonly scanning up the filesystem for any\n> gitdir above the current one is really expensive. When I tried that\n> approach for the purposes of including some shared config between\n> superproject and submodules, it slowed down the Git test suite by\n> something like 3-5x. So, I suppose that \"we think this is bare\" means\n> refs/ and objects/ present in a directory that isn't named '.git/',\n> and then we hunt anywhere above us for another .git/? Of course being\n> careful not to accept another bare repo as the \"parent\" gitdir.\n\nDefinitely worth considering. Fundamentally I think if you're going to\nallow checking out embedded bare repositories (which I strongly think\nthat we should), any search for \"is this repository embedded in another\none?\" needs to walk along its entire path up to the filesystem root.\n\nI'd think that we could offer some tools, perhaps \"safe.boundary\",\nspecifying a list of directories to halt our traversal at. IOW, if you\nhave a bunch of bare repositories underneath /data/repositories, and\nknow that they're all safe, you could set your boundary there to signal\nto Git \"if my path is a subdirectory of anything in safe.boundary, then\nI don't care whether the repo is embedded or not\".\n\n> Does it work for submodules? I suppose it works for non-bare\n> submodules - and for bare submodules, e.g.\n> 'submodule-having-project.git/modules/some-submodule/' we can rely on\n> the user to turn that flag on if they want their submodule's config\n> and hooks to be noticed from the gitdir. (From\n> 'worktree-for-submodule-having-project/some-submodule' there is a\n> '.git' pointer, so I'd expect things to work normally that way,\n> right?)\n\nI don't enough about submodules to comment here, sorry.\n\n> > Here's an alternative approach, which I haven't seen discussed thus\n> > far: [...]\n>\n> Nice - a more strict spin on proposal 3 from above, if I understand it\n> right. Rather than allowing any and all bare repos, avoid someone\n> sneaking in a malicious one next to legitimate ones by using an\n> allowlist. Seems reasonable to me.\n\nWe'd probably want to allow saying \"all embedded bare repositories are\nsafe to read config/hooks from\", too. I hadn't considered this approach\nas a way to read some embedded repos and not others; I suspect the\noverwhelmingly common use-case would be: `git config --local\nsafe.embeddedRepo '*'`.\n\nThanks,\nTaylor\n"},{"id":"454179","messageId":"CAJoAoZnd=BKycr0c71-BQJyO3zoymC7p++Zke+OSkV4neweAOQ@mail.gmail.com","threadId":"57685","inReplyTo":"YmGr0upicQJML+9B@nand.local","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2022-04-21T21:01:44Z","receivedAt":"2022-04-21T21:02:02Z","isPatch":false,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Apr 21, 2022 at 12:09 PM Taylor Blau wrote:\n> On Thu, Apr 21, 2022 at 11:25:39AM -0700, Emily Shaffer wrote:\n> > On Fri, Apr 15, 2022 at 6:28 PM Taylor Blau wrote:\n> > > 3. This seems the most promising approach so far. Similar to (1), I\n> > > would also want to make sure we provide an easy way to declare a\n> > > bare repository as \"safe\" in order to avoid permanently disrupting\n> > > valid workflows that have accumulated over the past >15 years.\n> >\n> > I'd like to think a little more about how we want to determine that a\n> > bare repo isn't embedded - wantonly scanning up the filesystem for any\n> > gitdir above the current one is really expensive. When I tried that\n> > approach for the purposes of including some shared config between\n> > superproject and submodules, it slowed down the Git test suite by\n> > something like 3-5x. So, I suppose that \"we think this is bare\" means\n> > refs/ and objects/ present in a directory that isn't named '.git/',\n> > and then we hunt anywhere above us for another .git/? Of course being\n> > careful not to accept another bare repo as the \"parent\" gitdir.\n>\n> Definitely worth considering. Fundamentally I think if you're going to\n> allow checking out embedded bare repositories (which I strongly think\n> that we should), any search for \"is this repository embedded in another\n> one?\" needs to walk along its entire path up to the filesystem root.\n>\n> I'd think that we could offer some tools, perhaps \"safe.boundary\",\n> specifying a list of directories to halt our traversal at. IOW, if you\n> have a bunch of bare repositories underneath /data/repositories, and\n> know that they're all safe, you could set your boundary there to signal\n> to Git \"if my path is a subdirectory of anything in safe.boundary, then\n> I don't care whether the repo is embedded or not\".\n\nAh, I'm sorry to say that I was more on board before I read this mail\nthan I am after :)\n\nI don't think there is much reason to continue searching above the\nfirst found '.git' because we disallow '.git' from being committed or\nchecked out, right? So this extra filesystem hunting and ceiling math\nseems unnecessary to me. I know I am slightly allergic to searching\nthe filesystem for a parent repo to begin with, so I'm sure I've got\nsome bias here ;)\n\n> > > Here's an alternative approach, which I haven't seen discussed thus\n> > > far: [...]\n> >\n> > Nice - a more strict spin on proposal 3 from above, if I understand it\n> > right. Rather than allowing any and all bare repos, avoid someone\n> > sneaking in a malicious one next to legitimate ones by using an\n> > allowlist. Seems reasonable to me.\n>\n> We'd probably want to allow saying \"all embedded bare repositories are\n> safe to read config/hooks from\", too. I hadn't considered this approach\n> as a way to read some embedded repos and not others; I suspect the\n> overwhelmingly common use-case would be: `git config --local\n> safe.embeddedRepo '*'`.\n\nAh, I dislike this option for the exact reason I mentioned - avoiding\na malicious repo being snuck in next to legitimate repos. I'd prefer\nto rely on exact matching only - but as the config needs to be set by\nevery contributor every time the set of bare repos changes, that\nsounds impossible to manage for a project which may be constantly\nadding and removing these repos.\n\nThat said.... I'm biased again, but if you want to be certain of the\nstate of another repository in order to run tests in it... why not use\na submodule for that repository? Not a helpful comment for those\nalready using embedded bare repos, though ;)\n\nThanks for the food for thought.\n - Emily\n"},{"id":"454181","messageId":"YmHK/GJ3qa7QuVUD@nand.local","threadId":"57685","inReplyTo":"CAJoAoZnd=BKycr0c71-BQJyO3zoymC7p++Zke+OSkV4neweAOQ@mail.gmail.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-04-21T21:22:04Z","receivedAt":"2022-04-21T21:22:14Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 21, 2022 at 02:01:44PM -0700, Emily Shaffer wrote:\n> I don't think there is much reason to continue searching above the\n> first found '.git' because we disallow '.git' from being committed or\n> checked out, right? So this extra filesystem hunting and ceiling math\n> seems unnecessary to me. I know I am slightly allergic to searching\n> the filesystem for a parent repo to begin with, so I'm sure I've got\n> some bias here ;)\n\nRight; once we find a non-bare embedding repository we should absolutely\nstop our search. I was suggesting we could stop our search earlier for\nenvironments that use non-embedded bare repositories with a fairly deep\ndirectory structure.\n\nE.g., if you store lots of bare repositories in /data/repositories and\nyou know that none of them are embedded, we could quickly determine\nwhether or not the cwd is a descendent of /data/repositories and avoid\nthe search entirely if so.\n\n> > We'd probably want to allow saying \"all embedded bare repositories are\n> > safe to read config/hooks from\", too. I hadn't considered this approach\n> > as a way to read some embedded repos and not others; I suspect the\n> > overwhelmingly common use-case would be: `git config --local\n> > safe.embeddedRepo '*'`.\n>\n> Ah, I dislike this option for the exact reason I mentioned - avoiding\n> a malicious repo being snuck in next to legitimate repos. I'd prefer\n> to rely on exact matching only - but as the config needs to be set by\n> every contributor every time the set of bare repos changes, that\n> sounds impossible to manage for a project which may be constantly\n> adding and removing these repos.\n\nI share your distaste for this sort of thing, but I think we have to\nrecognize that there are likely to be many repos that embed dozens or\nmore bare repos inside of them, and asking them to opt-in each one\nindividually seems like a fairly large request for Git to make of them.\n\n> That said.... I'm biased again, but if you want to be certain of the\n> state of another repository in order to run tests in it... why not use\n> a submodule for that repository? Not a helpful comment for those\n> already using embedded bare repos, though ;)\n\nExactly, re: your last sentence.\n\n> Thanks for the food for thought.\n\nDitto!\n\nThanks,\nTaylor\n"},{"id":"454644","messageId":"kl6ly1zno328.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57685","inReplyTo":"Ylobp7sntKeWTLDX@nand.local","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-29T23:57:51Z","receivedAt":"2022-04-29T23:57:58Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Thanks so much for the response - it's really helpful, and there's a lot of food\nfor thought here.\n\n(Sorry that I didn't get back to you sooner, there really is a lot of\ngreat stuff here to think about :))\n\nTaylor Blau writes:\n\n> On Fri, Apr 15, 2022 at 05:41:59PM -0700, Glen Choo wrote:\n>> * There are use cases for embedded bare repos that don't have great alternatives\n>> (e.g. libgit2 uses bare repos in its tests). Even if this workflow is frowned\n>> upon (I personally don't think we should support it), I don't think we're\n>> ready to categorically declare that Git should ban embedded bare repos\n>> altogether (e.g. the way we ban .GiT).\n>\n> I think there are a handful of legitimate reasons that we might want to\n> continue supporting this; for projects like libgit2 and Git LFS, it's\n> useful to have repositories in a known state to execute tests in. Having\n> bare Git repositories contained in some \"test fixture\" directory is a\n> really easy way to do just that.\n\nIf I were designing Git from scratch, I would probably block embedded bare repos\nfrom being committed altogether - if an embedded bare repo doesn't behave\nparticularly differently from `.git` (which we pretty much agree we should not\nsupport), then this is just an inherently dangerous way to work.\n\nBut yes, we have historically allowed embedded bare repos, and I don't think we\nshould stop supporting them altogether. For instance, I don't see a good\nalternative for the test fixture use case:\n\n- Submodules aren't a good fit because they only allow you to include the\n contents of a submodule's tree, whereas in a test fixture, you really do want\n the gitdir internals to be source controlled so that you get nice predictable\n results.\n- Users could store the repos in some other form e.g. CDN, tarball. It's fine\n when running from a test script, but it's pretty awful to author/review any\n changes.\n- Perhaps the users could munge the bare repo at commit time e.g. instead of\n storing (refs/, objects/, HEAD), they could store (test_refs/, test_objects/,\n test_HEAD), which would later get turned into the bare repo in the test\n script. It's a little silly, but not unreasonable for a test script, I think.\n\nSo I'll leave behind this idea of \"blocking embedding bare repos\" for\nnow; I think there are more promising proposals in this thread.\n\n>> 1. Prevent checking out an embedded bare repo.\n>> 2. Detect if the bare repo is embedded and refuse to work with it.\n>> 3. Detect if the bare repo is embedded and do not read its config/hooks, but\n>> everything else still 'works'.\n>> 4. Don't detect bare repos.\n>> 5. Only detect bare repos that are named `.git` [1].\n>>\n>> (I've responded with my thoughts on each of these approaches in-thread).\n>\n> 1. Likely disrupts too many legitimate workflows for us to adopt\n> without designing some way to declare an embedded bare repository\n> is \"safe\".\n> 2. Ditto.\n> 3. This seems the most promising approach so far. Similar to (1), I\n> would also want to make sure we provide an easy way to declare a\n> bare repository as \"safe\" in order to avoid permanently disrupting\n> valid workflows that have accumulated over the past >15 years.\n> 4. Seems like this approach is too heavy-handed.\n> 5. Ditto.\n\nIf I understand you correctly, it seems like we can ship any of the options from\n1.-3., provided there is an easy way to opt-in known, \"safe\" bare repos.\n\nAfter some more reflection, I tend to agree that 4.-5. are too heavy-handed for\nthe general population because they would break nearly all bare repo users. The\nperformance cost of detecting an embedded bare repo isn't ideal either, but that\ncan be easily fixed via GIT_CEILING_DIRECTORIES or an allow-list of \"safe\"\nrepos, and it's a lot gentler than just ignoring bare repos altogether.\n\nI suspect that there is a subset of the user population who would love to\ndisable bare repo detection because they a) never expect to see bare repos in a\ngiven environment and b) want to be defensive about the possibility of\nunexpected bare repos. Since this really heavy-handed, I don't think this fixes\nthe embedded bare repo problem for everyone, but that subset of users will be\nvery well-served :) I think that there must be some coherent framework that\nencompasses this idea + the allow-list you proposed, but I haven't come up with\none yet.\n\n>> With that in mind, here's what I propose we do next:\n>>\n>> * Merge the `git fsck` patch [2] if we think that it is useful despite the\n>> potentially huge number of false positives. That patch needs some fixing; I'll\n>> make the changes when I'm back.\n>\n> If there are lots of false positives, we should consider downgrading the\n> severity of the proposed `EMBEDDED_BARE_REPO` fsck check to \"info\". I'm\n> not clear if there is another reason why this patch would have a\n> significant number of false positives (i.e., if the detection mechanism\n> is over-zealous).\n>\n> But if not, and this does detect only legitimate embedded bare\n> repositories, we should use it as a reminder to consider how many\n> use-cases and workflows will be affected by whatever approach we take\n> here, if any.\n\nYes, that's good to keep in mind. After mulling about it some more, I don't have\na clear direction on the fsck patch to be honest, I'll leave this alone for now\nand I'll return to it if I get a clearer picture.\n\n>> * I'll experiment with (5), and if it seems promising, I'll propose this as an\n>> opt-in feature, with the intent of making it opt-out in the future. We'll\n>> opt-into this at Google to help figure out if this is a good default or not.\n>>\n>> * If that direction turns out not to be promising, I'll pursue (1), since that\n>> is the only option that can be configured per-repo, which should hopefully\n>> minimize the fallout.\n>\n> Here's an alternative approach, which I haven't seen discussed thus far:\n>\n> When a bare repository is embedded in another repository, avoid reading\n> its config by default. This means that most Git commands will still\n> work, but without the possibility of running any \"executable\" portions\n> of the config. To opt-out (i.e., to allow legitimate use-cases to start\n> reading embedded bare repository config again), the embedding repository\n> would have to set a multi-valued `safe.embeddedRepo` configuration. This\n> would specify a list of paths relative to the embedding repository's\n> root of known-safe bare repositories.\n>\n> I think there are a couple of desirable attributes of this approach:\n>\n> - It minimally disrupts bare repositories, restricting the change to\n> only embedded repositories.\n> - It allows most Git commands to continue working as expected (modulo\n> reading the config), hopefully making the population of users whose\n> workflows will suddenly break pretty small.\n> - It requires the user to explicitly opt-in to the unsafe behavior,\n> because an attacker could not influence the embedding repository's\n> `safe.embeddedRepo` config.\n>\n> If we were going to do something about this, I would strongly advocate\n> for something that resembles the above. Or at the very least, some\n> solution that captures the attributes I outlined there.\n\nI really like the `safe.embeddedRepo` idea, though I'm not convinced about\n\"respect only the safe parts of the embedded repo\". I'll address the latter\nfirst.\n\nI think brian m. carlson was coming from a similar direction, and the \"respect\nonly the safe parts of the embedded repo\" part of the proposal sounds similar to\nhis [1]. Both seem to be motivated by your second point - protect as many\nworkflows as we can. It's a good guiding principle, and I think it's a good\nplace to start from. That said, I'm not sure that this proposal serves these\nusers that well:\n\n- Not reading the config might break the embedded bare repos in ways we don't\n expect (e.g. not reading core.repositoryformatversion).\n\n- Users who use embedded bare repos as test fixtures presumably want their tests\n to mimic real-world scenarios as closely as possible; running in this\n half-state of \"use some parts of the repo but not others\" doesn't seem a good\n fit for that use case.\n\n- This complicates the rules significantly for the user, who now has to figure\n out which parts of the bare repo are respected and which are not.\n\n- I'm also of the opinion that changing the rules like this actually does affect\n workflows, even if it doesn't break libgit2's tests.\n - A diligent user still has to convince themselves that the tests are passing\n for the right reasons, possibly adapting to the new rules (e.g. by\n selectively enabling `safe.embeddedRepo` on the right test fixtures).\n - A less diligent user might not even realize the change has happened and\n end up with difficult to debug results somewhere down the line.\n\nI'm also not keen on it for other reasons:\n\n- This expands the attack surface significantly, and I'm pessimistic that we\n can maintain a list of the 'safe' parts of a bare repo. A lot of attention has\n been focused on config/hooks, but I wouldn't be surprised if a creative\n attacker finds some other avenue that we missed (maybe a buffer overflow\n attack on a malicious index file?).\n\n- I expect that this is also going to be really complex to implement and\n maintain; instead of looking in a single gitdir for everything, we now look in\n two gitdirs.\n\nWhat is promising is an allow-listing scheme like `safe.embeddedRepo` that can\nbe enabled on a per-repository basis. . Using an allowlist to selectively choose\n*entire* embedded bare repos preserves the first and third attributes you\ndescribed, and it keeps things simple(-ish) for us and users. Breaking libgit2\nand Git LFS this way is pretty harsh, but it will give us the confidence that we\nhave communicated the behavior change (and fix!) to the relevant users, rather\nthan having them find out the wrong way.\n\nSome extra thoughts (in case they're helpful):\n\n- It's pretty important to get the format of `safe.embeddedRepo` 'correct', but\n what 'correct' is is up for debate. For example, should we allow '*'? (I think\n so, but I know some don't ;)).\n\n- There might be some unifying principles behind \"allowlisting certain embedded\n bare repos\" and \"disabling/enabling bare repo detection\" that can guide our\n fix.\n - Perhaps we could allow different 'levels' of bare repo protection, like\n 'allow all bare repos', 'allow only non-embedded bare repos', 'allow no bare\n repos', 'allow embedded bare repos but not their configs\".\n\n - If we do want to discourage embedded bare repos (and flip the default), this\n kind of gradual roll-out might give projects a way to incrementally migrate.\n\n[1] https://lore.kernel.org/git/Yk9Wcr74gvhtyOi7@camp.crustytoothpaste.net\n\n>\n> I would be happy to work together with you (or anybody!) on developing\n> patches in that direction, so let me know if you are interested in\n> coordinating our efforts.\n\nThanks! Once again, I really appreciate the response. I think it's moved the\ndiscussion forward in a meaningful way :) I think the easiest next step is for\nme (or whoever :)) to propose patches and to hash out the details there.\n\n"},{"id":"454645","messageId":"YmyNacEAiRl4zLW4@nand.local","threadId":"57685","inReplyTo":"kl6ly1zno328.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-04-30T01:14:17Z","receivedAt":"2022-04-30T01:14:22Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 29, 2022 at 04:57:51PM -0700, Glen Choo wrote:\n> Thanks so much for the response - it's really helpful, and there's a lot of food\n> for thought here.\n>\n> (Sorry that I didn't get back to you sooner, there really is a lot of\n> great stuff here to think about :))\n\nNo problem, I'm glad that you found it helpful (and likewise!).\n\n> So I'll leave behind this idea of \"blocking embedding bare repos\" for\n> now; I think there are more promising proposals in this thread.\n\nI am in strong agreement with you here, but I would add an additional\npoint which is that even if we encouraged submodules over embedded bare\nrepos, or suggested storing bare repos on CDNs or what have you, there's\nsignificant momentum in the \"do nothing\" category which we have to take\ninto account, too.\n\nSo even if we made it as easy as possible to convert to, for e.g., using\nsubmodules, getting the millions (?) of repositories with embedded bare\nrepositories in them that have accumulated over the years to actually\nchange seems virtually impossible to me.\n\n> >> 1. Prevent checking out an embedded bare repo.\n> >> 2. Detect if the bare repo is embedded and refuse to work with it.\n> >> 3. Detect if the bare repo is embedded and do not read its config/hooks, but\n> >> everything else still 'works'.\n> >> 4. Don't detect bare repos.\n> >> 5. Only detect bare repos that are named `.git` [1].\n> >>\n> >> (I've responded with my thoughts on each of these approaches in-thread).\n> >\n> > 1. Likely disrupts too many legitimate workflows for us to adopt\n> > without designing some way to declare an embedded bare repository\n> > is \"safe\".\n> > 2. Ditto.\n> > 3. This seems the most promising approach so far. Similar to (1), I\n> > would also want to make sure we provide an easy way to declare a\n> > bare repository as \"safe\" in order to avoid permanently disrupting\n> > valid workflows that have accumulated over the past >15 years.\n> > 4. Seems like this approach is too heavy-handed.\n> > 5. Ditto.\n>\n> If I understand you correctly, it seems like we can ship any of the options from\n> 1.-3., provided there is an easy way to opt-in known, \"safe\" bare repos.\n\nAfter thinking about it some more, I think that we should probably try\nto ship (3) of the ones that we agree are viable, but more on that\nbelow...\n\n> Yes, that's good to keep in mind. After mulling about it some more, I don't have\n> a clear direction on the fsck patch to be honest, I'll leave this alone for now\n> and I'll return to it if I get a clearer picture.\n\nSounds good. I'm happy to have ideas bounced off of me in the meantime ;).\n\n> I really like the `safe.embeddedRepo` idea, though I'm not convinced about\n> \"respect only the safe parts of the embedded repo\". I'll address the latter\n> first.\n\nTo be clear, I am advocating for \"only the safe parts\" insofar as \"read\nrepository extensions, core.repositoryFormatVersion and literally\nnothing else\". I'm definitely not suggesting we go and enumerate every\nconfigurable value, determine whether it's safe or not, and then read\nonly the safe ones. That approach seems doomed to fail, since no matter\nhow clever we are, there will always be some slightly-cleverer attacker\nwho can find a vector that we missed.\n\n> I think brian m. carlson was coming from a similar direction, and the \"respect\n> only the safe parts of the embedded repo\" part of the proposal sounds similar to\n> his [1]. Both seem to be motivated by your second point - protect as many\n> workflows as we can. It's a good guiding principle, and I think it's a good\n> place to start from. That said, I'm not sure that this proposal serves these\n> users that well:\n>\n> - Not reading the config might break the embedded bare repos in ways we don't\n> expect (e.g. not reading core.repositoryformatversion).\n\nI'm hoping that if we include this among the list of essential- and\nknown-safe config values that we'll mitigate this reasonably well.\n\n> - Users who use embedded bare repos as test fixtures presumably want their tests\n> to mimic real-world scenarios as closely as possible; running in this\n> half-state of \"use some parts of the repo but not others\" doesn't seem a good\n> fit for that use case.\n\nIt's hard to estimate how many of these tests will get broken. But I\nthink the important trade-off to consider between \"abort all Git\noperations in embedded bare repositories\" and \"warn, and avoid reading\nconfig/hooks\" is how disruptive the change will be. The latter seems far\nless disruptive to me, so I would rather us favor that over an\noverly-conservative approach.\n\n> - This complicates the rules significantly for the user, who now has to figure\n> out which parts of the bare repo are respected and which are not.\n\nOn this point I disagree, but I suspect we weren't on the same page\nabout what \"only the safe parts\" meant when you wrote this. To be\nextra-extra clear, I don't think we should read some parts of config and\nnot other, I mean we should read _only_ the above listed parts (the\nformat version and extensions) and nothing else.\n\n> - I'm also of the opinion that changing the rules like this actually does affect\n> workflows, even if it doesn't break libgit2's tests.\n> - A diligent user still has to convince themselves that the tests are passing\n> for the right reasons, possibly adapting to the new rules (e.g. by\n> selectively enabling `safe.embeddedRepo` on the right test fixtures).\n> - A less diligent user might not even realize the change has happened and\n> end up with difficult to debug results somewhere down the line.\n\nI am sympathetic to what you're saying, but I (a) think there's still a\ntradeoff here that doesn't obviously point us in one direction or the\nother and (b) we should equally keep in mind other workflows besides\njust test fixtures. Does that change our thinking at all? I'm not sure.\n\n> I'm also not keen on it for other reasons:\n>\n> - This expands the attack surface significantly, and I'm pessimistic that we\n> can maintain a list of the 'safe' parts of a bare repo. A lot of attention has\n> been focused on config/hooks, but I wouldn't be surprised if a creative\n> attacker finds some other avenue that we missed (maybe a buffer overflow\n> attack on a malicious index file?).\n\nI disagree, though again I suspect we were thinking of different thing\nwhen saying \"only read safe parts of the config\". Still though, I would\nargue that it limits the attack surface at the right level, which is to\nsay any vector that we _did_ miss is something that we should just fix\n(e.g., preventing a buffer overflow) and not \"oops, this config value\ndoes specify an executable\".\n\n(We shouldn't have to deal with the index file, though, since a bare\nrepository would not read the index, no?).\n\n> - I expect that this is also going to be really complex to implement and\n> maintain; instead of looking in a single gitdir for everything, we now look in\n> two gitdirs.\n\nI'd think that any approach we take that has different behavior\nfor bare repositories depending on whether or not they are embedded has\nto do a similar check, so I don't think this adds significant\ncomplexity. Though not having written any code here yet, I'd take what I\nsay with a huge grain of salt ;-).\n\n> What is promising is an allow-listing scheme like `safe.embeddedRepo` that can\n> be enabled on a per-repository basis. . Using an allowlist to selectively choose\n> *entire* embedded bare repos preserves the first and third attributes you\n> described, and it keeps things simple(-ish) for us and users. Breaking libgit2\n> and Git LFS this way is pretty harsh, but it will give us the confidence that we\n> have communicated the behavior change (and fix!) to the relevant users, rather\n> than having them find out the wrong way.\n\nHmm. I still am pretty convicted that this (avoid working with unknown\nembedded bare repositories) is too harsh of a change to\nmake the default. Replace \"Breaking libgit2 and Git LFS\" with \"breaking\nmany hundreds of thousands of repositories\" [1], and I think that we\nwould need to come up with something more lightweight.\n\n> Some extra thoughts (in case they're helpful):\n>\n> - It's pretty important to get the format of `safe.embeddedRepo` 'correct', but\n> what 'correct' is is up for debate. For example, should we allow '*'? (I think\n> so, but I know some don't ;)).\n\nStolee (cc'd) will have an interesting perspective here (at least as it\nrelates to the 2.35.3 release), I think.\n\n> - There might be some unifying principles behind \"allowlisting certain embedded\n> bare repos\" and \"disabling/enabling bare repo detection\" that can guide our\n> fix.\n> - Perhaps we could allow different 'levels' of bare repo protection, like\n> 'allow all bare repos', 'allow only non-embedded bare repos', 'allow no bare\n> repos', 'allow embedded bare repos but not their configs\".\n>\n> - If we do want to discourage embedded bare repos (and flip the default), this\n> kind of gradual roll-out might give projects a way to incrementally migrate.\n\nAh! Are you suggesting a global configuration setting that controls the\nbehavior of embedded bare repositories that _aren't_ listed in a\nrepositories safe.embeddedRepo list?\n\nThat sort of thing could work, though I'd argue strongly that any Git\n2.x.y release should make the default behavior \"avoid config/hooks\"\nas opposed to \"refuse to work\". We could consider changing that default\nin Git 3.x.y, but I feel strongly that any Git 2.x.y release should\ncater as much to existing workflows as possible without significantly\ncompromising on attack surface area.\n\nThanks,\nTaylor\n\n[1]: I have no idea if that figure is right, but I suspect it's in the\nright order of magnitude. I could look into it further, though.\n"},{"id":"454716","messageId":"324301af-4ad3-739c-25bb-1ddbe069eb3e@iee.email","threadId":"57685","inReplyTo":"kl6ly1zno328.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2022-05-02T14:05:33Z","receivedAt":"2022-05-02T14:06:13Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 30/04/2022 00:57, Glen Choo wrote:\n> If I were designing Git from scratch, I would probably block embedded bare repos\n> from being committed altogether - if an embedded bare repo doesn't behave\n> particularly differently from `.git` (which we pretty much agree we should not\n> support), then this is just an inherently dangerous way to work.\n>\n> But yes, we have historically allowed embedded bare repos, and I don't think we\n> should stop supporting them altogether. For instance, I don't see a good\n> alternative for the test fixture use case:\n>\n> - Submodules aren't a good fit because they only allow you to include the\n> contents of a submodule's tree, whereas in a test fixture, you really do want\n> the gitdir internals to be source controlled so that you get nice predictable\n> results.\n> - Users could store the repos in some other form e.g. CDN, tarball. It's fine\n> when running from a test script, but it's pretty awful to author/review any\n> changes.\n\nCan I check that storing bundles (of other repos) within a repository is\nconsidered reasonably safe?\n\nI have been looking at how Git's documentation could carry with it small\nexemplar repositories that cover the commit hierarchies shown in the\nvarious man pages to allow users to see, explore and understand the man\npage examples. I'd settled on bundles as a reasonable compromise, with\nthe exemplar repositories being generated via the test suite\n(obviously). With the recent focus on security issues, I thought it\nworth asking now.\n\n> - Perhaps the users could munge the bare repo at commit time e.g. instead of\n> storing (refs/, objects/, HEAD), they could store (test_refs/, test_objects/,\n> test_HEAD), which would later get turned into the bare repo in the test\n> script. It's a little silly, but not unreasonable for a test script, I think.\n--\nPhilip\n"},{"id":"454743","messageId":"xmqqr15bydj3.fsf@gitster.g","threadId":"57685","inReplyTo":"324301af-4ad3-739c-25bb-1ddbe069eb3e@iee.email","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-02T18:50:40Z","receivedAt":"2022-05-02T18:51:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philip Oakley writes:\n\n> Can I check that storing bundles (of other repos) within a repository is\n> considered reasonably safe?\n\nI think the threat model we are protecting against is that we\nshouldn't have to worry about \"git clone\" and then \"cd\" into the\nhierarchy. If you did \"make\" in there and their Makefile (or\n\"runme.sh\" shipped as part of the tree) is malicious, that is not\nsomething we can solve.\n\nSo, I would say it is safe to have a bundle, as in its statinary\nstate it does not actively do anything bad, even when you did \"ls\"\nin a directory that stores it.\n\n> I have been looking at how Git's documentation could carry with it small\n> exemplar repositories that cover the commit hierarchies shown in the\n> various man pages to allow users to see, explore and understand the man\n> page examples. I'd settled on bundles as a reasonable compromise, with\n> the exemplar repositories being generated via the test suite\n> (obviously). With the recent focus on security issues, I thought it\n> worth asking now.\n\nA bundle would be an OK vehicle. This depends on the size of the\nsample project, but another that may be more suitable may be to\ncreate the repository on the fly in the \"test suite\" you were\nplanning to use to extract from the bundle.\n\n"},{"id":"454746","messageId":"kl6llevju3l6.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57685","inReplyTo":"YmyNacEAiRl4zLW4@nand.local","subject":"Re: Bare repositories in the working tree are a security risk","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-05-02T19:39:01Z","receivedAt":"2022-05-02T19:39:15Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Taylor Blau writes:\n\n>> >> 1. Prevent checking out an embedded bare repo.\n>> >> 2. Detect if the bare repo is embedded and refuse to work with it.\n>> >> 3. Detect if the bare repo is embedded and do not read its config/hooks, but\n>> >> everything else still 'works'.\n>> >> 4. Don't detect bare repos.\n>> >> 5. Only detect bare repos that are named `.git` [1].\n>> >>\n>> >> (I've responded with my thoughts on each of these approaches in-thread).\n>> >\n>> > 1. Likely disrupts too many legitimate workflows for us to adopt\n>> > without designing some way to declare an embedded bare repository\n>> > is \"safe\".\n>> > 2. Ditto.\n>> > 3. This seems the most promising approach so far. Similar to (1), I\n>> > would also want to make sure we provide an easy way to declare a\n>> > bare repository as \"safe\" in order to avoid permanently disrupting\n>> > valid workflows that have accumulated over the past >15 years.\n>> > 4. Seems like this approach is too heavy-handed.\n>> > 5. Ditto.\n>>\n>> If I understand you correctly, it seems like we can ship any of the options from\n>> 1.-3., provided there is an easy way to opt-in known, \"safe\" bare repos.\n>\n> After thinking about it some more, I think that we should probably try\n> to ship (3) of the ones that we agree are viable, but more on that\n> below...\n>\n>> I really like the `safe.embeddedRepo` idea, though I'm not convinced about\n>> \"respect only the safe parts of the embedded repo\". I'll address the latter\n>> first.\n>\n> To be clear, I am advocating for \"only the safe parts\" insofar as \"read\n> repository extensions, core.repositoryFormatVersion and literally\n> nothing else\". I'm definitely not suggesting we go and enumerate every\n> configurable value, determine whether it's safe or not, and then read\n> only the safe ones. That approach seems doomed to fail, since no matter\n> how clever we are, there will always be some slightly-cleverer attacker\n> who can find a vector that we missed.\n\n[...]\n\n>> - This complicates the rules significantly for the user, who now has to figure\n>> out which parts of the bare repo are respected and which are not.\n>\n> On this point I disagree, but I suspect we weren't on the same page\n> about what \"only the safe parts\" meant when you wrote this. To be\n> extra-extra clear, I don't think we should read some parts of config and\n> not other, I mean we should read _only_ the above listed parts (the\n> format version and extensions) and nothing else.\n\nAh, to clarify, I'm taking an even more pessimistic stance here, which\nis that I'd prefer to avoid trusting any parts of the repo (not the\nconfig) as \"safe\", e.g. I'm not even keen on something as\ninnocuous-looking as \"We trust _only_ HEAD, refs/ and objects/\".\n\nMaybe this is overly-pessimistic though; your response (further down)\nsuggests that this might not be as bad as I think.\n\n>> - I'm also of the opinion that changing the rules like this actually does affect\n>> workflows, even if it doesn't break libgit2's tests.\n>> - A diligent user still has to convince themselves that the tests are passing\n>> for the right reasons, possibly adapting to the new rules (e.g. by\n>> selectively enabling `safe.embeddedRepo` on the right test fixtures).\n>> - A less diligent user might not even realize the change has happened and\n>> end up with difficult to debug results somewhere down the line.\n>\n> I am sympathetic to what you're saying, but I (a) think there's still a\n> tradeoff here that doesn't obviously point us in one direction or the\n> other and (b) we should equally keep in mind other workflows besides\n> just test fixtures. Does that change our thinking at all? I'm not sure.\n\nJust musing here. I think (b) is particularly important to keep in mind.\nIf a workflow is well-served by something else, I really don't mind\nbreaking it at all.\n\nI suppose the test fixture use case comes up the most frequently because\nit is one that we are familiar with, and doesn't seem to have a good\nalternative.\n\nMaybe we're approaching the limits of what we can know without\nperforming some studies of users in the wild.\n\n>> I'm also not keen on it for other reasons:\n>>\n>> - This expands the attack surface significantly, and I'm pessimistic that we\n>> can maintain a list of the 'safe' parts of a bare repo. A lot of attention has\n>> been focused on config/hooks, but I wouldn't be surprised if a creative\n>> attacker finds some other avenue that we missed (maybe a buffer overflow\n>> attack on a malicious index file?).\n>\n> I disagree, though again I suspect we were thinking of different thing\n> when saying \"only read safe parts of the config\". Still though, I would\n> argue that it limits the attack surface at the right level, which is to\n> say any vector that we _did_ miss is something that we should just fix\n> (e.g., preventing a buffer overflow) and not \"oops, this config value\n> does specify an executable\".\n>\n> (We shouldn't have to deal with the index file, though, since a bare\n> repository would not read the index, no?).\n\nI think this comment highlights the difference in opinion (in a good\nway!).\n\nIMO 'only reading parts of the repo and not others' will fix this\nparticular arbitrary code execution problem, but it won't fix (and might\neven open us up to) other, future problems with embedded bare repos.\n\nFrom this comment, I get the impression (and I hope I'm representing you\ncorrectly) that you think these future problems are manageable, and\ndon't justify breaking more workflows than we need to in order to fix\nthis arbitrary code execution problem.\n\nI think it's a healthy disagreement though - there's enough common\nground for us to start working our approaches separately (plus I don't\nthink either of us will be able to convince the other by thinking up\nhypotheticals on a mailing list thread :p), and the discussion will\nprobably get even better once we start comparing, reviewing, and\ninternally testing the patches.\n\n(Ah, I'm not sure about the index file in particular, I was just\nspeculating.)\n\n>> - I expect that this is also going to be really complex to implement and\n>> maintain; instead of looking in a single gitdir for everything, we now look in\n>> two gitdirs.\n>\n> I'd think that any approach we take that has different behavior\n> for bare repositories depending on whether or not they are embedded has\n> to do a similar check, so I don't think this adds significant\n> complexity. Though not having written any code here yet, I'd take what I\n> say with a huge grain of salt ;-).\n\n;)\n\n>> - There might be some unifying principles behind \"allowlisting certain embedded\n>> bare repos\" and \"disabling/enabling bare repo detection\" that can guide our\n>> fix.\n>> - Perhaps we could allow different 'levels' of bare repo protection, like\n>> 'allow all bare repos', 'allow only non-embedded bare repos', 'allow no bare\n>> repos', 'allow embedded bare repos but not their configs\".\n>>\n>> - If we do want to discourage embedded bare repos (and flip the default), this\n>> kind of gradual roll-out might give projects a way to incrementally migrate.\n>\n> Ah! Are you suggesting a global configuration setting that controls the\n> behavior of embedded bare repositories that _aren't_ listed in a\n> repositories safe.embeddedRepo list?\n\nYes, and an even more restrictive mode that disables bare repo detection\naltogether. I suspect this is a safe, non-disruptive default for many\nuser, but it obviously can't be the default today (or possibly ever?\nwe'd need a lot of user testing for sure).\n"}]}