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

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

The previous round of this series was picked up by the Google-hosted "Review
Club" (the event is on http://tinyurl.com/gitcal). This round incorporates
Jonathan Tan's feedback (thanks!) as well some feedback from Review Club
itself.

This round only contains changes to commit messages and documentation. As
requested in Review Club, I've included a full "Description" section in this
cover letter for the convenience of new readers.

= 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 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           | 68 +++++++++++++++++++++++++
 t/t5544-pack-objects-hook.sh        |  7 ++-
 upload-pack.c                       | 27 ++++++----
 12 files changed, 320 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-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1261/chooglen/setup/disable-bare-repo-config-v5
Pull-Request: https://github.com/git/git/pull/1261

Range-diff vs v4:

 1:  c0e27ab3b3e ! 1:  ee9619f6ec0 Documentation/git-config.txt: add SCOPES section
     @@ Metadata
       ## Commit message ##
          Documentation/git-config.txt: add SCOPES section
      
     -    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.
     +    In a subsequent commit, we will introduce "protected configuration",
     +    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 "configuration
     +    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
     @@ Commit message
          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,
     +    configuration 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.
      
 2:  a5a1dcb03e1 ! 2:  43627c05c0b Documentation: define protected configuration
     @@ Commit message
          Documentation: define protected configuration
      
          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.
     +    when they are specified in certain 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.
     +    In our documentation, define 'protected configuration' 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
     +    configuration 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:
     +    This definition of protected config 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
     @@ Commit message
            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.
     +        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
     @@ Commit message
              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
     +    Only `uploadpack.packObjectsHook` is 'protected configuration 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
     +    - `safe.directory` should be 'protected configuration 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
     @@ Documentation/git-config.txt: Most configuration options are respected regardles
       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.
     ++Protected configuration
     ++~~~~~~~~~~~~~~~~~~~~~~~
      +
     ++Protected configuration refers to the 'system', 'global', and 'command' scopes.
      +For security reasons, certain options are only respected when they are
     -+specified in protected config, and ignored otherwise.
     ++specified in protected configuration, and ignored otherwise.
     ++
     ++Git treats these scopes as if they are controlled by the user or a trusted
     ++administrator. This is because 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.
      +
       ENVIRONMENT
       -----------
 3:  94b40907e66 ! 3:  3efe282e6b9 config: read protected config with `git_protected_config()`
     @@ Metadata
      Author: Glen Choo <chooglen@google.com>
      
       ## Commit message ##
     -    config: read protected config with `git_protected_config()`
     +    config: learn `git_protected_config()`
      
     -    `uploadpack.packObjectsHook` is the only 'protected config only'
     +    `uploadpack.packObjectsHook` is the only 'protected configuration only'
          variable today, but we've noted that `safe.directory` and the upcoming
     -    `discovery.bare` should also be 'protected config only'. So, for
     +    `discovery.bare` should also be 'protected configuration 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
     +    1. Reading from protected configuration 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.
      
     @@ Commit message
             `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.
     +    The chosen implementation in this commit is to read protected
     +    configuration 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
     +    Introduce git_protected_config(), which reads protected configuration
     +    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
     +    The protected configuration 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.
     +    time lookup functions for non-protected configuration
     +    (repo_config_get_*()), but for simplicity, this commit does not
     +    implement similar functions for protected configuration.
      
          An alternative that avoids introducing another configset is to continue
          to read all config using git_config(), but only accept values that have
     @@ Commit message
          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.
     +    these 'protected configuration 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.
 4:  156817966fa ! 4:  ec925823414 safe.directory: use git_protected_config()
     @@ Commit message
          safe.directory: use git_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.
     +    read_very_early_config(), making it 'protected configuration only'.
     +
     +    As a result, `safe.directory` now respects "-c", so update the tests and
     +    docs accordingly. It used to ignore "-c" due to how it was implemented,
     +    not because of security or correctness concerns [1].
     +
     +    [1] https://lore.kernel.org/git/xmqqlevabcsu.fsf@gitster.g/
      
          Signed-off-by: Glen Choo <chooglen@google.com>
      
 5:  29053d029f8 ! 5:  14411512783 setup.c: create `discovery.bare`
     @@ Commit message
          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).
     +    done, e.g. if the user passes `--git-dir=my-dir`, discovery will be
     +    skipped and my-dir will be used as the repo regardless of the
     +    `discovery.bare` value.
      
          This config is an enum of:
      
     @@ Documentation/config.txt: include::config/diff.txt[]
       ## Documentation/config/discovery.txt (new) ##
      @@
      +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]).
     ++	Specifies whether Git will work with a bare repository that it
     ++	found during repository discovery. If the repository is
     ++	specified directly via the --git-dir command-line option or the
     ++	GIT_DIR environment variable (see linkgit:git[1]), Git will
     ++	always use the specified repository, regardless of this value.
     +++
     ++This config setting is only respected in protected configuration (see
     ++<<SCOPES>>). This prevents the untrusted repository from tampering with
     ++this value.
      ++
      +The currently supported values are:
      ++

-- 
gitgitgadget