[v3,3/5] setup.c: create `discovery.bare`

Commit Message

Glen Choo May 27, 2022, 9:09 p.m. UTC

From: Glen Choo <[e-mail]>

There is a known social engineering attack that takes advantage of the
fact that a working tree can include an entire bare repository,
including a config file. A user could run a Git command inside the bare
repository thinking that the config file of the 'outer' repository would
be used, but in reality, the bare repository's config file (which is
attacker-controlled) is used, which may result in arbitrary code
execution. See [1] for a fuller description and deeper discussion.

A simple mitigation is to forbid bare repositories unless specified via
`--git-dir` or `GIT_DIR`. In environments that don't use bare
repositories, this would be minimally disruptive.

Create a config variable, `discovery.bare`, that tells Git whether or
not to die() when it discovers a bare repository. This only affects
repository discovery, thus it has no effect if discovery was not
done (e.g. `--git-dir` was passed).

This config is an enum of:

- "always": always allow bare repositories (this is the default)
- "never": never allow bare repositories

If we want to protect users from such attacks by default, neither value
will suffice - "always" provides no protection, but "never" is
impractical for bare repository users. A more usable default would be to
allow only non-embedded bare repositories ([2] contains one such
proposal), but detecting if a repository is embedded is potentially
non-trivial, so this work is not implemented in this series.

[1]: https://lore.kernel.org/git/[e-mail]
[2]: https://lore.kernel.org/git/[e-mail]

Signed-off-by: Glen Choo <[e-mail]>
---
 Documentation/config.txt           |  2 +
 Documentation/config/discovery.txt | 19 +++++++++
 setup.c                            | 66 +++++++++++++++++++++++++++++-
 t/t0035-discovery-bare.sh          | 64 +++++++++++++++++++++++++++++
 4 files changed, 150 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/config/discovery.txt
 create mode 100755 t/t0035-discovery-bare.sh

Comments

Junio C Hamano May 28, 2022, 12:59 a.m. UTC | #1

"Glen Choo via GitGitGadget" <[e-mail]> writes:

> +enum discovery_bare_config {
> +	DISCOVERY_BARE_UNKNOWN = -1,
> +	DISCOVERY_BARE_NEVER = 0,
> +	DISCOVERY_BARE_ALWAYS,
> +};
> +static enum discovery_bare_config discovery_bare_config =
> +	DISCOVERY_BARE_UNKNOWN;

Can discovery_bare come from anywhere other than config?

I am wondering if both the variable and the type should be called
"discovery_bare_allowed" instead.  That it comes from the config is
not the more important part.  That it determines if it is allowed
is.

> +static int check_bare_repo_allowed(void)
> +{
> +	if (discovery_bare_config == DISCOVERY_BARE_UNKNOWN) {
> +		discovery_bare_config = DISCOVERY_BARE_ALWAYS;
> +		git_protected_config(discovery_bare_cb, NULL);
> +	}

OK, so the thing is initialized to "unknown", and the first time we
want to use the value of it, we read from the file (or default to
"always").  Makes sense.

And then ...

> +	switch (discovery_bare_config) {
> +	case DISCOVERY_BARE_NEVER:
> +		return 0;
> +	case DISCOVERY_BARE_ALWAYS:
> +		return 1;
> +	case DISCOVERY_BARE_UNKNOWN:
> +		BUG("invalid discovery_bare_config %d", discovery_bare_config);

... this is being defensive; we know discovery_bare_cb() won't give
UNKNOWN, but we want to make sure.

> +	}
> +	return 0;
> +}
> +
> +static const char *discovery_bare_config_to_string(void)
> +{

But this one feels strangely asymmetrical, as there is no inherent
reason why one must be called before the other.  I would expect it
to either

 * take a parameter of type "enum discovery_bare" and return
   "never", "always", or "unset", without calling any BUG().

or

 * have the same "we lazily figure out the discovery_bare_config
   variable on demand" logic.

As both of these functions are file-scope static, we can live with
it, though.

> +	switch (discovery_bare_config) {
> +	case DISCOVERY_BARE_NEVER:
> +		return "never";
> +	case DISCOVERY_BARE_ALWAYS:
> +		return "always";
> +	case DISCOVERY_BARE_UNKNOWN:
> +		BUG("invalid discovery_bare_config %d", discovery_bare_config);
> +	}
> +	return NULL;
> +}

