{"thread":{"id":"60771","subject":"[PATCH] setup: allow cwd=.git w/ bareRepository=explicit","startedAt":"2024-01-20T00:08:26Z","lastAt":"2024-03-11T21:02:58Z","messageCount":17,"participants":["Kyle Lippincott via GitGitGadget","Junio C Hamano","Kyle Lippincott","Kyle Meyer"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"487118","messageId":"pull.1645.git.1705709303098.gitgitgadget@gmail.com","threadId":"60771","inReplyTo":null,"subject":"[PATCH] setup: allow cwd=.git w/ bareRepository=explicit","fromName":"Kyle Lippincott via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-20T00:08:22Z","receivedAt":"2024-01-20T00:08:26Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"From: Kyle Lippincott <spectral@google.com>\n\nThe safe.bareRepository setting can be set to 'explicit' to disallow\nimplicit uses of bare repositories, preventing an attack [1] where an\nartificial and malicious bare repository is embedded in another git\nrepository. Unfortunately, some tooling uses myrepo/.git/ as the cwd\nwhen executing commands, and this is blocked when\nsafe.bareRepository=explicit. Blocking is unnecessary, as git already\nprevents nested .git directories.\n\nTeach git to not reject uses of git inside of the .git directory: check\nif cwd is .git (or a subdirectory of it) and allow it even if\nsafe.bareRepository=explicit.\n\n[1] https://github.com/justinsteven/advisories/blob/main/2022_git_buried_bare_repos_and_fsmonitor_various_abuses.md\n\nSigned-off-by: Kyle Lippincott <spectral@google.com>\n---\n    setup: allow cwd=.git w/ bareRepository=explicit\n    \n    Please be aware that I'm a new contributor (this is my first patch to\n    git's code), so any style nits, suggestions about how to make this more\n    idiomatic, or any other suggestions are strongly encouraged.\n    \n    My primary concern with this patch is that I'm unsure if we need to\n    worry about case-insensitive filesystems (ex: cwd=my_repo/.GIT instead\n    of my_repo/.git, it might not trigger this logic and end up allowed).\n    I'm assuming this isn't a significant concern, for two reasons:\n    \n     * most filesystems/OSes in use today (by number of users) are at least\n       case-preserving, so users/tools will have had to type out .GIT\n       instead of getting it from readdir/wherever.\n     * this is primarily a \"quality of life\" change to the feature, and if\n       we get it wrong we still fail closed.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1645%2Fspectral54%2Fbare-repo-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1645/spectral54/bare-repo-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1645\n\n setup.c                         | 3 ++-\n t/t0035-safe-bare-repository.sh | 8 ++++++++\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex b38702718fb..b095e284979 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1371,7 +1371,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \n \t\tif (is_git_directory(dir->buf)) {\n \t\t\ttrace2_data_string(\"setup\", NULL, \"implicit-bare-repository\", dir->buf);\n-\t\t\tif (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)\n+\t\t\tif (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT &&\n+\t\t\t    !ends_with_path_components(dir->buf, \".git\"))\n \t\t\t\treturn GIT_DIR_DISALLOWED_BARE;\n \t\t\tif (!ensure_valid_ownership(NULL, NULL, dir->buf, report))\n \t\t\t\treturn GIT_DIR_INVALID_OWNERSHIP;\ndiff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh\nindex 038b8b788d7..80488563795 100755\n--- a/t/t0035-safe-bare-repository.sh\n+++ b/t/t0035-safe-bare-repository.sh\n@@ -78,4 +78,12 @@ test_expect_success 'no trace when GIT_DIR is explicitly provided' '\n \texpect_accepted_explicit \"$pwd/outer-repo/bare-repo\"\n '\n \n+test_expect_success 'no trace when \"bare repository\" is .git' '\n+\texpect_accepted_implicit -C outer-repo/.git\n+'\n+\n+test_expect_success 'no trace when \"bare repository\" is a subdir of .git' '\n+\texpect_accepted_implicit -C outer-repo/.git/objects\n+'\n+\n test_done\n\nbase-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5\n-- \ngitgitgadget\n"},{"id":"487159","messageId":"xmqqh6j7ej5w.fsf@gitster.g","threadId":"60771","inReplyTo":"pull.1645.git.1705709303098.gitgitgadget@gmail.com","subject":"Re: [PATCH] setup: allow cwd=.git w/ bareRepository=explicit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-20T22:26:03Z","receivedAt":"2024-01-20T22:26:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle Lippincott via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Kyle Lippincott <spectral@google.com>\n>\n> The safe.bareRepository setting can be set to 'explicit' to disallow\n> implicit uses of bare repositories, preventing an attack [1] where an\n> artificial and malicious bare repository is embedded in another git\n> repository. Unfortunately, some tooling uses myrepo/.git/ as the cwd\n> when executing commands, and this is blocked when\n> safe.bareRepository=explicit. Blocking is unnecessary, as git already\n> prevents nested .git directories.\n\nIn other words, if the directory $D that is the top level of the\nworking tree of a non-bare repository, you should be able to chdir\nto \"$D/.git\" and run your git command without explicitly saying that\nyou are inside $GIT_DIR (e.g. \"git --git-dir=$(pwd) cmd\")?\n\nIt makes very good sense.\n\nI briefly wondered if this would give us a great usability\nimprovement especially for hook scripts, but they are given GIT_DIR\nwhen called already so that is not a big upside, I guess.\n\n> Teach git to not reject uses of git inside of the .git directory: check\n> if cwd is .git (or a subdirectory of it) and allow it even if\n> safe.bareRepository=explicit.\n\n\n>     My primary concern with this patch is that I'm unsure if we need to\n>     worry about case-insensitive filesystems (ex: cwd=my_repo/.GIT instead\n>     of my_repo/.git, it might not trigger this logic and end up allowed).\n\nYou are additionally allowing \".git\" so even if somebody has \".GIT\"\nthat won't be allowed by this change, no?\n\n>     I'm assuming this isn't a significant concern, for two reasons:\n>     \n>      * most filesystems/OSes in use today (by number of users) are at least\n>        case-preserving, so users/tools will have had to type out .GIT\n>        instead of getting it from readdir/wherever.\n>      * this is primarily a \"quality of life\" change to the feature, and if\n>        we get it wrong we still fail closed.\n\nYup.\n\nIf we really cared (which I doubt), we could resort to checking with\nis_ntfs_dotgit() and is_hfs_dotgit(), but that would work in the\ndirection of loosening the check even further, which I do not know\nis needed.\n\n> -\t\t\tif (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)\n> +\t\t\tif (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT &&\n> +\t\t\t    !ends_with_path_components(dir->buf, \".git\"))\n>  \t\t\t\treturn GIT_DIR_DISALLOWED_BARE;\n\nThanks.\n"},{"id":"487222","messageId":"CAO_smViDR-JKRiKO-8-6mCGBpCR8Y1gLS9Y9DkoCFw=kHm5Mdw@mail.gmail.com","threadId":"60771","inReplyTo":"xmqqh6j7ej5w.fsf@gitster.g","subject":"Re: [PATCH] setup: allow cwd=.git w/ bareRepository=explicit","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-01-22T20:50:57Z","receivedAt":"2024-01-22T20:51:16Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Sat, Jan 20, 2024 at 2:26 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Kyle Lippincott via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > From: Kyle Lippincott <spectral@google.com>\n> >\n> > The safe.bareRepository setting can be set to 'explicit' to disallow\n> > implicit uses of bare repositories, preventing an attack [1] where an\n> > artificial and malicious bare repository is embedded in another git\n> > repository. Unfortunately, some tooling uses myrepo/.git/ as the cwd\n> > when executing commands, and this is blocked when\n> > safe.bareRepository=explicit. Blocking is unnecessary, as git already\n> > prevents nested .git directories.\n>\n> In other words, if the directory $D that is the top level of the\n> working tree of a non-bare repository, you should be able to chdir\n> to \"$D/.git\" and run your git command without explicitly saying that\n> you are inside $GIT_DIR (e.g. \"git --git-dir=$(pwd) cmd\")?\n\nCorrect.\n\n>\n> It makes very good sense.\n>\n> I briefly wondered if this would give us a great usability\n> improvement especially for hook scripts, but they are given GIT_DIR\n> when called already so that is not a big upside, I guess.\n>\n> > Teach git to not reject uses of git inside of the .git directory: check\n> > if cwd is .git (or a subdirectory of it) and allow it even if\n> > safe.bareRepository=explicit.\n>\n>\n> >     My primary concern with this patch is that I'm unsure if we need to\n> >     worry about case-insensitive filesystems (ex: cwd=my_repo/.GIT instead\n> >     of my_repo/.git, it might not trigger this logic and end up allowed).\n>\n> You are additionally allowing \".git\" so even if somebody has \".GIT\"\n> that won't be allowed by this change, no?\n\nMy concern was what happens if someone on a case-insensitive\nfilesystem does `cd .GIT` and then attempts to use it. If the cwd path\nisn't case-normalized at some point, we'll have a string like\n/path/to/my-repo/.GIT from getcwd() and it won't be allowed by this\ncode, since this code is checking for `.git` in a case sensitive\nfashion.\n\n>\n> >     I'm assuming this isn't a significant concern, for two reasons:\n> >\n> >      * most filesystems/OSes in use today (by number of users) are at least\n> >        case-preserving, so users/tools will have had to type out .GIT\n> >        instead of getting it from readdir/wherever.\n> >      * this is primarily a \"quality of life\" change to the feature, and if\n> >        we get it wrong we still fail closed.\n>\n> Yup.\n>\n> If we really cared (which I doubt), we could resort to checking with\n> is_ntfs_dotgit() and is_hfs_dotgit(), but that would work in the\n> direction of loosening the check even further, which I do not know\n> is needed.\n\nAgreed, it's not worth the additional complexity.\n\n>\n> > -                     if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)\n> > +                     if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT &&\n> > +                         !ends_with_path_components(dir->buf, \".git\"))\n> >                               return GIT_DIR_DISALLOWED_BARE;\n>\n> Thanks.\n"},{"id":"490105","messageId":"xmqqv85zqniu.fsf@gitster.g","threadId":"60771","inReplyTo":"pull.1645.git.1705709303098.gitgitgadget@gmail.com","subject":"Re: [PATCH] setup: allow cwd=.git w/ bareRepository=explicit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-06T17:27:05Z","receivedAt":"2024-03-06T17:27:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle Lippincott via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Teach git to not reject uses of git inside of the .git directory: check\n> if cwd is .git (or a subdirectory of it) and allow it even if\n> safe.bareRepository=explicit.\n\n> diff --git a/setup.c b/setup.c\n> index b38702718fb..b095e284979 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1371,7 +1371,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n>  \n>  \t\tif (is_git_directory(dir->buf)) {\n>  \t\t\ttrace2_data_string(\"setup\", NULL, \"implicit-bare-repository\", dir->buf);\n> -\t\t\tif (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)\n> +\t\t\tif (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT &&\n> +\t\t\t    !ends_with_path_components(dir->buf, \".git\"))\n>  \t\t\t\treturn GIT_DIR_DISALLOWED_BARE;\n>  \t\t\tif (!ensure_valid_ownership(NULL, NULL, dir->buf, report))\n>  \t\t\t\treturn GIT_DIR_INVALID_OWNERSHIP;\n\nI wish we had caught it much before we added DISALLOWED_BARE thing,\nbut I wonder how well would this escape-hatch interact with\nsecondary worktrees, where their git directory is not named \".git\"\nand not immediately below the root level of the working tree?\n\nIn a secondary worktree the root level of its working tree has a\nfile \".git\", whose contents may look like\n\n    gitdir: /home/gitster/git.git/.git/worktrees/git.old\n\nwhere\n\n - /home/gitster/git.git/ is the primary worktree with the\n   repository.\n\n - /home/gitster/git.git/.git/worktrees/git.old looks like a bare\n   repository.\n\n - /home/gitster/git.git/.git/worktrees/git.old/gitdir gives a way\n   to discover the secondary worktree, whose contents just records\n   the path to the \".git\" file, e.g., \"/home/gitster/git.old/.git\"\n   that had \"gitdir: ...\" in it.\n\nSo perhaps we can also use the presence of \"gitdir\" file, check the\ncontents of it tn ensure that \".git\" file there takes us back to\nthis (not quite) bare repository we are looking at, and allow access\nto it, or something?\n\nThoughts?\n\n"},{"id":"490268","messageId":"20240308211957.3758770-1-gitster@pobox.com","threadId":"60771","inReplyTo":"xmqqv85zqniu.fsf@gitster.g","subject":"[PATCH 0/2] Loosening safe.bareRepository=explicit even further","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-08T21:19:55Z","receivedAt":"2024-03-08T21:20:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Earlier 45bb9162 (setup: allow cwd=.git w/ bareRepository=explicit,\n2024-01-20) loosened safe.bareRepository=explicit in such a way that\nworking inside the \".git/\" directory (or its subdirectories) of a\nrepository that is not bare can be done without an explicit GIT_DIR\nor \"git --git-dir=<path>\".  The code needed for its change was\nalmost trivial---when it looks like we encountered a bare\nrepository, if the last path component of the discovered \"$GIT_DIR\"\nis \".git\", then it cannot be anything but the $GIT_DIR of a non-bare\nrepository, the root of whose working tree is the parent directory\nof that \".git\" directory.  This is because projects cannot create a\n\".git\" directory in their working tree and cause clone/checkout to\nextract them in the victim's working tree.\n\nThis almost works, until somebody starts using \"git worktree add\" to\ncreate a secondary worktree.  Their $GIT_DIR resides inside the\n$GIT_DIR of the primary worktree of the same repository, at\n$GIT_DIR/worktree/$name where $name is the name of the secondary\nworktree, which is not \".git\".\n\nThese two patches are to extend the \"if you can work in its working\ntree, you should be able to work in its $GIT_DIR\" for secondary\nworktrees.\n\nJunio C Hamano (2):\n  setup: detect to be in $GIT_DIR with a new helper\n  setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree\n\n setup.c                         | 57 ++++++++++++++++++++++++++++++++-\n t/t0035-safe-bare-repository.sh |  8 ++++-\n 2 files changed, 63 insertions(+), 2 deletions(-)\n\n-- \n2.44.0-165-ge09f1254c5\n\n"},{"id":"490269","messageId":"20240308211957.3758770-3-gitster@pobox.com","threadId":"60771","inReplyTo":"20240308211957.3758770-1-gitster@pobox.com","subject":"[PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-08T21:19:57Z","receivedAt":"2024-03-08T21:20:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"If you have /var/tmp/primary/ as a repository, and if you create a\nsecondary worktree of it at /var/tmp/secondary/, the layout would\nlook like this:\n\n    $ cd /var/tmp/\n    $ git init primary\n    $ cd primary\n    $ pwd\n    /var/tmp/primary\n    $ git worktree add ../secondary\n    $ cat ../seconary/.git\n    gitdir: /var/tmp/primary/.git/worktrees/secondary\n    $ ls /var/tmp/primary/.git/worktrees/secondary\n    commondir  gitdir  HEAD  index  refs\n    $ cat /var/tmp/primary/.git/worktrees/secondary/gitdir\n    /var/tmp/secondary/.git\n\nWhen the configuration variable 'safe.bareRepository=explicit' is\nset to explicit, the change made by 45bb9162 (setup: allow cwd=.git\nw/ bareRepository=explicit, 2024-01-20) allows you to work in the\n/var/tmp/primary/.git directory (i.e., $GIT_DIR of the primary\nworktree).  The idea is that if it is safe to work in the repository\nin its working tree, it should be equally safe to work in the\n\".git/\" directory of that working tree, too.\n\nNow, for the same reason, let's allow command execution from within\nthe $GIT_DIR directory of a secondary worktree.  This is useful for\ntools working with secondary worktrees when the 'bareRepository'\nsetting is set to 'explicit'.\n\nIn the previous commit, we created a helper function to house the\nlogic that checks if a directory that looks like a bare repository\nis actually a part of a non-bare repository.  Extend the helper\nfunction to also check if the apparent bare-repository is a $GIT_DIR\nof a secondary worktree, by checking three things:\n\n * The path to the $GIT_DIR must be a subdirectory of\n   \".git/worktrees/\", which is the primary worktree [*].\n\n * Such $GIT_DIR must have file \"gitdir\", that records the path of\n   the \".git\" file that is at the root level of the secondary\n   worktree.\n\n * That \".git\" file in turn points back at the $GIT_DIR we are\n   inspecting.\n\nThe latter two points are merely for checking sanity.  The security\nlies in the first requirement.\n\nRemember that a tree object with an entry whose pathname component\nis \".git\" is forbidden at various levels (fsck, object transfer and\ncheckout), so malicious projects cannot cause users to clone and\ncheckout a crafted \".git\" directory in a shell directory that\npretends to be a working tree with that \".git\" thing at its root\nlevel.  That is where 45bb9162 (setup: allow cwd=.git w/\nbareRepository=explicit, 2024-01-20) draws its security guarantee\nfrom.  And the solution for secondary worktrees in this commit draws\nits security guarantee from the same place.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n setup.c                         | 52 ++++++++++++++++++++++++++++++++-\n t/t0035-safe-bare-repository.sh |  8 ++++-\n 2 files changed, 58 insertions(+), 2 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 3081be4970..68860dcd18 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1231,9 +1231,59 @@ static const char *allowed_bare_repo_to_string(\n \treturn NULL;\n }\n \n+static int is_git_dir_of_secondary_worktree(const char *path)\n+{\n+\tint result = 0; /* assume not */\n+\tstruct strbuf gitfile_here = STRBUF_INIT;\n+\tstruct strbuf gitfile_there = STRBUF_INIT;\n+\tconst char *gitfile_contents;\n+\tint error_code = 0;\n+\n+\t/*\n+\t * We should be a subdirectory of /.git/worktrees inside\n+\t * the $GIT_DIR of the primary worktree.\n+\t *\n+\t * NEEDSWORK: some folks create secondary worktrees out of a\n+\t * bare repository; they don't count ;-), at least not yet.\n+\t */\n+\tif (!strstr(path, \"/.git/worktrees/\"))\n+\t\tgoto out;\n+\n+\t/*\n+\t * Does gitdir that points at the \".git\" file at the root of\n+\t * the secondary worktree roundtrip here?\n+\t */\n+\tstrbuf_addf(&gitfile_here, \"%s/gitdir\", path);\n+\tif (!file_exists(gitfile_here.buf))\n+\t\tgoto out;\n+\tif (strbuf_read_file(&gitfile_there, gitfile_here.buf, 0) < 0)\n+\t\tgoto out;\n+\tstrbuf_trim_trailing_newline(&gitfile_there);\n+\n+\tgitfile_contents = read_gitfile_gently(gitfile_there.buf, &error_code);\n+\tif ((!gitfile_contents) || strcmp(gitfile_contents, path))\n+\t\tgoto out;\n+\n+\t/* OK, we are happy */\n+\tresult = 1;\n+\n+out:\n+\tstrbuf_release(&gitfile_here);\n+\tstrbuf_release(&gitfile_there);\n+\treturn result;\n+}\n+\n static int is_repo_with_working_tree(const char *path)\n {\n-\treturn ends_with_path_components(path, \".git\");\n+\t/* $GIT_DIR immediately below the primary working tree */\n+\tif (ends_with_path_components(path, \".git\"))\n+\t\treturn 1;\n+\n+\t/* Are we in $GIT_DIR of a secondary worktree? */\n+\tif (is_git_dir_of_secondary_worktree(path))\n+\t\treturn 1;\n+\n+\treturn 0;\n }\n \n /*\ndiff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh\nindex 8048856379..62cdfcefc1 100755\n--- a/t/t0035-safe-bare-repository.sh\n+++ b/t/t0035-safe-bare-repository.sh\n@@ -31,7 +31,9 @@ expect_rejected () {\n \n test_expect_success 'setup bare repo in worktree' '\n \tgit init outer-repo &&\n-\tgit init --bare outer-repo/bare-repo\n+\tgit init --bare outer-repo/bare-repo &&\n+\tgit -C outer-repo worktree add ../outer-secondary &&\n+\ttest_path_is_dir outer-secondary\n '\n \n test_expect_success 'safe.bareRepository unset' '\n@@ -86,4 +88,8 @@ test_expect_success 'no trace when \"bare repository\" is a subdir of .git' '\n \texpect_accepted_implicit -C outer-repo/.git/objects\n '\n \n+test_expect_success 'no trace in $GIT_DIR of secondary worktree' '\n+\texpect_accepted_implicit -C outer-repo/.git/worktrees/outer-secondary\n+'\n+\n test_done\n-- \n2.44.0-165-ge09f1254c5\n\n"},{"id":"490270","messageId":"20240308211957.3758770-2-gitster@pobox.com","threadId":"60771","inReplyTo":"20240308211957.3758770-1-gitster@pobox.com","subject":"[PATCH 1/2] setup: detect to be in $GIT_DIR with a new helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-08T21:19:56Z","receivedAt":"2024-03-08T21:20:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Earlier, 45bb9162 (setup: allow cwd=.git w/ bareRepository=explicit,\n2024-01-20) loosened the \"safe.bareRepository=explicit\" to allow Git\noperations inside \".git/\" directory in the root level of a working\ntree of a non-bare repository.  It used the fact that the $GIT_DIR\nyou discover has \".git\" as the last path component, if you descended\ninto \".git\" of a non-bare repository.\n\nLet's move the logic into a separate helper function.  We can\nenhance this to detect the case where we are inside $GIT_DIR of a\nsecondary worktree (where \"ends with .git\" trick does not work) in\nthe next commit.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n setup.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex a09b7b87ec..3081be4970 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1231,6 +1231,11 @@ static const char *allowed_bare_repo_to_string(\n \treturn NULL;\n }\n \n+static int is_repo_with_working_tree(const char *path)\n+{\n+\treturn ends_with_path_components(path, \".git\");\n+}\n+\n /*\n  * We cannot decide in this function whether we are in the work tree or\n  * not, since the config can only be read _after_ this function was called.\n@@ -1360,7 +1365,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\tif (is_git_directory(dir->buf)) {\n \t\t\ttrace2_data_string(\"setup\", NULL, \"implicit-bare-repository\", dir->buf);\n \t\t\tif (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT &&\n-\t\t\t    !ends_with_path_components(dir->buf, \".git\"))\n+\t\t\t    !is_repo_with_working_tree(dir->buf))\n \t\t\t\treturn GIT_DIR_DISALLOWED_BARE;\n \t\t\tif (!ensure_valid_ownership(NULL, NULL, dir->buf, report))\n \t\t\t\treturn GIT_DIR_INVALID_OWNERSHIP;\n-- \n2.44.0-165-ge09f1254c5\n\n"},{"id":"490276","messageId":"xmqqil1wfjbg.fsf@gitster.g","threadId":"60771","inReplyTo":"20240308211957.3758770-3-gitster@pobox.com","subject":"Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-08T22:30:11Z","receivedAt":"2024-03-08T22:30:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> In the previous commit, we created a helper function to house the\n> logic that checks if a directory that looks like a bare repository\n> is actually a part of a non-bare repository.  Extend the helper\n> function to also check if the apparent bare-repository is a $GIT_DIR\n> of a secondary worktree, by checking three things:\n>\n>  * The path to the $GIT_DIR must be a subdirectory of\n>    \".git/worktrees/\", which is the primary worktree [*].\n>\n>  * Such $GIT_DIR must have file \"gitdir\", that records the path of\n>    the \".git\" file that is at the root level of the secondary\n>    worktree.\n>\n>  * That \".git\" file in turn points back at the $GIT_DIR we are\n>    inspecting.\n>\n> The latter two points are merely for checking sanity.  The security\n> lies in the first requirement.\n>\n> Remember that a tree object with an entry whose pathname component\n> is \".git\" is forbidden at various levels (fsck, object transfer and\n> checkout), so malicious projects cannot cause users to clone and\n> checkout a crafted \".git\" directory in a shell directory that\n> pretends to be a working tree with that \".git\" thing at its root\n> level.  That is where 45bb9162 (setup: allow cwd=.git w/\n> bareRepository=explicit, 2024-01-20) draws its security guarantee\n> from.  And the solution for secondary worktrees in this commit draws\n> its security guarantee from the same place.\n\nI wrote the \"[*]\" mark but forgot to add a footnote with an\nadditional information for it.  Something like this was what I had\nin mind to write there:\n\n[Footnote]\n\n * This does not help folks who create a new worktree out of a bare\n   repository, because in their set-up, there won't be \"/.git/\" in\n   front of \"worktrees\" directory.  It is fundamentally impossible\n   to lift this limitation, as long as safe.bareRepository is\n   considered to be a meaningful security measure.  The security of\n   both the loosening for a secondary worktree's GIT_DIR as well as\n   the loosening for the GIT_DIR of the primary worktree, hinge on\n   the fact that \".git/\" directory is impossible to create as\n   payload to be cloned.\n"},{"id":"490278","messageId":"CAO_smVjrKJeKr7QgQWryZRErStFk=Y+1T=dwrR_boXQD_X9_Mg@mail.gmail.com","threadId":"60771","inReplyTo":"20240308211957.3758770-3-gitster@pobox.com","subject":"Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-03-08T23:10:16Z","receivedAt":"2024-03-08T23:10:33Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Fri, Mar 8, 2024 at 1:20 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> If you have /var/tmp/primary/ as a repository, and if you create a\n> secondary worktree of it at /var/tmp/secondary/, the layout would\n> look like this:\n>\n>     $ cd /var/tmp/\n>     $ git init primary\n>     $ cd primary\n>     $ pwd\n>     /var/tmp/primary\n>     $ git worktree add ../secondary\n>     $ cat ../seconary/.git\n\nNit: typo, should be `secondary` (missing the `d`)\n\n\n>     gitdir: /var/tmp/primary/.git/worktrees/secondary\n>     $ ls /var/tmp/primary/.git/worktrees/secondary\n>     commondir  gitdir  HEAD  index  refs\n>     $ cat /var/tmp/primary/.git/worktrees/secondary/gitdir\n>     /var/tmp/secondary/.git\n>\n> When the configuration variable 'safe.bareRepository=explicit' is\n> set to explicit, the change made by 45bb9162 (setup: allow cwd=.git\n> w/ bareRepository=explicit, 2024-01-20) allows you to work in the\n> /var/tmp/primary/.git directory (i.e., $GIT_DIR of the primary\n> worktree).  The idea is that if it is safe to work in the repository\n> in its working tree, it should be equally safe to work in the\n> \".git/\" directory of that working tree, too.\n>\n> Now, for the same reason, let's allow command execution from within\n> the $GIT_DIR directory of a secondary worktree.  This is useful for\n> tools working with secondary worktrees when the 'bareRepository'\n> setting is set to 'explicit'.\n>\n> In the previous commit, we created a helper function to house the\n> logic that checks if a directory that looks like a bare repository\n> is actually a part of a non-bare repository.  Extend the helper\n> function to also check if the apparent bare-repository is a $GIT_DIR\n> of a secondary worktree, by checking three things:\n>\n>  * The path to the $GIT_DIR must be a subdirectory of\n>    \".git/worktrees/\", which is the primary worktree [*].\n>\n>  * Such $GIT_DIR must have file \"gitdir\", that records the path of\n>    the \".git\" file that is at the root level of the secondary\n>    worktree.\n>\n>  * That \".git\" file in turn points back at the $GIT_DIR we are\n>    inspecting.\n>\n> The latter two points are merely for checking sanity.  The security\n> lies in the first requirement.\n>\n> Remember that a tree object with an entry whose pathname component\n> is \".git\" is forbidden at various levels (fsck, object transfer and\n> checkout), so malicious projects cannot cause users to clone and\n> checkout a crafted \".git\" directory in a shell directory that\n> pretends to be a working tree with that \".git\" thing at its root\n> level.  That is where 45bb9162 (setup: allow cwd=.git w/\n> bareRepository=explicit, 2024-01-20) draws its security guarantee\n> from.  And the solution for secondary worktrees in this commit draws\n> its security guarantee from the same place.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  setup.c                         | 52 ++++++++++++++++++++++++++++++++-\n>  t/t0035-safe-bare-repository.sh |  8 ++++-\n>  2 files changed, 58 insertions(+), 2 deletions(-)\n>\n> diff --git a/setup.c b/setup.c\n> index 3081be4970..68860dcd18 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1231,9 +1231,59 @@ static const char *allowed_bare_repo_to_string(\n>         return NULL;\n>  }\n>\n> +static int is_git_dir_of_secondary_worktree(const char *path)\n> +{\n> +       int result = 0; /* assume not */\n> +       struct strbuf gitfile_here = STRBUF_INIT;\n> +       struct strbuf gitfile_there = STRBUF_INIT;\n> +       const char *gitfile_contents;\n> +       int error_code = 0;\n> +\n> +       /*\n> +        * We should be a subdirectory of /.git/worktrees inside\n> +        * the $GIT_DIR of the primary worktree.\n> +        *\n> +        * NEEDSWORK: some folks create secondary worktrees out of a\n> +        * bare repository; they don't count ;-), at least not yet.\n> +        */\n> +       if (!strstr(path, \"/.git/worktrees/\"))\n\nDo we need to be concerned about path separators being different on\nWindows? Or have they already been normalized here?\n\n> +               goto out;\n> +\n> +       /*\n> +        * Does gitdir that points at the \".git\" file at the root of\n> +        * the secondary worktree roundtrip here?\n> +        */\n\nWhat loss of security do we have if we don't have as stringent of a\ncheck? i.e. if we just did `return !!strstr(path, \"/.git/worktrees/)`?\nOr maybe we even combine the existing ends_with(.git) check with this\nand do something like:\n\nstatic int is_under_dotgit_dir(const char *path) {\n        char *dotgit = strstr(path, \"/.git\");\n        return dotgit && (dotgit[5] == '\\0' || dotgit[5] == '/');\n}\n\n\n\n> +       strbuf_addf(&gitfile_here, \"%s/gitdir\", path);\n> +       if (!file_exists(gitfile_here.buf))\n> +               goto out;\n> +       if (strbuf_read_file(&gitfile_there, gitfile_here.buf, 0) < 0)\n> +               goto out;\n> +       strbuf_trim_trailing_newline(&gitfile_there);\n> +\n> +       gitfile_contents = read_gitfile_gently(gitfile_there.buf, &error_code);\n> +       if ((!gitfile_contents) || strcmp(gitfile_contents, path))\n> +               goto out;\n> +\n> +       /* OK, we are happy */\n> +       result = 1;\n> +\n> +out:\n> +       strbuf_release(&gitfile_here);\n> +       strbuf_release(&gitfile_there);\n> +       return result;\n> +}\n> +\n>  static int is_repo_with_working_tree(const char *path)\n>  {\n> -       return ends_with_path_components(path, \".git\");\n> +       /* $GIT_DIR immediately below the primary working tree */\n> +       if (ends_with_path_components(path, \".git\"))\n> +               return 1;\n> +\n> +       /* Are we in $GIT_DIR of a secondary worktree? */\n> +       if (is_git_dir_of_secondary_worktree(path))\n> +               return 1;\n> +\n> +       return 0;\n>  }\n>\n>  /*\n> diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh\n> index 8048856379..62cdfcefc1 100755\n> --- a/t/t0035-safe-bare-repository.sh\n> +++ b/t/t0035-safe-bare-repository.sh\n> @@ -31,7 +31,9 @@ expect_rejected () {\n>\n>  test_expect_success 'setup bare repo in worktree' '\n>         git init outer-repo &&\n> -       git init --bare outer-repo/bare-repo\n> +       git init --bare outer-repo/bare-repo &&\n> +       git -C outer-repo worktree add ../outer-secondary &&\n> +       test_path_is_dir outer-secondary\n>  '\n>\n>  test_expect_success 'safe.bareRepository unset' '\n> @@ -86,4 +88,8 @@ test_expect_success 'no trace when \"bare repository\" is a subdir of .git' '\n>         expect_accepted_implicit -C outer-repo/.git/objects\n>  '\n>\n> +test_expect_success 'no trace in $GIT_DIR of secondary worktree' '\n> +       expect_accepted_implicit -C outer-repo/.git/worktrees/outer-secondary\n> +'\n> +\n>  test_done\n> --\n> 2.44.0-165-ge09f1254c5\n>\n"},{"id":"490279","messageId":"xmqqy1ase1vo.fsf@gitster.g","threadId":"60771","inReplyTo":"CAO_smVjrKJeKr7QgQWryZRErStFk=Y+1T=dwrR_boXQD_X9_Mg@mail.gmail.com","subject":"Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-08T23:32:11Z","receivedAt":"2024-03-08T23:32:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Lippincott <spectral@google.com> writes:\n\n>>     $ cat ../seconary/.git\n>\n> Nit: typo, should be `secondary` (missing the `d`)\n\nGood eyes.  Thanks.\n\n>> +       /*\n>> +        * We should be a subdirectory of /.git/worktrees inside\n>> +        * the $GIT_DIR of the primary worktree.\n>> +        *\n>> +        * NEEDSWORK: some folks create secondary worktrees out of a\n>> +        * bare repository; they don't count ;-), at least not yet.\n>> +        */\n>> +       if (!strstr(path, \"/.git/worktrees/\"))\n>\n> Do we need to be concerned about path separators being different on\n> Windows? Or have they already been normalized here?\n\nI am not certain offhand, but if they need to tolerate different\nseparators, they can send in patches.\n\n>> +               goto out;\n>> +\n>> +       /*\n>> +        * Does gitdir that points at the \".git\" file at the root of\n>> +        * the secondary worktree roundtrip here?\n>> +        */\n>\n> What loss of security do we have if we don't have as stringent of a\n> check? i.e. if we just did `return !!strstr(path, \"/.git/worktrees/)`?\n\nNo loss of security.\n\nThese \"keep result the status we want to return if we want to return\nimmediately here, and always jump to the out label instead of\nreturning\" patterns is mere a disciplined way to make it easier to\nwrite code that does not leak.  The very first one may not have to\ndo the \"goto out\" and instead immediately return, but by writing\nthis way, I do not need to be always looking out to shoot down\npatches that adds new check and/or allocations before this\ncondition and early \"out\".\n\n> Or maybe we even combine the existing ends_with(.git) check with this\n\nAt the mechanical level, perhaps, but I'd want logically separate\nthings treated as distinct cases.  One is about being inside\n$GIT_DIR of the primary worktrees (where more than majority of users\nwill encounter) and the new one is about being inside $GIT_DIR of\nsecondaries.\n"},{"id":"490280","messageId":"CAO_smVjD8DFcvveAg2iiWGhtNJGCT1ieAUzJbX3TNNJjm-5rMw@mail.gmail.com","threadId":"60771","inReplyTo":"xmqqy1ase1vo.fsf@gitster.g","subject":"Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-03-09T00:12:07Z","receivedAt":"2024-03-09T00:12:24Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Fri, Mar 8, 2024 at 3:32 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Kyle Lippincott <spectral@google.com> writes:\n>\n> >>     $ cat ../seconary/.git\n> >\n> > Nit: typo, should be `secondary` (missing the `d`)\n>\n> Good eyes.  Thanks.\n>\n> >> +       /*\n> >> +        * We should be a subdirectory of /.git/worktrees inside\n> >> +        * the $GIT_DIR of the primary worktree.\n> >> +        *\n> >> +        * NEEDSWORK: some folks create secondary worktrees out of a\n> >> +        * bare repository; they don't count ;-), at least not yet.\n> >> +        */\n> >> +       if (!strstr(path, \"/.git/worktrees/\"))\n> >\n> > Do we need to be concerned about path separators being different on\n> > Windows? Or have they already been normalized here?\n>\n> I am not certain offhand, but if they need to tolerate different\n> separators, they can send in patches.\n>\n> >> +               goto out;\n> >> +\n> >> +       /*\n> >> +        * Does gitdir that points at the \".git\" file at the root of\n> >> +        * the secondary worktree roundtrip here?\n> >> +        */\n> >\n> > What loss of security do we have if we don't have as stringent of a\n> > check? i.e. if we just did `return !!strstr(path, \"/.git/worktrees/)`?\n>\n> No loss of security.\n\nThen should we just do that?\n\n+ /* Assumption: `path` points to the root of a $GIT_DIR. */\n static int is_repo_with_working_tree(const char *path)\n {\n-       return ends_with_path_components(path, \".git\");\n+       /* $GIT_DIR immediately below the primary working tree */\n+       if (ends_with_path_components(path, \".git\"))\n+               return 1;\n+\n+       /*\n+        * Also allow $GIT_DIRs in secondary worktrees.\n+        * These do not end in .git, but are still considered safe because\n+        * of the .git component in the path.\n+        */\n+       if (strstr(path, \"/.git/worktrees/\"))\n+               return 1;\n+\n+       return 0;\n }\n\n>\n> These \"keep result the status we want to return if we want to return\n> immediately here, and always jump to the out label instead of\n> returning\" patterns is mere a disciplined way to make it easier to\n> write code that does not leak.  The very first one may not have to\n> do the \"goto out\" and instead immediately return, but by writing\n> this way, I do not need to be always looking out to shoot down\n> patches that adds new check and/or allocations before this\n> condition and early \"out\".\n>\n> > Or maybe we even combine the existing ends_with(.git) check with this\n>\n> At the mechanical level, perhaps, but I'd want logically separate\n> things treated as distinct cases.  One is about being inside\n> $GIT_DIR of the primary worktrees (where more than majority of users\n> will encounter) and the new one is about being inside $GIT_DIR of\n> secondaries.\n"},{"id":"490281","messageId":"xmqqttlgdx4t.fsf@gitster.g","threadId":"60771","inReplyTo":"CAO_smVjD8DFcvveAg2iiWGhtNJGCT1ieAUzJbX3TNNJjm-5rMw@mail.gmail.com","subject":"Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-09T01:14:42Z","receivedAt":"2024-03-09T01:14:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Lippincott <spectral@google.com> writes:\n\n>> > What loss of security do we have if we don't have as stringent of a\n>> > check? i.e. if we just did `return !!strstr(path, \"/.git/worktrees/)`?\n>>\n>> No loss of security.\n>\n> Then should we just do that?\n\nI do not see what you mean.\n\n> + /* Assumption: `path` points to the root of a $GIT_DIR. */\n>  static int is_repo_with_working_tree(const char *path)\n>  {\n> -       return ends_with_path_components(path, \".git\");\n> +       /* $GIT_DIR immediately below the primary working tree */\n> +       if (ends_with_path_components(path, \".git\"))\n> +               return 1;\n> +\n> +       /*\n> +        * Also allow $GIT_DIRs in secondary worktrees.\n> +        * These do not end in .git, but are still considered safe because\n> +        * of the .git component in the path.\n> +        */\n> +       if (strstr(path, \"/.git/worktrees/\"))\n> +               return 1;\n> +\n> +       return 0;\n>  }\n\nAh, no.  I thought you were asking \"goto out\" vs \"return\", and my\nanswer was about these two.  Whether you leave with \"goto out\" or\n\"return\", it does not change the security posture.  Direct return\nwill raise the risk of leaking resources after careless future\nupdates to the code.\n\nI didn't get that you do not want to see the other two \"sanity\ncheck\".\n\nLosing these sanity checks may not lose \"security\" per-se, but it\nmay raise the risk of misidentification.  A healthy GIT_DIR of a\nsecondary worktree should satisfy these two extra conditions.\n"},{"id":"490284","messageId":"87msr8qef9.fsf@kyleam.com","threadId":"60771","inReplyTo":"20240308211957.3758770-3-gitster@pobox.com","subject":"Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2024-03-09T03:20:26Z","receivedAt":"2024-03-09T03:20:33Z","isPatch":true,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Junio C Hamano writes:\n\n> Now, for the same reason, let's allow command execution from within\n> the $GIT_DIR directory of a secondary worktree.  This is useful for\n> tools working with secondary worktrees when the 'bareRepository'\n> setting is set to 'explicit'.\n\nDoes the same reason also apply to .git/modules/$name ?\n\n> In the previous commit, we created a helper function to house the\n> logic that checks if a directory that looks like a bare repository\n> is actually a part of a non-bare repository.  Extend the helper\n> function to also check if the apparent bare-repository is a $GIT_DIR\n> of a secondary worktree, by checking three things:\n>\n>  * The path to the $GIT_DIR must be a subdirectory of\n>    \".git/worktrees/\", which is the primary worktree [*].\n>\n>  * Such $GIT_DIR must have file \"gitdir\", that records the path of\n>    the \".git\" file that is at the root level of the secondary\n>    worktree.\n>\n>  * That \".git\" file in turn points back at the $GIT_DIR we are\n>    inspecting.\n>\n> The latter two points are merely for checking sanity.  The security\n> lies in the first requirement.\n\nIn the case of .git/modules/, the second point doesn't apply because\nthere's no gitdir file.  But perhaps the core.worktree setting could be\nused for the same purpose.\n\n  $ pwd\n  /path/to/super/.git/modules/sub\n  $ git config core.worktree\n  ../../../sub\n"},{"id":"490288","messageId":"xmqqle6sdk8i.fsf@gitster.g","threadId":"60771","inReplyTo":"87msr8qef9.fsf@kyleam.com","subject":"Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-09T05:53:17Z","receivedAt":"2024-03-09T05:53:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Meyer <kyle@kyleam.com> writes:\n\n>> Now, for the same reason, let's allow command execution from within\n>> the $GIT_DIR directory of a secondary worktree.  This is useful for\n>> tools working with secondary worktrees when the 'bareRepository'\n>> setting is set to 'explicit'.\n>\n> Does the same reason also apply to .git/modules/$name ?\n\nPerhaps.  I do not actively work on submodules so unlike those who\nare always thinking about improving the user experience around them,\nI did not think of those \".git/modules/$name\" things as something\nsimilar to the \".git/worktrees/$name\" things.\n\nOften hooks (and probably third-party tools) run after chdir to be\nin $GIT_DIR, so the problems they face when their /etc/gitconfig\nforces them to use safe.bareRepository=explicit are probably very\nsimilar either way.\n"},{"id":"490314","messageId":"xmqq5xxv0ywi.fsf_-_@gitster.g","threadId":"60771","inReplyTo":"20240308211957.3758770-1-gitster@pobox.com","subject":"[PATCH v2] setup: notice more types of implicit bare repositories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-09T23:27:09Z","receivedAt":"2024-03-09T23:27:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Builds directly on top of 45bb9162 (setup: allow cwd=.git w/\nbareRepository=explicit, 2024-01-20).\n\nInstead of saying \"primary worktree's $GIT_DIR is OK\", \"secondary\nworktree's $GIT_DIR is OK\", and \"submodule's $GIT_DIR is OK\"\nseparately, let's give them a name to call them collectively,\n\"implicit bare repository\" (for now, to reuse what an earlier commit\nused, which may not be an optimum name), as these share the same\nsecurity guarantee and convenience benefit.\n\nThe code got significantly simpler, and test moderately more\ncomplex, having to set up submodule tests.\n\n------- >8 ------------- >8 ------------- >8 -------\nSetting the safe.bareRepository configuration variable to explicit\nstops git from using a bare repository, unless the repository is\nexplicitly specified, either by the \"--git-dir=<path>\" command line\noption, or by exporting $GIT_DIR environment variable.  This may be\na reasonable measure to safeguard users from accidentally straying\ninto a bare repository in unexpected places, but often gets in the\nway of users who need valid accesses too the repository.\n\nEarlier, 45bb9162 (setup: allow cwd=.git w/ bareRepository=explicit,\n2024-01-20) loosened the rule such that being inside the \".git\"\ndirectory of a non-bare repository does not really count as\naccessing a \"bare\" repository.  The reason why such a loosening is\nneeded is because often hooks and third-party tools run from within\n$GIT_DIR while working with a non-bare repository.\n\nMore importantly, the reason why this is safe is because a directory\nwhose contents look like that of a \"bare\" repository cannot be a\nbare repository that came embedded within a checkout of a malicious\nproject, as long as its directory name is \".git\", because \".git\" is\nnot a name allowed for a directory in payload.\n\nThere are at least two other cases where tools have to work in a\nbare-repository looking directory that is not an embedded bare\nrepository, and accesses to them are still not allowed by the recent\nchange.\n\n - A secondary worktree (whose name is $name) has its $GIT_DIR\n   inside \"worktrees/$name/\" subdirectory of the $GIT_DIR of the\n   primary worktree of the same repository.\n\n - A submodule worktree (whose name is $hame) has its $GIT_DIR\n   inside \"modules/$name/\" subdirectory of the $GIT_DIR of its\n   superproject.\n\nAs long as the primary worktree or the superproject in these cases\nare not bare, the pathname of these \"looks like bare but not really\"\ndirectories will have \"/.git/worktrees/\" and \"/.git/modules/\" as a\nsubstring in its leading part, and we can take advantage of the same\nsecurity guarantee allow git to work from these places.\n\nExtend the earlier \"in a directory called '.git' we are OK\" logic\nused for the primary worktree to also cover the secondary worktree's\nand non-embedded submodule's $GIT_DIR, by moving the logic to a\nhelper function \"is_implicit_bare_repo()\".  We deliberately exclude\nsecondary worktrees and submodules of a bare repository, as these\nare exactly what safe.bareRepository=explicit setting is designed to\nforbid accesses to without an explicit GIT_DIR/--git-dir=<path>\n\nHelped-by: Kyle Lippincott <spectral@google.com>\nHelped-by: Kyle Meyer <kyle@kyleam.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n setup.c                         | 28 +++++++++++++++++++++++++++-\n t/t0035-safe-bare-repository.sh | 26 ++++++++++++++++++++++----\n 2 files changed, 49 insertions(+), 5 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex a09b7b87ec..25d98ee6dd 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1231,6 +1231,32 @@ static const char *allowed_bare_repo_to_string(\n \treturn NULL;\n }\n \n+static int is_implicit_bare_repo(const char *path)\n+{\n+\t/*\n+\t * what we found is a \".git\" directory at the root of\n+\t * the working tree.\n+\t */\n+\tif (ends_with_path_components(path, \".git\"))\n+\t\treturn 1;\n+\n+\t/*\n+\t * we are inside $GIT_DIR of a secondary worktree of a\n+\t * non-bare repository.\n+\t */\n+\tif (strstr(path, \"/.git/worktrees/\"))\n+\t\treturn 1;\n+\n+\t/*\n+\t * we are inside $GIT_DIR of a worktree of a non-embedded\n+\t * submodule, whose superproject is not a bare repository.\n+\t */\n+\tif (strstr(path, \"/.git/modules/\"))\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n /*\n  * We cannot decide in this function whether we are in the work tree or\n  * not, since the config can only be read _after_ this function was called.\n@@ -1360,7 +1386,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\tif (is_git_directory(dir->buf)) {\n \t\t\ttrace2_data_string(\"setup\", NULL, \"implicit-bare-repository\", dir->buf);\n \t\t\tif (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT &&\n-\t\t\t    !ends_with_path_components(dir->buf, \".git\"))\n+\t\t\t    !is_implicit_bare_repo(dir->buf))\n \t\t\t\treturn GIT_DIR_DISALLOWED_BARE;\n \t\t\tif (!ensure_valid_ownership(NULL, NULL, dir->buf, report))\n \t\t\t\treturn GIT_DIR_INVALID_OWNERSHIP;\ndiff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh\nindex 8048856379..d3cb2a1cb9 100755\n--- a/t/t0035-safe-bare-repository.sh\n+++ b/t/t0035-safe-bare-repository.sh\n@@ -29,9 +29,20 @@ expect_rejected () {\n \tgrep -F \"implicit-bare-repository:$pwd\" \"$pwd/trace.perf\"\n }\n \n-test_expect_success 'setup bare repo in worktree' '\n+test_expect_success 'setup an embedded bare repo, secondary worktree and submodule' '\n \tgit init outer-repo &&\n-\tgit init --bare outer-repo/bare-repo\n+\tgit init --bare --initial-branch=main outer-repo/bare-repo &&\n+\tgit -C outer-repo worktree add ../outer-secondary &&\n+\ttest_path_is_dir outer-secondary &&\n+\t(\n+\t\tcd outer-repo &&\n+\t\ttest_commit A &&\n+\t\tgit push bare-repo +HEAD:refs/heads/main &&\n+\t\tgit -c protocol.file.allow=always \\\n+\t\t\tsubmodule add --name subn -- ./bare-repo subd\n+\t) &&\n+\ttest_path_is_dir outer-repo/.git/worktrees/outer-secondary &&\n+\ttest_path_is_dir outer-repo/.git/modules/subn\n '\n \n test_expect_success 'safe.bareRepository unset' '\n@@ -53,8 +64,7 @@ test_expect_success 'safe.bareRepository in the repository' '\n \t# safe.bareRepository must not be \"explicit\", otherwise\n \t# git config fails with \"fatal: not in a git directory\" (like\n \t# safe.directory)\n-\ttest_config -C outer-repo/bare-repo safe.bareRepository \\\n-\t\tall &&\n+\ttest_config -C outer-repo/bare-repo safe.bareRepository all &&\n \ttest_config_global safe.bareRepository explicit &&\n \texpect_rejected -C outer-repo/bare-repo\n '\n@@ -86,4 +96,12 @@ test_expect_success 'no trace when \"bare repository\" is a subdir of .git' '\n \texpect_accepted_implicit -C outer-repo/.git/objects\n '\n \n+test_expect_success 'no trace in $GIT_DIR of secondary worktree' '\n+\texpect_accepted_implicit -C outer-repo/.git/worktrees/outer-secondary\n+'\n+\n+test_expect_success 'no trace in $GIT_DIR of a submodule' '\n+\texpect_accepted_implicit -C outer-repo/.git/modules/subn\n+'\n+\n test_done\n-- \n2.44.0-165-ge09f1254c5\n\n"},{"id":"490391","messageId":"CAO_smVhAp4V1pb7LQV7yvhs98JVrtDgW5LzjzyJHupGuGSA+sg@mail.gmail.com","threadId":"60771","inReplyTo":"xmqq5xxv0ywi.fsf_-_@gitster.g","subject":"Re: [PATCH v2] setup: notice more types of implicit bare repositories","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-03-11T19:23:08Z","receivedAt":"2024-03-11T19:23:28Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Sat, Mar 9, 2024 at 3:27 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Builds directly on top of 45bb9162 (setup: allow cwd=.git w/\n> bareRepository=explicit, 2024-01-20).\n>\n> Instead of saying \"primary worktree's $GIT_DIR is OK\", \"secondary\n> worktree's $GIT_DIR is OK\", and \"submodule's $GIT_DIR is OK\"\n> separately, let's give them a name to call them collectively,\n> \"implicit bare repository\" (for now, to reuse what an earlier commit\n> used, which may not be an optimum name), as these share the same\n> security guarantee and convenience benefit.\n>\n> The code got significantly simpler, and test moderately more\n> complex, having to set up submodule tests.\n>\n> ------- >8 ------------- >8 ------------- >8 -------\n> Setting the safe.bareRepository configuration variable to explicit\n> stops git from using a bare repository, unless the repository is\n> explicitly specified, either by the \"--git-dir=<path>\" command line\n> option, or by exporting $GIT_DIR environment variable.  This may be\n> a reasonable measure to safeguard users from accidentally straying\n> into a bare repository in unexpected places, but often gets in the\n> way of users who need valid accesses too the repository.\n\nnit: 'to', not 'too'\n\n>\n> Earlier, 45bb9162 (setup: allow cwd=.git w/ bareRepository=explicit,\n> 2024-01-20) loosened the rule such that being inside the \".git\"\n> directory of a non-bare repository does not really count as\n> accessing a \"bare\" repository.  The reason why such a loosening is\n> needed is because often hooks and third-party tools run from within\n> $GIT_DIR while working with a non-bare repository.\n>\n> More importantly, the reason why this is safe is because a directory\n> whose contents look like that of a \"bare\" repository cannot be a\n> bare repository that came embedded within a checkout of a malicious\n> project, as long as its directory name is \".git\", because \".git\" is\n> not a name allowed for a directory in payload.\n>\n> There are at least two other cases where tools have to work in a\n> bare-repository looking directory that is not an embedded bare\n> repository, and accesses to them are still not allowed by the recent\n> change.\n>\n>  - A secondary worktree (whose name is $name) has its $GIT_DIR\n>    inside \"worktrees/$name/\" subdirectory of the $GIT_DIR of the\n>    primary worktree of the same repository.\n>\n>  - A submodule worktree (whose name is $hame) has its $GIT_DIR\n\nnit: '$name', not '$hame'\n\n\n>    inside \"modules/$name/\" subdirectory of the $GIT_DIR of its\n>    superproject.\n>\n> As long as the primary worktree or the superproject in these cases\n> are not bare, the pathname of these \"looks like bare but not really\"\n> directories will have \"/.git/worktrees/\" and \"/.git/modules/\" as a\n> substring in its leading part, and we can take advantage of the same\n> security guarantee allow git to work from these places.\n>\n> Extend the earlier \"in a directory called '.git' we are OK\" logic\n> used for the primary worktree to also cover the secondary worktree's\n> and non-embedded submodule's $GIT_DIR, by moving the logic to a\n> helper function \"is_implicit_bare_repo()\".  We deliberately exclude\n> secondary worktrees and submodules of a bare repository, as these\n> are exactly what safe.bareRepository=explicit setting is designed to\n> forbid accesses to without an explicit GIT_DIR/--git-dir=<path>\n>\n> Helped-by: Kyle Lippincott <spectral@google.com>\n> Helped-by: Kyle Meyer <kyle@kyleam.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  setup.c                         | 28 +++++++++++++++++++++++++++-\n>  t/t0035-safe-bare-repository.sh | 26 ++++++++++++++++++++++----\n>  2 files changed, 49 insertions(+), 5 deletions(-)\n\nLooks good, thanks!\n"},{"id":"490397","messageId":"xmqqsf0wpjlv.fsf@gitster.g","threadId":"60771","inReplyTo":"CAO_smVhAp4V1pb7LQV7yvhs98JVrtDgW5LzjzyJHupGuGSA+sg@mail.gmail.com","subject":"Re: [PATCH v2] setup: notice more types of implicit bare repositories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-11T21:02:52Z","receivedAt":"2024-03-11T21:02:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Lippincott <spectral@google.com> writes:\n\n>> into a bare repository in unexpected places, but often gets in the\n>> way of users who need valid accesses too the repository.\n>\n> nit: 'to', not 'too'\n> ...\n>>  - A submodule worktree (whose name is $hame) has its $GIT_DIR\n>\n> nit: '$name', not '$hame'\n> ...\n>> Helped-by: Kyle Lippincott <spectral@google.com>\n>> Helped-by: Kyle Meyer <kyle@kyleam.com>\n>> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n>> ---\n>>  setup.c                         | 28 +++++++++++++++++++++++++++-\n>>  t/t0035-safe-bare-repository.sh | 26 ++++++++++++++++++++++----\n>>  2 files changed, 49 insertions(+), 5 deletions(-)\n>\n> Looks good, thanks!\n\nThanks.\n"}]}