| Message ID | pull.1645.git.1705709303098.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 45bb91624804d3e3a70cfc1ba0eae5577f81fc38 |
| Headers | show |
| Series | setup: allow cwd=.git w/ bareRepository=explicit | expand |
"Kyle Lippincott via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Kyle Lippincott <spectral@google.com> > > The safe.bareRepository setting can be set to 'explicit' to disallow > implicit uses of bare repositories, preventing an attack [1] where an > artificial and malicious bare repository is embedded in another git > repository. Unfortunately, some tooling uses myrepo/.git/ as the cwd > when executing commands, and this is blocked when > safe.bareRepository=explicit. Blocking is unnecessary, as git already > prevents nested .git directories. In other words, if the directory $D that is the top level of the working tree of a non-bare repository, you should be able to chdir to "$D/.git" and run your git command without explicitly saying that you are inside $GIT_DIR (e.g. "git --git-dir=$(pwd) cmd")? It makes very good sense. I briefly wondered if this would give us a great usability improvement especially for hook scripts, but they are given GIT_DIR when called already so that is not a big upside, I guess. > Teach git to not reject uses of git inside of the .git directory: check > if cwd is .git (or a subdirectory of it) and allow it even if > safe.bareRepository=explicit. > My primary concern with this patch is that I'm unsure if we need to > worry about case-insensitive filesystems (ex: cwd=my_repo/.GIT instead > of my_repo/.git, it might not trigger this logic and end up allowed). You are additionally allowing ".git" so even if somebody has ".GIT" that won't be allowed by this change, no? > I'm assuming this isn't a significant concern, for two reasons: > > * most filesystems/OSes in use today (by number of users) are at least > case-preserving, so users/tools will have had to type out .GIT > instead of getting it from readdir/wherever. > * this is primarily a "quality of life" change to the feature, and if > we get it wrong we still fail closed. Yup. If we really cared (which I doubt), we could resort to checking with is_ntfs_dotgit() and is_hfs_dotgit(), but that would work in the direction of loosening the check even further, which I do not know is needed. > - if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT) > + if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT && > + !ends_with_path_components(dir->buf, ".git")) > return GIT_DIR_DISALLOWED_BARE; Thanks.
On Sat, Jan 20, 2024 at 2:26 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Kyle Lippincott via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Kyle Lippincott <spectral@google.com> > > > > The safe.bareRepository setting can be set to 'explicit' to disallow > > implicit uses of bare repositories, preventing an attack [1] where an > > artificial and malicious bare repository is embedded in another git > > repository. Unfortunately, some tooling uses myrepo/.git/ as the cwd > > when executing commands, and this is blocked when > > safe.bareRepository=explicit. Blocking is unnecessary, as git already > > prevents nested .git directories. > > In other words, if the directory $D that is the top level of the > working tree of a non-bare repository, you should be able to chdir > to "$D/.git" and run your git command without explicitly saying that > you are inside $GIT_DIR (e.g. "git --git-dir=$(pwd) cmd")? Correct. > > It makes very good sense. > > I briefly wondered if this would give us a great usability > improvement especially for hook scripts, but they are given GIT_DIR > when called already so that is not a big upside, I guess. > > > Teach git to not reject uses of git inside of the .git directory: check > > if cwd is .git (or a subdirectory of it) and allow it even if > > safe.bareRepository=explicit. > > > > My primary concern with this patch is that I'm unsure if we need to > > worry about case-insensitive filesystems (ex: cwd=my_repo/.GIT instead > > of my_repo/.git, it might not trigger this logic and end up allowed). > > You are additionally allowing ".git" so even if somebody has ".GIT" > that won't be allowed by this change, no? My concern was what happens if someone on a case-insensitive filesystem does `cd .GIT` and then attempts to use it. If the cwd path isn't case-normalized at some point, we'll have a string like /path/to/my-repo/.GIT from getcwd() and it won't be allowed by this code, since this code is checking for `.git` in a case sensitive fashion. > > > I'm assuming this isn't a significant concern, for two reasons: > > > > * most filesystems/OSes in use today (by number of users) are at least > > case-preserving, so users/tools will have had to type out .GIT > > instead of getting it from readdir/wherever. > > * this is primarily a "quality of life" change to the feature, and if > > we get it wrong we still fail closed. > > Yup. > > If we really cared (which I doubt), we could resort to checking with > is_ntfs_dotgit() and is_hfs_dotgit(), but that would work in the > direction of loosening the check even further, which I do not know > is needed. Agreed, it's not worth the additional complexity. > > > - if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT) > > + if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT && > > + !ends_with_path_components(dir->buf, ".git")) > > return GIT_DIR_DISALLOWED_BARE; > > Thanks.
"Kyle Lippincott via GitGitGadget" <gitgitgadget@gmail.com> writes: > Teach git to not reject uses of git inside of the .git directory: check > if cwd is .git (or a subdirectory of it) and allow it even if > safe.bareRepository=explicit. > diff --git a/setup.c b/setup.c > index b38702718fb..b095e284979 100644 > --- a/setup.c > +++ b/setup.c > @@ -1371,7 +1371,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir, > > if (is_git_directory(dir->buf)) { > trace2_data_string("setup", NULL, "implicit-bare-repository", dir->buf); > - if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT) > + if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT && > + !ends_with_path_components(dir->buf, ".git")) > return GIT_DIR_DISALLOWED_BARE; > if (!ensure_valid_ownership(NULL, NULL, dir->buf, report)) > return GIT_DIR_INVALID_OWNERSHIP; I wish we had caught it much before we added DISALLOWED_BARE thing, but I wonder how well would this escape-hatch interact with secondary worktrees, where their git directory is not named ".git" and not immediately below the root level of the working tree? In a secondary worktree the root level of its working tree has a file ".git", whose contents may look like gitdir: /home/gitster/git.git/.git/worktrees/git.old where - /home/gitster/git.git/ is the primary worktree with the repository. - /home/gitster/git.git/.git/worktrees/git.old looks like a bare repository. - /home/gitster/git.git/.git/worktrees/git.old/gitdir gives a way to discover the secondary worktree, whose contents just records the path to the ".git" file, e.g., "/home/gitster/git.old/.git" that had "gitdir: ..." in it. So perhaps we can also use the presence of "gitdir" file, check the contents of it tn ensure that ".git" file there takes us back to this (not quite) bare repository we are looking at, and allow access to it, or something? Thoughts?
diff --git a/setup.c b/setup.c index b38702718fb..b095e284979 100644 --- a/setup.c +++ b/setup.c @@ -1371,7 +1371,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir, if (is_git_directory(dir->buf)) { trace2_data_string("setup", NULL, "implicit-bare-repository", dir->buf); - if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT) + if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT && + !ends_with_path_components(dir->buf, ".git")) return GIT_DIR_DISALLOWED_BARE; if (!ensure_valid_ownership(NULL, NULL, dir->buf, report)) return GIT_DIR_INVALID_OWNERSHIP; diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh index 038b8b788d7..80488563795 100755 --- a/t/t0035-safe-bare-repository.sh +++ b/t/t0035-safe-bare-repository.sh @@ -78,4 +78,12 @@ test_expect_success 'no trace when GIT_DIR is explicitly provided' ' expect_accepted_explicit "$pwd/outer-repo/bare-repo" ' +test_expect_success 'no trace when "bare repository" is .git' ' + expect_accepted_implicit -C outer-repo/.git +' + +test_expect_success 'no trace when "bare repository" is a subdir of .git' ' + expect_accepted_implicit -C outer-repo/.git/objects +' + test_done