git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Derrick Stolee <derrickstolee@github.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Glen Choo <chooglen@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] fsck: detect bare repos in trees and warn
Date: Thu, 7 Apr 2022 09:21:35 -0400	[thread overview]
Message-ID: <e81cdc6e-da42-d1d1-5d66-7d5e2a8aebbe@github.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2204071440520.347@tvgsbejvaqbjf.bet>

On 4/7/2022 8:42 AM, Johannes Schindelin wrote:
> 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

  reply	other threads:[~2022-04-07 13:21 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-06 22:43 Bare repositories in the working tree are a security risk Glen Choo
2022-04-06 23:22 ` [PATCH] fsck: detect bare repos in trees and warn Glen Choo
2022-04-07 12:42   ` Johannes Schindelin
2022-04-07 13:21     ` Derrick Stolee [this message]
2022-04-07 14:14       ` Ævar Arnfjörð Bjarmason
2022-04-14 20:02         ` Glen Choo
2022-04-15 12:46           ` Ævar Arnfjörð Bjarmason
2022-04-07 15:11       ` Junio C Hamano
2022-04-13 22:24       ` Glen Choo
2022-04-07 13:12   ` Ævar Arnfjörð Bjarmason
2022-04-07 15:20   ` Junio C Hamano
2022-04-07 18:38 ` Bare repositories in the working tree are a security risk John Cai
2022-04-07 21:24 ` brian m. carlson
2022-04-07 21:53   ` Justin Steven
2022-04-07 22:10     ` brian m. carlson
2022-04-07 22:40       ` rsbecker
2022-04-08  5:54       ` Junio C Hamano
2022-04-14  0:03         ` Junio C Hamano
2022-04-14  0:04         ` Glen Choo
2022-04-13 23:44       ` Glen Choo
2022-04-13 20:37 ` Glen Choo
2022-04-13 23:36   ` Junio C Hamano
2022-04-14 16:41     ` Glen Choo
2022-04-14 17:35       ` Junio C Hamano
2022-04-14 18:19         ` Junio C Hamano
2022-04-15 21:33         ` Glen Choo
2022-04-15 22:17           ` Junio C Hamano
2022-04-16  0:52             ` Taylor Blau
2022-04-15 22:43           ` Glen Choo
2022-04-15 20:13       ` Junio C Hamano
2022-04-15 23:45         ` Glen Choo
2022-04-15 23:59           ` Glen Choo
2022-04-16  1:00           ` Taylor Blau
2022-04-16  1:18             ` Junio C Hamano
2022-04-16  1:30               ` Taylor Blau
2022-04-16  0:34 ` Glen Choo
2022-04-16  0:41 ` Glen Choo
2022-04-16  1:28   ` Taylor Blau
2022-04-21 18:25     ` Emily Shaffer
2022-04-21 18:29       ` Emily Shaffer
2022-04-21 18:47         ` Junio C Hamano
2022-04-21 18:54           ` Taylor Blau
2022-04-21 19:09       ` Taylor Blau
2022-04-21 21:01         ` Emily Shaffer
2022-04-21 21:22           ` Taylor Blau
2022-04-29 23:57     ` Glen Choo
2022-04-30  1:14       ` Taylor Blau
2022-05-02 19:39         ` Glen Choo
2022-05-02 14:05       ` Philip Oakley
2022-05-02 18:50         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e81cdc6e-da42-d1d1-5d66-7d5e2a8aebbe@github.com \
    --to=derrickstolee@github.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=chooglen@google.com \
    --cc=git@vger.kernel.org \
    --subject='Re: [PATCH] fsck: detect bare repos in trees and warn' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).