Git.pm: trust rev-parse to find bare repositories

When initializing a repository object, we run "git rev-parse --git-dir"
to let the C version of Git find the correct directory. But curiously,
if this fails we don't automatically say "not a git repository".
Instead, we do our own pure-perl check to see if we're in a bare
repository.

This makes little sense, as rev-parse will report both bare and non-bare
directories. This logic comes from d5c7721d58 (Git.pm: Add support for
subdirectories inside of working copies, 2006-06-24), but I don't see
any reason given why we can't just rely on rev-parse. Worse, because we
treat any non-error response from rev-parse as a non-bare repository,
we'll erroneously set the object's WorkingCopy, even in a bare
repository.

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.

We can fix this by just relying on rev-parse to tell us when we're not
in a repository, which fixes the vulnerability. Furthermore, we'll ask
its --is-bare-repository function to tell us if we're bare or not, and
rely on that.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/perl/Git.pm b/perl/Git.pm
index cf15ead664a7b0368ba6e429c9d4e5fa9abe032e..117765dc73c4a8c30bfbcf9b3b37bad6b26a9ede 100644 (file)
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -177,16 +177,27 @@ sub repository {
                -d $opts{Directory} or throw Error::Simple("Directory not found: $opts{Directory} $!");
 
                my $search = Git->repository(WorkingCopy => $opts{Directory});
-               my $dir;
+
+               # This rev-parse will throw an exception if we're not in a
+               # repository, which is what we want, but it's kind of noisy.
+               # Ideally we'd capture stderr and relay it, but doing so is
+               # awkward without depending on it fitting in a pipe buffer. So
+               # we just reproduce a plausible error message ourselves.
+               my $out;
                try {
-                       $dir = $search->command_oneline(['rev-parse', '--git-dir'],
-                                                       STDERR => 0);
+                 # Note that "--is-bare-repository" must come first, as
+                 # --git-dir output could contain newlines.
+                 $out = $search->command([qw(rev-parse --is-bare-repository --git-dir)],
+                                         STDERR => 0);
                } catch Git::Error::Command with {
-                       $dir = undef;
+                       throw Error::Simple("fatal: not a git repository: $opts{Directory}");
                };
 
+               chomp $out;
+               my ($bare, $dir) = split /\n/, $out, 2;
+
                require Cwd;
-               if ($dir) {
+               if ($bare ne 'true') {
                        require File::Spec;
                        File::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir;
                        $opts{Repository} = Cwd::abs_path($dir);
@@ -204,21 +215,6 @@ sub repository {
                        $opts{WorkingSubdir} = $prefix;
 
                } else {
-                       # A bare repository? Let's see...
-                       $dir = $opts{Directory};
-
-                       unless (-d "$dir/refs" and -d "$dir/objects" and -e "$dir/HEAD") {
-                               # Mimic git-rev-parse --git-dir error message:
-                               throw Error::Simple("fatal: Not a git repository: $dir");
-                       }
-                       my $search = Git->repository(Repository => $dir);
-                       try {
-                               $search->command('symbolic-ref', 'HEAD');
-                       } catch Git::Error::Command with {
-                               # Mimic git-rev-parse --git-dir error message:
-                               throw Error::Simple("fatal: Not a git repository: $dir");
-                       };
-
                        $opts{Repository} = Cwd::abs_path($dir);
                }
 

diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
index 4aa5d90d328aca4adcac5f0d3a2ee4946a393812..b105d6d9d57722bc7101e45b6726aef3da6b7d5c 100755 (executable)
--- a/t/t9700-perl-git.sh
+++ b/t/t9700-perl-git.sh
@@ -45,6 +45,10 @@ test_expect_success \
      git config --add test.pathmulti bar
      '
 
+test_expect_success 'set up bare repository' '
+       git init --bare bare.git
+'
+
 test_expect_success 'use t9700/test.pl to test Git.pm' '
        "$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl 2>stderr &&
        test_must_be_empty stderr

diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index e046f7db762d355228425ff5b77639be65b1272c..6d753708d2acb6c7bcf47e5a057d99845c1006a3 100755 (executable)
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -30,6 +30,18 @@ BEGIN { use_ok('Git') }
 # set up
 our $abs_repo_dir = cwd();
 ok(our $r = Git->repository(Directory => "."), "open repository");
+{
+       local $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1;
+       my $failed;
+
+       $failed = eval { Git->repository(Directory => $abs_repo_dir) };
+       ok(!$failed, "reject unsafe non-bare repository");
+       like($@, qr/not a git repository/i, "unsafe error message");
+
+       $failed = eval { Git->repository(Directory => "$abs_repo_dir/bare.git") };
+       ok(!$failed, "reject unsafe bare repository");
+       like($@, qr/not a git repository/i, "unsafe error message");
+}
 
 # config
 is($r->config("test.string"), "value", "config scalar: string");