setup.c: make bare repo discovery optional

From: Glen Choo <[email protected]>

Add a config variable, safe.barerepository, that tells Git whether or not to recognize bare repositories when it is trying to discover the repository. This only affects repository discovery, thus it has no effect if discovery was not done (e.g. --git-dir was passed).

This is motivated by the fact that some workflows don't use bare repositories at all, and users may prefer to opt out of bare repository discovery altogether:

- An easy assumption for a user to make is that Git commands run anywhere inside a repository's working tree will use the same repository. However, if the working tree contains a bare repository below the root-level (".git" is preferred at the root-level), any operations inside that bare repository use the bare repository instead.

In the worst case, attackers can use this confusion to trick users into running arbitrary code (see [1] for a deeper discussion). But even in benign situations (e.g. a user renames ".git/" to ".git.old/" and commits it for archival purposes), disabling bare repository discovery can be a simpler mode of operation (e.g. because the user doesn't actually want to use ".git.old/") [2].

- Git won't "accidentally" recognize a directory that wasn't meant to be a bare repository, but happens to resemble one. While such accidents are probably very rare in practice, this lets users reduce the chance to zero.

This config is designed to be used like an allow-list, but it is not yet clear what a good format for this allow-list would be. As such, this patch limits the config value to a tri-state of [true|false|unset]:

- [*|(unset)] recognize all bare repositories (like Git does today)
- (empty) recognize no bare repositories

and leaves the full format to be determined later.

[1]: https://lore.kernel.org/git/[email protected]
[2]: I don't personally know anyone who does this as part of their
normal workflow, but a cursory search on GitHub suggests that there is a not insubstantial number of people who munge ".git" in order to store
its contents.

https://github.com/search?l=&o=desc&p=1&q=ref+size%3A%3C1000+filename%3AHEAD&s=indexed&type=Code
(aka search for the text "ref", size:<1000, filename:HEAD)

---
RFC setup.c: make bare repo discovery optional

(Forgive the non-standard RFC tag, I haven't figured out how to send as RFC using GGG. I also didn't realize that /preview would also respect CC...)

= Description

A relatively easy win that came out of the discussions around embedded bare repos [1], is to just let users opt-out of discovering bare repos. This patch does exactly that, by adding a 'boolean' config variable, safe.barerepository.

safe.barerepository is presented to users as an allow-list of directories that Git will recognize as a bare repository during the repository discovery process (much like safe.directory), but this patch only implements (and permits) boolean behavior (i.e. on, off and unset). Hopefully, this gives us some room to discuss and experiment with possible formats.

Thanks to Taylor for suggesting the allow-list idea :)

I think the core concept of letting users toggle bare repo discovery is solid, but I'm sending this as RFC for the following reasons:

* I don't love the name safe.barerepository, because it feels like Git is saying that bare repos are unsafe and consequently, that bare repo users are behaving unsafely. On the other hand, this is quite similar to safe.directory in a few respects, so it might make sense for the naming to reflect that.

* The *-gcc CI jobs don't pass. I haven't discerned any kind of pattern yet.

= How this relates to embedded bare repos

This does not change the default behavior (i.e. Git will still discover all bare repos by default) because that would be catastrophic for bare repo users [2]. As such, this patch isn't intended to solve the problem of embedded bare repos for all users once and for all, but I think it does improve the our stance on the matter:

* In the short-term, users who know they won't need bare repos (or those who are willing to set GIT_DIR for all of their bare repos) can opt-in to a safer, easier to reason about mode of operation.

* In the longer-term, we might identify a usable-enough default that we can give opt-out protection that works for the vast majority of users.

= Other questions/Concerns

* Maybe it's more informative for the user if we die() (or warn()) when we find a bare repo instead of silently ignoring it?

* I wonder if it makes sense to separate the toggle for bare repo discovery and the allow-list of bare repositories. Something like core.barediscovery or discovery.barerepository has a lot less baggage than safe.*, and boolean enable/disable is a lot simpler, but this isn't good from an extensibility perspective.

* Is there any reason why safe.barerepository shouldn't use the same format as (its obvious inspiration) safe.directory?

* Are the docs clear enough? I found those hard to put into words, so I'd especially appreciate wording suggestions :)

= Future work

* Like safe.directory, safe.barerepository is only read from system and global config. I anticipate that this is too restrictive; there has already been some discussion of adding a GIT_SAFE_DIRECTORIES environment variable for safe.directory [3], and it would be useful to have the same thing for safe.barerepository.

[1]
https://lore.kernel.org/git/[email protected]
[2] In https://lore.kernel.org/git/[email protected], Junio experimented with switching off bare repo discovery altogether and relying solely on GIT_DIR. The resulting fallout was deemed too big to be feasible. [3] https://lore.kernel.org/git/[email protected]/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1261%2Fchooglen%2Fsetup%2Fdisable-bare-repo-config-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1261/chooglen/setup/disable-bare-repo-config-v1
Pull-Request: https://github.com/git/git/pull/1261

 Documentation/config/safe.txt | 24 +++++++++++++++
 setup.c                       | 36 ++++++++++++++++++++++-
 t/t1510-repo-setup.sh         | 55 +++++++++++++++++++++++++++++++++++
 3 files changed, 114 insertions(+), 1 deletion(-)

index 6d764fe0ccf..02032251ffd 100644
@@ -1,3 +1,27 @@
+safe.barerepository::
+	This config entry specifies directories that Git can recognize as
+	a bare repository when looking for the repository (aka repository
+	discovery). This has no effect if repository discovery is not
+	performed e.g. the path to the repository is set via `--git-dir`
+	(see linkgit:git[1]).
++
+It is recommended that you set this value so that Git will only use the bare
+repositories you intend it to. This prevents certain types of security and
+non-security problems, such as:
+
+* `git clone`-ing a repository containing a maliciously bare repository
+  inside it.
+* Git recognizing a directory that isn't mean to be a bare repository,
+  but happens to look like one.
++
+The currently supported values are `*` (Git recognizes all bare
+repositories) and the empty value (Git never recognizes bare repositories).
+Defaults to `*`.
++
+This config setting is only respected when specified in a system or global
+config, not when it is specified in a repository config or via the command
+line option `-c safe.barerepository=<path>`.
+
 safe.directory::
 	These config entries specify Git-tracked directories that are
 	considered safe even if they are owned by someone other than the

index a7b36f3ffbf..9b5dd877273 100644
@@ -1133,6 +1133,40 @@ static int ensure_valid_ownership(const char *path)
 	return data.is_safe;
 }
 
+/*
+ * This is similar to safe_directory_data, but only supports true/false.
+ */
+struct safe_bare_repository_data {
+	int is_safe;
+};
+
+static int safe_bare_repository_cb(const char *key, const char *value, void *d)
+{
+	struct safe_bare_repository_data *data = d;
+
+	if (strcmp(key, "safe.barerepository"))
+		return 0;
+
+	if (!value || !strcmp(value, "*")) {
+		data->is_safe = 1;
+		return 0;
+	}
+	if (!*value) {
+		data->is_safe = 0;
+		return 0;
+	}
+	return -1;
+}
+
+static int should_detect_bare(void)
+{
+	struct safe_bare_repository_data data;
+
+	read_very_early_config(safe_bare_repository_cb, &data);
+
+	return data.is_safe;
+}
+
 enum discovery_result {
 	GIT_DIR_NONE = 0,
 	GIT_DIR_EXPLICIT,

@@ -1238,7 +1272,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 			return GIT_DIR_DISCOVERED;
 		}
 
