Re: [PATCH] setup: allow cwd=.git w/ bareRepository=explicit

[Junio C Hamano <gitster@pobox.com>]

"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.