On Sat, Jan 20, 2024 at 2:26 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > "Kyle Lippincott via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > > > From: Kyle Lippincott <spectral@xxxxxxxxxx> > > > > 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.
