Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Oct 21 2022, Michael McClimon wrote:

> The previous commit exposes a security flaw: it is possible to bypass
> unsafe repository checks by using Git.pm, where before the syntax error
> accidentally prohibited it. This problem occurs because Git.pm sets
> GIT_DIR explicitly, which bypasses the safe repository checks.
>
> Fix this by introducing a new environment variable,
> GIT_PERL_FORCE_OWNERSHIP_CHECK, which we set true in Git.pm. In setup.c,
> if that environment variable is true, force ownership checks even if
> an explicit GIT_DIR is provided.

The 1/2 is unambiguously good, thanks.

As for this step, I think it's good if we want to go in this direction.

As to whether it's the direction we want...

The vulnerability safe.directory was supposed to address was one where
you'd set your fsmonitor hook via a config variable, so running "diff",
"status" etc. would unexpectedly execute arbitrary code.

Especially on Windows where apparently the equivalent of the root of a
shared mounted volume routinely has global write permissions (user's
subdirectories being less permissive).

An alternative I raised on the security list was to narrowly target just
the fsmonitor config, since that was the vulnerability.

But it was decided to cast a wider net, as it wasn't disclosed yet, and
the list members might have missed some other config variable that would
allow shenanigans, fair enough, especially for a time constrained
security fix.

Now that the cat's thoroughly out of the bag I think we'd do well to
reconsider that.

I'm unaware of any other variable(s) that provide a similar vector, and
safe.directory is encouraging users (especially in core.sharedRepository
settings) to mark a dir as "safe", and we'd then later have an exploit
from a user with rw access who'd use the fsmonitor hook vector.

Better have them "safe by default", and start refusing to read the
config when we detect that fsmonitor setting, and insist the user mark
*that* safe.

Anyway, that's all on the general topic, and the above is just my
opinion on it.

But on this much more narrow topic: If we couldn't come up with an issue
other than the fsmonitor config for git's C codebase, I think extending
the same mitigation to the very small part of git that's still in Perl
is thoroughly into the tail wagging the dog territory.

I.e. what we'd presumably get out of this is mitigation against some
exploit via a variable that git-send-email or git-svn use (or the
handful of other more obscure Perl tooling we have).

Can we think of one that could be an issue, and if not is the current
behavior in 1/2 maybe OK as-is?

>  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 e046f7db..1c91019f 100755
> --- a/t/t9700/test.pl
> +++ b/t/t9700/test.pl
> @@ -142,6 +142,24 @@ sub adjust_dirsep {
>  		     "abc\"\\ \x07\x08\x09\x0a\x0b\x0c\x0d\x01 ",
>  		     'unquote escape sequences');
>  
> +# safe directory
> +{
> +	local $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1;
> +	# Save stderr to a tempfile so we can check the contents
> +	open our $tmpstderr2, ">&STDERR" or die "cannot save STDERR";
> +	my $tmperr = "unsafeerr.tmp";
> +	open STDERR, ">", "$tmperr" or die "cannot redirect STDERR to $tmperr";
> +	my $failed = eval { Git->repository(Directory => "$abs_repo_dir/bare.git") };
> +	ok(!$failed, "reject unsafe repository");
> +	like($@, qr/not a git repository/i, "unsafe error message");
> +	open TEMPFILE, "<", "$tmperr" or die "Can't open $tmperr $!";
> +	my $errcontents;
> +	{ local $/; $errcontents = <TEMPFILE>; }
> +	like($errcontents, qr/dubious ownership/, "dubious ownership message");
> +	close STDERR or die "cannot close temp stderr";
> +	open STDERR, ">&", $tmpstderr2 or die "cannot restore STDERR";
> +}

This whole "save stderr to a file" etc. seems much more complex than
just writing the same test in our normal *.sh tests, and doing something
like:


	! GIT_TEST_ASSUME_DIFFERENT_OWNER=1 perl -MGit -we 'Git->repository(Directory => shift)' 2>expect &&
	grep "reject unusafe" ...

I.e. you're spending a lot of effort on capturing STDERR within a single
Perl process, then restoring it etc., when we can just run one-off
command and have it die (no eval), and just inspect the exit code &
stderr perl emitted.