[v4,2/5] Documentation: define protected configuration

Commit Message

Glen Choo June 7, 2022, 8:57 p.m. UTC

From: Glen Choo <[e-mail]>

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

Signed-off-by: Glen Choo <[e-mail]>
---
 Documentation/config/uploadpack.txt |  6 +++---
 Documentation/git-config.txt        | 13 +++++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

Comments

Jonathan Tan June 22, 2022, 9:58 p.m. UTC | #1

"Glen Choo via GitGitGadget" <[e-mail]> writes:
> From: Glen Choo <[e-mail]>
> 
> For security reasons, there are config variables that are only trusted
> when they are specified in extra-trustworthy configuration scopes, which

Probably better to delete "extra-trustworthy", or at least "extra-" -
it's better to explain why and how they're trustworthy, which you have
already done in the commit message.

> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 5e4c95f2423..2b4334faec9 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt

[snip]

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

Also "especially trustworthy" here.

Glen Choo June 23, 2022, 6:21 p.m. UTC | #2

Jonathan Tan <[e-mail]> writes:

> "Glen Choo via GitGitGadget" <[e-mail]> writes:
>> From: Glen Choo <[e-mail]>
>> 
>> For security reasons, there are config variables that are only trusted
>> when they are specified in extra-trustworthy configuration scopes, which
>
> Probably better to delete "extra-trustworthy", or at least "extra-" -
> it's better to explain why and how they're trustworthy, which you have
> already done in the commit message.

Hm, do you find it superfluous, misleading or something else entirely?

The use of "extra-" was quite intentional. I'm afraid that if we
describe protected config as "trustworthy", we insinuate that
local/worktree config is "untrustworthy" (but of course this isn't
always true, Git usually uses repo config.)

>> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
>> index 5e4c95f2423..2b4334faec9 100644
>> --- a/Documentation/git-config.txt
>> +++ b/Documentation/git-config.txt
>
> [snip]
>
>> +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.
>
> Also "especially trustworthy" here.

Patch

diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
index 32fad5bbe81..029abbefdff 100644
--- a/Documentation/config/uploadpack.txt
+++ b/Documentation/config/uploadpack.txt
@@ -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
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 5e4c95f2423..2b4334faec9 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -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
 -----------