git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Emily Shaffer <emilyshaffer@google.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: "Glen Choo" <chooglen@google.com>,
	"Git List" <git@vger.kernel.org>,
	justin@justinsteven.com,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Derrick Stolee" <derrickstolee@github.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	"Randall S. Becker" <rsbecker@nexbridge.com>,
	"Martin von Zweigbergk" <martinvonz@google.com>
Subject: Re: Bare repositories in the working tree are a security risk
Date: Thu, 21 Apr 2022 14:01:44 -0700	[thread overview]
Message-ID: <CAJoAoZnd=BKycr0c71-BQJyO3zoymC7p++Zke+OSkV4neweAOQ@mail.gmail.com> (raw)
In-Reply-To: <YmGr0upicQJML+9B@nand.local>

On Thu, Apr 21, 2022 at 12:09 PM Taylor Blau <me@ttaylorr.com> wrote:
> On Thu, Apr 21, 2022 at 11:25:39AM -0700, Emily Shaffer wrote:
> > On Fri, Apr 15, 2022 at 6:28 PM Taylor Blau <me@ttaylorr.com> wrote:
> > >   3. This seems the most promising approach so far. Similar to (1), I
> > >      would also want to make sure we provide an easy way to declare a
> > >      bare repository as "safe" in order to avoid permanently disrupting
> > >      valid workflows that have accumulated over the past >15 years.
> >
> > I'd like to think a little more about how we want to determine that a
> > bare repo isn't embedded - wantonly scanning up the filesystem for any
> > gitdir above the current one is really expensive. When I tried that
> > approach for the purposes of including some shared config between
> > superproject and submodules, it slowed down the Git test suite by
> > something like 3-5x. So, I suppose that "we think this is bare" means
> > refs/ and objects/ present in a directory that isn't named '.git/',
> > and then we hunt anywhere above us for another .git/? Of course being
> > careful not to accept another bare repo as the "parent" gitdir.
>
> Definitely worth considering. Fundamentally I think if you're going to
> allow checking out embedded bare repositories (which I strongly think
> that we should), any search for "is this repository embedded in another
> one?" needs to walk along its entire path up to the filesystem root.
>
> I'd think that we could offer some tools, perhaps "safe.boundary",
> specifying a list of directories to halt our traversal at. IOW, if you
> have a bunch of bare repositories underneath /data/repositories, and
> know that they're all safe, you could set your boundary there to signal
> to Git "if my path is a subdirectory of anything in safe.boundary, then
> I don't care whether the repo is embedded or not".

Ah, I'm sorry to say that I was more on board before I read this mail
than I am after :)

I don't think there is much reason to continue searching above the
first found '.git' because we disallow '.git' from being committed or
checked out, right? So this extra filesystem hunting and ceiling math
seems unnecessary to me. I know I am slightly allergic to searching
the filesystem for a parent repo to begin with, so I'm sure I've got
some bias here ;)

> > > Here's an alternative approach, which I haven't seen discussed thus
> > > far: [...]
> >
> > Nice - a more strict spin on proposal 3 from above, if I understand it
> > right. Rather than allowing any and all bare repos, avoid someone
> > sneaking in a malicious one next to legitimate ones by using an
> > allowlist. Seems reasonable to me.
>
> We'd probably want to allow saying "all embedded bare repositories are
> safe to read config/hooks from", too. I hadn't considered this approach
> as a way to read some embedded repos and not others; I suspect the
> overwhelmingly common use-case would be: `git config --local
> safe.embeddedRepo '*'`.

Ah, I dislike this option for the exact reason I mentioned - avoiding
a malicious repo being snuck in next to legitimate repos. I'd prefer
to rely on exact matching only - but as the config needs to be set by
every contributor every time the set of bare repos changes, that
sounds impossible to manage for a project which may be constantly
adding and removing these repos.

That said.... I'm biased again, but if you want to be certain of the
state of another repository in order to run tests in it... why not use
a submodule for that repository? Not a helpful comment for those
already using embedded bare repos, though ;)

Thanks for the food for thought.
 - Emily

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

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-06 22:43 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
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 [this message]
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='CAJoAoZnd=BKycr0c71-BQJyO3zoymC7p++Zke+OSkV4neweAOQ@mail.gmail.com' \
    --to=emilyshaffer@google.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=chooglen@google.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=justin@justinsteven.com \
    --cc=martinvonz@google.com \
    --cc=me@ttaylorr.com \
    --cc=rsbecker@nexbridge.com \
    --cc=sandals@crustytoothpaste.net \
    --subject='Re: Bare repositories in the working tree are a security risk' \
    /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).