Re: [PATCH] setup: allow cwd=.git w/ bareRepository=explicit
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 20, 2024, 22:26 UTC
- Message-ID
- <xmqqh6j7ej5w.fsf@gitster.g>
- In-Reply-To
- <pull.1645.git.1705709303098.gitgitgadget@gmail.com>
"Kyle Lippincott via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 9 quoted lines
> 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?
Show 7 quoted lines
> 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.