From: Emily Shaffer <firstname.lastname@example.org> To: Taylor Blau <email@example.com> Cc: "Glen Choo" <firstname.lastname@example.org>, "Git List" <email@example.com>, firstname.lastname@example.org, "Johannes Schindelin" <Johannes.Schindelin@gmx.de>, "Ævar Arnfjörð Bjarmason" <email@example.com>, "Derrick Stolee" <firstname.lastname@example.org>, "Junio C Hamano" <email@example.com>, "brian m. carlson" <firstname.lastname@example.org>, "Randall S. Becker" <email@example.com> Subject: Re: Bare repositories in the working tree are a security risk Date: Thu, 21 Apr 2022 11:25:39 -0700 [thread overview] Message-ID: <CAJoAoZkgnnvdymuBsM9Ja3+eYSnyohr=FQZMVX_uzZ_pkQhgaw@mail.gmail.com> (raw) In-Reply-To: <Ylobp7sntKeWTLDX@nand.local> On Fri, Apr 15, 2022 at 6:28 PM Taylor Blau <firstname.lastname@example.org> wrote: > > On Fri, Apr 15, 2022 at 05:41:59PM -0700, Glen Choo wrote: > > * We want additional protection on the client besides `git fsck`; there are > > a few ways to do this: > > I'm a little late to chime into the thread, but I appreciate you > summarizing some of the approaches discussed so far. Let me add my > thoughts on each of these in order: > > > 1. Prevent checking out an embedded bare repo. > > 2. Detect if the bare repo is embedded and refuse to work with it. > > 3. Detect if the bare repo is embedded and do not read its config/hooks, but > > everything else still 'works'. > > 4. Don't detect bare repos. > > 5. Only detect bare repos that are named `.git` . > > > > (I've responded with my thoughts on each of these approaches in-thread). > > 1. Likely disrupts too many legitimate workflows for us to adopt > without designing some way to declare an embedded bare repository > is "safe". > 2. Ditto. > 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. Does it work for submodules? I suppose it works for non-bare submodules - and for bare submodules, e.g. 'submodule-having-project.git/modules/some-submodule/' we can rely on the user to turn that flag on if they want their submodule's config and hooks to be noticed from the gitdir. (From 'worktree-for-submodule-having-project/some-submodule' there is a '.git' pointer, so I'd expect things to work normally that way, right?) As long as we are careful to avoid searching the filesystem in *every* case, this seems like a pretty reasonable approach to me. > 4. Seems like this approach is too heavy-handed. > 5. Ditto. > > > With that in mind, here's what I propose we do next: > > > > * Merge the `git fsck` patch  if we think that it is useful despite the > > potentially huge number of false positives. That patch needs some fixing; I'll > > make the changes when I'm back. > > If there are lots of false positives, we should consider downgrading the > severity of the proposed `EMBEDDED_BARE_REPO` fsck check to "info". I'm > not clear if there is another reason why this patch would have a > significant number of false positives (i.e., if the detection mechanism > is over-zealous). > > But if not, and this does detect only legitimate embedded bare > repositories, we should use it as a reminder to consider how many > use-cases and workflows will be affected by whatever approach we take > here, if any. > > > * I'll experiment with (5), and if it seems promising, I'll propose this as an > > opt-in feature, with the intent of making it opt-out in the future. We'll > > opt-into this at Google to help figure out if this is a good default or not. > > > > * If that direction turns out not to be promising, I'll pursue (1), since that > > is the only option that can be configured per-repo, which should hopefully > > minimize the fallout. > > Here's an alternative approach, which I haven't seen discussed thus far: > > When a bare repository is embedded in another repository, avoid reading > its config by default. This means that most Git commands will still > work, but without the possibility of running any "executable" portions > of the config. To opt-out (i.e., to allow legitimate use-cases to start > reading embedded bare repository config again), the embedding repository > would have to set a multi-valued `safe.embeddedRepo` configuration. This > would specify a list of paths relative to the embedding repository's > root of known-safe bare repositories. > > I think there are a couple of desirable attributes of this approach: > > - It minimally disrupts bare repositories, restricting the change to > only embedded repositories. > - It allows most Git commands to continue working as expected (modulo > reading the config), hopefully making the population of users whose > workflows will suddenly break pretty small. > - It requires the user to explicitly opt-in to the unsafe behavior, > because an attacker could not influence the embedding repository's > `safe.embeddedRepo` config. > > If we were going to do something about this, I would strongly advocate > for something that resembles the above. Or at the very least, some > solution that captures the attributes I outlined there. 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. - Emily
next prev parent reply other threads:[~2022-04-21 18:26 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 [this message] 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='CAJoAoZkgnnvdymuBsM9Ja3+eYSnyohr=FQZMVX_uzZ_pkQhgaw@mail.gmail.com' \ --email@example.com \ --cc=Johannes.Schindelin@gmx.de \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --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).