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

From: Kyle Lippincott <spectral@google.com>
Date: Jan 22, 2024, 20:50 UTC
Message-ID: <CAO_smViDR-JKRiKO-8-6mCGBpCR8Y1gLS9Y9DkoCFw=kHm5Mdw@mail.gmail.com>
In-Reply-To: <xmqqh6j7ej5w.fsf@gitster.g>

On Sat, Jan 20, 2024 at 2:26 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

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

Show 18 quoted lines

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

Show 15 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.

Agreed, it's not worth the additional complexity.

Show 7 quoted lines

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