Re: [PATCH v7 2/5] Documentation: define protected configuration

[Junio C Hamano <gitster@pobox.com>]

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Glen Choo <chooglen@google.com>
>
> For security reasons, there are config variables that are only trusted
> 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 configuration' as the system,
> global and command config scopes. As a shorthand, I will refer to
> variables that are only respected in protected configuration 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:
>
> - 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`.

This is only about the formatting, but have a blank line between
each bullet-point (e.g. before the line that talks about "On some
shared user enviornments, ..." and "git upload-pack").  A paragraph
break within a single bullet-point (i.e. the paragraph that talks
about `safe.directory` is a second paragraph of hte same bullet
point as the paragraph before it) looks like a stronger break than
separation between each bullet-point, which you wrote without any
blank lines in between.

> Only `uploadpack.packObjectsHook` is 'protected configuration only'. The
> following variables are intentionally excluded:
>
> - `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
>   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.

Very well reasoned.