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

Commit Message

Glen Choo June 30, 2022, 6:13 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. if the user passes `--git-dir=my-dir`, discovery will be
skipped and my-dir will be used as the repo regardless of the
`discovery.bare` value.

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 | 23 ++++++++++++
 setup.c                            | 57 +++++++++++++++++++++++++++++-
 t/t0035-discovery-bare.sh          | 52 +++++++++++++++++++++++++++
 4 files changed, 133 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/config/discovery.txt
 create mode 100755 t/t0035-discovery-bare.sh

Comments

Taylor Blau July 1, 2022, 1:30 a.m. UTC | #1

On Thu, Jun 30, 2022 at 06:13:59PM +0000, Glen Choo via GitGitGadget wrote:
> 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.

I think that everything you said in your patch message makes sense, but
I appreciate this paragraph in particular. The historical record is
definitely important and worth preserving here, and I hope that it'll be
helpful to future readers who may wonder why the default wasn't chosen
as "never".

> [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 | 23 ++++++++++++
>  setup.c                            | 57 +++++++++++++++++++++++++++++-
>  t/t0035-discovery-bare.sh          | 52 +++++++++++++++++++++++++++
>  4 files changed, 133 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/config/discovery.txt
>  create mode 100755 t/t0035-discovery-bare.sh
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index e284b042f22..9a5e1329772 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -409,6 +409,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..bbcf89bb0b5
> --- /dev/null
> +++ b/Documentation/config/discovery.txt
> @@ -0,0 +1,23 @@
> +discovery.bare::
> +	Specifies whether Git will work with a bare repository that it
> +	found during repository discovery. If the repository is

Is it clear from the context what "discovery" means here? It's probably
easier to describe what it isn't, which you kind of do in the next
sentence. But it may be clearer to say something like:

    Specifies whether Git will recognize bare repositories that aren't
    specified via the top-level `--git-dir` command-line option, or the
    `GIT_DIR` environment variable (see linkgit:git[1]).

> +This defaults to `always`, but this default may change in the future.

I think the default being subject to change is par for the course. It's
probably easy enough to just say "Defaults to 'always'" and leave it at
that.

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

I think we still don't have a great answer for people who trust some
bare repositories (e.g., known-embedded repositories that are used for
testing) but not others. To be clear, I think that is a fine point to
concede with this direction.

But we should be clear about that limitation by stating that Git does
not support the "I trust some bare repositories to be safely
discoverable but not others".

> +static enum discovery_bare_allowed get_discovery_bare(void)
> +{
> +	enum discovery_bare_allowed result = DISCOVERY_BARE_ALWAYS;
> +	git_protected_config(discovery_bare_cb, &result);
> +	return result;
> +}
> +
> +static const char *discovery_bare_allowed_to_string(
> +	enum discovery_bare_allowed discovery_bare_allowed)
> +{
> +	switch (discovery_bare_allowed) {
> +	case DISCOVERY_BARE_NEVER:
> +		return "never";
> +	case DISCOVERY_BARE_ALWAYS:
> +		return "always";

> +	default:
> +		BUG("invalid discovery_bare_allowed %d",
> +		    discovery_bare_allowed);

Should we have a default case here since the case arms above are
exhaustive?

> +	}
> +	return NULL;
> +}
> +
>  enum discovery_result {
>  	GIT_DIR_NONE = 0,
>  	GIT_DIR_EXPLICIT,
> @@ -1151,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,
>  };
>
>  /*
> @@ -1248,6 +1293,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>  		}
>
>  		if (is_git_directory(dir->buf)) {
> +			if (!get_discovery_bare())

Relying on NEVER being the zero value here seems fragile to me. Should
we check that `if (get_discovery_bare() == DISCOVERY_BARE_NEVER)` to be
more explicit here?

> +				return GIT_DIR_DISALLOWED_BARE;
>  			if (!ensure_valid_ownership(dir->buf))
>  				return GIT_DIR_INVALID_OWNERSHIP;
>  			strbuf_addstr(gitdir, ".");
> @@ -1394,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_allowed_to_string(get_discovery_bare()));
> +		}
> +		*nongit_ok = 1;
> +		break;
>  	case GIT_DIR_NONE:
>  		/*
>  		 * As a safeguard against setup_git_directory_gently_1 returning

Thanks,
Taylor

Glen Choo July 7, 2022, 7:55 p.m. UTC | #2

Taylor Blau <[e-mail]> writes:

> On Thu, Jun 30, 2022 at 06:13:59PM +0000, Glen Choo via GitGitGadget wrote:
>> [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 | 23 ++++++++++++
>>  setup.c                            | 57 +++++++++++++++++++++++++++++-
>>  t/t0035-discovery-bare.sh          | 52 +++++++++++++++++++++++++++
>>  4 files changed, 133 insertions(+), 1 deletion(-)
>>  create mode 100644 Documentation/config/discovery.txt
>>  create mode 100755 t/t0035-discovery-bare.sh
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index e284b042f22..9a5e1329772 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -409,6 +409,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..bbcf89bb0b5
>> --- /dev/null
>> +++ b/Documentation/config/discovery.txt
>> @@ -0,0 +1,23 @@
>> +discovery.bare::
>> +	Specifies whether Git will work with a bare repository that it
>> +	found during repository discovery. If the repository is
>
> Is it clear from the context what "discovery" means here? It's probably
> easier to describe what it isn't, which you kind of do in the next
> sentence. But it may be clearer to say something like:
>
>     Specifies whether Git will recognize bare repositories that aren't
>     specified via the top-level `--git-dir` command-line option, or the
>     `GIT_DIR` environment variable (see linkgit:git[1]).

Hm that's a good point and the suggestion is very well-worded. In
addition to what you have, I think we should make reference to
"discovery" _somewhere_ in here since the option is named
`discovery.bare`, and this seems like a good teaching opportunity.

>> +This defaults to `always`, but this default may change in the future.
>
> I think the default being subject to change is par for the course. It's
> probably easy enough to just say "Defaults to 'always'" and leave it at
> that.

Makes sense.

>> +static enum discovery_bare_allowed get_discovery_bare(void)
>> +{
>> +	enum discovery_bare_allowed result = DISCOVERY_BARE_ALWAYS;
>> +	git_protected_config(discovery_bare_cb, &result);
>> +	return result;
>> +}
>> +
>> +static const char *discovery_bare_allowed_to_string(
>> +	enum discovery_bare_allowed discovery_bare_allowed)
>> +{
>> +	switch (discovery_bare_allowed) {
>> +	case DISCOVERY_BARE_NEVER:
>> +		return "never";
>> +	case DISCOVERY_BARE_ALWAYS:
>> +		return "always";
>
>> +	default:
>> +		BUG("invalid discovery_bare_allowed %d",
>> +		    discovery_bare_allowed);
>
> Should we have a default case here since the case arms above are
> exhaustive?

Ah, this "default:" was suggested by Stolee in
https://lore.kernel.org/git/[e-mail]

  This case should be a "default:" in case somehow an arbitrary integer
  value was placed in the variable. [...]

I'm not sure where we stand on this kind of defensiveness. It's not
really necessary, but I suppose having a "default:" won't hurt here,
especially if it BUG()-s instead of silently passing.

>> +	}
>> +	return NULL;
>> +}
>> +
>>  enum discovery_result {
>>  	GIT_DIR_NONE = 0,
>>  	GIT_DIR_EXPLICIT,
>> @@ -1151,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,
>>  };
>>
>>  /*
>> @@ -1248,6 +1293,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>>  		}
>>
>>  		if (is_git_directory(dir->buf)) {
>> +			if (!get_discovery_bare())
>
> Relying on NEVER being the zero value here seems fragile to me. Should
> we check that `if (get_discovery_bare() == DISCOVERY_BARE_NEVER)` to be
> more explicit here?

This was also originally suggested by Stolee in 
https://lore.kernel.org/git/[e-mail]

  With (some changes to return the enum), we can [...] let the caller
  treat the response as a simple boolean.

but.. your suggestion does seem less fragile. It won't really matter
when we add a third enum and replace the "if" with a "switch", but it
does matter if we ever muck around with the integer values of
DISCOVER_BARE_*.

>> +				return GIT_DIR_DISALLOWED_BARE;
>>  			if (!ensure_valid_ownership(dir->buf))
>>  				return GIT_DIR_INVALID_OWNERSHIP;
>>  			strbuf_addstr(gitdir, ".");
>> @@ -1394,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_allowed_to_string(get_discovery_bare()));
>> +		}
>> +		*nongit_ok = 1;
>> +		break;
>>  	case GIT_DIR_NONE:
>>  		/*
>>  		 * As a safeguard against setup_git_directory_gently_1 returning
>
> Thanks,
> Taylor

Patch

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e284b042f22..9a5e1329772 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -409,6 +409,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..bbcf89bb0b5
--- /dev/null
+++ b/Documentation/config/discovery.txt
@@ -0,0 +1,23 @@ 
+discovery.bare::
+	Specifies whether Git will work with a bare repository that it
+	found during repository discovery. If the repository is
+	specified directly via the --git-dir command-line option or the
+	GIT_DIR environment variable (see linkgit:git[1]), Git will
+	always use the specified repository, regardless of this value.
++
+This config setting is only respected in protected configuration (see
+<<SCOPES>>). This prevents the untrusted repository from tampering with
+this value.
++
+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 c8e3c32814d..16938fd5a24 100644
--- a/setup.c
+++ b/setup.c
@@ -10,6 +10,10 @@ 
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
 static int work_tree_config_is_bogus;
+enum discovery_bare_allowed {
+	DISCOVERY_BARE_NEVER = 0,
+	DISCOVERY_BARE_ALWAYS,
+};
 
 static struct startup_info the_startup_info;
 struct startup_info *startup_info = &the_startup_info;
@@ -1142,6 +1146,46 @@  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)
+{
+	enum discovery_bare_allowed *discovery_bare_allowed = d;
+
+	if (strcmp(key, "discovery.bare"))
+		return 0;
+
+	if (!strcmp(value, "never")) {
+		*discovery_bare_allowed = DISCOVERY_BARE_NEVER;
+		return 0;
+	}
+	if (!strcmp(value, "always")) {
+		*discovery_bare_allowed = DISCOVERY_BARE_ALWAYS;
+		return 0;
+	}
+	return -1;
+}
+
+static enum discovery_bare_allowed get_discovery_bare(void)
+{
+	enum discovery_bare_allowed result = DISCOVERY_BARE_ALWAYS;
+	git_protected_config(discovery_bare_cb, &result);
+	return result;
+}
+
+static const char *discovery_bare_allowed_to_string(
+	enum discovery_bare_allowed discovery_bare_allowed)
+{
+	switch (discovery_bare_allowed) {
+	case DISCOVERY_BARE_NEVER:
+		return "never";
+	case DISCOVERY_BARE_ALWAYS:
+		return "always";
+	default:
+		BUG("invalid discovery_bare_allowed %d",
+		    discovery_bare_allowed);
+	}
+	return NULL;
+}
+
 enum discovery_result {
 	GIT_DIR_NONE = 0,
 	GIT_DIR_EXPLICIT,
@@ -1151,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,
 };
 
 /*
@@ -1248,6 +1293,8 @@  static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 		}
 
 		if (is_git_directory(dir->buf)) {
+			if (!get_discovery_bare())
+				return GIT_DIR_DISALLOWED_BARE;
 			if (!ensure_valid_ownership(dir->buf))
 				return GIT_DIR_INVALID_OWNERSHIP;
 			strbuf_addstr(gitdir, ".");
@@ -1394,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_allowed_to_string(get_discovery_bare()));
+		}
+		*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..8f802746530
--- /dev/null
+++ b/t/t0035-discovery-bare.sh
@@ -0,0 +1,52 @@ 
+#!/bin/sh
+
+test_description='verify discovery.bare checks'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+pwd="$(pwd)"
+
+expect_accepted () {
+	git "$@" rev-parse --git-dir
+}
+
+expect_rejected () {
+	test_must_fail git "$@" rev-parse --git-dir 2>err &&
+	grep -F "cannot use bare repository" 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' '
+	expect_accepted -C outer-repo/bare-repo
+'
+
+test_expect_success 'discovery.bare=always' '
+	test_config_global discovery.bare always &&
+	expect_accepted -C outer-repo/bare-repo
+'
+
+test_expect_success 'discovery.bare=never' '
+	test_config_global discovery.bare never &&
+	expect_rejected -C outer-repo/bare-repo
+'
+
+test_expect_success 'discovery.bare in the repository' '
+	# discovery.bare must not be "never", otherwise git config fails
+	# with "fatal: not in a git directory" (like safe.directory)
+	test_config -C outer-repo/bare-repo discovery.bare always &&
+	test_config_global discovery.bare never &&
+	expect_rejected -C outer-repo/bare-repo
+'
+
+test_expect_success 'discovery.bare on the command line' '
+	test_config_global discovery.bare never &&
+	expect_accepted -C outer-repo/bare-repo \
+		-c discovery.bare=always
+'
+
+test_done