-		if (is_git_directory(dir->buf)) {
+		if (should_detect_bare() && is_git_directory(dir->buf)) {
 			if (!ensure_valid_ownership(dir->buf))
 				return GIT_DIR_INVALID_OWNERSHIP;
 			strbuf_addstr(gitdir, ".");

base-commit: 0f828332d5ac36fc63b7d8202652efa152809856

gitgitgadget

"mean to be" -> "meant to be".

{
	static int should = -1; /* unknown yet */

	if (should < 0) {
		struct safe_bare_repository_data data = { 0 };
		read_very_early_config(safe_bare_repository_cb, &data);
		should = data.is_safe;
	}
	return should;
}

Hi Glen,

Thanks for working on this! I'm excited to see some patches here, though I'm not totally convinced of this direction. More below.

To summarize, this proposal attempts to work around the problem of embedding bare repositories in non-bare checkouts by providing a way to opt-out of bare repository discovery (which is to only discover things that are listed in the safe.bareRepository configuration).

I agree that this would prevent the problem you're trying to solve, but I have significant concerns that this patch is going too far (at the risk of future damage to unrelated workflows) in order to accomplish that goal.

My concern is that if we ever flipped the default (i.e. that "safe.bareRepository" might someday be ""), that many legitimate cases of using bare repositories would be broken. I think there are many such legitimate use cases that _do_ rely on discovering bare repositories (i.e., Git invocations that do not have a --git-dir in their command-line). One such example would be forges, but I imagine that there are many other uses we don't even know about, and I would like to avoid breaking those if we ended up changing the default.

If it's possible to pursue a more targeted fix that leaves non-embedded bare repositories alone, I'd like to try and focus these efforts on a more narrow fix that would address just the case of embedded bare repositories. I think that the direction I outlined in:

https://lore.kernel.org/git/[email protected]/

could be a good place to start (see the paragraph beginning with "Here's an alternative approach" and below for the details).

One potential problem with that approach (that this patch doesn't suffer from) is that any discovery which finds a bare repository would have to continue up to the root of the volume in order to figure out whether or not that bare repository is embedded in another non-bare one. That is probably a non-starter due to performance, but I think you could easily work around with a top-level setting that controls whether or not you even _care_ about embedded bare repositories.

For example, if I set safe.bareRepository='*' in my top-level /etc/gitconfig, then we can avoid having to continue discovery for bare repositories altogether because we know we'll allow it anyway.

To pursue a change that targets just embedded bare repositories, I think you fundamentally have to do an exhaustive repository discovery in order to figure out whether the (bare) repository you're dealing with is embedded or not. So having an opt-out for users that either (a) don't care or (b) can't accept the performance degradation that Emily mentioned as a result of doing unbounded filesystem traversal would be sensible.

Playing devil's advocate for a moment, though, even if we had something like the proposal I outlined, flipping the top-level default from '*' to some value that implies we stop working in embedded bare repositories will break existing workflows. But that breakage would just be limited to embedded bare repositories, and not non-embedded ones. So I think on balance that breakage would affect fewer real-world users, while still being just as easy to recover from.

I did suggest an allow-list, but not this one ;-).

Yes, the concerns I outlined above are definitely echoing this sentiment. Another way to say it is that this feels like too big of a hammer (i.e., it is targeting _all_ bare repositories, not just embedded ones) for too small of a nail (embedded bare repositories). As you're probably sick of hearing me say by now, I would strongly prefer a more targeted solution (perhaps what I outlined, or perhaps something else, so long as it doesn't break non-embedded bare repositories if/ever we decided to change the default value of safe.bareRepository).

Interesting. I wouldn't expect this to be the case (since the default is to allow everything right now).

Perhaps, and I think if this were the case then I would feel differently about this patch. But I don't want us to paint ourselves into a corner, either. It would be unfortunate to, say, find ourselves in a position where the only protection against some novel embedded bare repository attack is to change a default that would break many existing workflows for _non_-embedded bare repositories.

We should definitely provide more feedback to the user. If I set safe.bareRepository to the empty string via a global config, and then execute a Git command in a non-embedded bare repository, I get:

    $ git.compile config --get --global --default='*' safe.bareRepository

    $ git.compile rev-parse --absolute-git-dir
    fatal: not a git repository (or any of the parent directories): .git

whereas on the last release of Git, I get instead:

    $ git rev-parse --absolute-git-dir
    /home/ttaylorr/repo.git

I'm still not convinced that just reading repository extensions while ignoring the rest of config and hooks is too confusing, so I'd be more in favor of something like:

    $ git.compile rev-parse --absolute-git-dir
    warning: ignoring repository config and hooks
    advice: to permit bare repository discovery (which
    advice: will read config and hooks), consider running:
    advice:
    advice:   $ git config --global --add safe.bareRepository /home/ttaylorr/repo.git
    /home/ttaylorr/repo.git

(though I still feel strongly that we should pursue a more targeted approach here).

Thanks,
Taylor

I think 99% of such use is to chdir into the directory with HEAD, refs/ and objects/ in it and let git recognise the cwd is a git directory. Am I mistaken, or are there tools that chdir into objects/08/ and rely on setup_git_directory_gently_1() to find the parent directory of that 'objects' directory to be a git directory?

I am wondering if another knob to help that particular use case easier may be sufficient. If you are a forge operator, you'd just set a boolean configuration variable to say "it is sufficient to chdir into a directory to use it a bare repository without exporting the environment variable GIT_DIR=."

It is likely that end-user human users would not want to enable such a variable, of course, but I wonder if a simple single knob would be sufficient to help other use cases you are worried about?

While I wish "extensions and nothing else", i.e. we use "degraded access", not "refuse to give access at all", were workable, I am pessimistic that it would work well in practice.

Saying "nothing else" is easy, but we do "if X exists, use it" for hook, and to implement "nothing else", you'd need to find such a code and say "even if X exists, because we are in this strange embedded bare thing, ignore this part of the logic" for every X. We've been casually saying "potentially risky config" and then started mixing "hooks" in the discussion, but who knows what other things are used from the repository by third-party tools that we need to yet add to the mix?

I do not think it would be confusing. I am worried about it being error prone.

Is the last line meant to be an output from "rev-parse --absolute-git-dir"? IOW, the warning says you are ignoring, but we are still recognising it as a repository?

By the way, do we need safe.bareRepository? Shouldn't safe.directory cover the same purpose?

If a directory is on the latter, you are saying that (1) the directory is OK to use as a repository, and (2) it is so even if the directory is owned by somebody else, not you.

Theoretically you can argue that there can be cases where you only want (1) and not (2), but as long as you control such a directory (like an embedded repository in your project's checkout) yourself, you do not have to worry about the "ok even if it is owned by somebody else" part.

If you took this change, and then at some point in the future we changed the default value of safe.bareRepository to "", wouldn't that break that 99% of use cases you are talking about?

When I read your "I think 99% of such use is ...", it makes me think that this change won't disrupt bare repo discovery when we only traverse one layer above $CWD. But this change disrupts the case where we don't need to traverse at all to do discovery (i.e., when $CWD is the root of a bare repository).

Yes, GitHub would almost certainly set safe.bareRepository to "*" regardless of what Git's own default would be.

I'm not sure I agree that end-users wouldn't want to touch this knob. If they have embedded bare repositories that they rely on as test fixtures, for example, wouldn't safe.bareRepository need to be tweaked?

(On a separate but somewhat-related note, I still think that this setting should be read from the repository config, too, i.e., it seems odd that we'd force a user to set safe.bareRepository to some deeply nested repository (in the embedded case) via their global config.)

Yeah, on this and the above quoted hunk, I am fine if our behavior eventually became "call die()" for when we are in an embedded bare repository. But I do think this transition should be gradual, i.e., we should likely emit a warning in those cases that would be broken in the future to say "this will break, run this git config invocation if you want it to remain working".

In this example, yes. But again, I'm not so deeply attached to the idea that we *have* to run in those cases. So I would equally be OK with the above s/warning/fatal and minus the last line, too (i.e., that we call die(), obviously we'd have to emit the advice before calling die()).

I'm not sure yet, but will think more about it.

Thanks,
Taylor

Our spawning (e.g. "fetch" run_command()s "upload-pack" in a local repository, or "fetch" runs "upload-pack" over ssh connection, or http gateway runs "upload-pack" after learning which repository the request is fetching from) of subcommands can and should be fixed by exporting "GIT_DIR=." when we spawn them in the target directory, and such a fix should be more or less trivial. It must happen before such a switch of default happens (if it is what we plan to do, that is). Also, the trivial fix must be conveyed to third-party tool authors and give them time to adjust their ware.

That's part of the usual migration process, and I am not so worried about it.

If some third-party tool for whatever reason wants to start from a random subdirectory in a bare repository, that is a different story. Fixing such a third-party tool would be more involved than "more or less trivial".

By "this change" you mean what Glen proposes? I think it was designed to break the use case where you go there to signal that you want to use the directory as a repository.

And such a boolean, without safe.bareRepository setting, should be sufficient to cover that 99% of such use, because it disables that deliberate refusal of treating CWD as a repository without explicitly saying that is what you want with "GIT_DIR=.". One thing I wasn't sure about was if that 99% number is close to reality, hence my question.

And with such a boolean, I am hoping that GitHub do not have to make such a wildly open setting. Only $CWD that is the top of a repository, without allowing it to be any random subdirectory, would be allowed.

But not in the "My $CWD is always fine" knob, whose only reason is to simplify things without opening you up unnecessarily too widely for hosting sites.

Hi Taylor,

Thanks again for the careful read. As I understand it, your concern is that making bare repository discovery configurable and then flipping the default to e.g. never detecting bare repositories is too disruptive to fix the embedded bare repository problem. And to avoid disrupting non-embedded bare repositories, you would prefer to pursue a more targeted fix.

If the problem statement were limited to embedded bare repositories, then I agree that this is way more than overkill, and that a targeted solution would be preferable.

More generally however, the problem of embedded bare repositories seems to suggest that bare repository discovery doesn't serve all users well, and in fact, may even be a net negative for a subset of users. I'd be interested in hearing your thoughts from that perspective, e.g.

- Should bare repository discovery should be configurable?
- What is a good default for bare repository discovery? (regardless of how feasible changing the default is)

This is a somewhat different direction from how the conversation started (I hope it doesn't look like I'm shifting the goal posts), but I think it's a good opportunity to step back and simplify something that we wished we got right in the beginning.

And even if we don't flip the default, shipping the config value still seems useful e.g. there's a good amount of interest in disabling bare repository discovery at $DAYJOB (and I think we'll get a lot of interesting results once we do).

Ah, yes. Oops. Sorry if it looked like I was putting words in your mouth.

What I really meant was that an allow-list (untethered from any specific purpose) seems like a useful 'UI primitive', so thanks for bringing up the option.

Ok, yeah I think safe.barerepository is a terrible way to achieve my purported goal of 'making bare repository discovery configurable/simpler/' - using the "safe." namespace makes it impossible to see this as anything other than protection against dangerous, unknown bare repositories. I'll drop the idea of safe-listing known bare repositories for now, that seems unproductive.

'Optionally disable bare repository discovery' still sounds like it's on the table though, but probably with a different kind of UX e.g. "discovery.barerepository" with the options:

- always: always discover bare repos
- never: never discover bare repos
- cwd-only: only discover bare repos if they are the cwd
- dotgit-only: only discover bare repos if they are a descendant of .git/

This might be a false alarm - I saw similar failures on an unrelated patch. I think my "master" is just out of date :(

Thanks all for the comments on v1, I've expanded this series somewhat to address them, notably:

* The config value is now named discovery.bare and is an enum (instead of an allow-list). This hopefully moves us away from "bare repos are unsafe and need to be guarded against" and towards "bare repos can be made optional if it serves your needs better".
* discovery.bare now causes git to die() instead of silently ignoring the bare repo.
* Add an option that allows a bare repo if it is the CWD, since this is presumably a reasonable default for 99% of bare repo users [1].

= Questions/Concerns

* die()-ing is necessary if we're trying to flip the default value of discovery.bare. We'd expect many bare repo users to be broken, and it's more helpful to fail loudly than to silently ignore the bare repo.

But in the long term, long after we've flipped the default and users know that they need to opt into bare repo discovery, would it be a better UX to just silently ignore the bare repo?

= Patch organization

* Patch 1 introduces discovery.bare with allowed values [always|never].

* Patch 2 adds discover.bare=cwd, which is useful when users don't always set GIT_DIR e.g. their workflow really depends on it, they are in the midst of migration.

= Series history

Changes since v1:

* Rename safe.barerepository to discovery.bare and make it die()
* Move tests into t/t0034-discovery-bare.sh
* Avoid unnecessary config reading by using a static variable
* Add discovery.bare=cwd
* Fix typos

[1] I tried this 'cwd' setting on our test suite, with some pretty promising results.

https://github.com/chooglen/git/actions/runs/2321914777

Out of the 8 failing scripts:

* 6 are of the form "make sure we 'do the right thing' inside a subdirectory of a bare repo" (which typically means .git) e.g. t9903-bash-prompt.sh. We should be setting discovery.bare=always for these tests, so this is a non-issue.
* t5323-pack-redundant.sh can be rewritten to -C into the root of the bare repo instead of a subdirectory.
* t3310-notes-merge-manual-resolve.sh: not sure what the test is checking in particular, but I think this can be rewritten.

IOW, I don't think we have any commands that require that CWD is a subdirectory of a bare repo, and we could use discovery.bare without much hassle.

Glen Choo (2):
setup.c: make bare repo discovery optional
setup.c: learn discovery.bareRepository=cwd

 Documentation/config/discovery.txt | 26 +++++++++
 setup.c                            | 89 ++++++++++++++++++++++++++++--
 t/t0034-discovery-bare.sh          | 69 +++++++++++++++++++++++
 3 files changed, 178 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/config/discovery.txt
 create mode 100755 t/t0034-discovery-bare.sh

base-commit: e8005e4871f130c4e402ddca2032c111252f070a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1261%2Fchooglen%2Fsetup%2Fdisable-bare-repo-config-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1261/chooglen/setup/disable-bare-repo-config-v2
Pull-Request: https://github.com/git/git/pull/1261

Range-diff vs v1:

1: 3370258c4b3 ! 1: 22b10bf9da8 [RFC] setup.c: make bare repo discovery optional

     @@ Metadata
      Author: Glen Choo <[email protected]>
      
       ## Commit message ##
     -    [RFC] setup.c: make bare repo discovery optional
     +    setup.c: make bare repo discovery optional
      
     -    Add a config variable, `safe.barerepository`, that tells Git whether or
     -    not to recognize bare repositories when it is trying to discover the
     -    repository. This only affects repository discovery, thus it has no
     +    Add a config variable, `discovery.bare`, that tells Git whether or not
     +    it should work with the bare repository it has discovered i.e. Git will
     +    die() if it discovers a bare repository, but it is not allowed by
     +    `discovery.bare`. This only affects repository discovery, thus it has no
          effect if discovery was not done (e.g. `--git-dir` was passed).
      
          This is motivated by the fact that some workflows don't use bare
     @@ Commit message
            are probably very rare in practice, this lets users reduce the chance
            to zero.
      
     -    This config is designed to be used like an allow-list, but it is not yet
     -    clear what a good format for this allow-list would be. As such, this
     -    patch limits the config value to a tri-state of [true|false|unset]:
     +    This config is an enum of:
      
     -    - [*|(unset)] recognize all bare repositories (like Git does today)
     -    - (empty) recognize no bare repositories
     +    - ["always"|(unset)]: always recognize bare repositories (like Git does
     +      today)
     +    - "never": never recognize bare repositories
      
     -    and leaves the full format to be determined later.
     +    More values are expected to be added later, and the default is expected
     +    to change (i.e. to something other than "always").
      
          [1]: https://lore.kernel.org/git/[email protected]
          [2]: I don't personally know anyone who does this as part of their
     @@ Commit message
      
          Signed-off-by: Glen Choo <[email protected]>
      
     - ## Documentation/config/safe.txt ##
     +    WIP setup.c: make discovery.bare die on failure
     +
     +    Signed-off-by: Glen Choo <[email protected]>
     +
     + ## Documentation/config/discovery.txt (new) ##
      @@
     -+safe.barerepository::
     -+	This config entry specifies directories that Git can recognize as
     -+	a bare repository when looking for the repository (aka repository
     ++discovery.bare::
     ++	Specifies what kinds of directories Git can recognize as a bare
     ++	repository when looking for the repository (aka repository
      +	discovery). This has no effect if repository discovery is not
      +	performed e.g. the path to the repository is set via `--git-dir`
      +	(see linkgit:git[1]).
      ++
     -+It is recommended that you set this value so that Git will only use the bare
     -+repositories you intend it to. This prevents certain types of security and
     -+non-security problems, such as:
     -+
     -+* `git clone`-ing a repository containing a maliciously bare repository
     -+  inside it.
     -+* Git recognizing a directory that isn't mean to be a bare repository,
     -+  but happens to look like one.
     -++
     -+The currently supported values are `*` (Git recognizes all bare
     -+repositories) and the empty value (Git never recognizes bare repositories).
     -+Defaults to `*`.
     -++
      +This config setting is only respected when specified in a system or global
      +config, not when it is specified in a repository config or via the command
     -+line option `-c safe.barerepository=<path>`.
     ++line option `-c discovery.bare=<value>`.
     +++
     ++The currently supported values are `always` (Git always recognizes bare
     ++repositories) and `never` (Git never recognizes bare repositories).
     ++This defaults to `always`, but this default is likely to change.
     +++
     ++If your workflow does not rely on bare repositories, it is recommended that
     ++you set this value to `never`. This makes repository discovery easier to
     ++reason about and prevents certain types of security and non-security
     ++problems, such as:
      +
     - safe.directory::
     - 	These config entries specify Git-tracked directories that are
     - 	considered safe even if they are owned by someone other than the
     ++* `git clone`-ing a repository containing a malicious bare repository
     ++  inside it.
     ++* Git recognizing a directory that isn't meant to be a bare repository,
     ++  but happens to look like one.
      
       ## setup.c ##
     +@@
     + 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;
      @@ setup.c: static int ensure_valid_ownership(const char *path)
       	return data.is_safe;
       }
       
     -+/*
     -+ * This is similar to safe_directory_data, but only supports true/false.
     -+ */
     -+struct safe_bare_repository_data {
     -+	int is_safe;
     -+};
     -+
     -+static int safe_bare_repository_cb(const char *key, const char *value, void *d)
     ++static int discovery_bare_cb(const char *key, const char *value, void *d)
      +{
     -+	struct safe_bare_repository_data *data = d;
     -+
     -+	if (strcmp(key, "safe.barerepository"))
     ++	if (strcmp(key, "discovery.bare"))
      +		return 0;
      +
     -+	if (!value || !strcmp(value, "*")) {
     -+		data->is_safe = 1;
     ++	if (!strcmp(value, "never")) {
     ++		discovery_bare_config = DISCOVERY_BARE_NEVER;
      +		return 0;
      +	}
     -+	if (!*value) {
     -+		data->is_safe = 0;
     ++	if (!strcmp(value, "always")) {
     ++		discovery_bare_config = DISCOVERY_BARE_ALWAYS;
      +		return 0;
      +	}
      +	return -1;
      +}
      +
     -+static int should_detect_bare(void)
     ++static int check_bare_repo_allowed(void)
      +{
     -+	struct safe_bare_repository_data data;
     -+
     -+	read_very_early_config(safe_bare_repository_cb, &data);
     ++	if (discovery_bare_config == DISCOVERY_BARE_UNKNOWN) {
     ++		read_very_early_config(discovery_bare_cb, NULL);
     ++		/* We didn't find a value; use the default. */
     ++		if (discovery_bare_config == DISCOVERY_BARE_UNKNOWN)
     ++			discovery_bare_config = DISCOVERY_BARE_ALWAYS;
     ++	}
     ++	switch (discovery_bare_config) {
     ++	case DISCOVERY_BARE_NEVER:
     ++		return 0;
     ++	case DISCOVERY_BARE_ALWAYS:
     ++		return 1;
     ++	default:
     ++		BUG("invalid discovery_bare_config %d", discovery_bare_config);
     ++	}
     ++}
      +
     -+	return data.is_safe;
     ++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";
     ++	default:
     ++		BUG("invalid discovery_bare_config %d", discovery_bare_config);
     ++	}
      +}
      +
       enum discovery_result {
       	GIT_DIR_NONE = 0,
       	GIT_DIR_EXPLICIT,
     +@@ setup.c: 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
     + };
     + 
     + /*
      @@ setup.c: static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
     - 			return GIT_DIR_DISCOVERED;
       		}
       
     --		if (is_git_directory(dir->buf)) {
     -+		if (should_detect_bare() && is_git_directory(dir->buf)) {
     + 		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, ".");
     +@@ setup.c: 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
      
     - ## t/t1510-repo-setup.sh ##
     -@@ t/t1510-repo-setup.sh: test_expect_success '#16e: bareness preserved by --bare' '
     - 	)
     - '
     - 
     -+# Test the tri-state of [(unset)|""|"*"].
     -+test_expect_success '#16f: bare repo in worktree' '
     -+	test_when_finished "git config --global --unset safe.barerepository" &&
     -+	setup_repo 16f unset "" unset &&
     + ## t/t0034-discovery-bare.sh (new) ##
     +@@
     ++#!/bin/sh
      +
     -+	git init --bare 16f/default/bare &&
     -+	git init --bare 16f/default/bare/bare &&
     -+	try_case 16f/default/bare unset unset \
     -+		. "(null)" "$here/16f/default/bare" "(null)" &&
     -+	try_case 16f/default/bare/bare unset unset \
     -+		. "(null)" "$here/16f/default/bare/bare" "(null)" &&
     ++test_description='verify discovery.bare checks'
      +
     -+	git config --global safe.barerepository "*" &&
     -+	git init --bare 16f/all/bare &&
     -+	git init --bare 16f/all/bare/bare &&
     -+	try_case 16f/all/bare unset unset \
     -+		. "(null)" "$here/16f/all/bare" "(null)" &&
     -+	try_case 16f/all/bare/bare unset unset \
     -+		. "(null)" "$here/16f/all/bare/bare" "(null)" &&
     ++. ./test-lib.sh
      +
     -+	git config --global safe.barerepository "" &&
     -+	git init --bare 16f/never/bare &&
     -+	git init --bare 16f/never/bare/bare &&
     -+	try_case 16f/never/bare unset unset \
     -+		".git" "$here/16f" "$here/16f" "never/bare/" &&
     -+	try_case 16f/never/bare/bare unset unset \
     -+		".git" "$here/16f" "$here/16f" "never/bare/bare/"
     -+'
     ++pwd="$(pwd)"
      +
     -+test_expect_success '#16g: inside .git with safe.barerepository' '
     -+	test_when_finished "git config --global --unset safe.barerepository" &&
     ++expect_allowed () {
     ++	git rev-parse --absolute-git-dir >actual &&
     ++	echo "$pwd/outer-repo/bare-repo" >expected &&
     ++	test_cmp expected actual
     ++}
      +
     -+	# Omit the "default" case; it is covered by 16a.
     ++expect_rejected () {
     ++	test_must_fail git rev-parse --absolute-git-dir 2>err &&
     ++	grep "discovery.bare" err
     ++}
      +
     -+	git config --global safe.barerepository "*" &&
     -+	setup_repo 16g/all unset "" unset &&
     -+	mkdir -p 16g/all/.git/wt/sub &&
     -+	try_case 16g/all/.git unset unset \
     -+		. "(null)" "$here/16g/all/.git" "(null)" &&
     -+	try_case 16g/all/.git/wt unset unset \
     -+		"$here/16g/all/.git" "(null)" "$here/16g/all/.git/wt" "(null)" &&
     -+	try_case 16g/all/.git/wt/sub unset unset \
     -+		"$here/16g/all/.git" "(null)" "$here/16g/all/.git/wt/sub" "(null)" &&
     ++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 &&
     ++		expect_allowed &&
     ++		cd refs/ &&
     ++		expect_allowed
     ++	)
     ++'
     ++
     ++test_expect_success 'discovery.bare=always' '
     ++	git config --global discovery.bare always &&
     ++	(
     ++		cd outer-repo/bare-repo &&
     ++		expect_allowed &&
     ++		cd refs/ &&
     ++		expect_allowed
     ++	)
     ++'
      +
     -+	git config --global safe.barerepository "" &&
     -+	setup_repo 16g/never unset "" unset &&
     -+	mkdir -p 16g/never/.git/wt/sub &&
     -+	try_case 16g/never/.git unset unset \
     -+		".git" "$here/16g/never" "$here/16g/never" ".git/" &&
     -+	try_case 16g/never/.git/wt unset unset \
     -+		".git" "$here/16g/never" "$here/16g/never" ".git/wt/" &&
     -+	try_case 16g/never/.git/wt/sub unset unset \
     -+		".git" "$here/16g/never" "$here/16g/never" ".git/wt/sub/"
     ++test_expect_success 'discovery.bare=never' '
     ++	git config --global discovery.bare never &&
     ++	(
     ++		cd outer-repo/bare-repo &&
     ++		expect_rejected &&
     ++		cd refs/ &&
     ++		expect_rejected
     ++	) &&
     ++	(
     ++		GIT_DIR=outer-repo/bare-repo &&
     ++		export GIT_DIR &&
     ++		expect_allowed
     ++	)
      +'
      +
     - test_expect_success '#17: GIT_WORK_TREE without explicit GIT_DIR is accepted (bare case)' '
     - 	# Just like #16.
     - 	setup_repo 17a unset "" true &&
     ++test_done

-: ----------- > 2: 62070aab7eb setup.c: learn discovery.bareRepository=cwd

gitgitgadget

From: Glen Choo <[email protected]>

Add a config variable, discovery.bare, that tells Git whether or not it should work with the bare repository it has discovered i.e. Git will die() if it discovers a bare repository, but it is not allowed by discovery.bare. This only affects repository discovery, thus it has no effect if discovery was not done (e.g. --git-dir was passed).

This is motivated by the fact that some workflows don't use bare repositories at all, and users may prefer to opt out of bare repository discovery altogether:

- An easy assumption for a user to make is that Git commands run anywhere inside a repository's working tree will use the same repository. However, if the working tree contains a bare repository below the root-level (".git" is preferred at the root-level), any operations inside that bare repository use the bare repository instead.

In the worst case, attackers can use this confusion to trick users into running arbitrary code (see [1] for a deeper discussion). But even in benign situations (e.g. a user renames ".git/" to ".git.old/" and commits it for archival purposes), disabling bare repository discovery can be a simpler mode of operation (e.g. because the user doesn't actually want to use ".git.old/") [2].

- Git won't "accidentally" recognize a directory that wasn't meant to be a bare repository, but happens to resemble one. While such accidents are probably very rare in practice, this lets users reduce the chance to zero.

This config is an enum of:

- ["always"|(unset)]: always recognize bare repositories (like Git does today)
- "never": never recognize bare repositories

More values are expected to be added later, and the default is expected to change (i.e. to something other than "always").

[1]: https://lore.kernel.org/git/[email protected]
[2]: I don't personally know anyone who does this as part of their
normal workflow, but a cursory search on GitHub suggests that there is a not insubstantial number of people who munge ".git" in order to store
its contents.

https://github.com/search?l=&o=desc&p=1&q=ref+size%3A%3C1000+filename%3AHEAD&s=indexed&type=Code
(aka search for the text "ref", size:<1000, filename:HEAD)

WIP setup.c: make discovery.bare die on failure

---
 Documentation/config/discovery.txt | 24 +++++++++++
 setup.c                            | 66 +++++++++++++++++++++++++++++-
 t/t0034-discovery-bare.sh          | 59 ++++++++++++++++++++++++++
 3 files changed, 148 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/config/discovery.txt
 create mode 100755 t/t0034-discovery-bare.sh

new file mode 100644
index 00000000000..761cabe6e70
@@ -0,0 +1,24 @@
+discovery.bare::
+	Specifies what kinds of directories Git can recognize as a bare
+	repository when looking for the repository (aka repository
+	discovery). This has no effect if repository discovery is not
+	performed e.g. the path to the repository is set via `--git-dir`
+	(see linkgit:git[1]).
++
+This config setting is only respected when specified in a system or global
+config, not when it is specified in a repository config or via the command
+line option `-c discovery.bare=<value>`.
++
+The currently supported values are `always` (Git always recognizes bare
+repositories) and `never` (Git never recognizes bare repositories).
+This defaults to `always`, but this default is likely to change.
++
+If your workflow does not rely on bare repositories, it is recommended that
+you set this value to `never`. This makes repository discovery easier to
+reason about and prevents certain types of security and non-security
+problems, such as:
+
+* `git clone`-ing a repository containing a malicious bare repository
+  inside it.
+* Git recognizing a directory that isn't meant to be a bare repository,
+  but happens to look like one.

gitgitgadget

From: Glen Choo <[email protected]>

Add a 'cwd' option to discovery.bareRepository, which allows a bare repository to be used if and only if the cwd is the root of a bare repository. This covers the common case where a user works with a bare repository by cd-ing into the repository's root.

---
 Documentation/config/discovery.txt |  6 ++++--
 setup.c                            | 27 ++++++++++++++++++++-------
 t/t0034-discovery-bare.sh          | 10 ++++++++++
 3 files changed, 34 insertions(+), 9 deletions(-)

index 761cabe6e70..d7cdee3a5e1 100644
@@ -10,8 +10,10 @@ config, not when it is specified in a repository config or via the command
 line option `-c discovery.bare=<value>`.
 +
 The currently supported values are `always` (Git always recognizes bare
-repositories) and `never` (Git never recognizes bare repositories).
-This defaults to `always`, but this default is likely to change.
+repositories), `cwd` (Git only recognizes bare repositories if they are the
+current working directory) and `never` (Git never recognizes bare
+repositories). This defaults to `always`, but this default is likely to
+change.
 +
 If your workflow does not rely on bare repositories, it is recommended that
 you set this value to `never`. This makes repository discovery easier to

index cee01d86f0c..ead999f404c 100644
@@ -14,6 +14,7 @@ enum discovery_bare_config {
 	DISCOVERY_BARE_UNKNOWN = -1,
 	DISCOVERY_BARE_NEVER = 0,
 	DISCOVERY_BARE_ALWAYS,
+	DISCOVERY_BARE_CWD,
 };
 static enum discovery_bare_config discovery_bare_config =
 	DISCOVERY_BARE_UNKNOWN;

@@ -1153,10 +1154,14 @@ static int discovery_bare_cb(const char *key, const char *value, void *d)
 		discovery_bare_config = DISCOVERY_BARE_ALWAYS;
 		return 0;
 	}
+	if (!strcmp(value, "cwd")) {
+		discovery_bare_config = DISCOVERY_BARE_CWD;
+		return 0;
+	}
 	return -1;
 }
 
-static int check_bare_repo_allowed(void)
+static int check_bare_repo_allowed(const char *cwd, const char *path)
 {
 	if (discovery_bare_config == DISCOVERY_BARE_UNKNOWN) {
 		read_very_early_config(discovery_bare_cb, NULL);

@@ -1169,6 +1174,8 @@ static int check_bare_repo_allowed(void)
 		return 0;
 	case DISCOVERY_BARE_ALWAYS:
 		return 1;
+	case DISCOVERY_BARE_CWD:
+		return !strcmp(cwd, path);
 	default:
 		BUG("invalid discovery_bare_config %d", discovery_bare_config);
 	}

@@ -1181,6 +1188,8 @@ static const char *discovery_bare_config_to_string(void)
 		return "never";
 	case DISCOVERY_BARE_ALWAYS:
 		return "always";
+	case DISCOVERY_BARE_CWD:
+		return "cwd";
 	default:
 		BUG("invalid discovery_bare_config %d", discovery_bare_config);
 	}

@@ -1212,7 +1221,8 @@ enum discovery_result {
  * the discovered .git/ directory, if any. If `gitdir` is not absolute, it
  * is relative to `dir` (i.e. *not* necessarily the cwd).
  */
-static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
+static enum discovery_result setup_git_directory_gently_1(struct strbuf *cwd,
+							  struct strbuf *dir,
 							  struct strbuf *gitdir,
 							  int die_on_error)
 {

@@ -1293,7 +1303,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 		}
 
 		if (is_git_directory(dir->buf)) {
-			if (!check_bare_repo_allowed())
+			if (!check_bare_repo_allowed(cwd->buf, dir->buf))
 				return GIT_DIR_DISALLOWED_BARE;
 			if (!ensure_valid_ownership(dir->buf))
 				return GIT_DIR_INVALID_OWNERSHIP;

@@ -1319,16 +1329,18 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 int discover_git_directory(struct strbuf *commondir,
 			   struct strbuf *gitdir)
 {
-	struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
+	struct strbuf cwd = STRBUF_INIT, dir = STRBUF_INIT, err = STRBUF_INIT;
 	size_t gitdir_offset = gitdir->len, cwd_len;
 	size_t commondir_offset = commondir->len;
 	struct repository_format candidate = REPOSITORY_FORMAT_INIT;
 
-	if (strbuf_getcwd(&dir))
+	if (strbuf_getcwd(&cwd))
 		return -1;
+	strbuf_addbuf(&dir, &cwd);
 
 	cwd_len = dir.len;
-	if (setup_git_directory_gently_1(&dir, gitdir, 0) <= 0) {
+	if (setup_git_directory_gently_1(&cwd, &dir, gitdir, 0) <= 0) {
+		strbuf_release(&cwd);
 		strbuf_release(&dir);
 		return -1;
 	}

@@ -1351,6 +1363,7 @@ int discover_git_directory(struct strbuf *commondir,
 	strbuf_reset(&dir);
 	strbuf_addf(&dir, "%s/config", commondir->buf + commondir_offset);
 	read_repository_format(&candidate, dir.buf);
+	strbuf_release(&cwd);
 	strbuf_release(&dir);
 
 	if (verify_repository_format(&candidate, &err) < 0) {

@@ -1400,7 +1413,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		die_errno(_("Unable to read current working directory"));
 	strbuf_addbuf(&dir, &cwd);
 
-	switch (setup_git_directory_gently_1(&dir, &gitdir, 1)) {
+	switch (setup_git_directory_gently_1(&cwd, &dir, &gitdir, 1)) {
 	case GIT_DIR_EXPLICIT:
 		prefix = setup_explicit_git_dir(gitdir.buf, &cwd, &repo_fmt, nongit_ok);
 		break;

index 9c774872c4e..ba44cf19c99 100755
@@ -56,4 +56,14 @@ test_expect_success 'discovery.bare=never' '
 	)
 '
 
+test_expect_success 'discovery.bare=cwd' '
+	git config --global discovery.bare cwd &&
+	(
+		cd outer-repo/bare-repo &&
+		expect_allowed &&
+		cd refs/ &&
+		expect_rejected
+	)
+'
+
 test_done

gitgitgadget

Would a middle-ground of giving a warning() message help? Can it be loud and annoying enough to knudge the users to adjust without breaking the functionality?

The longer-term default should be "cwd is allowed, but we do not bother going up from object/04 subdirectory of a bare repository", not "bare repositories should not be usable at all without GIT_DIR".

Missing comma before "i.e."

;-)

Hopefully "git fetch" over ssh:// and file:/// would run the other side with GIT_DIR explicitly set? As long as this recommendation does not break these use cases, I think we are OK, but I do not yet find these "problems, such as..." so convincing.

This number is already in use by an in-flight topic, if I am not mistaken. Please make it a habit to always check your topic works well when merged to 'next' and to 'seen'.

Thanks.

The intended commit message ends here...

Ugh, dumb mistake (bad squash). Fortunately this was one of my more professional-sounding WIP commit messages.

Personally, when my tool changes its behavior, I would strongly prefer it to die than to "change behavior + warn". I'd feel more comfortable knowing that the tool did nothing as opposed to doing the wrong thing and only being informed after the fact. Also, I sometimes ignore warnings ;)

When we _do_ transition away from die(), ignore + warning() sounds like a good first step.

But if any of this flies in the face of the project's conventions, let me know as such.

Thanks.

Ah, I'll check this and get back to you.

What would be a convincing rationale to you? I'll capture that here.

I'm assuming that you already have such an rationale in mind when you say that the longer-term default is that "we respect bare repositories only if they are the cwd.". I'm also assuming that this rationale is something other than embedded bare repos, because "cwd-only" does not protect against that.

Perhaps "never" sounds better to folks who don't ever expect bare repositories and want to lock down the environment. Randall (cc-ed) suggests one such use case in [1].

(To Randall: Oops, I actually meant to cc you earlier, since you were the first to suggest a practical use case for never allowing bare repos. It must've slipped my mind).

[1] https://lore.kernel.org/git/[email protected].

I think it is fine to include the "never" option for users to opt-in to this super-protected state, but I want to make it very clear that we should never move to it as a new default. This phrasing of 'something other than "always"' is key, but it might be good to point out that "never" is very unlikely to be that default.

Accidental concatenation of squashed commit?

I don't consider this case valuable. In addition to allowing the most-common use case, it also allows the most-common route that an attacker would use to try to get a user to run a Git command in a malicious embedded bare repo. I think we are better off without it.

Thanks,
-Stolee

Please include a full cover letter with each version, so reviewers can respond to the full series goals.

Your series here intends to start protecting against malicious embedded bare repositories by allowing users to opt-in to a more protected state. When the 'discovery.bare' option is set, then Git may die() on a bare repository that is discovered based on the current working directory (these protections are ignored if the user specifies the directory directly through --git-dir or $GIT_DIR).

The 'discovery.bare' option has these values at the end of your series:

* 'always' (default) allows all bare repos, matching the current behavior of Git.

* 'never' avoids operating in bare repositories altogether.

* 'cwd' operates in a bare repository only if the current directory is exactly the root of the bare repository.

It is important that we keep 'always' as the default at first, because we do not want to introduce a breaking change without warning (at least for an issue like this that has been around for a long time).

The 'never' option is a good one for very security-conscious users who really want to avoid problems. I don't anticipate that users who know about this option and set it themselves are the type that would fall for the social engineering required to attack using this vector, but I can imagine an IT department installing the value in system config across a fleet of machines.

I find the 'cwd' option to not be valuable. It unblocks most existing users, but also almost completely removes the protection that the option was supposed to provide.

I find neither the 'never' or 'cwd' options an acceptable choice for a future default.

I also think that this protection is too rigid: it restricts _all_ bare repositories, not just embedded ones. There is no check to see if the parent directory of the bare repository is inside a non-bare repository.

This leads to what I think would be a valuable replacement for the 'cwd' option:

* 'no-embedded' allows non-embedded bare repositories. An _embedded bare repository_ is a bare repository whose parent directory is contained in the worktree of a non-bare Git repository. When in this mode, embedded bare repositories are not allowed unless the parent non-bare Git repository has a 'safe.embedded' config value storing the path to the current embedded bare repository.

That was certainly difficult to write, but here it is as pseudo-code to hopefully remove some doubt as to how this might work:

if repo is bare:

    if value == "always":
       return ALLOWED
    if value == "never":
       return FORBIDDEN;

    path = get_parent_repo()

    if !path:
       return ALLOWED
    
    if config_file_has_value("{path}/.git/config", "safe.embedded", repo):
       return ALLOWED

    return FORBIDDEN

With this kind of option, we can protect users from these social engineering attacks while providing an opt-in protection for scenarios where embedded bare repos are currently being used (while also not breaking anyone using non-embedded bare repos).

I think Taylor was mentioning something like this in his previous replies, perhaps even to the previous thread on this topic.

This 'no-embedded' option is something that I could see as a potential new default, after it has proven itself in a released version of Git.

There are performance drawbacks to checking the parent path for a Git repo, which is why it is only done when in "no-embedded" mode.

I mentioned some other concerns in your PATCH 1 about how we are now adding the third use of read_very_early_config() and that we should probably refactor that before adding the third option, in order to avoid additional performance costs as well as it being difficult to audit which config options are only checked from these "protected" config files.

Thanks,
-Stolee

Heh, personally I would try very hard not to change the behaviour without explicitly asked by the users with configuration or command line option. Flipping the default has traditionally been done in two or three phases.

(1) We start by giving a loud and annoying warning to those who haven't configured and tell them the default *will* change, how to keep the current behaviour forever, and how to live in the future by adopting the future default early.

(2) After a while, we flip the default. Those who haven't configured are given a notice that the default has changed, how to keep the old behaviour forever, and how to explicitly choose the same value as the default to squelch the notice.

(3) After yet another while, we stop giving the notice. If we omitted (2), here is where we flip the default.

Strictly speaking, we can have (1) in one release and then could directly jump to (3), but some distros may skip the releases that has (1), and (2) is an attempt to help users of such distros.

That is a wrong question. You are the one pushing for castrating the bare repositories.

No, I do not have such a "different" rationale to justify the change proposed in this patch. I was saying that the claim "embedded bare repos are risky", backed by your two examples, did not sound all that serious a problem. Presented with a more serious brekage scenario, it may make the description more convincing.

Thanks.

Ah, that is very helpful. Thanks. It's pretty clear that I misunderstood what you meant by "giving a warning() message" - the warning() is there to prepare users in advance of the change; we don't actually want the warning() in the long term.

For something as disruptive as discovering bare repos, having all of (1), (2) and (3) sounds appropriate.

Let me clarify in case this wasn't received the way I intended. Earlier in the thread, you mentioned:

The longer-term default should be "cwd is allowed, but we do not bother going up from object/04 subdirectory of a bare repository", [...]

which I took to mean "Junio thinks that, by default, Git should stop walking up to find a bare repo, and thinks this is better because of rationale X.", and not, "Junio does not think that the default needs to change, but is just suggesting a better default than Glen's".

If it is the former, then there is obviously some thought process here that is worth sharing.

If it the latter, then I'm in favor of taking Stolee's suggestion to drop "cwd", since nobody else finds it useful enough. (I like the 'simplification' story, but not enough to push "cwd" through, especially since it does quite little security-wise.)

Fair. I'll mull over this.

The X is "it would not break existing use case too badly, just to address a 'security' story whose severity is not so clearly expressed".

As long as you'll be there to answer the angry mob that complain loudly (and irritatingly enough, the only do so after a release is made to flip the default), I do not care too much either way ;-).

Thanks.

I am confused, then.

What does a user who has some legitimate (non-embedded) bare repositories do if they are skeptical of other bare repositories? I suspect the best answer we would be able to provide with these patches is "use --git-dir".

What happens to a user who has a combination of legitimate bare repositories, embedded bare repositories that they trust, and other embedded bare repositories that they don't?

As far as I can tell, our recommendation with these tools would be to:

- run git config --global discovery.bare never, and
- include --git-dir=$(pwd) in any git invocations in bare repositories that they do trust

This gets at my concerns from [1] and [2] (mostly [2], in this case) that we're trying to close the embedded bare repos problem with an overly broad solution, at the expense of usability.

I can't shake the feeling that something like I described towards the bottom of [2] would give you all of the security guarantees you're after without compromising on usability for non-embedded bare repositories.

I'm happy to explore this direction more myself if you don't want to. I would just much rather see us adopt an approach that doesn't break more use-cases than it has to if such a thing can be avoided.

I cannot endorse these patches as-is.

Thanks,
Taylor

[1]: https://lore.kernel.org/git/[email protected]/
[2]: https://lore.kernel.org/git/[email protected]/

Thanks for the summary, which I think will be especially helpful to others looking at this series for the very first time.

When I first read this, I disagreed, since presumably that same crowd has legitimate bare repositories that they want to continue being able to operate in without having to pass --git-dir or $GIT_DIR in.

In fact...

...this resonates quite a bit more with me. "never" isn't a good option unless you aren't a user of bare repositories _and_ don't have any embedded bare repositories (either at all, or any ones that you trust).

This is indeed very similar to a proposal I had made upthread (which you note lower down in this email). One thing that's nice is that we only have to traverse up to the parent repo when in the "no-embedded" mode. That may be slow (since it's unbounded all the way up to the filesystem root or a ceiling directory, whichever we encounter first), but I think it's unavoidable if you need to distinguish between embedded and non-embedded bare repositories.

Yep, see: https://lore.kernel.org/git/[email protected]/.a

I would be totally happy to see "no-embedded" become the default. It might be nice to issue a warning when the top-level config is unset, to give users a heads up about cases that may be broken, perhaps like:

    if repo is bare:
      switch (value) {
      case "always":
        return ALLOWED;
      case "never":
        return FORBIDDEN;
      case "no-embedded": # fallthrough
      case "":
        path = get_parent_repo()
        if !path
          return ALLOWED;

        if config_file_has_value("{path}/.git/config", "safe.embedded", repo)
          return ALLOWED;

        if value == "no-embedded":
          return FORBIDDEN;

        # otherwise, we're in an embedded bare repository with an unset
        # discovery.bare config.
        #
        # warn that this will break in the future...
        warning(_("%s is embedded within %s"), the_repository.path, path);
        advise(_("to allow discovery for this embedded repo, either run"));
        advise(_(""));
        advise(_("  $ git config --global discovery.bare always, or"));
        advise(_("  $ git -C '%s' config --local safe.embedded '%s'"),
               path, relpath(path, the_repository.path));

        # ...but allow the invocation for now until the default is
        # changed.
        return ALLOWED;
      default:
        die(_("unrecognized value of discovery.bare: '%s'"), value);
      }

...where relpath is similar to Go's path/filepath.Rel function.

With an appropriate deprecation period, I think we could even get away from the "continue executing, but don't read config+hooks", which in retrospect is more error-prone and difficult to reason about than I initially had given it credit for.

Thanks,
Taylor

Sounds sensible. I wonder how expensive this will be in practice, but the behaviour seems well thought out.

Thanks, I think this has advanced the conversation quite a bit.

My mistake, I should have prepared this summary myself. Thanks again.

Yes.

Yes. Setting the 'never' option in a system config is the use case that motivated this.

Ok, I agree that it provides next-to-no protection. I'll drop it in this series; it's easy enough to reimplement if users really want it anyway.

[...]

I agree, this sounds like a good default that should work for most users.

That said, I don't think I will implement it, and even if I do, it won't be in this series. I have serious doubts that I'd be able to deliver it in a reasonable amount of time (I tried preparing patches to this effect and failed [1]), and 'never' is sufficient for $DAYJOB's current needs.

I would be very happy to see this come to fruition though. I have no objections to anyone preparing patches for this, and I'll gladly review those if that's helpful.

[1] The specific trouble I had was figuring out whether or not the 'parent' repo was tracking the bare repo, since an untracked bare repo in the working tree isn't (in some sense) really "embedded" and it can't have come from a remote.

But maybe the tracking check is unnecessary. We would break a few more users without it, but 'safe.embedded' is an easy enough way for a user to unbreak themselves.

Makes sense. I'll ask about specifics on that subthread.

Thanks for being thorough, I find it really helpful.

For brevity, I won't reply to comments that I think are obviously good, so you can assume I'll incorproate anything that isn't commented on.

Agree, and I think defining the term will be useful in future on-list discussions.

Looking at all of the read_very_early_config() calls,

- check_bare_repo_allowed() can use git_configset_get_string()
- ensure_valid_ownership() can use git_configset_get_value_multi()
- tr2_sysenv_load() reads every value with the "trace2." prefix. AFAICT configsets only support exact key lookups and I don't see an easy way teach configsets to support prefix lookups.

(I didn't look too closely at uploadpack.packobjectshook because I don't know enough about config scopes to comment.)

So using a configset, we'll still need to read the config files at least twice. That's better than thrice, but it doesn't cover the tr2_sysenv_load() use case, and we'll run into this yet again if add function that reads all config values with a given prefix.

An hacky alternative that covers all of these use cases would be to read all protected config in a single pass, e.g.

  static struct protected_config {
         struct safe_directory_data safe_directory_data;
         const char *discovery_bare;
         struct string_list tr2_sysenv;
  };

  static int protected_config_cb()
  {
    /* Parse EVERYTHING that belongs in protected_config. */
  }

but protected_config_cb() would have to parse too many unrelated things for my liking.

So I'll use the configset for the cases where the key is known, and perhaps we'll punt on tr2_sysenv_load().

This comment doesn't apply because unlike most other config reading functions, read_very_early_config() and read_early_config() die when the callback returns -1.

I'm not sure why this is the case though, and maybe you think there is value in having a non-die()-ing variant, e.g. read_very_early_config_gently()?

Yes, it will fail if run inside of a .git/ directory. "never" prevents you from working from inside .git/ unless you set GIT_DIR.

IIRC, we don't show "fatal: this operation must be run in a work tree" for every Git command, e.g. "git log" works just fine. It makes sense to show this warning when the CWD supports 'some, but not all' Git commands, but I don't think this is valuable if we forbid *all* Git commands.

Instead of trying to make "never" accomodate this use case, perhaps what we want is a "dotgit-only" option that allows a bare repository if it is below a .git/ directory. Since we forbid .git in the index, this seems somewhat safe, but I hadn't proposed this sooner because I don't know if we need it yet, and I'm certain that there are less secure edge cases that need to be thought through.

Since I'm trying to replace read_very_early_config() anyway, is this a good time to teach git to respect "-c safe.directory"?

My understanding of [1] is that we only ignore "-c safe.directory" because read_very_early_config() doesn't support it, but we would prefer to support it if we could.

[1] https://lore.kernel.org/git/[email protected]/

Thanks everyone for the feedback. This round has two major changes compared to the last:

* This feature is now purely motivated by the embedded bare repo attack; I've dropped all of language pertaining to 'simplifying' bare repository discovery.

* Protected config gets a formal definition and is implemented using a config_set, which should hopefully address most of the performance concerns of using read_very_early_config() (see 2/5 for more discussion).

With this new implementation, it's easy to teach protected config to include "-c". This round includes some patches (4-5/5) that do this but I'm ok to drop them if we don't agree that they are a good idea.

This round is newly rebased onto master because sg/safe-directory-tests-and-docs has been recently integrated and the "-c" patches directly contradict that.

= Description

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.

This series implements a simple way of preventing such attacks: create a config option, discovery.bare, that tells Git whether or not to die when it finds a bare repository. discovery.bare has two values:

* "always": always allow bare repositories (default), identical to current behavior
* "never": never allow bare repositories

and users/system administrators who never expect to work with bare repositories can secure their environments using "never". discovery.bare has no effect if --git-dir or GIT_DIR is passed because we are confident that the user is not confused about which repository is being used.

This series does not change the default behavior, but in the long-run, a "no-embedded" option might be a safe and usable default [2]. "never" is too restrictive and unlikely to be the default.

For security reasons, discovery.bare cannot be read from repository-level config (because we would end up trusting the embedded bare repository that we aren't supposed to trust to begin with). Since this would introduce a 3rd variable that is only read from 'protected/trusted config' (the others are safe.directory and uploadpack.packObjectsHook) this series also defines and creates a shared implementation for 'protected config'

= Patch organization

* Patches 1-2 define "protected config" and create a shared implementation.
* Patch 3 introduces discovery.bare.
* Patches 4-5 expand the definition of "protected config" to include the CLI option "-c" [3]. Since this is identical to how uploadpack.packObjectsHook currently behaves, it is refactored to a "protected config only" variable.

= Series history

Changes in v3:

* Rebase onto a more recent 'master'
* Reframe this feature in only in terms of the 'embedded bare repo' attack.
* Other docs improvements (thanks Stolee in particular!)
* Protected config no longer uses read_very_early_config() and is only read once
* Protected config now includes "-c"
* uploadpack.packObjectsHook now uses protected config instead of ignoring repo config using config scopes

Changes in v2:

* Rename safe.barerepository to discovery.bare and make it die()
* Move tests into t/t0034-discovery-bare.sh
* Avoid unnecessary config reading by using a static variable
* Add discovery.bare=cwd
* Fix typos

= Future work

* This series does not implement the "no-embedded" option [2] and I won't work on it any time soon, but I'd be more than happy to review if someone sends patches.
* With discovery.bare, if a builtin is marked RUN_SETUP_GENTLY, setup.c doesn't die() and we don't tell users why their repository was rejected, e.g. "git config" gives an opaque "fatal: not in a git directory". This isn't a new problem though, since safe.directory has the same issue.

[1]
https://lore.kernel.org/git/[email protected]
[2] This was first suggested in
https://lore.kernel.org/git/[email protected]
[3] https://lore.kernel.org/git/[email protected]/ suggests that safe.directory doesn't need to ignore "-c", it just happened to be implemented that way.

Glen Choo (5):
Documentation: define protected configuration
config: read protected config with git_protected_config() setup.c: create discovery.bare
config: include "-c" in protected config
upload-pack: make uploadpack.packObjectsHook protected

 Documentation/config.txt            |  8 ++++
 Documentation/config/discovery.txt  | 19 ++++++++
 Documentation/config/safe.txt       | 19 ++++----
 Documentation/config/uploadpack.txt | 22 ++++------
 Documentation/glossary-content.txt  | 18 ++++++++
 config.c                            | 41 +++++++++++++++++
 config.h                            | 17 ++++++++
 repository.c                        |  5 +++
 repository.h                        |  8 ++++
 setup.c                             | 68 ++++++++++++++++++++++++++++-
 t/t0033-safe-directory.sh           | 24 +++++-----
 t/t0035-discovery-bare.sh           | 63 ++++++++++++++++++++++++++
 upload-pack.c                       | 17 +++++---
 13 files changed, 283 insertions(+), 46 deletions(-)
 create mode 100644 Documentation/config/discovery.txt
 create mode 100755 t/t0035-discovery-bare.sh

base-commit: f9b95943b68b6b8ca5a6072f50a08411c6449b55
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1261%2Fchooglen%2Fsetup%2Fdisable-bare-repo-config-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1261/chooglen/setup/disable-bare-repo-config-v3
Pull-Request: https://github.com/git/git/pull/1261

Range-diff vs v2:

 -:  ----------- > 1:  575676c760d Documentation: define protected configuration
 -:  ----------- > 2:  7499a280961 config: read protected config with `git_protected_config()`
 1:  22b10bf9da8 ! 3:  d5a3e9f9845 setup.c: make bare repo discovery optional

@@ Metadata
Author: Glen Choo <[email protected]>

## Commit message ##
- setup.c: make bare repo discovery optional
+ setup.c: create discovery.bare

- Add a config variable, discovery.bare, that tells Git whether or not
- it should work with the bare repository it has discovered i.e. Git will
- die() if it discovers a bare repository, but it is not allowed by
- discovery.bare. This only affects repository discovery, thus it has no
- effect if discovery was not done (e.g. --git-dir was passed).
+ 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.

- This is motivated by the fact that some workflows don't use bare
- repositories at all, and users may prefer to opt out of bare repository
- discovery altogether:
+ 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.

- - An easy assumption for a user to make is that Git commands run
- anywhere inside a repository's working tree will use the same
- repository. However, if the working tree contains a bare repository
- below the root-level (".git" is preferred at the root-level), any
- operations inside that bare repository use the bare repository
- instead.
-
- In the worst case, attackers can use this confusion to trick users
- into running arbitrary code (see [1] for a deeper discussion). But
- even in benign situations (e.g. a user renames ".git/" to ".git.old/"
- and commits it for archival purposes), disabling bare repository
- discovery can be a simpler mode of operation (e.g. because the user
- doesn't actually want to use ".git.old/") [2].
-
- - Git won't "accidentally" recognize a directory that wasn't meant to be
- a bare repository, but happens to resemble one. While such accidents
- are probably very rare in practice, this lets users reduce the chance
- to zero.
+ 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"|(unset)]: always recognize bare repositories (like Git does
- today)
- - "never": never recognize bare repositories
+ - "always": always allow bare repositories (this is the default)
+ - "never": never allow bare repositories

- More values are expected to be added later, and the default is expected
- to change (i.e. to something other than "always").
+ 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/[email protected]
- [2]: I don't personally know anyone who does this as part of their
- normal workflow, but a cursory search on GitHub suggests that there is a
- not insubstantial number of people who munge ".git" in order to store
- its contents.
-
- https://github.com/search?l=&o=desc&p=1&q=ref+size%3A%3C1000+filename%3AHEAD&s=indexed&type=Code
- (aka search for the text "ref", size:<1000, filename:HEAD)
+ [2]: https://lore.kernel.org/git/[email protected]

- WIP setup.c: make discovery.bare die on failure
-
- Signed-off-by: Glen Choo <[email protected]>
+ ## Documentation/config.txt ##
+@@ Documentation/config.txt: include::config/diff.txt[] +
+ include::config/difftool.txt[]
+
++include::config/discovery.txt[]
++
+ include::config/extensions.txt[]
+
+ include::config/fastimport.txt[]

## Documentation/config/discovery.txt (new) ##
@@
+discovery.bare::
-+ Specifies what kinds of directories Git can recognize as a bare
-+ repository when looking for the repository (aka repository
-+ discovery). This has no effect if repository discovery is not
-+ performed e.g. the path to the repository is set via --git-dir
-+ (see linkgit:git[1]).
++ '(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]).
++
-+This config setting is only respected when specified in a system or global -+config, not when it is specified in a repository config or via the command -+line option -c discovery.bare=<value>.
++The currently supported values are:
++
-+The currently supported values are always (Git always recognizes bare -+repositories) and never (Git never recognizes bare repositories). -+This defaults to always, but this default is likely to change. ++* always: Git always works with bare repositories
++* never: Git never works with bare repositories
++
-+If your workflow does not rely on bare repositories, it is recommended that -+you set this value to never. This makes repository discovery easier to -+reason about and prevents certain types of security and non-security -+problems, such as:
-+
-+* git clone-ing a repository containing a malicious bare repository -+ inside it.
-+* Git recognizing a directory that isn't meant to be a bare repository, -+ but happens to look like one.
++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.

## setup.c ##
@@
@@ setup.c: static int ensure_valid_ownership(const char *path)
+static int check_bare_repo_allowed(void)
+{
+ if (discovery_bare_config == DISCOVERY_BARE_UNKNOWN) {
-+ read_very_early_config(discovery_bare_cb, NULL);
-+ /* We didn't find a value; use the default. */
-+ if (discovery_bare_config == DISCOVERY_BARE_UNKNOWN)
-+ discovery_bare_config = DISCOVERY_BARE_ALWAYS;
++ 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;
-+ default:
++ case DISCOVERY_BARE_UNKNOWN:
+ BUG("invalid discovery_bare_config %d", discovery_bare_config);
+ }
++ return 0;
+}
+
+static const char *discovery_bare_config_to_string(void)
@@ setup.c: static int ensure_valid_ownership(const char *path)
+ return "never";
+ case DISCOVERY_BARE_ALWAYS:
+ return "always";
-+ default:
++ case DISCOVERY_BARE_UNKNOWN:
+ BUG("invalid discovery_bare_config %d", discovery_bare_config);
+ }
++ return NULL;
+}
+
enum discovery_result {
@@ setup.c: enum discovery_result {
GIT_DIR_INVALID_GITFILE = -3,
- GIT_DIR_INVALID_OWNERSHIP = -4
+ GIT_DIR_INVALID_OWNERSHIP = -4,
-+ GIT_DIR_DISALLOWED_BARE = -5
++ GIT_DIR_DISALLOWED_BARE = -5,
};

/*
@@ setup.c: const char *setup_git_directory_gently(int *nongit_ok)
/*
* As a safeguard against setup_git_directory_gently_1 returning

- ## t/t0034-discovery-bare.sh (new) ##
+ ## t/t0035-discovery-bare.sh (new) ##
@@
+#!/bin/sh
+
@@ t/t0034-discovery-bare.sh (new)
+
+pwd="$(pwd)"
+
-+expect_allowed () {
-+ git rev-parse --absolute-git-dir >actual &&
-+ echo "$pwd/outer-repo/bare-repo" >expected &&
-+ test_cmp expected actual
-+}
-+
+expect_rejected () {
-+ test_must_fail git rev-parse --absolute-git-dir 2>err &&
++ test_must_fail git rev-parse --git-dir 2>err &&
+ grep "discovery.bare" err
+}
+
@@ t/t0034-discovery-bare.sh (new)
+test_expect_success 'discovery.bare unset' '
+ (
+ cd outer-repo/bare-repo &&
-+ expect_allowed &&
-+ cd refs/ &&
-+ expect_allowed
++ git rev-parse --git-dir
+ )
+'
+
@@ t/t0034-discovery-bare.sh (new)
+ git config --global discovery.bare always &&
+ (
+ cd outer-repo/bare-repo &&
-+ expect_allowed &&
-+ cd refs/ &&
-+ expect_allowed
++ git rev-parse --git-dir
+ )
+'
+
@@ t/t0034-discovery-bare.sh (new)
+ git config --global discovery.bare never &&
+ (
+ cd outer-repo/bare-repo &&
-+ expect_rejected &&
-+ cd refs/ &&
+ 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 &&
+ (
-+ GIT_DIR=outer-repo/bare-repo &&
-+ export GIT_DIR &&
-+ expect_allowed
++ cd outer-repo/bare-repo &&
++ test_must_fail git -c discovery.bare=always rev-parse --git-dir 2>err &&
++ grep "discovery.bare" err
+ )
+'
+

 2:  62070aab7eb < -:  ----------- setup.c: learn discovery.bareRepository=cwd
 -:  ----------- > 4:  66a0a208176 config: include "-c" in protected config
 -:  ----------- > 5:  e25d5907cd1 upload-pack: make uploadpack.packObjectsHook protected

gitgitgadget

From: Glen Choo <[email protected]>

For security reasons, some config variables are only trusted when they are specified in so-called 'protected configuration' [1]. A future commit will introduce another such config variable, so this is a good time to standardize the documentation and implementation of 'protected configuration'.

Define 'protected configuration' as global and system-level config, and mark safe.directory 'Protected config only'. In a future commit, protected configuration will also include "-c".

The following variables are intentionally not marked 'Protected config only':

- uploadpack.packObjectsHook has the same security concerns as safe.directory, but due to a different implementation, it also respects the "-c" option.

When protected configuration includes "-c", upload.packObjectsHook will be marked 'Protected config only'.

- trace2.* happens to read the same config as safe.directory because they share an implementation. However, this is not for security reasons; it is because we want to start tracing so early that repository-level config and "-c" are not available [2].

This requirement is unique to trace2.*, so it does not makes sense for protected configuration to be subject to the same constraints.

[1] For example,
https://lore.kernel.org/git/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/

---
 Documentation/config.txt           |  6 ++++++
 Documentation/config/safe.txt      | 19 ++++++++-----------
 Documentation/glossary-content.txt | 18 ++++++++++++++++++
 3 files changed, 32 insertions(+), 11 deletions(-)

index e284b042f22..07832de1a6c 100644
@@ -369,6 +369,12 @@ inventing new variables for use in your own tool, make sure their
 names do not conflict with those that are used by Git itself and
 other popular tools, and describe them in your documentation.
 
+Variables marked with '(Protected config only)' are only respected when
+they are specified in protected configuration. This includes global and
+system-level config, and excludes repository config, the command line
+option `-c`, and environment variables. For more details, see the
+'protected configuration' entry in linkgit:gitglossary[7].
+
 include::config/advice.txt[]
 
 include::config/core.txt[]

index ae0e2e3bdb4..c1caec460e8 100644
@@ -1,21 +1,18 @@
 safe.directory::
-	These config entries specify Git-tracked directories that are
-	considered safe even if they are owned by someone other than the
-	current user. By default, Git will refuse to even parse a Git
-	config of a repository owned by someone else, let alone run its
-	hooks, and this config setting allows users to specify exceptions,
-	e.g. for intentionally shared repositories (see the `--shared`
-	option in linkgit:git-init[1]).
+	'(Protected config only) ' These config entries specify
+	Git-tracked directories that are considered safe even if they
+	are owned by someone other than the current user. By default,
+	Git will refuse to even parse a Git config of a repository owned
+	by someone else, let alone run its hooks, and this config
+	setting allows users to specify exceptions, e.g. for
+	intentionally shared repositories (see the `--shared` option in
+	linkgit:git-init[1]).
 +
 This is a multi-valued setting, i.e. you can add more than one directory
 via `git config --add`. To reset the list of safe directories (e.g. to
 override any such directories specified in the system config), add a
 `safe.directory` entry with an empty value.
 +
-This config setting is only respected when specified in a system or global
-config, not when it is specified in a repository config, via the command
-line option `-c safe.directory=<path>`, or in environment variables.
-+
 The value of this setting is interpolated, i.e. `~/<path>` expands to a
 path relative to the home directory and `%(prefix)/<path>` expands to a
 path relative to Git's (runtime) prefix.

index aa2f41f5e70..a669983abd6 100644
@@ -483,6 +483,24 @@ exclude;;
 	head ref. If the remote <<def_head,head>> is not an
 	ancestor to the local head, the push fails.
 
+[[def_protected_config]]protected configuration::
+	Protected configuration is configuration that Git considers more
+	trustworthy because it is unlikely to be tampered with by an
+	attacker. For security reasons, some configuration variables are
+	only respected when they are defined in protected configuration.
++
+Protected configuration includes:
++
+- system-level config, e.g. `/etc/git/config`
+- global config, e.g. `$XDG_CONFIG_HOME/git/config` and
+  `$HOME/.gitconfig`
++
+Protected configuration excludes:
++
+- repository config, e.g. `$GIT_DIR/config` and
+  `$GIT_DIR/config.worktree`
+- the command line option `-c` and its equivalent environment variables
+
 [[def_reachable]]reachable::
 	All of the ancestors of a given <<def_commit,commit>> are said to be
 	"reachable" from that commit. More

gitgitgadget

From: Glen Choo <[email protected]>

Protected config is read using read_very_early_config(), which has several downsides:

- Every call to read_very_early_config() parses global and system-level config files anew, but this can be optimized by just parsing them once [1].
- Protected variables should respect "-c" because we can reasonably assume that it comes from the user. But, read_very_early_config() can't use "-c" because it is called so early that it does not have access to command line arguments.

Introduce git_protected_config(), which reads protected config and caches the values in the_repository.protected_config. Then, refactor safe.directory to use git_protected_config().

This implementation can still be improved, however:

- git_protected_config() iterates through every variable in the_repository.protected_config, which may still be too expensive to be called in every "git" invocation. There exist constant time lookup functions for non-protected config (repo_config_get_*()), but for simplicity, this commit does not implement similar functions for protected config.

- Protected config is stored in the_repository so that we don't need to statically allocate it. But this might be confusing since protected config ignores repository config by definition.

[1] While git_protected_config() should save on file I/O, I wasn't able to measure a meaningful difference between that and read_very_early_config() on my machine (which has an SSD).

---
 config.c     | 35 +++++++++++++++++++++++++++++++++++
 config.h     |  8 ++++++++
 repository.c |  5 +++++
 repository.h |  8 ++++++++
 setup.c      |  2 +-
 5 files changed, 57 insertions(+), 1 deletion(-)

index fa471dbdb89..c30bb7c5d09 100644
@@ -2614,6 +2614,41 @@ int repo_config_get_pathname(struct repository *repo,
 	return ret;
 }
 
+/* Read protected config into the_repository->protected_config. */
+static void read_protected_config(void)
+{
+	char *xdg_config = NULL, *user_config = NULL, *system_config = NULL;
+
+	CALLOC_ARRAY(the_repository->protected_config, 1);
+	git_configset_init(the_repository->protected_config);
+
+	system_config = git_system_config();
+	git_global_config(&user_config, &xdg_config);
+
+	git_configset_add_file(the_repository->protected_config, system_config);
+	git_configset_add_file(the_repository->protected_config, xdg_config);
+	git_configset_add_file(the_repository->protected_config, user_config);
+
+	free(system_config);
+	free(xdg_config);
+	free(user_config);
+}
+
+/* Ensure that the_repository->protected_config has been initialized. */
+static void git_protected_config_check_init(void)
+{
+	if (the_repository->protected_config &&
+	    the_repository->protected_config->hash_initialized)
+		return;
+	read_protected_config();
+}
+
+void git_protected_config(config_fn_t fn, void *data)
+{
+	git_protected_config_check_init();
+	configset_iter(the_repository->protected_config, fn, data);
+}
+
 /* Functions used historically to read configuration from 'the_repository' */
 void git_config(config_fn_t fn, void *data)
 {

index 7654f61c634..411965f52b5 100644
@@ -505,6 +505,14 @@ int repo_config_get_maybe_bool(struct repository *repo,
 int repo_config_get_pathname(struct repository *repo,
 			     const char *key, const char **dest);
 
+/*
+ * Functions for reading protected config. By definition, protected
+ * config ignores repository config, so it is unnecessary to read
+ * protected config from any `struct repository` other than
+ * the_repository.
+ */
+void git_protected_config(config_fn_t fn, void *data);
+
 /**
  * Querying For Specific Variables
  * -------------------------------

index 5d166b692c8..ec319a5e09a 100644
@@ -295,6 +295,11 @@ void repo_clear(struct repository *repo)
 		FREE_AND_NULL(repo->remote_state);
 	}
 
+	if (repo->protected_config) {
+		git_configset_clear(repo->protected_config);
+		FREE_AND_NULL(repo->protected_config);
+	}
+
 	repo_clear_path_cache(&repo->cached_paths);
 }
 

index 6cc661e5a43..24251aac553 100644
@@ -126,6 +126,14 @@ struct repository {
 
 	struct repo_settings settings;
 
+	/*
+	 * Config that comes from trusted sources, namely
+	 * - system config files (e.g. /etc/gitconfig)
+	 * - global config files (e.g. $HOME/.gitconfig,
+	 *   $XDG_CONFIG_HOME/git)
+	 */
+	struct config_set *protected_config;
+
 	/* Subsystems */
 	/*
 	 * Repository's config which contains key-value pairs from the usual

index f818dd858c6..847d47f9195 100644
@@ -1128,7 +1128,7 @@ static int ensure_valid_ownership(const char *path)
 	    is_path_owned_by_current_user(path))
 		return 1;
 
-	read_very_early_config(safe_directory_cb, &data);
+	git_protected_config(safe_directory_cb, &data);
 
 	return data.is_safe;
 }

gitgitgadget

From: Glen Choo <[email protected]>

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/[email protected]
[2]: https://lore.kernel.org/git/[email protected]

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

index 07832de1a6c..34133288d75 100644
@@ -415,6 +415,8 @@ include::config/diff.txt[]
 
 include::config/difftool.txt[]
 
+include::config/discovery.txt[]
+
 include::config/extensions.txt[]
 
 include::config/fastimport.txt[]

new file mode 100644
index 00000000000..fbe93597e7c
@@ -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.

gitgitgadget

From: Glen Choo <[email protected]>

Now that protected config includes "-c", "uploadpack.packObjectsHook" behaves identically to a 'Protected config only' variable. Refactor it to use git_protected_config() and mark it 'Protected config only'.

---
 Documentation/config/uploadpack.txt | 22 +++++++++-------------
 upload-pack.c                       | 17 +++++++++++------
 2 files changed, 20 insertions(+), 19 deletions(-)

index 32fad5bbe81..57e5e021323 100644
@@ -39,19 +39,15 @@ uploadpack.keepAlive::
 	disables keepalive packets entirely. The default is 5 seconds.
 
 uploadpack.packObjectsHook::
-	If this option is set, when `upload-pack` would run
-	`git pack-objects` to create a packfile for a client, it will
-	run this shell command instead.  The `pack-objects` command and
-	arguments it _would_ have run (including the `git pack-objects`
-	at the beginning) are appended to the shell command. The stdin
-	and stdout of the hook are treated as if `pack-objects` itself
-	was run. I.e., `upload-pack` will feed input intended for
-	`pack-objects` to the hook, and expects a completed packfile on
-	stdout.
-+
-Note that this configuration variable is ignored if it is seen in the
-repository-level config (this is a safety measure against fetching from
-untrusted repositories).
+	'(Protected config only)' If this option is set, when
+	`upload-pack` would run `git pack-objects` to create a packfile
+	for a client, it will run this shell command instead. The
+	`pack-objects` command and arguments it _would_ have run
+	(including the `git pack-objects` at the beginning) are appended
+	to the shell command. The stdin and stdout of the hook are
+	treated as if `pack-objects` itself was run. I.e., `upload-pack`
+	will feed input intended for `pack-objects` to the hook, and
+	expects a completed packfile on stdout.
 
 uploadpack.allowFilter::
 	If this option is set, `upload-pack` will support partial

index 3a851b36066..2a39391369d 100644
@@ -1321,18 +1321,21 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data)
 		data->advertise_sid = git_config_bool(var, value);
 	}
 
-	if (current_config_scope() != CONFIG_SCOPE_LOCAL &&
-	    current_config_scope() != CONFIG_SCOPE_WORKTREE) {
-		if (!strcmp("uploadpack.packobjectshook", var))
-			return git_config_string(&data->pack_objects_hook, var, value);
-	}
-
 	if (parse_object_filter_config(var, value, data) < 0)
 		return -1;
 
 	return parse_hide_refs_config(var, value, "uploadpack");
 }
 
+static int upload_pack_protected_config(const char *var, const char *value, void *cb_data)
+{
+	struct upload_pack_data *data = cb_data;
+
+	if (!strcmp("uploadpack.packobjectshook", var))
+		return git_config_string(&data->pack_objects_hook, var, value);
+	return 0;
+}
+
 void upload_pack(const int advertise_refs, const int stateless_rpc,
 		 const int timeout)
 {

@@ -1342,6 +1345,7 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,
 	upload_pack_data_init(&data);
 
 	git_config(upload_pack_config, &data);
+	git_protected_config(upload_pack_protected_config, &data);
 
 	data.stateless_rpc = stateless_rpc;
 	data.timeout = timeout;

@@ -1697,6 +1701,7 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request)
 	data.use_sideband = LARGE_PACKET_MAX;
 
 	git_config(upload_pack_config, &data);
+	git_protected_config(upload_pack_protected_config, &data);
 
 	while (state != FETCH_DONE) {
 		switch (state) {

gitgitgadget

From: Glen Choo <[email protected]>

Protected config should include the command line (aka "-c") because we can be quite certain that this config is specified by the user.

Introduce a function, git_configset_add_parameters(), that adds "-c" config to a config_set, and use it to add "-c" to protected config.

---
 Documentation/config.txt           |  6 +++---
 Documentation/glossary-content.txt |  2 +-
 config.c                           |  6 ++++++
 config.h                           |  9 +++++++++
 t/t0033-safe-directory.sh          | 24 ++++++++++--------------
 t/t0035-discovery-bare.sh          |  3 +--
 6 files changed, 30 insertions(+), 20 deletions(-)

index 34133288d75..f40a3e297ce 100644
@@ -370,9 +370,9 @@ names do not conflict with those that are used by Git itself and
 other popular tools, and describe them in your documentation.
 
 Variables marked with '(Protected config only)' are only respected when
-they are specified in protected configuration. This includes global and
-system-level config, and excludes repository config, the command line
-option `-c`, and environment variables. For more details, see the
+they are specified in protected configuration. This includes global,
+system-level config, the command line option `-c`, and environment
+variables, and excludes repository config. For more details, see the
 'protected configuration' entry in linkgit:gitglossary[7].
 
 include::config/advice.txt[]

index a669983abd6..4190c410a00 100644
@@ -494,12 +494,12 @@ Protected configuration includes:
 - system-level config, e.g. `/etc/git/config`
 - global config, e.g. `$XDG_CONFIG_HOME/git/config` and
   `$HOME/.gitconfig`
+- the command line option `-c` and its equivalent environment variables
 +
 Protected configuration excludes:
 +
 - repository config, e.g. `$GIT_DIR/config` and
   `$GIT_DIR/config.worktree`
-- the command line option `-c` and its equivalent environment variables
 
 [[def_reachable]]reachable::
 	All of the ancestors of a given <<def_commit,commit>> are said to be

index c30bb7c5d09..22192ca1d63 100644
@@ -2373,6 +2373,11 @@ int git_configset_add_file(struct config_set *cs, const char *filename)
 	return git_config_from_file(config_set_callback, filename, cs);
 }
 
+int git_configset_add_parameters(struct config_set *cs)
+{
+	return git_config_from_parameters(config_set_callback, cs);
+}
+
 int git_configset_get_value(struct config_set *cs, const char *key, const char **value)
 {
 	const struct string_list *values = NULL;

@@ -2628,6 +2633,7 @@ static void read_protected_config(void)
 	git_configset_add_file(the_repository->protected_config, system_config);
 	git_configset_add_file(the_repository->protected_config, xdg_config);
 	git_configset_add_file(the_repository->protected_config, user_config);
+	git_configset_add_parameters(the_repository->protected_config);
 
 	free(system_config);
 	free(xdg_config);

index 411965f52b5..e3ff1fcf683 100644
@@ -446,6 +446,15 @@ void git_configset_init(struct config_set *cs);
  */
 int git_configset_add_file(struct config_set *cs, const char *filename);
 
+/**
+ * Parses command line options and environment variables, and adds the
+ * variable-value pairs to the `config_set`. Returns 0 on success, or -1
+ * if there is an error in parsing. The caller decides whether to free
+ * the incomplete configset or continue using it when the function
+ * returns -1.
+ */
+int git_configset_add_parameters(struct config_set *cs);
+
 /**
  * Finds and returns the value list, sorted in order of increasing priority
  * for the configuration variable `key` and config set `cs`. When the

index 238b25f91a3..5a1cd0d0947 100755
@@ -16,24 +16,20 @@ test_expect_success 'safe.directory is not set' '
 	expect_rejected_dir
 '
 
-test_expect_success 'ignoring safe.directory on the command line' '
-	test_must_fail git -c safe.directory="$(pwd)" status 2>err &&
-	grep "unsafe repository" err
+test_expect_success 'safe.directory on the command line' '
+	git -c safe.directory="$(pwd)" status
 '
 
-test_expect_success 'ignoring safe.directory in the environment' '
-	test_must_fail env GIT_CONFIG_COUNT=1 \
-		GIT_CONFIG_KEY_0="safe.directory" \
-		GIT_CONFIG_VALUE_0="$(pwd)" \
-		git status 2>err &&
-	grep "unsafe repository" err
+test_expect_success 'safe.directory in the environment' '
+	env GIT_CONFIG_COUNT=1 \
+	    GIT_CONFIG_KEY_0="safe.directory" \
+	    GIT_CONFIG_VALUE_0="$(pwd)" \
+	    git status
 '
 
-test_expect_success 'ignoring safe.directory in GIT_CONFIG_PARAMETERS' '
-	test_must_fail env \
-		GIT_CONFIG_PARAMETERS="${SQ}safe.directory${SQ}=${SQ}$(pwd)${SQ}" \
-		git status 2>err &&
-	grep "unsafe repository" err
+test_expect_success 'safe.directory in GIT_CONFIG_PARAMETERS' '
+	env GIT_CONFIG_PARAMETERS="${SQ}safe.directory${SQ}=${SQ}$(pwd)${SQ}" \
+	    git status
 '
 
 test_expect_success 'ignoring safe.directory in repo config' '

index 94c2f76d774..0d5983df307 100755
@@ -56,8 +56,7 @@ 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
+		git -c discovery.bare=always rev-parse --git-dir
 	)
 '
 

gitgitgadget

Now we are talking about protected "variable". Is that a synonym for "config", or are there some distinctions between them?

Yes, it indeed is. Is it because we were over-eager when we introduced the "struct repository *repo" parameter to many functions and the configuration system wants you to have some repository, even when you know you are not reading from any repository?

I am wondering if it is a cleaner solution *not* to hang the protected config as a configset in the_repository, but keep the configset as a separate global variable, perhaps static to config.c and is meant to be only accessed via git_protected_config() and the like.

Sorry, that's an old term I was toying with (this somehow snuck through my proofreading). I just meant "variable that is only read from protected config", aka a "protected config only variable".

A goal in this version was to introduce as little jargon as possible, so
- "protected config" refers to the set of config sources, and
- "protected config only" refers to config variables/settings that are only read from protected config.

Ah no, I was just trying to avoid yet-another global variable (since IIRC we want to move towards a more lib-like Git), and the_repository was a convenient global variable to (ab)use.

I think your suggestion to use a global variable is better, as much as I want to avoid another global variable. Protected config would affect any repositories that we work with in-core, so using a global sounds ok.

environment.c might be a better place since we already make a concerted effort to put global config variables there instead of config.c.

As an aside, I wonder how we could get rid of all of the globals in environment.c in the long term. Maybe we would have yet-another all encompassing global, the_environment, and then figure out which variables belong to the repository and which belong to the environment.

Yes.

I'll have to think about this some more. My initial thoughts are that we should do this if we formalize 'protected' as a scope-like concept, but I don't see the lack of "--protected" as a significant hindrance to users because they can use "--global" and "--system" (albeit in two invocations instead of one).

Ah.. I think it would be best to pretend that the "Protected variable" typo never happened. That term was destined to be confusing and meaningless.

Instead, we can use "protected config" to refer to the config and "protected config only" to refer to variables. Since "protected config" is defined as (global + system + CLI) config, then yes, we would say that it is "protected config". But since we do not enforce that "user.name" _must_ come from only protected config, it is not "protected config only".

OK. Let's have such a clear pair of definitions somewhere in the doc or at least in a proposed log message.

If this does not have to be known only inside config.c, until we introduce a more global bag of things, which may have the current the_repository as one of its components, I do not think it hurts to have a file-scope static there. Then, perhaps git_configset_get*() helper functions can recognize cs==NULL as a sign that the caller wants to grab from the "protected config", or something? If we do not want to expose the underying global variable to the public, that is.

I think we are on the same page, we'd probably need something called the_world ;-)

Very clear. Thanks.

I think the answers to these two questions is "yes", so they can be turned into an affirmative sentence:

We do not event try to protect ourselves from those who can write anywhere...

This requires an example:

Some workflows use repositories stored in shared directories, which are writable by multiple unprivileged users.

With this argument, I agree that -c config can be considered protected. At the very least, it is visible to the user when they are running a command. This would unify our expectations with uploadPack.packObjectsHook, too.

Thanks,
-Stolee

I originally thought that we should jump to that "right" solution, but the existing logic in ensure_valid_ownership() uses the iterator method, mostly because it uses a multi-valued string. There are helpers that allow iterating over a specific multi-valued key, but there is no reason to complicate the current patch with that amount of refactoring. That can be handled as a completely separate topic.

I agree with Junio's suggestion of keeping this as a static global in config.c, accessible only by the public methods from config.h. A future where we have "the_world" might be nice for inventory on all these globals. Definitely not something to hold up this series.

I'm really glad to see this simplification at the end of your series.

...

{
	git_config(upload_pack_config, data);
	git_protected_config(upload_pack_protected_config, data);
}

s/event/even/.

Hmph, "... and we do not trust these colleagues"? It might be true, but sounds a bit weak rationale, at least to me. A natural reaction coming form a devil's advocate naïve me would be "well, then I would not be directly interacting with such a repository; I'd work in a clone of it of my own, and pull and push as needed".

Isn't the reason more like "users may go spelunking random places in the filesystem, with PS1 settings and the like that causes some "git" command invoked automatically in their current directory, and we want to protect these users from getting harmed by a random repository with hostile contents in their configuration and hooks without even realizing they have wandered into such a repository"?

Yup, that matches my understanding.

In any case, I'd prefer to see not just the definition but the reasoning behind the decision that made some "protected" while leaving others not-"protected" clearly documented to help users.

Thanks.

Hm, this is my understanding as well, i.e. safe.directory is meant to protect you from shared repositories that you didn't expect, but it lets you trust the shared repositories that you need (and there is no protection once you decide to trust the repo).

Silly typo. Thanks for the catch :)

Good point. I think using scope would be a lot clearer, and maybe I will consider s/protected configuration/protected scope. I'm hesitant to call the scope "trusted", because I don't want to insinuate that repository config is "untrusted" since we _do_ trust it in most cases.

I don't think Documentation/git-config.txt has adequately defined what a 'scope' is though, even though scopes have been with us since 9acc591111 (config: add a notion of "scope", 2016-05-18). The best I could find is "--show-scope", introduced in 145d59f482 (config: add '--show-scope' to print the scope of a config value, 2020-02-10), which mentions scopes but doesn't link the idea back to the specific files or CLI options ("--system", "--global", etc).

So I'll see if I can improve the docs around scopes since that will help the language in this patch.

From: Glen Choo <[email protected]>

Use git_protected_config() to read safe.directory instead of read_very_early_config(), making it 'protected config only'. As a result, safe.directory now respects "-c", so update the tests and docs accordingly.

---
 Documentation/config/safe.txt |  6 +++---
 setup.c                       |  2 +-
 t/t0033-safe-directory.sh     | 24 ++++++++++--------------
 3 files changed, 14 insertions(+), 18 deletions(-)

index ae0e2e3bdb4..2a7d2324250 100644
@@ -12,9 +12,9 @@ via `git config --add`. To reset the list of safe directories (e.g. to
 override any such directories specified in the system config), add a
 `safe.directory` entry with an empty value.
 +
-This config setting is only respected when specified in a system or global
-config, not when it is specified in a repository config, via the command
-line option `-c safe.directory=<path>`, or in environment variables.
+This config setting is only respected in protected configuration (see
+<<SCOPES>>). This prevents the untrusted repository from tampering with this
+value.
 +
 The value of this setting is interpolated, i.e. `~/<path>` expands to a
 path relative to the home directory and `%(prefix)/<path>` expands to a

index f818dd858c6..847d47f9195 100644
@@ -1128,7 +1128,7 @@ static int ensure_valid_ownership(const char *path)
 	    is_path_owned_by_current_user(path))
 		return 1;
 
-	read_very_early_config(safe_directory_cb, &data);
+	git_protected_config(safe_directory_cb, &data);
 
 	return data.is_safe;
 }

index 238b25f91a3..5a1cd0d0947 100755
@@ -16,24 +16,20 @@ test_expect_success 'safe.directory is not set' '
 	expect_rejected_dir
 '
 
-test_expect_success 'ignoring safe.directory on the command line' '
-	test_must_fail git -c safe.directory="$(pwd)" status 2>err &&
-	grep "unsafe repository" err
+test_expect_success 'safe.directory on the command line' '
+	git -c safe.directory="$(pwd)" status
 '
 
-test_expect_success 'ignoring safe.directory in the environment' '
-	test_must_fail env GIT_CONFIG_COUNT=1 \
-		GIT_CONFIG_KEY_0="safe.directory" \
-		GIT_CONFIG_VALUE_0="$(pwd)" \
-		git status 2>err &&
-	grep "unsafe repository" err
+test_expect_success 'safe.directory in the environment' '
+	env GIT_CONFIG_COUNT=1 \
+	    GIT_CONFIG_KEY_0="safe.directory" \
+	    GIT_CONFIG_VALUE_0="$(pwd)" \
+	    git status
 '
 
-test_expect_success 'ignoring safe.directory in GIT_CONFIG_PARAMETERS' '
-	test_must_fail env \
-		GIT_CONFIG_PARAMETERS="${SQ}safe.directory${SQ}=${SQ}$(pwd)${SQ}" \
-		git status 2>err &&
-	grep "unsafe repository" err
+test_expect_success 'safe.directory in GIT_CONFIG_PARAMETERS' '
+	env GIT_CONFIG_PARAMETERS="${SQ}safe.directory${SQ}=${SQ}$(pwd)${SQ}" \
+	    git status
 '
 
 test_expect_success 'ignoring safe.directory in repo config' '

gitgitgadget

From: Glen Choo <[email protected]>

uploadpack.packObjectsHook is the only 'protected config only' variable today, but we've noted that safe.directory and the upcoming discovery.bare should also be 'protected config only'. So, for consistency, we'd like to have a single implementation for protected config.

The primary constraints are:

1. Reading from protected config should be as fast as possible. Nearly all "git" commands inside a bare repository will read both safe.directory and discovery.bare, so we cannot afford to be slow.

2. Protected config must be readable when the gitdir is not known. safe.directory and discovery.bare both affect repository discovery and the gitdir is not known at that point [1].

The chosen implementation in this commit is to read protected config and cache the values in a global configset. This is similar to the caching behavior we get with the_repository->config.

Introduce git_protected_config(), which reads protected config and caches them in the global configset protected_config. Then, refactor uploadpack.packObjectsHook to use git_protected_config().

The protected config functions are named similarly to their non-protected counterparts, e.g. git_protected_config_check_init() vs git_config_check_init().

In light of constraint 1, this implementation can still be improved since git_protected_config() iterates through every variable in protected_config, which may still be too expensive. There exist constant time lookup functions for non-protected config (repo_config_get_*()), but for simplicity, this commit does not implement similar functions for protected config.

An alternative that avoids introducing another configset is to continue to read all config using git_config(), but only accept values that have the correct config scope [2]. This technically fulfills constraint 2, because git_config() simply ignores the local and worktree config when the gitdir is not known. However, this would read incomplete config into the_repository->config, which would need to be reset when the gitdir is known and git_config() needs to read the local and worktree config. Resetting the_repository->config might be reasonable while we only have these 'protected config only' variables, but it's not clear whether this extends well to future variables.

[1] In this case, we do have a candidate gitdir though, so with a little refactoring, it might be possible to provide a gitdir.
[2] This is how uploadpack.packObjectsHook was implemented prior to this commit.

---
 config.c                     | 51 ++++++++++++++++++++++++++++++++++++
 config.h                     | 17 ++++++++++++
 t/t5544-pack-objects-hook.sh |  7 ++++-
 upload-pack.c                | 27 ++++++++++++-------
 4 files changed, 91 insertions(+), 11 deletions(-)

index fa471dbdb89..56b7ed5ffe8 100644
@@ -81,6 +81,18 @@ static enum config_scope current_parsing_scope;
 static int pack_compression_seen;
 static int zlib_compression_seen;
 
+/*
+ * Config that comes from trusted sources, namely:
+ * - system config files (e.g. /etc/gitconfig)
+ * - global config files (e.g. $HOME/.gitconfig,
+ *   $XDG_CONFIG_HOME/git)
+ * - the command line.
+ *
+ * This is declared here for code cleanliness, but unlike the other
+ * static variables, this does not hold config parser state.
+ */
+static struct config_set protected_config;
+
 static int config_file_fgetc(struct config_source *conf)
 {
 	return getc_unlocked(conf->u.file);

@@ -2373,6 +2385,11 @@ int git_configset_add_file(struct config_set *cs, const char *filename)
 	return git_config_from_file(config_set_callback, filename, cs);
 }
 
+int git_configset_add_parameters(struct config_set *cs)
+{
+	return git_config_from_parameters(config_set_callback, cs);
+}
+
 int git_configset_get_value(struct config_set *cs, const char *key, const char **value)
 {
 	const struct string_list *values = NULL;

@@ -2614,6 +2631,40 @@ int repo_config_get_pathname(struct repository *repo,
 	return ret;
 }
 
+/* Read values into protected_config. */
+static void read_protected_config(void)
+{
+	char *xdg_config = NULL, *user_config = NULL, *system_config = NULL;
+
+	git_configset_init(&protected_config);
+
+	system_config = git_system_config();
+	git_global_config(&user_config, &xdg_config);
+
+	git_configset_add_file(&protected_config, system_config);
+	git_configset_add_file(&protected_config, xdg_config);
+	git_configset_add_file(&protected_config, user_config);
+	git_configset_add_parameters(&protected_config);
+
+	free(system_config);
+	free(xdg_config);
+	free(user_config);
+}
+
+/* Ensure that protected_config has been initialized. */
+static void git_protected_config_check_init(void)
+{
+	if (protected_config.hash_initialized)
+		return;
+	read_protected_config();
+}
+
+void git_protected_config(config_fn_t fn, void *data)
+{
+	git_protected_config_check_init();
+	configset_iter(&protected_config, fn, data);
+}
+
 /* Functions used historically to read configuration from 'the_repository' */
 void git_config(config_fn_t fn, void *data)
 {

index 7654f61c634..e3ff1fcf683 100644
@@ -446,6 +446,15 @@ void git_configset_init(struct config_set *cs);
  */
 int git_configset_add_file(struct config_set *cs, const char *filename);
 
+/**
+ * Parses command line options and environment variables, and adds the
+ * variable-value pairs to the `config_set`. Returns 0 on success, or -1
+ * if there is an error in parsing. The caller decides whether to free
+ * the incomplete configset or continue using it when the function
+ * returns -1.
+ */
+int git_configset_add_parameters(struct config_set *cs);
+
 /**
  * Finds and returns the value list, sorted in order of increasing priority
  * for the configuration variable `key` and config set `cs`. When the

@@ -505,6 +514,14 @@ int repo_config_get_maybe_bool(struct repository *repo,
 int repo_config_get_pathname(struct repository *repo,
 			     const char *key, const char **dest);
 
+/*
+ * Functions for reading protected config. By definition, protected
+ * config ignores repository config, so it is unnecessary to read
+ * protected config from any `struct repository` other than
+ * the_repository.
+ */
+void git_protected_config(config_fn_t fn, void *data);
+
 /**
  * Querying For Specific Variables
  * -------------------------------

index dd5f44d986f..54f54f8d2eb 100755
@@ -56,7 +56,12 @@ test_expect_success 'hook does not run from repo config' '
 	! grep "hook running" stderr &&
 	test_path_is_missing .git/hook.args &&
 	test_path_is_missing .git/hook.stdin &&
-	test_path_is_missing .git/hook.stdout
+	test_path_is_missing .git/hook.stdout &&
+
+	# check that global config is used instead
+	test_config_global uploadpack.packObjectsHook ./hook &&
+	git clone --no-local . dst2.git 2>stderr &&
+	grep "hook running" stderr
 '
 
 test_expect_success 'hook works with partial clone' '

index 3a851b36066..09f48317b02 100644
@@ -1321,18 +1321,27 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data)
 		data->advertise_sid = git_config_bool(var, value);
 	}
 
-	if (current_config_scope() != CONFIG_SCOPE_LOCAL &&
-	    current_config_scope() != CONFIG_SCOPE_WORKTREE) {
-		if (!strcmp("uploadpack.packobjectshook", var))
-			return git_config_string(&data->pack_objects_hook, var, value);
-	}
-
 	if (parse_object_filter_config(var, value, data) < 0)
 		return -1;
 
 	return parse_hide_refs_config(var, value, "uploadpack");
 }
 
+static int upload_pack_protected_config(const char *var, const char *value, void *cb_data)
+{
+	struct upload_pack_data *data = cb_data;
+
+	if (!strcmp("uploadpack.packobjectshook", var))
+		return git_config_string(&data->pack_objects_hook, var, value);
+	return 0;
+}
+
+static void get_upload_pack_config(struct upload_pack_data *data)
+{
+	git_config(upload_pack_config, data);
+	git_protected_config(upload_pack_protected_config, data);
+}
+
 void upload_pack(const int advertise_refs, const int stateless_rpc,
 		 const int timeout)
 {

@@ -1340,8 +1349,7 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,
 	struct upload_pack_data data;
 
 	upload_pack_data_init(&data);
-
-	git_config(upload_pack_config, &data);
+	get_upload_pack_config(&data);
 
 	data.stateless_rpc = stateless_rpc;
 	data.timeout = timeout;

@@ -1695,8 +1703,7 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request)
 
 	upload_pack_data_init(&data);
 	data.use_sideband = LARGE_PACKET_MAX;
-
-	git_config(upload_pack_config, &data);
+	get_upload_pack_config(&data);
 
 	while (state != FETCH_DONE) {
 		switch (state) {

gitgitgadget

From: Glen Choo <[email protected]>

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/[email protected]
[2]: https://lore.kernel.org/git/[email protected]

---
 Documentation/config.txt           |  2 +
 Documentation/config/discovery.txt | 19 +++++++++
 setup.c                            | 57 ++++++++++++++++++++++++-
 t/t0035-discovery-bare.sh          | 68 ++++++++++++++++++++++++++++++
 4 files changed, 145 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/config/discovery.txt
 create mode 100755 t/t0035-discovery-bare.sh

index e284b042f22..9a5e1329772 100644
@@ -409,6 +409,8 @@ include::config/diff.txt[]
 
 include::config/difftool.txt[]
 
+include::config/discovery.txt[]
+
 include::config/extensions.txt[]
 
 include::config/fastimport.txt[]

new file mode 100644
index 00000000000..fbe93597e7c
@@ -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.

gitgitgadget

Thanks again for the kind feedback, everyone :)

The motivation has remained the same as the last round; you can find it in the "Description" section in the previous cover letter [1].

This round doesn't introduce any major code changes. The most notable changes are:

* I've reorganized the patches so that protected config includes "-c" from the beginning (3/5) (instead of trying to avoid changing safe.directory). As a result, uploadpack.packObjectsHook becomes the first 'protected config only' variable instead of safe.directory.

Since we start the conversion with uploadpack.packObjectHook, I was curious whether we might be able to reuse its approach of "reading the full set of config, but checking the scope of each value", which might be nice because we could reuse the cache in the_repository->config instead of creating an entirely new configset. I didn't pursue it further, but I've noted this alternative in 3/5's commit message.

* 'Protected configuration' is now defined as a set of configuration scopes (2/5). This follows a suggestion from Junio [2], which I thought read very clearly. We haven't done a great job at describing 'scopes' in Documentation/git-config.txt though, so I tried to remedy that by cleaning up a little and adding a SCOPES section (1/5).

Frankly, I'm not very happy with the end result - it's not nearly as clear as I had hoped, and I think I might be introducing some confusion between the words "config" and "scope". I'd appreciate any feedback that helps us get to a good final wording.

* I added a test for "git config --show-scope" and the 'worktree' scope, since 'worktree' wasn't listed in Documentation/git-config.txt (6/5).

= Patch organization

* Patch 1 add a section on config scopes to our docs
* Patches 2-3 define 'protected config' and create a shared implementation.
* Patch 4 refactors safe.directory to use protected config
* Patch 5 adds discovery.bare

= Series history

Changes in v4:

* 2/5's commit message now justifies what scopes are included in protected config
* The global configset is now a file-scope static inside config.c (previously it was a member of the_repository).
* Rename discovery_bare_config to discovery_bare_allowed
* Make discovery_bare_allowed function-scoped (instead of global).
* Add an expect_accepted helper to the discovery.bare tests.
* Add a helper to "upload-pack" that reads the protected and non-protected config

Changes in v3:

* Rebase onto a more recent 'master'
* Reframe this feature in only in terms of the 'embedded bare repo' attack.
* Other docs improvements (thanks Stolee in particular!)
* Protected config no longer uses read_very_early_config() and is only read once
* Protected config now includes "-c"
* uploadpack.packObjectsHook now uses protected config instead of ignoring repo config using config scopes

Changes in v2:

* Rename safe.barerepository to discovery.bare and make it die()
* Move tests into t/t0034-discovery-bare.sh
* Avoid unnecessary config reading by using a static variable
* Add discovery.bare=cwd
* Fix typos

= Future work

* This series does not implement the "no-embedded" option [3] and I won't work on it any time soon, but I'd be more than happy to review if someone sends patches.
* With discovery.bare, if a builtin is marked RUN_SETUP_GENTLY, setup.c doesn't die() and we don't tell users why their repository was rejected, e.g. "git config" gives an opaque "fatal: not in a git directory". This isn't a new problem though, since safe.directory has the same issue.

[1]
https://lore.kernel.org/git/[email protected]
[2] https://lore.kernel.org/git/[email protected] [3] This was first suggested in
https://lore.kernel.org/git/[email protected]

Glen Choo (5):
Documentation/git-config.txt: add SCOPES section
Documentation: define protected configuration
config: read protected config with git_protected_config() safe.directory: use git_protected_config() setup.c: create discovery.bare

 Documentation/config.txt            |  2 +
 Documentation/config/discovery.txt  | 19 +++++++
 Documentation/config/safe.txt       |  6 +--
 Documentation/config/uploadpack.txt |  6 +--
 Documentation/git-config.txt        | 77 +++++++++++++++++++++++------
 config.c                            | 51 +++++++++++++++++++
 config.h                            | 17 +++++++
 setup.c                             | 59 +++++++++++++++++++++-
 t/t0033-safe-directory.sh           | 24 ++++-----
 t/t0035-discovery-bare.sh           | 68 +++++++++++++++++++++++++
 t/t5544-pack-objects-hook.sh        |  7 ++-
 upload-pack.c                       | 27 ++++++----
 12 files changed, 316 insertions(+), 47 deletions(-)
 create mode 100644 Documentation/config/discovery.txt
 create mode 100755 t/t0035-discovery-bare.sh

base-commit: f9b95943b68b6b8ca5a6072f50a08411c6449b55
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1261%2Fchooglen%2Fsetup%2Fdisable-bare-repo-config-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1261/chooglen/setup/disable-bare-repo-config-v4
Pull-Request: https://github.com/git/git/pull/1261

Range-diff vs v3:

 1:  575676c760d < -:  ----------- Documentation: define protected configuration
 -:  ----------- > 1:  c0e27ab3b3e Documentation/git-config.txt: add SCOPES section
 5:  e25d5907cd1 ! 2:  a5a1dcb03e1 upload-pack: make uploadpack.packObjectsHook protected

@@ Metadata
Author: Glen Choo <[email protected]>

## Commit message ##
- upload-pack: make uploadpack.packObjectsHook protected
+ Documentation: define protected configuration

- Now that protected config includes "-c", "uploadpack.packObjectsHook"
- behaves identically to a 'Protected config only' variable. Refactor it
- to use git_protected_config() and mark it 'Protected config only'.
+ For security reasons, there are config variables that are only trusted
+ when they are specified in extra-trustworthy configuration scopes, which
+ are sometimes referred to on-list as 'protected configuration' [1]. A
+ future commit will introduce another such variable, so let's define our
+ terms so that we can have consistent documentation and implementation.
+
+ In our documentation, define 'protected config' as the system, global
+ and command config scopes. As a shorthand, I will refer to variables
+ that are only respected in protected config as 'protected config only',
+ but this term is not used in the documentation.
+
+ This definition of protected configuration is based on whether or not
+ Git can reasonably protect the user by ignoring the configuration scope:
+
+ - System, global and command line config are considered protected
+ because an attacker who has control over any of those can do plenty of
+ harm without Git, so we gain very little by ignoring those scopes.
+ - On the other hand, local (and similarly, worktree) config are not
+ considered protected because it is relatively easy for an attacker to
+ control local config, e.g.:
+ - On some shared user environments, a non-admin attacker can create a
+ repository high up the directory hierarchy (e.g. C:\.git on Windows),
+ and a user may accidentally use it when their PS1 automatically
+ invokes "git" commands.
+
+ safe.directory prevents attacks of this form by making sure that
+ the user intended to use the shared repository. It obviously
+ shouldn't be read from the repository, because that would end up
+ trusting the repository that Git was supposed to reject.
+ - "git upload-pack" is expected to run in repositories that may not be
+ controlled by the user. We cannot ignore all config in that
+ repository (because "git upload-pack" would fail), but we can limit
+ the risks by ignoring uploadpack.packObjectsHook.
+
+ Only uploadpack.packObjectsHook is 'protected config only'. The
+ following variables are intentionally excluded:
+
+ - safe.directory should be 'protected config only', but it does not
+ technically fit the definition because it is not respected in the
+ "command" scope. A future commit will fix this.
+
+ - trace2.* happens to read the same scopes as safe.directory because
+ they share an implementation. However, this is not for security
+ reasons; it is because we want to start tracing so early that
+ repository-level config and "-c" are not available [2].
+
+ This requirement is unique to trace2.*, so it does not makes sense
+ for protected configuration to be subject to the same constraints.
+
+ [1] For example,
+ https://lore.kernel.org/git/[email protected]/
+ [2] https://lore.kernel.org/git/[email protected]/

## Documentation/config/uploadpack.txt ##
-@@ Documentation/config/uploadpack.txt: uploadpack.keepAlive::
- disables keepalive packets entirely. The default is 5 seconds.
-
- uploadpack.packObjectsHook::
-- If this option is set, when upload-pack would run
-- git pack-objects to create a packfile for a client, it will
-- run this shell command instead. The pack-objects command and
-- arguments it _would_ have run (including the git pack-objects
-- at the beginning) are appended to the shell command. The stdin
-- and stdout of the hook are treated as if pack-objects itself
-- was run. I.e., upload-pack will feed input intended for
-- pack-objects to the hook, and expects a completed packfile on
-- stdout.
--+
+@@ Documentation/config/uploadpack.txt: uploadpack.packObjectsHook::
+ pack-objects to the hook, and expects a completed packfile on
+ stdout.
+ +
-Note that this configuration variable is ignored if it is seen in the -repository-level config (this is a safety measure against fetching from -untrusted repositories).
-+ '(Protected config only)' If this option is set, when
-+ upload-pack would run git pack-objects to create a packfile
-+ for a client, it will run this shell command instead. The
-+ pack-objects command and arguments it _would_ have run
-+ (including the git pack-objects at the beginning) are appended
-+ to the shell command. The stdin and stdout of the hook are
-+ treated as if pack-objects itself was run. I.e., upload-pack
-+ will feed input intended for pack-objects to the hook, and
-+ expects a completed packfile on stdout.
++Note that this configuration variable is only respected when it is specified ++in protected config (see <<SCOPES>>). This is a safety measure against ++fetching from untrusted repositories.

uploadpack.allowFilter::
If this option is set, upload-pack will support partial

- ## upload-pack.c ##
-@@ upload-pack.c: static int upload_pack_config(const char *var, const char *value, void *cb_data)
- data->advertise_sid = git_config_bool(var, value);
- }
+ ## Documentation/git-config.txt ##
+@@ Documentation/git-config.txt: You can change the way options are read/written by specifying the path to a
+ file (--file), or by specifying a configuration scope (--system,
+ --global, --local, --worktree); see <<OPTIONS>> above.

-- if (current_config_scope() != CONFIG_SCOPE_LOCAL &&
-- current_config_scope() != CONFIG_SCOPE_WORKTREE) {
-- if (!strcmp("uploadpack.packobjectshook", var))
-- return git_config_string(&data->pack_objects_hook, var, value);
-- }
--
- if (parse_object_filter_config(var, value, data) < 0)
- return -1;
++[[SCOPES]]
+ SCOPES
+ ------

- return parse_hide_refs_config(var, value, "uploadpack");
- }
+@@ Documentation/git-config.txt: Most configuration options are respected regardless of the scope it is
+ defined in, but some options are only respected in certain scopes. See the
+ option's documentation for the full details.

-+static int upload_pack_protected_config(const char *var, const char *value, void *cb_data) -+{
-+ struct upload_pack_data *data = cb_data;
++Protected config
++~~~~~~~~~~~~~~~~
+
-+ if (!strcmp("uploadpack.packobjectshook", var))
-+ return git_config_string(&data->pack_objects_hook, var, value);
-+ return 0;
-+}
++Protected config refers to the 'system', 'global', and 'command' scopes. Git ++considers these scopes to be especially trustworthy because they are likely ++to be controlled by the user or a trusted administrator. An attacker who ++controls these scopes can do substantial harm without using Git, so it is ++assumed that the user's environment protects these scopes against attackers.
+
- void upload_pack(const int advertise_refs, const int stateless_rpc,
- const int timeout)
- {
-@@ upload-pack.c: void upload_pack(const int advertise_refs, const int stateless_rpc,
- upload_pack_data_init(&data);
-
- git_config(upload_pack_config, &data);
-+ git_protected_config(upload_pack_protected_config, &data);
-
- data.stateless_rpc = stateless_rpc;
- data.timeout = timeout;
-@@ upload-pack.c: int upload_pack_v2(struct repository *r, struct packet_reader *request)
- data.use_sideband = LARGE_PACKET_MAX;
-
- git_config(upload_pack_config, &data);
-+ git_protected_config(upload_pack_protected_config, &data);
++For security reasons, certain options are only respected when they are ++specified in protected config, and ignored otherwise.
++
+ ENVIRONMENT
+ -----------

- while (state != FETCH_DONE) {
- switch (state) {
2: 7499a280961 ! 3: 94b40907e66 config: read protected config with git_protected_config()
@@ Metadata
## Commit message ##
config: read protected config with git_protected_config()

- Protected config is read using read_very_early_config(), which has
- several downsides:
-
- - Every call to read_very_early_config() parses global and
- system-level config files anew, but this can be optimized by just
- parsing them once [1].
- - Protected variables should respect "-c" because we can reasonably
- assume that it comes from the user. But, read_very_early_config()
- can't use "-c" because it is called so early that it does not have
- access to command line arguments.
-
- Introduce git_protected_config(), which reads protected config and
- caches the values in the_repository.protected_config. Then, refactor
- safe.directory to use git_protected_config().
-
- This implementation can still be improved, however:
-
- - git_protected_config() iterates through every variable in
- the_repository.protected_config, which may still be too expensive to
- be called in every "git" invocation. There exist constant time lookup
- functions for non-protected config (repo_config_get_*()), but for
- simplicity, this commit does not implement similar functions for
- protected config.
-
- - Protected config is stored in the_repository so that we don't need
- to statically allocate it. But this might be confusing since protected
- config ignores repository config by definition.
-
- [1] While git_protected_config() should save on file I/O, I wasn't
- able to measure a meaningful difference between that and
- read_very_early_config() on my machine (which has an SSD).
+ uploadpack.packObjectsHook is the only 'protected config only'
+ variable today, but we've noted that safe.directory and the upcoming
+ discovery.bare should also be 'protected config only'. So, for
+ consistency, we'd like to have a single implementation for protected
+ config.
+
+ The primary constraints are:
+
+ 1. Reading from protected config should be as fast as possible. Nearly
+ all "git" commands inside a bare repository will read both
+ safe.directory and discovery.bare, so we cannot afford to be
+ slow.
+
+ 2. Protected config must be readable when the gitdir is not known.
+ safe.directory and discovery.bare both affect repository
+ discovery and the gitdir is not known at that point [1].
+
+ The chosen implementation in this commit is to read protected config and
+ cache the values in a global configset. This is similar to the caching
+ behavior we get with the_repository->config.
+
+ Introduce git_protected_config(), which reads protected config and
+ caches them in the global configset protected_config. Then, refactor
+ uploadpack.packObjectsHook to use git_protected_config().
+
+ The protected config functions are named similarly to their
+ non-protected counterparts, e.g. git_protected_config_check_init() vs
+ git_config_check_init().
+
+ In light of constraint 1, this implementation can still be improved
+ since git_protected_config() iterates through every variable in
+ protected_config, which may still be too expensive. There exist constant
+ time lookup functions for non-protected config (repo_config_get_*()),
+ but for simplicity, this commit does not implement similar functions for
+ protected config.
+
+ An alternative that avoids introducing another configset is to continue
+ to read all config using git_config(), but only accept values that have
+ the correct config scope [2]. This technically fulfills constraint 2,
+ because git_config() simply ignores the local and worktree config when
+ the gitdir is not known. However, this would read incomplete config into
+ the_repository->config, which would need to be reset when the gitdir is
+ known and git_config() needs to read the local and worktree config.
+ Resetting the_repository->config might be reasonable while we only have
+ these 'protected config only' variables, but it's not clear whether this
+ extends well to future variables.
+
+ [1] In this case, we do have a candidate gitdir though, so with a little
+ refactoring, it might be possible to provide a gitdir.
+ [2] This is how uploadpack.packObjectsHook was implemented prior to
+ this commit.

## config.c ##
+@@ config.c: static enum config_scope current_parsing_scope;
+ static int pack_compression_seen;
+ static int zlib_compression_seen;
+
++/*
++ * Config that comes from trusted sources, namely:
++ * - system config files (e.g. /etc/gitconfig)
++ * - global config files (e.g. $HOME/.gitconfig,
++ * $XDG_CONFIG_HOME/git)
++ * - the command line.
++ *
++ * This is declared here for code cleanliness, but unlike the other
++ * static variables, this does not hold config parser state.
++ */
++static struct config_set protected_config;
++
+ static int config_file_fgetc(struct config_source *conf)
+ {
+ return getc_unlocked(conf->u.file);
+@@ config.c: int git_configset_add_file(struct config_set *cs, const char *filename)
+ return git_config_from_file(config_set_callback, filename, cs);
+ }
+
++int git_configset_add_parameters(struct config_set *cs)
++{
++ return git_config_from_parameters(config_set_callback, cs);
++}
++
+ int git_configset_get_value(struct config_set *cs, const char *key, const char **value)
+ {
+ const struct string_list *values = NULL;
@@ config.c: int repo_config_get_pathname(struct repository *repo,
return ret;
}

-+/* Read protected config into the_repository->protected_config. */ ++/* Read values into protected_config. */
+static void read_protected_config(void)
+{
+ char *xdg_config = NULL, *user_config = NULL, *system_config = NULL;
+
-+ CALLOC_ARRAY(the_repository->protected_config, 1);
-+ git_configset_init(the_repository->protected_config);
++ git_configset_init(&protected_config);
+
+ system_config = git_system_config();
+ git_global_config(&user_config, &xdg_config);
+
-+ git_configset_add_file(the_repository->protected_config, system_config);
-+ git_configset_add_file(the_repository->protected_config, xdg_config);
-+ git_configset_add_file(the_repository->protected_config, user_config);
++ git_configset_add_file(&protected_config, system_config);
++ git_configset_add_file(&protected_config, xdg_config);
++ git_configset_add_file(&protected_config, user_config);
++ git_configset_add_parameters(&protected_config);
+
+ free(system_config);
+ free(xdg_config);
+ free(user_config);
+}
+
-+/* Ensure that the_repository->protected_config has been initialized. */ ++/* Ensure that protected_config has been initialized. */
+static void git_protected_config_check_init(void)
+{
-+ if (the_repository->protected_config &&
-+ the_repository->protected_config->hash_initialized)
++ if (protected_config.hash_initialized)
+ return;
+ read_protected_config();
+}
@@ config.c: int repo_config_get_pathname(struct repository *repo,
+void git_protected_config(config_fn_t fn, void *data)
+{
+ git_protected_config_check_init();
-+ configset_iter(the_repository->protected_config, fn, data);
++ configset_iter(&protected_config, fn, data);
+}
+
/* Functions used historically to read configuration from 'the_repository' */
@@ config.c: int repo_config_get_pathname(struct repository *repo,
{

## config.h ##
+@@ config.h: void git_configset_init(struct config_set *cs);
+ */
+ int git_configset_add_file(struct config_set *cs, const char *filename); +
++/**
++ * Parses command line options and environment variables, and adds the ++ * variable-value pairs to the config_set. Returns 0 on success, or -1 ++ * if there is an error in parsing. The caller decides whether to free ++ * the incomplete configset or continue using it when the function ++ * returns -1.
++ */
++int git_configset_add_parameters(struct config_set *cs);
++
+ /**
+ * Finds and returns the value list, sorted in order of increasing priority
+ * for the configuration variable key and config set cs. When the @@ config.h: int repo_config_get_maybe_bool(struct repository *repo, int repo_config_get_pathname(struct repository *repo,
const char *key, const char **dest);
@@ config.h: int repo_config_get_maybe_bool(struct repository *repo,
* Querying For Specific Variables
* -------------------------------

- ## repository.c ##
-@@ repository.c: void repo_clear(struct repository *repo)
- FREE_AND_NULL(repo->remote_state);
- }
-
-+ if (repo->protected_config) {
-+ git_configset_clear(repo->protected_config);
-+ FREE_AND_NULL(repo->protected_config);
-+ }
+ ## t/t5544-pack-objects-hook.sh ##
+@@ t/t5544-pack-objects-hook.sh: test_expect_success 'hook does not run from repo config' '
+ ! grep "hook running" stderr &&
+ test_path_is_missing .git/hook.args &&
+ test_path_is_missing .git/hook.stdin &&
+- test_path_is_missing .git/hook.stdout
++ test_path_is_missing .git/hook.stdout &&
+
- repo_clear_path_cache(&repo->cached_paths);
- }
++ # check that global config is used instead
++ test_config_global uploadpack.packObjectsHook ./hook &&
++ git clone --no-local . dst2.git 2>stderr &&
++ grep "hook running" stderr
+ '

+ test_expect_success 'hook works with partial clone' '

- ## repository.h ##
-@@ repository.h: struct repository {
+ ## upload-pack.c ##
+@@ upload-pack.c: static int upload_pack_config(const char *var, const char *value, void *cb_data)
+ data->advertise_sid = git_config_bool(var, value);
+ }

- struct repo_settings settings;
+- if (current_config_scope() != CONFIG_SCOPE_LOCAL &&
+- current_config_scope() != CONFIG_SCOPE_WORKTREE) {
+- if (!strcmp("uploadpack.packobjectshook", var))
+- return git_config_string(&data->pack_objects_hook, var, value);
+- }
+-
+ if (parse_object_filter_config(var, value, data) < 0)
+ return -1;
+
+ return parse_hide_refs_config(var, value, "uploadpack");
+ }

-+ /*
-+ * Config that comes from trusted sources, namely
-+ * - system config files (e.g. /etc/gitconfig)
-+ * - global config files (e.g. $HOME/.gitconfig,
-+ * $XDG_CONFIG_HOME/git)
-+ */
-+ struct config_set *protected_config;
++static int upload_pack_protected_config(const char *var, const char *value, void *cb_data) ++{
++ struct upload_pack_data *data = cb_data;
++
++ if (!strcmp("uploadpack.packobjectshook", var))
++ return git_config_string(&data->pack_objects_hook, var, value);
++ return 0;
++}
++
++static void get_upload_pack_config(struct upload_pack_data *data)
++{
++ git_config(upload_pack_config, data);
++ git_protected_config(upload_pack_protected_config, data);
++}
+
- /* Subsystems */
- /*
- * Repository's config which contains key-value pairs from the usual
-
- ## setup.c ##
-@@ setup.c: static int ensure_valid_ownership(const char *path)
- is_path_owned_by_current_user(path))
- return 1;
+ void upload_pack(const int advertise_refs, const int stateless_rpc,
+ const int timeout)
+ {
+@@ upload-pack.c: void upload_pack(const int advertise_refs, const int stateless_rpc,
+ struct upload_pack_data data;

-- read_very_early_config(safe_directory_cb, &data);
-+ git_protected_config(safe_directory_cb, &data);
+ upload_pack_data_init(&data);
+-
+- git_config(upload_pack_config, &data);
++ get_upload_pack_config(&data);

- return data.is_safe;
- }
+ data.stateless_rpc = stateless_rpc;
+ data.timeout = timeout;
+@@ upload-pack.c: int upload_pack_v2(struct repository *r, struct packet_reader *request) +
+ upload_pack_data_init(&data);
+ data.use_sideband = LARGE_PACKET_MAX;
+-
+- git_config(upload_pack_config, &data);
++ get_upload_pack_config(&data);
+
+ while (state != FETCH_DONE) {
+ switch (state) {
4: 66a0a208176 ! 4: 156817966fa config: include "-c" in protected config
@@ Metadata
Author: Glen Choo <[email protected]>

## Commit message ##
- config: include "-c" in protected config
+ safe.directory: use git_protected_config()

- Protected config should include the command line (aka "-c") because we
- can be quite certain that this config is specified by the user.
-
- Introduce a function, git_configset_add_parameters(), that adds "-c"
- config to a config_set, and use it to add "-c" to protected config.
+ Use git_protected_config() to read safe.directory instead of
+ read_very_early_config(), making it 'protected config only'. As a
+ result, safe.directory now respects "-c", so update the tests and docs
+ accordingly.

- ## Documentation/config.txt ##
-@@ Documentation/config.txt: names do not conflict with those that are used by Git itself and
- other popular tools, and describe them in your documentation.
-
- Variables marked with '(Protected config only)' are only respected when
--they are specified in protected configuration. This includes global and --system-level config, and excludes repository config, the command line
--option -c, and environment variables. For more details, see the
-+they are specified in protected configuration. This includes global,
-+system-level config, the command line option -c, and environment
-+variables, and excludes repository config. For more details, see the
- 'protected configuration' entry in linkgit:gitglossary[7].
-
- include::config/advice.txt[]
-
- ## Documentation/glossary-content.txt ##
-@@ Documentation/glossary-content.txt: Protected configuration includes:
- - system-level config, e.g. /etc/git/config
- - global config, e.g. $XDG_CONFIG_HOME/git/config and
- $HOME/.gitconfig
-+- the command line option -c and its equivalent environment variables
+ ## Documentation/config/safe.txt ##
+@@ Documentation/config/safe.txt: via git config --add. To reset the list of safe directories (e.g. to
+ override any such directories specified in the system config), add a
+ safe.directory entry with an empty value.
+
- Protected configuration excludes:
+-This config setting is only respected when specified in a system or global
+-config, not when it is specified in a repository config, via the command
+-line option -c safe.directory=<path>, or in environment variables.
++This config setting is only respected in protected configuration (see
++<<SCOPES>>). This prevents the untrusted repository from tampering with this
++value.
+
- - repository config, e.g. $GIT_DIR/config and
- $GIT_DIR/config.worktree
--- the command line option -c and its equivalent environment variables
-
- [[def_reachable]]reachable::
- All of the ancestors of a given <<def_commit,commit>> are said to be
+ The value of this setting is interpolated, i.e. ~/<path> expands to a
+ path relative to the home directory and %(prefix)/<path> expands to a

- ## config.c ##
-@@ config.c: int git_configset_add_file(struct config_set *cs, const char *filename)
- return git_config_from_file(config_set_callback, filename, cs);
- }
+ ## setup.c ##
+@@ setup.c: static int ensure_valid_ownership(const char *path)
+ is_path_owned_by_current_user(path))
+ return 1;

-+int git_configset_add_parameters(struct config_set *cs)
-+{
-+ return git_config_from_parameters(config_set_callback, cs);
-+}
-+
- int git_configset_get_value(struct config_set *cs, const char *key, const char **value)
- {
- const struct string_list *values = NULL;
-@@ config.c: static void read_protected_config(void)
- git_configset_add_file(the_repository->protected_config, system_config);
- git_configset_add_file(the_repository->protected_config, xdg_config);
- git_configset_add_file(the_repository->protected_config, user_config);
-+ git_configset_add_parameters(the_repository->protected_config);
+- read_very_early_config(safe_directory_cb, &data);
++ git_protected_config(safe_directory_cb, &data);

- free(system_config);
- free(xdg_config);
-
- ## config.h ##
-@@ config.h: void git_configset_init(struct config_set *cs);
- */
- int git_configset_add_file(struct config_set *cs, const char *filename); -
-+/**
-+ * Parses command line options and environment variables, and adds the -+ * variable-value pairs to the config_set. Returns 0 on success, or -1 -+ * if there is an error in parsing. The caller decides whether to free -+ * the incomplete configset or continue using it when the function -+ * returns -1.
-+ */
-+int git_configset_add_parameters(struct config_set *cs);
-+
- /**
- * Finds and returns the value list, sorted in order of increasing priority
- * for the configuration variable key and config set cs. When the
+ return data.is_safe;
+ }

## t/t0033-safe-directory.sh ##
@@ t/t0033-safe-directory.sh: test_expect_success 'safe.directory is not set' '
@@ t/t0033-safe-directory.sh: test_expect_success 'safe.directory is not set' '
'

test_expect_success 'ignoring safe.directory in repo config' '
-
- ## t/t0035-discovery-bare.sh ##
-@@ t/t0035-discovery-bare.sh: 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
-+ git -c discovery.bare=always rev-parse --git-dir
- )
- '
-
3: d5a3e9f9845 ! 5: 29053d029f8 setup.c: create discovery.bare

     @@ setup.c
       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,
     ++enum discovery_bare_allowed {
      +	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;
     @@ setup.c: static int ensure_valid_ownership(const char *path)
       
      +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_config = DISCOVERY_BARE_NEVER;
     ++		*discovery_bare_allowed = DISCOVERY_BARE_NEVER;
      +		return 0;
      +	}
      +	if (!strcmp(value, "always")) {
     -+		discovery_bare_config = DISCOVERY_BARE_ALWAYS;
     ++		*discovery_bare_allowed = DISCOVERY_BARE_ALWAYS;
      +		return 0;
      +	}
      +	return -1;
      +}
      +
     -+static int check_bare_repo_allowed(void)
     ++static enum discovery_bare_allowed get_discovery_bare(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;
     ++	enum discovery_bare_allowed result = DISCOVERY_BARE_ALWAYS;
     ++	git_protected_config(discovery_bare_cb, &result);
     ++	return result;
      +}
      +
     -+static const char *discovery_bare_config_to_string(void)
     ++static const char *discovery_bare_allowed_to_string(
     ++	enum discovery_bare_allowed discovery_bare_allowed)
      +{
     -+	switch (discovery_bare_config) {
     ++	switch (discovery_bare_allowed) {
      +	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);
     ++	default:
     ++		BUG("invalid discovery_bare_allowed %d",
     ++		    discovery_bare_allowed);
      +	}
      +	return NULL;
      +}
     @@ setup.c: static enum discovery_result setup_git_directory_gently_1(struct strbuf
       		}
       
       		if (is_git_directory(dir->buf)) {
     -+			if (!check_bare_repo_allowed())
     ++			if (!get_discovery_bare())
      +				return GIT_DIR_DISALLOWED_BARE;
       			if (!ensure_valid_ownership(dir->buf))
       				return GIT_DIR_INVALID_OWNERSHIP;
     @@ setup.c: const char *setup_git_directory_gently(int *nongit_ok)
      +		if (!nongit_ok) {
      +			die(_("cannot use bare repository '%s' (discovery.bare is '%s')"),
      +			    dir.buf,
     -+			    discovery_bare_config_to_string());
     ++			    discovery_bare_allowed_to_string(get_discovery_bare()));
      +		}
      +		*nongit_ok = 1;
      +		break;
     @@ t/t0035-discovery-bare.sh (new)
      +
      +pwd="$(pwd)"
      +
     ++expect_accepted () {
     ++	git "$@" rev-parse --git-dir
     ++}
     ++
      +expect_rejected () {
     -+	test_must_fail git rev-parse --git-dir 2>err &&
     ++	test_must_fail git "$@" rev-parse --git-dir 2>err &&
      +	grep "discovery.bare" err
      +}
      +
     @@ t/t0035-discovery-bare.sh (new)
      +test_expect_success 'discovery.bare unset' '
      +	(
      +		cd outer-repo/bare-repo &&
     -+		git rev-parse --git-dir
     ++		expect_accepted
      +	)
      +'
      +
     @@ t/t0035-discovery-bare.sh (new)
      +	git config --global discovery.bare always &&
      +	(
      +		cd outer-repo/bare-repo &&
     -+		git rev-parse --git-dir
     ++		expect_accepted
      +	)
      +'
      +
     @@ t/t0035-discovery-bare.sh (new)
      +	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
     ++		expect_accepted -c discovery.bare=always &&
     ++		expect_rejected -c discovery.bare=
      +	)
      +'
      +

gitgitgadget

From: Glen Choo <[email protected]>

For security reasons, there are config variables that are only trusted when they are specified in extra-trustworthy configuration scopes, which are sometimes referred to on-list as 'protected configuration' [1]. A future commit will introduce another such variable, so let's define our terms so that we can have consistent documentation and implementation.

In our documentation, define 'protected config' as the system, global and command config scopes. As a shorthand, I will refer to variables that are only respected in protected config as 'protected config only', but this term is not used in the documentation.

This definition of protected configuration is based on whether or not Git can reasonably protect the user by ignoring the configuration scope:

- System, global and command line config are considered protected because an attacker who has control over any of those can do plenty of harm without Git, so we gain very little by ignoring those scopes.
- On the other hand, local (and similarly, worktree) config are not considered protected because it is relatively easy for an attacker to control local config, e.g.:
- On some shared user environments, a non-admin attacker can create a repository high up the directory hierarchy (e.g. C:\.git on Windows), and a user may accidentally use it when their PS1 automatically invokes "git" commands.

safe.directory prevents attacks of this form by making sure that the user intended to use the shared repository. It obviously shouldn't be read from the repository, because that would end up trusting the repository that Git was supposed to reject.
- "git upload-pack" is expected to run in repositories that may not be controlled by the user. We cannot ignore all config in that repository (because "git upload-pack" would fail), but we can limit the risks by ignoring uploadpack.packObjectsHook.

Only uploadpack.packObjectsHook is 'protected config only'. The following variables are intentionally excluded:

- safe.directory should be 'protected config only', but it does not technically fit the definition because it is not respected in the "command" scope. A future commit will fix this.

- trace2.* happens to read the same scopes as safe.directory because they share an implementation. However, this is not for security reasons; it is because we want to start tracing so early that repository-level config and "-c" are not available [2].

This requirement is unique to trace2.*, so it does not makes sense for protected configuration to be subject to the same constraints.

[1] For example,
https://lore.kernel.org/git/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/

---
 Documentation/config/uploadpack.txt |  6 +++---
 Documentation/git-config.txt        | 13 +++++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

index 32fad5bbe81..029abbefdff 100644
@@ -49,9 +49,9 @@ uploadpack.packObjectsHook::
 	`pack-objects` to the hook, and expects a completed packfile on
 	stdout.
 +
-Note that this configuration variable is ignored if it is seen in the
-repository-level config (this is a safety measure against fetching from
-untrusted repositories).
+Note that this configuration variable is only respected when it is specified
+in protected config (see <<SCOPES>>). This is a safety measure against
+fetching from untrusted repositories.
 
 uploadpack.allowFilter::
 	If this option is set, `upload-pack` will support partial

index 5e4c95f2423..2b4334faec9 100644
@@ -343,6 +343,7 @@ You can change the way options are read/written by specifying the path to a
 file (`--file`), or by specifying a configuration scope (`--system`,
 `--global`, `--local`, `--worktree`); see <<OPTIONS>> above.
 
+[[SCOPES]]
 SCOPES
 ------
 

@@ -380,6 +381,18 @@ Most configuration options are respected regardless of the scope it is
 defined in, but some options are only respected in certain scopes. See the
 option's documentation for the full details.
 
+Protected config
+~~~~~~~~~~~~~~~~
+
+Protected config refers to the 'system', 'global', and 'command' scopes. Git
+considers these scopes to be especially trustworthy because they are likely
+to be controlled by the user or a trusted administrator. An attacker who
+controls these scopes can do substantial harm without using Git, so it is
+assumed that the user's environment protects these scopes against attackers.
+
+For security reasons, certain options are only respected when they are
+specified in protected config, and ignored otherwise.
+
 ENVIRONMENT
 -----------
 

gitgitgadget

From: Glen Choo <[email protected]>

In a subsequent commit, we will introduce "protected config", which is easiest to describe in terms of configuration scopes (i.e. it's the union of the 'system', 'global', and 'command' scopes). This description is fine for ML discussions, but it's inadequate for end users because we don't provide a good description of "config scopes" in the public docs.

145d59f482 (config: add '--show-scope' to print the scope of a config value, 2020-02-10) introduced the word "scope" to our public docs, but that only enumerates the scopes and assumes the user can figure out those values mean.

Add a SCOPES section to Documentation/git-config.txt that describes the config scopes, their corresponding CLI options, and mentions that some configuration options are only respected in certain scopes. Then, use the word "scope" to simplify the FILES section and change some confusing wording.

---
 Documentation/git-config.txt | 64 ++++++++++++++++++++++++++++--------
 1 file changed, 50 insertions(+), 14 deletions(-)

index bdcfd94b642..5e4c95f2423 100644
@@ -297,8 +297,8 @@ The default is to use a pager.
 FILES
 -----
 
-If not set explicitly with `--file`, there are four files where
-'git config' will search for configuration options:
+By default, 'git config' will read configuration options from multiple
+files:
 
 $(prefix)/etc/gitconfig::
 	System-wide configuration file.

@@ -322,27 +322,63 @@ $GIT_DIR/config.worktree::
 	This is optional and is only searched when
 	`extensions.worktreeConfig` is present in $GIT_DIR/config.
 
-If no further options are given, all reading options will read all of these
-files that are available. If the global or the system-wide configuration
-file are not available they will be ignored. If the repository configuration
-file is not available or readable, 'git config' will exit with a non-zero
-error code. However, in neither case will an error message be issued.
+You may also provide additional configuration parameters when running any
+git command by using the `-c` option. See linkgit:git[1] for details.
+
+Options will be read from all of these files that are available. If the
+global or the system-wide configuration file are not available they will be
+ignored. If the repository configuration file is not available or readable,
+'git config' will exit with a non-zero error code. However, in neither case
+will an error message be issued.
 
 The files are read in the order given above, with last value found taking
 precedence over values read earlier.  When multiple values are taken then all
 values of a key from all files will be used.
 
-You may override individual configuration parameters when running any git
-command by using the `-c` option. See linkgit:git[1] for details.
-
-All writing options will per default write to the repository specific
+By default, options are only written to the repository specific
 configuration file. Note that this also affects options like `--replace-all`
 and `--unset`. *'git config' will only ever change one file at a time*.
 
-You can override these rules using the `--global`, `--system`,
-`--local`, `--worktree`, and `--file` command-line options; see
-<<OPTIONS>> above.
+You can change the way options are read/written by specifying the path to a
+file (`--file`), or by specifying a configuration scope (`--system`,
+`--global`, `--local`, `--worktree`); see <<OPTIONS>> above.
+
+SCOPES
+------
+
+Each configuration source falls within a configuration scope. The scopes
+are:
+
+system::
+	$(prefix)/etc/gitconfig
+
+global::
+	$XDG_CONFIG_HOME/git/config
++
+~/.gitconfig
+
+local::
+	$GIT_DIR/config
+
+worktree::
+	$GIT_DIR/config.worktree
+
+command::
+	environment variables
++
+the `-c` option
+
+With the exception of 'command', each scope corresponds to a command line
+option - `--system`, `--global`, `--local`, `--worktree`.
+
+When reading options, specifying a scope will only read options from the
+files within that scope. When writing options, specifying a scope will write
+to the files within that scope (instead of the repository specific
+configuration file). See <<OPTIONS>> above for a complete description.
 
+Most configuration options are respected regardless of the scope it is
+defined in, but some options are only respected in certain scopes. See the
+option's documentation for the full details.
 
 ENVIRONMENT
 -----------

gitgitgadget

Yes, and as noted in the commit message, this approach seems to work for safe.directory and discovery.bare too.