From: Glen Choo <firstname.lastname@example.org> To: Junio C Hamano <email@example.com> Cc: firstname.lastname@example.org, Emily Shaffer <email@example.com>, firstname.lastname@example.org, Taylor Blau <email@example.com>, firstname.lastname@example.org, "brian m. carlson" <email@example.com> Subject: Re: Bare repositories in the working tree are a security risk Date: Fri, 15 Apr 2022 16:45:32 -0700 [thread overview] Message-ID: <firstname.lastname@example.org> (raw) In-Reply-To: <email@example.com> Junio C Hamano <firstname.lastname@example.org> writes: > Glen Choo <email@example.com> writes: > >> Yes, I mean that even the current directory will be ignored when >> discovery is disabled. >> >>> I am not sure that >>> is realistically feasible (I am thinking of cases like "git fetch" >>> going to the remote repository on the local disk that is bare to run >>> "git upload-pack"), but if the fallout is not too bad, it may be a >>> good heuristics. >> >> Good detail - I hadn't considered the impact on our own child processes. >> I suspect this might be a huge undertaking. Unless there is significant >> interest in this option, I probably won't pursue it further. > > So, I was doing a bit of experiment. The change to the set-up > essentially means that working in either a bare repository or in the > .git/ subdirectory of a non-bare repository that you used to be able > to do with > > cd bare.git && git cmd > cd .git/ && git cmd > > is now forbidden, unless you explicitly say which directory you want > to use as the GIT_DIR, i.e. > > cd bare.git && GIT_DIR=. git cmd > cd .git/ && GIT_DIR=. git cmd > > The required change to the code is surprisingly small, but the > fallout is very big. Partial patches to two tests are attached with > some commentary at the end. Thanks for running the experiment and the thorough writeup! Within the Google Git team, there was quite a bit of interest in this approach because of how simple it is, but the projected impact on users is potentially very high; this experiment is a great starting heuristic. > Interestingly, many breakages in tests are not due to "clone" > failing to operate in "the other side". For example, this one tries > to inspect a bare repository that was created by a clone operation, > and we need to tell "git" where the GIT_DIR is, even after we chdir > to it. I do not think this is so common but with our popularity, > not-so-common population is still an absolute large number of users > that we do not want to hurt. > > diff --git c/t/t5601-clone.sh w/t/t5601-clone.sh > index 4a61f2c901..822ee3164a 100755 > --- c/t/t5601-clone.sh > +++ w/t/t5601-clone.sh > @@ -111,9 +111,9 @@ test_expect_success 'clone --mirror' ' > git clone --mirror src mirror && > test -f mirror/HEAD && > test ! -f mirror/file && > - FETCH="$(cd mirror && git config remote.origin.fetch)" && > + FETCH="$(cd mirror && GIT_DIR=. git config remote.origin.fetch)" && > test "+refs/*:refs/*" = "$FETCH" && > - MIRROR="$(cd mirror && git config --bool remote.origin.mirror)" && > + MIRROR="$(cd mirror && GIT_DIR=. git config --bool remote.origin.mirror)" && > test "$MIRROR" = true > > ' > @@ -151,7 +151,7 @@ test_expect_success 'clone --mirror does not repeat tags' ' > git tag some-tag HEAD) && > git clone --mirror src mirror2 && > (cd mirror2 && > - git show-ref 2> clone.err > clone.out) && > + GIT_DIR=. git show-ref 2>clone.err >clone.out) && > ! grep Duplicate mirror2/clone.err && > grep some-tag mirror2/clone.out The assumption when I made this proposal is that --git-dir is a sufficient workaround when we don't detect bare repos. As such, I'm of the opinion that this is the "expected" result of such a change - we really do expect that `git clone --bare` no longer works without --git-dir. I don't think this case (by itself) means that we can ignore bare repos. > The next example is "init" test, which seems to reveal that refusing > to discover a bare repository is not exactly an approach that is > workable. > > The first two hunks just show the same pattern as above. The > check_config helper is given a directory, under which 'config' and > 'refs' should exist (in other words, the first part of the helper > emulates setup.c::is_git_directory()). Then it tries to read from > the configuration file found in that directory. Be it a bare > repository or .git/ subdirectory of a non-bare repository the caller > has given us, we need to do the same GIT_DIR=. dance as the above. > > The last hunk does not really fix. The scenario is this: > > * A bare repository bare-ancestor-aliased.git is prepared > > * Its config file says "[alias] aliasedinit=init" in it > > * In that bare repository (next to 'refs', 'config'), we create > a subdirectory 'plain-nested'. > > * We go to that bare-ancestor-aliased.git/plain-nested/ > directory and say "git aliasedinit". > > This wants to be able to create a subdirectory in this bare > repository and make that subdirectory an independent repository (it > happens to be a non-bare repository, but a bare repository should > also work---think of .git/modules/name/ bare repository that is > designed to be pointed at with gitfile: link from a submodule > working tree). > > Now, in order for the last step to be able to even _use_ the alias, > it has to treat the bare-ancestor-aliased.git directory as a bare > repository and read its configuration. But by explicitly setting > GIT_DIR to that location, the behaviour of "git init" itself is > changed. It no longer initializes the current directory, > i.e. plain-nested subdirectory of the bare-ancestor-aliased.git; > GIT_DIR tells us to (re)initialize it instead. However, this case shows that --git-dir won't work at all with "git init", and I wouldn't be surprised if there are other breakages hiding just out of plain sight. This worries me a lot more, and I think disabling bare repo detection wholesale might be a nonstarter. I wonder if all we need to do is make setup.c a little smarter - we recognize bare repos, but *only* if they are contained in a directory named `.git/`. This should fix "git init" I think, because even though we'd ignore "./ (bare)", we'd recognize `../.git/` and we get the right behavior again. I haven't tested it yet, but this proposal sounds like it has quite a few merits to me: - Easy to explain. - Easy rationalization ("We used to be quite cavalier about detecting bare repos, but now we're being more strict by default"). - Fixes the embedded bare repo problem (since we don't allow .git). - No-op and neglible performance hit for non-bare repo users, even if they occasionally run "git" inside ".git". As with the original proposal of "ignore bare repos altogether", this will still be a headache for bare repo users (unless they don't mind renaming their bare repo directory to ".git"), so this behavior needs to be configurable, but perhaps the fallout is small enough that this could be a safe default. > This fundamentally does not work. > > It is a separate matter if it makes sense to allow creating a new > repository inside a bare repository (it does---that is what the > modern submodule layout uses), or to allow an alias to help doing so > defined in the top-level bare repository (it probably does---the > end-user may want to have a handy way to customize how submodules > are configured). But it seems to tell us that with today's external > interface we have for "git init", we cannot take configuration from > a shallower level and use it to drive "git init" to create a new > repository at a deeper level. > '
next prev parent reply other threads:[~2022-04-15 23:45 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 [this message] 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 \ --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).