Re: [PATCH] fsck: detect bare repos in trees and warn

From: Derrick Stolee <derrickstolee@github.com>
Date: Apr 7, 2022, 13:21 UTC
Message-ID: <e81cdc6e-da42-d1d1-5d66-7d5e2a8aebbe@github.com>
In-Reply-To: <nycvar.QRO.7.76.6.2204071440520.347@tvgsbejvaqbjf.bet>

On 4/7/2022 8:42 AM, Johannes Schindelin wrote:

Show 21 quoted lines

> Hi Glen,
> 
> On Wed, 6 Apr 2022, Glen Choo wrote:
> 
>> Git tries not to distribute configs in-repo because they are a security
>> risk. However, an attacker can do exactly this if they embed a bare
>> repo inside of another repo.
>>
>> Teach fsck to detect whether a tree object contains a bare repo (as
>> determined by setup.c) and warn. This will help hosting sites detect and
>> prevent transmission of such malicious repos.
>>
>> See [1] for a more in-depth discussion, including future steps and
>> alternatives.
>>
>> [1] https://lore.kernel.org/git/kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com/
> 
> Out of curiosity: does this new check trigger with
> https://github.com/libgit2/libgit2? AFAIR it has embedded repositories
> that are used in its test suite. In other words, libgit2 has a legitimate
> use case for embedded bare repositories, I believe.

It is definitely good to keep in mind that other repositories have included bare repositories for convenience. I'm not sure that the behavior of some good actors should outweigh the benefits of protecting against this attack vector.

The trouble here is: how could the libgit2 repo change their project to not trigger this warning? These bare repos are in their history forever if they don't do go through significant work and pain to remove them from their history. We would want to have a way to make the warnings less severe for special cases like this.

Simultaneously, we wouldn't want to bless all _forks_ of libgit2.

Finally, the real thing we want to avoid is having the Git client write these trees to disk, for example during a 'git checkout', unless the user gives an override. (We would want 'git bisect' to still work on the libgit2 repo, for example.)

A more complete protection here would be:

 1. Warn when finding a bare repo as a tree (this patch).

 2. Suppress warnings on trusted repos, scoped to a specific set of known
    trees _or_ based on some set of known commits (in case the known trees
    are too large).

 3. Prevent writing a bare repo to the worktree, unless the user provided
    an opt-in to that behavior.

Since your patch is moving in the right direction here, I don't think steps (2) and (3) are required to move forward with your patch. However, it is a good opportunity to discuss the full repercussions of this issue.

Thanks, -Stolee