Derrick Stolee June 2, 2022, 1:11 p.m. UTC | #2

On 5/27/2022 5:09 PM, Glen Choo via GitGitGadget wrote:
> +enum discovery_bare_config {
> +	DISCOVERY_BARE_UNKNOWN = -1,
> +	DISCOVERY_BARE_NEVER = 0,
> +	DISCOVERY_BARE_ALWAYS,
> +};
> +static enum discovery_bare_config discovery_bare_config =
> +	DISCOVERY_BARE_UNKNOWN;

Using this static global is fine, I think.

> +static int discovery_bare_cb(const char *key, const char *value, void *d)
> +{
> +	if (strcmp(key, "discovery.bare"))
> +		return 0;
> +
> +	if (!strcmp(value, "never")) {
> +		discovery_bare_config = DISCOVERY_BARE_NEVER;
> +		return 0;
> +	}
> +	if (!strcmp(value, "always")) {
> +		discovery_bare_config = DISCOVERY_BARE_ALWAYS;
> +		return 0;
> +	}
> +	return -1;
> +}

However, I do think that this _cb method could benefit from interpreting
the 'd' pointer as a 'enum discovery_bare_config *' and assigning the
value at the pointer. We can then pass the global to the
git_protected_config() call below.

This is probably over-defensive future-proofing, but this kind of change
would be necessary if we ever wanted to return the enum instead of
simply an integer, as below:

> +
> +static int check_bare_repo_allowed(void)
> +{
> +	if (discovery_bare_config == DISCOVERY_BARE_UNKNOWN) {
> +		discovery_bare_config = DISCOVERY_BARE_ALWAYS;
> +		git_protected_config(discovery_bare_cb, NULL);
> +	}
> +	switch (discovery_bare_config) {
> +	case DISCOVERY_BARE_NEVER:
> +		return 0;
> +	case DISCOVERY_BARE_ALWAYS:
> +		return 1;
> +	case DISCOVERY_BARE_UNKNOWN:
> +		BUG("invalid discovery_bare_config %d", discovery_bare_config);
> +	}
> +	return 0;
> +}

With the recommended change to the _cb method, we could rewrite this as

static enum discovery_bare_config get_discovery_bare(void)
{
	enum discovery_bare_config result = DISCOVERY_BARE_ALWAYS;
	git_protected_config(discovery_bare_cb, &result);
	return result;
}

With this, we can drop the UNKNOWN and let the caller treat the response
as a simple boolean.

I think this is simpler overall, but also makes it easier to extend in the
future to have "discovery.bare=non-embedded" by adding a new mode and
adjusting the consumer in setup_git_directory_gently_1() to use a switch()
on the resurned enum.

