Re: Bare repositories in the working tree are a security risk

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

Glen Choo <chooglen@google.com> writes:

> Yes, I mean that even the current directory will be ignored when
> discovery is disabled.
>
>>                                                I am not sure that
>> is realistically feasible (I am thinking of cases like "git fetch"
>> going to the remote repository on the local disk that is bare to run
>> "git upload-pack"), but if the fallout is not too bad, it may be a
>> good heuristics.
>
> Good detail - I hadn't considered the impact on our own child processes.
> I suspect this might be a huge undertaking. Unless there is significant
> interest in this option, I probably won't pursue it further.

So, I was doing a bit of experiment.  The change to the set-up
essentially means that working in either a bare repository or in the
.git/ subdirectory of a non-bare repository that you used to be able
to do with

    cd bare.git && git cmd
    cd .git/ && git cmd

is now forbidden, unless you explicitly say which directory you want
to use as the GIT_DIR, i.e.

    cd bare.git && GIT_DIR=. git cmd
    cd .git/ && GIT_DIR=. git cmd

The required change to the code is surprisingly small, but the
fallout is very big.  Partial patches to two tests are attached with
some commentary at the end.

 setup.c          | 9 +++++++--

diff --git c/setup.c w/setup.c
index a7b36f3ffb..52304ae57e 100644
--- c/setup.c
+++ w/setup.c
@@ -1203,12 +1203,15 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 	 * Test in the following order (relative to the dir):
 	 * - .git (file containing "gitdir: <path>")
 	 * - .git/
-	 * - ./ (bare)
+	 * - ./ (bare) --- not
 	 * - ../.git
 	 * - ../.git/
-	 * - ../ (bare)
+	 * - ../ (bare) --- not
 	 * - ../../.git
 	 *   etc.
+	 * we used to test the directory itself as a bare one
+	 * after checking if the directory has .git/ that is a repository
+	 * and before moving up the hierarchy, but no longer do so.
 	 */
 	one_filesystem = !git_env_bool("GIT_DISCOVERY_ACROSS_FILESYSTEM", 0);
 	if (one_filesystem)
@@ -1238,12 +1241,14 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 			return GIT_DIR_DISCOVERED;
 		}
 
+#if 0
 		if (is_git_directory(dir->buf)) {
 			if (!ensure_valid_ownership(dir->buf))
 				return GIT_DIR_INVALID_OWNERSHIP;
 			strbuf_addstr(gitdir, ".");
 			return GIT_DIR_BARE;
 		}
+#endif
 
 		if (offset <= min_offset)
 			return GIT_DIR_HIT_CEILING;


THe above is all it takes to disable the recognition of a bare
repository during the repositiry discovery.  The code to handle
explicitly specified GIT_DIR is way before the loop being touched
here and works OK as before.

Interestingly, many breakages in tests are not due to "clone"
failing to operate in "the other side".  For example, this one tries
to inspect a bare repository that was created by a clone operation,
and we need to tell "git" where the GIT_DIR is, even after we chdir
to it.  I do not think this is so common but with our popularity,
not-so-common population is still an absolute large number of users
that we do not want to hurt.

diff --git c/t/t5601-clone.sh w/t/t5601-clone.sh
index 4a61f2c901..822ee3164a 100755
--- c/t/t5601-clone.sh
+++ w/t/t5601-clone.sh
@@ -111,9 +111,9 @@ test_expect_success 'clone --mirror' '
 	git clone --mirror src mirror &&
 	test -f mirror/HEAD &&
 	test ! -f mirror/file &&
-	FETCH="$(cd mirror && git config remote.origin.fetch)" &&
+	FETCH="$(cd mirror && GIT_DIR=. git config remote.origin.fetch)" &&
 	test "+refs/*:refs/*" = "$FETCH" &&
-	MIRROR="$(cd mirror && git config --bool remote.origin.mirror)" &&
+	MIRROR="$(cd mirror && GIT_DIR=. git config --bool remote.origin.mirror)" &&
 	test "$MIRROR" = true
 
 '
@@ -151,7 +151,7 @@ test_expect_success 'clone --mirror does not repeat tags' '
 	 git tag some-tag HEAD) &&
 	git clone --mirror src mirror2 &&
 	(cd mirror2 &&
-	 git show-ref 2> clone.err > clone.out) &&
+	 GIT_DIR=. git show-ref 2>clone.err >clone.out) &&
 	! grep Duplicate mirror2/clone.err &&
 	grep some-tag mirror2/clone.out
 


The next example is "init" test, which seems to reveal that refusing
to discover a bare repository is not exactly an approach that is
workable.

The first two hunks just show the same pattern as above.  The
check_config helper is given a directory, under which 'config' and
'refs' should exist (in other words, the first part of the helper
emulates setup.c::is_git_directory()).  Then it tries to read from
the configuration file found in that directory.  Be it a bare
repository or .git/ subdirectory of a non-bare repository the caller
has given us, we need to do the same GIT_DIR=. dance as the above.

The last hunk does not really fix.  The scenario is this:

    * A bare repository bare-ancestor-aliased.git is prepared

    * Its config file says "[alias] aliasedinit=init" in it

    * In that bare repository (next to 'refs', 'config'), we create
      a subdirectory 'plain-nested'.

    * We go to that bare-ancestor-aliased.git/plain-nested/
      directory and say "git aliasedinit".

This wants to be able to create a subdirectory in this bare
repository and make that subdirectory an independent repository (it
happens to be a non-bare repository, but a bare repository should
also work---think of .git/modules/name/ bare repository that is
designed to be pointed at with gitfile: link from a submodule
working tree).

Now, in order for the last step to be able to even _use_ the alias,
it has to treat the bare-ancestor-aliased.git directory as a bare
repository and read its configuration.  But by explicitly setting
GIT_DIR to that location, the behaviour of "git init" itself is
changed.  It no longer initializes the current directory,
i.e. plain-nested subdirectory of the bare-ancestor-aliased.git;
GIT_DIR tells us to (re)initialize it instead.

This fundamentally does not work.

It is a separate matter if it makes sense to allow creating a new
repository inside a bare repository (it does---that is what the
modern submodule layout uses), or to allow an alias to help doing so
defined in the top-level bare repository (it probably does---the
end-user may want to have a handy way to customize how submodules
are configured).  But it seems to tell us that with today's external
interface we have for "git init", we cannot take configuration from
a shallower level and use it to drive "git init" to create a new
repository at a deeper level.

diff --git c/t/t0001-init.sh w/t/t0001-init.sh
index d479303efa..57f2eedac0 100755
--- c/t/t0001-init.sh
+++ w/t/t0001-init.sh
@@ -6,6 +6,7 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check_config () {
+	# NEEDSWORK: refs could be a file in post reftable world.
 	if test_path_is_dir "$1" &&
 	   test_path_is_file "$1/config" && test_path_is_dir "$1/refs"
 	then
@@ -21,8 +22,8 @@ check_config () {
 		return 1
 	fi
 
-	bare=$(cd "$1" && git config --bool core.bare)
-	worktree=$(cd "$1" && git config core.worktree) ||
+	bare=$(cd "$1" && GIT_DIR=. git config --bool core.bare)
+	worktree=$(cd "$1" && GIT_DIR=. git config core.worktree) ||
 	worktree=unset
 
 	test "$bare" = "$2" && test "$worktree" = "$3" || {
@@ -84,7 +85,7 @@ test_expect_success 'plain nested in bare through aliased command' '
 		echo "[alias] aliasedinit = init" >>config &&
 		mkdir plain-nested &&
 		cd plain-nested &&
-		git aliasedinit
+		GIT_DIR=.. git aliasedinit ;# does not work
 	) &&
 	check_config bare-ancestor-aliased.git/plain-nested/.git false unset
 '