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

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

Michael McClimon <michael@mcclimon.org> writes:

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

Drop "PERL_" from the name, as any third-party tool that does its
own repository discovery and sets GIT_DIR before it invokes git
would need a similar "Yes, I am setting GIT_DIR, but I still want
you to check if that passes the usual ownership check", and there
is no reason to expect that these tools are always written in Perl.

How about "GIT_SAFE_DIRECTORY_STRICT" or something along that line,
instead?

I also have to wonder (and this is *not* a suggestion for inventing
an alternative fix for perl/Git.pm) if we were creating perl/Git.pm
from scratch today, we even need to be worried about this issue.  We
may have Git::repo_path() helper but if we call it in a natural way
(read: as if an interactive end-user would type commands), it is
likely that we would run "git rev-parse --git-dir" or something
without setting GIT_DIR, and when we need to run "git" command, say
"git diff", we would also run "git diff" as if the end user would
type from within their interactive session and without setting
GIT_DIR, and everything should work.  IOW, why do we even setting
and exporting the auto-detected value in GIT_DIR?

Also, if the end user had GIT_DIR in the environment _before_
calling something that uses "import Git", what should happen?  In
that case, the end-user is who is telling us that the named
directory is OK without any forced ownership check, so it feels
WRONG that this patch UNCONDITIONALLY exports FORCE_CHECK.  Only
when we did auto-discovery via Git::repo_path() without end-user
supplied GIT_DIR and exported GIT_DIR, we should also export
FORCE_CHECK to tell the underlying "git" that our auto-detection
may be flawed and it needs to double check, no?

> Signed-off-by: Michael McClimon <michael@mcclimon.org>
> ---
>  perl/Git.pm         |  1 +
>  setup.c             |  3 +++
>  t/t9700-perl-git.sh |  4 ++++
>  t/t9700/test.pl     | 18 ++++++++++++++++++
>  4 files changed, 26 insertions(+)
>
> diff --git a/perl/Git.pm b/perl/Git.pm
> index cf15ead6..002c29bb 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -1674,6 +1674,7 @@ sub _cmd_exec {
>  sub _setup_git_cmd_env {
>  	my $self = shift;
>  	if ($self) {
> +		$ENV{GIT_PERL_FORCE_OWNERSHIP_CHECK} = 1;
>  		$self->repo_path() and $ENV{'GIT_DIR'} = $self->repo_path();
>  		$self->repo_path() and $self->wc_path()
>  			and $ENV{'GIT_WORK_TREE'} = $self->wc_path();
> diff --git a/setup.c b/setup.c
> index cefd5f63..33d4e6fd 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1250,6 +1250,9 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>  	 */
>  	gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
>  	if (gitdirenv) {
> +		if (git_env_bool("GIT_PERL_FORCE_OWNERSHIP_CHECK", 0) &&
> +		    !ensure_valid_ownership(NULL, NULL, gitdirenv, report))
> +			return GIT_DIR_INVALID_OWNERSHIP;
>  		strbuf_addstr(gitdir, gitdirenv);
>  		return GIT_DIR_EXPLICIT;
>  	}
> diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
> index 4aa5d90d..b14a40b1 100755
> --- 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 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";
> +}
> +
>  printf "1..%d\n", Test::More->builder->current_test;
>  
>  my $is_passing = eval { Test::More->is_passing };