> +
> +static const char *discovery_bare_config_to_string(void)
> +{
> +	switch (discovery_bare_config) {
> +	case DISCOVERY_BARE_NEVER:
> +		return "never";
> +	case DISCOVERY_BARE_ALWAYS:
> +		return "always";
> +	case DISCOVERY_BARE_UNKNOWN:
> +		BUG("invalid discovery_bare_config %d", discovery_bare_config);

This case should be a "default:" in case somehow an arbitrary integer
value was placed in the variable. This could also take an enum as a
parameter, to avoid being coupled to the global.

> +++ b/t/t0035-discovery-bare.sh
> @@ -0,0 +1,64 @@
> +#!/bin/sh
> +
> +test_description='verify discovery.bare checks'
> +
> +. ./test-lib.sh
> +
> +pwd="$(pwd)"
> +
> +expect_rejected () {
> +	test_must_fail git rev-parse --git-dir 2>err &&
> +	grep "discovery.bare" err
> +}

Should we make a simple "expect_accepted" helper in case we ever
want to replace the "git rev-parse --git-dir" with anything else?

> +
> +test_expect_success 'setup bare repo in worktree' '
> +	git init outer-repo &&
> +	git init --bare outer-repo/bare-repo
> +'
> +
> +test_expect_success 'discovery.bare unset' '
> +	(
> +		cd outer-repo/bare-repo &&
> +		git rev-parse --git-dir
> +	)
> +'
> +
> +test_expect_success 'discovery.bare=always' '
> +	git config --global discovery.bare always &&
> +	(
> +		cd outer-repo/bare-repo &&
> +		git rev-parse --git-dir
> +	)
> +'
> +
> +test_expect_success 'discovery.bare=never' '
> +	git config --global discovery.bare never &&
> +	(
> +		cd outer-repo/bare-repo &&
> +		expect_rejected
> +	)
> +'
> +
> +test_expect_success 'discovery.bare in the repository' '
> +	(
> +		cd outer-repo/bare-repo &&
> +		# Temporarily set discovery.bare=always, otherwise git
> +		# config fails with "fatal: not in a git directory"
> +		# (like safe.directory)
> +		git config --global discovery.bare always &&
> +		git config discovery.bare always &&
> +		git config --global discovery.bare never &&
> +		expect_rejected
> +	)
> +'
> +
> +test_expect_success 'discovery.bare on the command line' '
> +	git config --global discovery.bare never &&> +	(
> +		cd outer-repo/bare-repo &&
> +		test_must_fail git -c discovery.bare=always rev-parse --git-dir 2>err &&
> +		grep "discovery.bare" err
> +	)

Ok, at the current place in the series, this test_must_fail matches
expectation. If you reorder to have this patch after your current patch 4,
then we can write this test immediately as a successful case.

We could also reuse some information from the expect_rejected helper by
adding this:

expect_rejected () {
	test_must_fail git $* rev-parse --git-dir 2>err &&
	grep "discovery.bare" err
}

Then you can test the -c options in the tests as

	expect_rejected -c discovery.bare=always

Thanks,
-Stolee

Patch

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 07832de1a6c..34133288d75 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -415,6 +415,8 @@  include::config/diff.txt[]
 
 include::config/difftool.txt[]
 
+include::config/discovery.txt[]
+
 include::config/extensions.txt[]
 
 include::config/fastimport.txt[]
diff --git a/Documentation/config/discovery.txt b/Documentation/config/discovery.txt
new file mode 100644
index 00000000000..fbe93597e7c
--- /dev/null
+++ b/Documentation/config/discovery.txt
@@ -0,0 +1,19 @@ 
+discovery.bare::
+	'(Protected config only)' Specifies whether Git will work with a
+	bare repository that it found during repository discovery. This
+	has no effect if the repository is specified directly via the
+	--git-dir command-line option or the GIT_DIR environment
+	variable (see linkgit:git[1]).
++
+The currently supported values are:
++
+* `always`: Git always works with bare repositories
+* `never`: Git never works with bare repositories
++
+This defaults to `always`, but this default may change in the future.
++
+If you do not use bare repositories in your workflow, then it may be
+beneficial to set `discovery.bare` to `never` in your global 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.
diff --git a/setup.c b/setup.c
index 847d47f9195..6686743ab7d 100644
--- a/setup.c
+++ b/setup.c
@@ -10,6 +10,13 @@ 
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
 static int work_tree_config_is_bogus;
+enum discovery_bare_config {
+	DISCOVERY_BARE_UNKNOWN = -1,
+	DISCOVERY_BARE_NEVER = 0,
+	DISCOVERY_BARE_ALWAYS,
+};
+static enum discovery_bare_config discovery_bare_config =
+	DISCOVERY_BARE_UNKNOWN;
 
 static struct startup_info the_startup_info;
 struct startup_info *startup_info = &the_startup_info;
@@ -1133,6 +1140,52 @@  static int ensure_valid_ownership(const char *path)
 	return data.is_safe;
 }
 
+static int discovery_bare_cb(const char *key, const char *value, void *d)
+{
+	if (strcmp(key, "discovery.bare"))
+		return 0;
+
+	if (!strcmp(value, "never")) {
+		discovery_bare_config = DISCOVERY_BARE_NEVER;
+		return 0;
+	}
+	if (!strcmp(value, "always")) {
+		discovery_bare_config = DISCOVERY_BARE_ALWAYS;
+		return 0;
+	}
+	return -1;
+}
+
+static int check_bare_repo_allowed(void)
+{
+	if (discovery_bare_config == DISCOVERY_BARE_UNKNOWN) {
+		discovery_bare_config = DISCOVERY_BARE_ALWAYS;
+		git_protected_config(discovery_bare_cb, NULL);
+	}
+	switch (discovery_bare_config) {
+	case DISCOVERY_BARE_NEVER:
+		return 0;
+	case DISCOVERY_BARE_ALWAYS:
+		return 1;
+	case DISCOVERY_BARE_UNKNOWN:
+		BUG("invalid discovery_bare_config %d", discovery_bare_config);
+	}
+	return 0;
+}
+
+static const char *discovery_bare_config_to_string(void)
+{
+	switch (discovery_bare_config) {
+	case DISCOVERY_BARE_NEVER:
+		return "never";
+	case DISCOVERY_BARE_ALWAYS:
+		return "always";
+	case DISCOVERY_BARE_UNKNOWN:
+		BUG("invalid discovery_bare_config %d", discovery_bare_config);
+	}
+	return NULL;
+}
+
 enum discovery_result {
 	GIT_DIR_NONE = 0,
 	GIT_DIR_EXPLICIT,
@@ -1142,7 +1195,8 @@  enum discovery_result {
 	GIT_DIR_HIT_CEILING = -1,
 	GIT_DIR_HIT_MOUNT_POINT = -2,
 	GIT_DIR_INVALID_GITFILE = -3,
-	GIT_DIR_INVALID_OWNERSHIP = -4
+	GIT_DIR_INVALID_OWNERSHIP = -4,
+	GIT_DIR_DISALLOWED_BARE = -5,
 };
 
 /*
@@ -1239,6 +1293,8 @@  static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 		}
 
 		if (is_git_directory(dir->buf)) {
+			if (!check_bare_repo_allowed())
+				return GIT_DIR_DISALLOWED_BARE;
 			if (!ensure_valid_ownership(dir->buf))
 				return GIT_DIR_INVALID_OWNERSHIP;
 			strbuf_addstr(gitdir, ".");
@@ -1385,6 +1441,14 @@  const char *setup_git_directory_gently(int *nongit_ok)
 		}
 		*nongit_ok = 1;
 		break;
+	case GIT_DIR_DISALLOWED_BARE:
+		if (!nongit_ok) {
+			die(_("cannot use bare repository '%s' (discovery.bare is '%s')"),
+			    dir.buf,
+			    discovery_bare_config_to_string());
+		}
+		*nongit_ok = 1;
+		break;
 	case GIT_DIR_NONE:
 		/*
 		 * As a safeguard against setup_git_directory_gently_1 returning
diff --git a/t/t0035-discovery-bare.sh b/t/t0035-discovery-bare.sh
new file mode 100755
index 00000000000..94c2f76d774
--- /dev/null
+++ b/t/t0035-discovery-bare.sh
@@ -0,0 +1,64 @@ 
+#!/bin/sh
+
+test_description='verify discovery.bare checks'
+
+. ./test-lib.sh
+
+pwd="$(pwd)"
+
+expect_rejected () {
+	test_must_fail git rev-parse --git-dir 2>err &&
+	grep "discovery.bare" err
+}
+
+test_expect_success 'setup bare repo in worktree' '
+	git init outer-repo &&
+	git init --bare outer-repo/bare-repo
+'
+
+test_expect_success 'discovery.bare unset' '
+	(
+		cd outer-repo/bare-repo &&
+		git rev-parse --git-dir
+	)
+'
+
+test_expect_success 'discovery.bare=always' '
+	git config --global discovery.bare always &&
+	(
+		cd outer-repo/bare-repo &&
+		git rev-parse --git-dir
+	)
+'
+
+test_expect_success 'discovery.bare=never' '
+	git config --global discovery.bare never &&
+	(
+		cd outer-repo/bare-repo &&
+		expect_rejected
+	)
+'
+
+test_expect_success 'discovery.bare in the repository' '
+	(
+		cd outer-repo/bare-repo &&
+		# Temporarily set discovery.bare=always, otherwise git
+		# config fails with "fatal: not in a git directory"
+		# (like safe.directory)
+		git config --global discovery.bare always &&
+		git config discovery.bare always &&
+		git config --global discovery.bare never &&
+		expect_rejected
+	)
+'
+
+test_expect_success 'discovery.bare on the command line' '
+	git config --global discovery.bare never &&
+	(
+		cd outer-repo/bare-repo &&
+		test_must_fail git -c discovery.bare=always rev-parse --git-dir 2>err &&
+		grep "discovery.bare" err
+	)
+'
+
+test_done