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

From: Glen Choo <chooglen@google.com>
Date: Jul 6, 2022, 18:21 UTC
Message-ID: <kl6lfsjertj2.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <Yr42b+MYsuw8ihgG@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 21 quoted lines

> On Thu, Jun 30, 2022 at 06:13:56PM +0000, Glen Choo via GitGitGadget wrote:
>> @@ -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 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 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.
>> +
>
> I think this description is a good starting point, but I think I would
> have liked to see some more from the commit description make it into the
> documentation here.

Yeah, there's a bit of a tradeoff here. Glossing over some of the details helps keep the documentation briefer and easier to understand for the less experienced/invested, but is bound to frustrate others. I'd appreciate any wording suggestions if you have any.

Show 10 quoted lines

> One thing that I didn't see mentioned in either is that the list of
> protected configuration is far from exhaustive. There are dozens upon
> dozens of configuration values that Git will happily execute as a
> subprocess (core.editor, core.pager, core.alternateRefsCommand, to name
> just a few).
>
> I don't think we should try and enumerate every possible path from
> configuration to command execution. But it is worth noting in the
> documentation that the list of configuration values which are only read
> in the protected context is non-exhaustive and best-effort only.

By referencing command execution, I think you are alluding to Stolee's Security Boundary discussion thread [1], and in particular, the "Example Security Boundary Question: Unprotected Config and Executing Code" section?

That section discusses the problem of arbitrary command execution based on repository-local config, and how protected configuration might give us a way to prevent that. That's a reasonable extension to this series, though it seems a little premature to include allusions to command execution, especially since I don't think we're anywhere close to a long-term direction on what should/shouldn't be inside protected configuration. For example, Stolee noted that, most of the command execution options really do want per-repository customization, so if we want to continue to support that, we'll need to use protected configuration in a somewhat sophisticated manner (and not, e.g. only respect command execution options in protected configuration). Perhaps we could shelve this wording change until we've committed to such a direction.

[1] https://lore.kernel.org/git/6af83767-576b-75c4-c778-0284344a8fe7@github.com

> Thanks,
> Taylor