Re: [BUG] Git 2.39.0+ Git.pm ignores Directory=> argument for bare repos

* Re: [BUG] Git 2.39.0+ Git.pm ignores Directory=> argument for bare repos
@ 2023-08-01  8:35 Hank Leininger
  0 siblings, 0 replies; 3+ messages in thread
From: Hank Leininger @ 2023-08-01  8:35 UTC (permalink / raw)
  To: git

[-- Attachment #1: Type: text/plain, Size: 7313 bytes --]

tl;dr: in the process of digging into this I've identified a case where
Git.pm still fails safe directory checking; skip to the end.

On 2023-07-31, Junio C Hamano wrote:
> > Hank Leininger <hlein@korelogic.com> writes:

> > Recent git versions (2.39.0 through 2.41.0) Git.pm seems to forget its
> > Directory argument for bare repos. Initial creation of a
> > Git->repository object will succeed, but subsequent $repo->command()
> > fails unless the repo is in pwd or is set in the GIT_DIR environment
> > argument.

> $ git log --oneline v2.38.0..v2.39.0 -- perl/Git.pm
> 20da61f25f Git.pm: trust rev-parse to find bare repositories
> 77a1310e6b Git.pm: add semicolon after catch statement

> My guess is 20da61f25f is likely the source of the differences, but
> it is unclear if that should be called a "bug", as it was done as a
> fix for misbehaviour.
>
> commit 20da61f25f8f61a2b581b60f8820ad6116f88e6f
> Author: Jeff King <peff@peff.net>
> Date:   Sat Oct 22 18:08:59 2022 -0400
...
>     But it gets worse. Since 8959555cee (setup_git_directory(): add an owner
>     check for the top-level directory, 2022-03-02), it's actively wrong (and
>     dangerous). The perl code doesn't implement the same ownership checks.
>     And worse, after "finding" the bare repository, it sets GIT_DIR in the
>     environment, which tells any subsequent Git commands that we've
>     confirmed the directory is OK, and to trust us. I.e., it re-opens the
>     vulnerability plugged by 8959555cee when using Git.pm's repository
>     discovery code.

Thanks for looking.  I think you're right that 20da61f25f is what's
responsible, but... of course it is a bug? (At the risk of you saying
"Don't quote the deep lore to me, I was there when it was written".)

Git.pm's documentation says that it supports passing a Directory
argument to specify where to look, and in the case of a bare repository:

  [...] If no ".git" directory was found, the "Directory" is assumed to
  be a bare repository, "Repository" is set to point at it and
  "WorkingCopy" is left undefined.

But it seems this isn't happening any more.

For non-bare repos, Directory is used for the initial rev-parse
--is-bare-repository check and for subsequent operations.

For bare repos, Directory is used for the initial rev-parse
--is-bare-repository check but then _not_ for subsequent operations.

Either breaking bare repo support for Directory is unintentional, in
which case this is a bug, or it's intentional, in which case the
documentation still stating Directory is supported for bare repos is a
bug.

However, this did point me at a workaround: the Repository argument
*does* work, so one can call:

  Git->repository(Repository=>"/path/to/foo.git")

...On a bare repo that is not pwd and things will work as intended.

Hm, then I wondered why that worked, looked closer at the logic, and
found a remaining safe.directory problem.

When considering these arguments and checking if a repo is bare, Git.pm
does:

        if (defined $opts{Directory}) {
                -d $opts{Directory} or throw Error::Simple("Directory not found: $opts{Directory} $!");

                my $search = Git->repository(WorkingCopy => $opts{Directory});
...
                  $out = $search->command([qw(rev-parse --is-bare-repository --git-dir)],
                                          STDERR => 0);
...
                chomp $out;
                my ($bare, $dir) = split /\n/, $out, 2;
...
                if ($bare ne 'true') {
                        File::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir;
                        $opts{Repository} = Cwd::abs_path($dir);
...
               } else {
                        $opts{Repository} = Cwd::abs_path($dir);

So:

- First we create a new Git->repository object with WorkingCopy set to
  the specified directory

- Then we run rev-parse --is-bare-repository --git-dir ...

- ...Which internally cd's to the path currently set in WorkingCopy.
  strace shows a child forking and chdir'ing to the specified path prior
  to execve("git", "rev-parse",...)

- Because it has cd'd first, rev-parse --is-bare-repository output is:
  true
  .

- We peel that . off, get its absolute path, and set Repository to that.
  Except, we didn't chdir, only the child did, and we discarded
  WorkingCopy and the original Repository argument here.  We are sitting
  in the program's original pwd, not the Repository path.

- Satisifed that we're looking at a valid bare git repo, we proceed, but
  any subsequent ->command() or whatever will be looking at our
  clobbered Directory / Repository, not the one specified.

So that explains why setting Directory for bare repos currently fails.

Conversely, if Repository is specified and Directory is not, then all
the above logic gets sidestepped, and the provided Repository is simply
remembered and used with zero other complications:

        if (not defined $opts{Repository} and not defined $opts{WorkingCopy}
                and not defined $opts{Directory}) {
...[not followed]
        }
        if (defined $opts{Directory}) {
...[not followed]
                  $out = $search->command([qw(rev-parse --is-bare-repository --git-dir)],
...
        }
        $self = { opts => \%opts };
        bless $self, $class;

Which, come to think of it, is a problem.

In the case where $opts{Repository} is defined, then no git rev-parse...
is ever done. We rely on that initial git rev-parse to enforce git's
ownership (safe.directory again). Therefore, using Repository=> is
unsafe:

  patsy@ultron ~/tmp/repo-test $ ls -ld test3.git
  drwxrwxr-x 6 hlein hlein 4096 Aug  1 01:57 test3.git

  # Git.pm is willing
  patsy@ultron ~/tmp/repo-test $ perl -MGit -e 'my $repo = Git->repository(Repository=>"/home/patsy/tmp/repo-test/test3.git")||die "git open failed: $!\n"; $repo->command("rev-list", "--all", "-1"); print "rev-list succeeded\n"' 
  rev-list succeeded

  # regular git is not
  patsy@ultron ~/tmp/repo-test $ git -C test3.git rev-parse --is-bare-repository --git-dir 
  fatal: detected dubious ownership in repository at '/home/patsy/tmp/repo-test/test3.git'
  To add an exception for this directory, call:

        git config --global --add safe.directory /home/patsy/tmp/repo-test/test3.git

  patsy@ultron ~/tmp/repo-test/test3.git $ git rev-parse --is-bare-repository --git-dir 
  fatal: detected dubious ownership in repository at '/home/patsy/tmp/repo-test/test3.git'
  To add an exception for this directory, call:

        git config --global --add safe.directory /home/patsy/tmp/repo-test/test3.git

So in the case of Git->repository(Repository=>"..."), Git.pm misses the
safe directory check entirely. I think that's for the same reason called
out in that earlier commit message: once it has decided the specified
path is good (in this case by doing no checks whatsoever), that path
will be set in $GIT_DIR prior to launching user-specified git commands,
disabling  those checks - exactly the concern discussed in the commit
message for 20da61f25f.

Thanks,

-- 

Hank Leininger <hlein@korelogic.com>
9606 3BF9 B593 4CBC E31A  A384 6200 F6E3 781E 3DD7

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread