[PATCH v2 8/8] safe.bareRepository: default to "explicit" with WITH_BREAKING_CHANGES

["Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When an attacker can convince a user to clone a crafted repository
that contains an embedded bare repository with malicious hooks, any Git
command the user runs after entering that subdirectory will discover
the bare repository and execute the hooks. The user does not even need
to run a Git command explicitly: many shell prompts run `git status`
in the background to display branch and dirty state information, and
`git status` in turn may invoke the fsmonitor hook if so configured,
making the user vulnerable the moment they `cd` into the directory. The
`safe.bareRepository` configuration variable (introduced in 8959555cee7e
(setup_git_directory(): add an owner check for the top-level directory,
2022-03-02)) already provides protection against this attack vector by
allowing users to set it to "explicit", but the default remained "all"
for backwards compatibility.

Since Git 3.0 is the natural point to change defaults to safer
values, flip the default from "all" to "explicit" when built with
`WITH_BREAKING_CHANGES`. This means Git will refuse to work with bare
repositories that are discovered implicitly by walking up the directory
tree. Bare repositories specified via `--git-dir` or `GIT_DIR` continue
to work, and directories that look like `.git`, worktrees, or submodule
directories are unaffected (the existing `is_implicit_bare_repo()`
whitelist handles those cases).

Users who rely on implicit bare repository discovery can restore the
previous behavior by setting `safe.bareRepository=all` in their global
or system configuration.

The test for the "safe.bareRepository in the repository" scenario
needed a more involved fix: it writes a `safe.bareRepository=all`
entry into the bare repository's own config to verify that repo-local
config does not override the protected (global) setting. Previously,
`test_config -C` was used to write that entry, but its cleanup runs `git
-C <bare-repo> config --unset`, which itself fails when the default is
"explicit" and the global config has already been cleaned up. Switching
to direct git config --file access avoids going through repository
discovery entirely.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/BreakingChanges.adoc | 24 ++++++++++++++++++++++++
 Documentation/config/safe.adoc     | 10 ++++++++--
 setup.c                            |  4 ++++
 t/t0035-safe-bare-repository.sh    | 10 ++++++++--
 4 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/Documentation/BreakingChanges.adoc b/Documentation/BreakingChanges.adoc
index af59c43f42..73bb939359 100644
--- a/Documentation/BreakingChanges.adoc
+++ b/Documentation/BreakingChanges.adoc
@@ -216,6 +216,30 @@ would be significant, we may decide to defer this change to a subsequent minor
 release. This evaluation will also take into account our own experience with
 how painful it is to keep Rust an optional component.
 
+* The default value of `safe.bareRepository` will change from `all` to
+  `explicit`. It is all too easy for an attacker to trick a user into cloning a
+  repository that contains an embedded bare repository with malicious hooks
+  configured. If the user enters that subdirectory and runs any Git command, Git
+  discovers the bare repository and the hooks fire. The user does not even need
+  to run a Git command explicitly: many shell prompts run `git status` in the
+  background to display branch and dirty state information, and `git status` in
+  turn may invoke the fsmonitor hook if so configured, making the user
+  vulnerable the moment they `cd` into the directory. The `safe.bareRepository`
+  configuration variable was introduced in 8959555cee (setup_git_directory():
+  add an owner check for the top-level directory, 2022-03-02) with a default of
+  `all` to preserve backwards compatibility.
++
+Changing the default to `explicit` means that Git will refuse to work with bare
+repositories that are discovered implicitly by walking up the directory tree.
+Bare repositories specified explicitly via the `--git-dir` command-line option
+or the `GIT_DIR` environment variable continue to work regardless of this
+setting. Repositories that look like a `.git` directory, a worktree, or a
+submodule directory are also unaffected.
++
+Users who rely on implicit discovery of bare repositories can restore the
+previous behavior by setting `safe.bareRepository=all` in their global or
+system configuration.
+
 === Removals
 
 * Support for grafting commits has long been superseded by git-replace(1).
diff --git a/Documentation/config/safe.adoc b/Documentation/config/safe.adoc
index 2d45c98b12..5b1690aebe 100644
--- a/Documentation/config/safe.adoc
+++ b/Documentation/config/safe.adoc
@@ -2,10 +2,12 @@ safe.bareRepository::
 	Specifies which bare repositories Git will work with. The currently
 	supported values are:
 +
-* `all`: Git works with all bare repositories. This is the default.
+* `all`: Git works with all bare repositories. This is the default in
+  Git 2.x.
 * `explicit`: Git only works with bare repositories specified via
   the top-level `--git-dir` command-line option, or the `GIT_DIR`
-  environment variable (see linkgit:git[1]).
+  environment variable (see linkgit:git[1]). This will be the default
+  in Git 3.0.
 +
 If you do not use bare repositories in your workflow, then it may be
 beneficial to set `safe.bareRepository` to `explicit` in your global
@@ -13,6 +15,10 @@ config. This will protect you from attacks that involve cloning a
 repository that contains a bare repository and running a Git command
 within that directory.
 +
+If you use bare repositories regularly and want to preserve the current
+behavior after upgrading to Git 3.0, set `safe.bareRepository` to `all`
+in your global or system config.
++
 This config setting is only respected in protected configuration (see
 <<SCOPES>>). This prevents untrusted repositories from tampering with
 this value.
diff --git a/setup.c b/setup.c
index 7ec4427368..17c0662076 100644
--- a/setup.c
+++ b/setup.c
@@ -1485,7 +1485,11 @@ static int allowed_bare_repo_cb(const char *key, const char *value,
 
 static enum allowed_bare_repo get_allowed_bare_repo(void)
 {
+#ifdef WITH_BREAKING_CHANGES
+	enum allowed_bare_repo result = ALLOWED_BARE_REPO_EXPLICIT;
+#else
 	enum allowed_bare_repo result = ALLOWED_BARE_REPO_ALL;
+#endif
 	git_protected_config(allowed_bare_repo_cb, &result);
 	return result;
 }
diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
index ae7ef092ab..1d3d19f5b4 100755
--- a/t/t0035-safe-bare-repository.sh
+++ b/t/t0035-safe-bare-repository.sh
@@ -44,11 +44,16 @@ test_expect_success 'setup an embedded bare repo, secondary worktree and submodu
 	test_path_is_dir outer-repo/.git/modules/subn
 '
 
-test_expect_success 'safe.bareRepository unset' '
+test_expect_success !WITH_BREAKING_CHANGES 'safe.bareRepository unset' '
 	test_unconfig --global safe.bareRepository &&
 	expect_accepted_implicit -C outer-repo/bare-repo
 '
 
+test_expect_success WITH_BREAKING_CHANGES 'safe.bareRepository unset (defaults to explicit)' '
+	test_unconfig --global safe.bareRepository &&
+	expect_rejected -C outer-repo/bare-repo
+'
+
 test_expect_success 'safe.bareRepository=all' '
 	test_config_global safe.bareRepository all &&
 	expect_accepted_implicit -C outer-repo/bare-repo
@@ -63,7 +68,8 @@ test_expect_success 'safe.bareRepository in the repository' '
 	# safe.bareRepository must not be "explicit", otherwise
 	# git config fails with "fatal: not in a git directory" (like
 	# safe.directory)
-	test_config -C outer-repo/bare-repo safe.bareRepository all &&
+	test_when_finished "git config --file outer-repo/bare-repo/config --unset safe.bareRepository" &&
+	git config --file outer-repo/bare-repo/config safe.bareRepository all &&
 	test_config_global safe.bareRepository explicit &&
 	expect_rejected -C outer-repo/bare-repo
 '
-- 
gitgitgadget