[PATCH v6 0/5] config: introduce discovery.bare and protected config

["Glen Choo via GitGitGadget" <gitgitgadget@gmail.com>]

This is a quick re-roll to address Ævar's comments on the tests (thanks!).

This version is mostly simple refactoring, and I also removed a useless test
assertion (see "Series history").

= 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 configuration' (the
others are safe.directory and uploadpack.packObjectsHook) this series also
defines and creates a shared implementation for 'protected configuration'

= Patch organization

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

= Series history

Changes in v6:

 * Add TEST_PASSES_SANITIZE_LEAK=true
 * Replace all sub-shells with -C and use test_config_global
 * Change the expect_rejected helper to use "grep -F" with a more specific
   message.
   * This reveals that the "-c discovery.bare=" assertion in the last test
     was passing for the wrong reason (because '' is an invalid value for
     "discovery.bare"). I removed it because it wasn't doing anything useful
     anyway - I was trying to make discovery.bare unset in the command line,
     but the whole point of that test is to assert that we respect the CLI
     arg.

Changes in v5:

 * Standardize the usage of "protected configuration" instead of mixing
   "config" and "configuration". This required some unfortunate rewrapping.
 * Remove mentions of "trustworthiness" when discussing protected
   configuration and focus on what Git does instead.
   * The rationale of protected vs non-protected is still kept.
 * Fix the stale documentation entry for discovery.bare.
 * Include a fuller description of how discovery.bare and "--git-dir"
   interact instead of saying "has no effect".

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 [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/kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com

[2] This was first suggested in
https://lore.kernel.org/git/5b969c5e-e802-c447-ad25-6acc0b784582@github.com

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

 Documentation/config.txt            |  2 +
 Documentation/config/discovery.txt  | 23 +++++++++
 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           | 52 +++++++++++++++++++
 t/t5544-pack-objects-hook.sh        |  7 ++-
 upload-pack.c                       | 27 ++++++----
 12 files changed, 304 insertions(+), 47 deletions(-)
 create mode 100644 Documentation/config/discovery.txt
 create mode 100755 t/t0035-discovery-bare.sh


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

Range-diff vs v5:

 1:  ee9619f6ec0 = 1:  ee9619f6ec0 Documentation/git-config.txt: add SCOPES section
 2:  43627c05c0b = 2:  43627c05c0b Documentation: define protected configuration
 3:  3efe282e6b9 = 3:  3efe282e6b9 config: learn `git_protected_config()`
 4:  ec925823414 = 4:  ec925823414 safe.directory: use git_protected_config()
 5:  14411512783 ! 5:  a1323d963f9 setup.c: create `discovery.bare`
     @@ t/t0035-discovery-bare.sh (new)
      +
      +test_description='verify discovery.bare checks'
      +
     ++TEST_PASSES_SANITIZE_LEAK=true
      +. ./test-lib.sh
      +
      +pwd="$(pwd)"
     @@ t/t0035-discovery-bare.sh (new)
      +
      +expect_rejected () {
      +	test_must_fail git "$@" rev-parse --git-dir 2>err &&
     -+	grep "discovery.bare" err
     ++	grep -F "cannot use bare repository" err
      +}
      +
      +test_expect_success 'setup bare repo in worktree' '
     @@ t/t0035-discovery-bare.sh (new)
      +'
      +
      +test_expect_success 'discovery.bare unset' '
     -+	(
     -+		cd outer-repo/bare-repo &&
     -+		expect_accepted
     -+	)
     ++	expect_accepted -C outer-repo/bare-repo
      +'
      +
      +test_expect_success 'discovery.bare=always' '
     -+	git config --global discovery.bare always &&
     -+	(
     -+		cd outer-repo/bare-repo &&
     -+		expect_accepted
     -+	)
     ++	test_config_global discovery.bare always &&
     ++	expect_accepted -C outer-repo/bare-repo
      +'
      +
      +test_expect_success 'discovery.bare=never' '
     -+	git config --global discovery.bare never &&
     -+	(
     -+		cd outer-repo/bare-repo &&
     -+		expect_rejected
     -+	)
     ++	test_config_global discovery.bare never &&
     ++	expect_rejected -C outer-repo/bare-repo
      +'
      +
      +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
     -+	)
     ++	# discovery.bare must not be "never", otherwise git config fails
     ++	# with "fatal: not in a git directory" (like safe.directory)
     ++	test_config -C outer-repo/bare-repo discovery.bare always &&
     ++	test_config_global discovery.bare never &&
     ++	expect_rejected -C outer-repo/bare-repo
      +'
      +
      +test_expect_success 'discovery.bare on the command line' '
     -+	git config --global discovery.bare never &&
     -+	(
     -+		cd outer-repo/bare-repo &&
     -+		expect_accepted -c discovery.bare=always &&
     -+		expect_rejected -c discovery.bare=
     -+	)
     ++	test_config_global discovery.bare never &&
     ++	expect_accepted -C outer-repo/bare-repo \
     ++		-c discovery.bare=always
      +'
      +
      +test_done

-- 
gitgitgadget