From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Glen Choo <chooglen@google.com>
Cc: Glen Choo via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org, Taylor Blau <me@ttaylorr.com>,
"brian m. carlson" <sandals@crustytoothpaste.net>,
Derrick Stolee <derrickstolee@github.com>,
Junio C Hamano <gitster@pobox.com>,
Emily Shaffer <emilyshaffer@google.com>,
Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH v6 0/5] config: introduce discovery.bare and protected config
Date: Fri, 08 Jul 2022 23:58:22 +0200 [thread overview]
Message-ID: <220709.86zghj8d6i.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <kl6lr134spi0.fsf@chooglen-macbookpro.roam.corp.google.com>
On Fri, Jul 01 2022, Glen Choo wrote:
Sorry for the late reply, I see there's a v7, but this seems to also
apply to it, so...
> Thanks for weighing in :) Despite the different proposed approaches, I
> think we actually are in broad agreement.
>
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Thu, Jun 30 2022, Glen Choo via GitGitGadget wrote:
>>
>>> This is a quick re-roll to address Ævar's comments on the tests (thanks!).
>>
>> Thanks!
>>
>>> = Description
>>
>> Just more generally on this series & approach. I know this is a v6 by
>> now, but I haven't kept up with this topic, but to be fair I did mention
>> pretty much this in:
>> https://lore.kernel.org/git/220407.86lewhc6bz.gmgdl@evledraar.gmail.com/
>>
>> So...
>>
>>> There is a known social engineering attack that takes advantage of the fact
>>> that a working tree can include an entire bare repository, including a
>>> config file. A user could run a Git command inside the bare repository
>>> thinking that the config file of the 'outer' repository would be used, but
>>> in reality, the bare repository's config file (which is attacker-controlled)
>>> is used, which may result in arbitrary code execution. See [1] for a fuller
>>> description and deeper discussion.
>>>
>>> This series implements a simple way of preventing such attacks: create a
>>> config option, discovery.bare, that tells Git whether or not to die when it
>>> finds a bare repository. discovery.bare has two values:
>>>
>>> * "always": always allow bare repositories (default), identical to current
>>> behavior
>>> * "never": never allow bare repositories
>>>
>>> and users/system administrators who never expect to work with bare
>>> repositories can secure their environments using "never". discovery.bare has
>>> no effect if --git-dir or GIT_DIR is passed because we are confident that
>>> the user is not confused about which repository is being used.
>>
>> I'm not insisting that the entire approach here should be changed, but
>> in the above exchange you seemed to have performance concerns about the
>> "just walk up in setup.c" approach I mentioned, but it's not clear if
>> that's still the only thing that necessitates taking this approach.
>>
>> There may be security subtleties that I've missed, but from the
>> description here it seems like that would work equally well, and
>> wouldn't require configuration, except insofar as we'd need to opt-in to
>> reading config from bare repositores *that also exist in a parent tree*.
>>
>> And it would be a more narrow & more secure solution, since it would
>> e.g. allow you to intentionally navigate to /var/repos/git/git.git in a
>> server setup and read the config there, which it could distinguish from
>> a case of /var/repos/.git existing, and git/git.git being brought in as
>> a part of that "parent" repo.
>
> Performance is one major concern, yes, and I agree that your findings
> show that the "just walk up" approach is cheap enough to consider doing.
> Though in the few cases where it isn't cheap to walk, wouldn't it still
> be useful to be able to opt out of it?
Maybe, but until we at least have a reason to think that there is a
performance concern this all seems rather hypothetical.
In the thread(s) I linked to I noted that I tested my POC implementation
on AIX, which has a sloooooooow filesystem.
Why are you concerned about this having a performance impact?
> The other concern is simplicity and correctness. Are we confident that
> we'll get the design of "just walk up" correct (including edge cases
> like "bare repo in bare repo in non bare repo")? I'm 100% confident that
> we'll get it right eventually, and that this approach will be a good
> default for all users. But in comparison, "never" is so much easier to
> understand and implement that I don't see why we shouldn't start by
> presenting this option to the 0.1-1% of users who would find it useful.
If you run "git status" or whatever in a directory anywhere on your FS
now it'll confidently tell you if you're not within a git repository.
We're just talking about re-using that code, why would we be concerned
that:
1. Finding a repo at a given <PATH>
2. Appending "/.." to that <PATH>
3. Feeding it to a "given a path, find me the git repo" (code setup.c
already has)
Is something we'd get wrong?
> And on the topic of simplicity, there's significant interest in
> maintaining backwards-compatibility with repos with workflows that
> absolutely depend on embedded bare repos, e.g. libgit2 and Git-LFS.
> That's yet another special case that we'd have to get right. Stolee's
> "no-embedded" proposal [1] pretty much covers that, but I don't see the
> harm in simplifying the design space by making bare repo support a
> non-goal.
>
> [1] https://lore.kernel.org/git/5b969c5e-e802-c447-ad25-6acc0b784582@github.com
Sure, and we could have a config know to still use such repos (for
config or otherwise) with the "walk up" method, does that address your
concern here?
>> The "more narrow" and "more secure" go hand-in-hand, since if you work
>> on such servers you'd turn this to "always" because you want to read
>> such config, but then be left vulnerable to the actual (and muche rarer)
>> exploit we're trying to prevent.
>
> The point that we're not defending bare repo users is fair, but maybe
> the group we're trying to protect isn't really dedicated Git-serving
> servers. This exploit requires you to have a bare repo inside the
> working tree of a non-bare repo. So I think this is less of an issue for
> a server, and more for "mixed-use" environments with both regular and
> bare clones.
Yes, but this is only something that's even a question because of an
artificial limitation your proposal here suffers from.
I.e. in trying to detect nefarious repos where you've got "looks like
bare" content *tracked* in another repo you're conflating it with *any
bare repo*.
And the only reason we're doing that seems to me to be a premature
optimization.
>> Which, it seems...
>>
>>> This series does not change the default behavior, but in the long-run, a
>>> "no-embedded" option might be a safe and usable default [2]. "never" is too
>>> restrictive and unlikely to be the default.
>>
>> This series has (since v3?) been noting aspirations to have a
>> "no-embedded" variant of this config, which your 5/5 here notes would be
>> better, but isn't implemented by this series.
>>
>> But your 5/5 also notes:
>>
>> but detecting if a repository is embedded is potentially
>> non-trivial, so this work is not implemented in this series.
>>
>> Hrm, well, the diff-stat isn't quite that trivial either :) :
>
> Well.. a lot of it is refactoring :P
>
>>> [...]
>>> upload-pack.c | 27 ++++++----
>>> 12 files changed, 304 insertions(+), 47 deletions(-)
>>
>> In threads linked from the above ML link I linked to some POC code
>> showing how to hack a second .git discovery walk into setup.c. This was
>> as part of the "submodule parent dir" proposal, which is a different
>> feature, but also needs such "find the parent" code:
>> https://lore.kernel.org/git/211109.86v912dtfw.gmgdl@evledraar.gmail.com/
>>
>> Now, obviously that's a dirty hack, but it's not that hard to just
>> change the part of setup.c where we're satisfied that we've found the
>> git dir, then walk up "$THAT_DIR/..", and start our search again.
>>
>> Then:
>>
>> if (first_dir_was_bare() && found_parent_dir())
>> enforce_no_embedded();
>>
>> Isn't that what your proposed "no embedded" option would need to do?
>> Well, maybe we'd also check if the "first dir" is in the index of the
>> parent, as opposed to just being a bare .git somewhere in ~/Downloads,
>> e.g. if you have a ~/.git and keep your dot-files in git.
>>
>> But I think for an initial implementation just doing the walk would be
>> good enough, and would have a more narrow scope than this configuration
>> setting.
>
> A narrow scope is good, but I don't agree on this definition of
> "narrow". My preference is to give an obvious solution to a 'narrow'
> group of users, instead of a more tricky solution that affects all users
> in a 'narrow' set of cases.
We could still have an option to say "I never want to consider bare
repos from config", but if we're able to distinguish *tracked* bare
repos from non-tracked ones this is entirely unrelated to the initial
stated motivation for this series, isn't it?
I.e. to solve the security issue at hand.
I've got nothing against *also* providing a "I never want config from
xyz repos", but that seems to be orthagonal.
>> AFAICT the performance concerns aren't supported by any data, in the
>> case of the "submodule superproject" feature it turned out to not be the
>> directory walk, but us shelling out in a loop in git-submodule.sh.
>>
>> Well, *maybe* that's not the case, I think I have managed to read
>> between the lines of some of these past exchanges that there's some odd
>> propriterary internal NFS-like setup at Google where *parent dirs* are
>> auto-mounted and searched on access, so a "walk up" pattern would be
>> much more expensive.
>>
>> I do worry a bit about us ending up with design choices in git that we
>> wouldn't have ended up with, if not to cater to some in-house setup
>> somwhere that 99.99% of git users will never see.
>
> At the very least, I don't think you're saying that it's a bad idea to
> have "never", just that we might not have come up with it if not for
> some Google NFS thing.
I'm just speculating as to why we've ended up with this approach, but
maybe I'm wrong.
> Another use case I can think of is CI bots, which have no need for bare
> repos. To some folks (maybe in very security-sensitive environments),
> "never" might give more peace of mind than "no-embedded".
Sure, but again, I've got nothing against having *more config knobs*,
and I've often e.g. wanted some way to tell git to read no config at all
(which you can sort of do with the right environment variables, but it's
a hassle).
But I don't see how that's not unrelated to addressing the issue this
series aims to address, and what I'm pointing out that it's doing so
with a method that's less accurate than the "walk up" method, and less
secure.
next prev parent reply other threads:[~2022-07-08 22:15 UTC|newest]
Thread overview: 113+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-06 18:30 [PATCH] [RFC] setup.c: make bare repo discovery optional Glen Choo via GitGitGadget
2022-05-06 20:33 ` Junio C Hamano
2022-05-09 21:42 ` Taylor Blau
2022-05-09 22:54 ` Junio C Hamano
2022-05-09 23:57 ` Taylor Blau
2022-05-10 0:23 ` Junio C Hamano
2022-05-10 22:00 ` Glen Choo
2022-05-13 23:37 ` [PATCH v2 0/2] " Glen Choo via GitGitGadget
2022-05-13 23:37 ` [PATCH v2 1/2] " Glen Choo via GitGitGadget
2022-05-16 18:12 ` Glen Choo
2022-05-16 18:46 ` Derrick Stolee
2022-05-16 22:25 ` Taylor Blau
2022-05-17 20:24 ` Glen Choo
2022-05-17 21:51 ` Glen Choo
2022-05-13 23:37 ` [PATCH v2 2/2] setup.c: learn discovery.bareRepository=cwd Glen Choo via GitGitGadget
2022-05-16 18:49 ` Derrick Stolee
2022-05-16 16:40 ` [PATCH v2 0/2] setup.c: make bare repo discovery optional Junio C Hamano
2022-05-16 18:36 ` Glen Choo
2022-05-16 19:16 ` Junio C Hamano
2022-05-16 20:27 ` Glen Choo
2022-05-16 22:16 ` Junio C Hamano
2022-05-16 16:43 ` Junio C Hamano
2022-05-16 19:07 ` Derrick Stolee
2022-05-16 22:43 ` Taylor Blau
2022-05-16 23:19 ` Junio C Hamano
2022-05-17 18:56 ` Glen Choo
2022-05-27 21:09 ` [PATCH v3 0/5] config: introduce discovery.bare and protected config Glen Choo via GitGitGadget
2022-05-27 21:09 ` [PATCH v3 1/5] Documentation: define protected configuration Glen Choo via GitGitGadget
2022-05-27 23:29 ` Junio C Hamano
2022-06-02 12:42 ` Derrick Stolee
2022-06-02 16:53 ` Junio C Hamano
2022-06-02 17:39 ` Glen Choo
2022-06-03 15:57 ` Glen Choo
2022-05-27 21:09 ` [PATCH v3 2/5] config: read protected config with `git_protected_config()` Glen Choo via GitGitGadget
2022-05-28 0:28 ` Junio C Hamano
2022-05-31 17:43 ` Glen Choo
2022-06-01 15:58 ` Junio C Hamano
2022-06-02 12:56 ` Derrick Stolee
2022-05-27 21:09 ` [PATCH v3 3/5] setup.c: create `discovery.bare` Glen Choo via GitGitGadget
2022-05-28 0:59 ` Junio C Hamano
2022-06-02 13:11 ` Derrick Stolee
2022-05-27 21:09 ` [PATCH v3 4/5] config: include "-c" in protected config Glen Choo via GitGitGadget
2022-06-02 13:15 ` Derrick Stolee
2022-05-27 21:09 ` [PATCH v3 5/5] upload-pack: make uploadpack.packObjectsHook protected Glen Choo via GitGitGadget
2022-06-02 13:18 ` Derrick Stolee
2022-06-07 20:57 ` [PATCH v4 0/5] config: introduce discovery.bare and protected config Glen Choo via GitGitGadget
2022-06-07 20:57 ` [PATCH v4 1/5] Documentation/git-config.txt: add SCOPES section Glen Choo via GitGitGadget
2022-06-07 20:57 ` [PATCH v4 2/5] Documentation: define protected configuration Glen Choo via GitGitGadget
2022-06-22 21:58 ` Jonathan Tan
2022-06-23 18:21 ` Glen Choo
2022-06-07 20:57 ` [PATCH v4 3/5] config: read protected config with `git_protected_config()` Glen Choo via GitGitGadget
2022-06-07 22:49 ` Junio C Hamano
2022-06-08 0:22 ` Glen Choo
2022-06-07 20:57 ` [PATCH v4 4/5] safe.directory: use git_protected_config() Glen Choo via GitGitGadget
2022-06-07 20:57 ` [PATCH v4 5/5] setup.c: create `discovery.bare` Glen Choo via GitGitGadget
2022-06-07 21:37 ` Glen Choo
2022-06-22 22:03 ` [PATCH v4 0/5] config: introduce discovery.bare and protected config Jonathan Tan
2022-06-23 17:13 ` Glen Choo
2022-06-23 18:32 ` Junio C Hamano
2022-06-27 17:34 ` Glen Choo
2022-06-27 18:19 ` Glen Choo
2022-06-27 18:36 ` [PATCH v5 " Glen Choo via GitGitGadget
2022-06-27 18:36 ` [PATCH v5 1/5] Documentation/git-config.txt: add SCOPES section Glen Choo via GitGitGadget
2022-06-27 18:36 ` [PATCH v5 2/5] Documentation: define protected configuration Glen Choo via GitGitGadget
2022-06-27 18:36 ` [PATCH v5 3/5] config: learn `git_protected_config()` Glen Choo via GitGitGadget
2022-06-27 18:36 ` [PATCH v5 4/5] safe.directory: use git_protected_config() Glen Choo via GitGitGadget
2022-06-27 18:36 ` [PATCH v5 5/5] setup.c: create `discovery.bare` Glen Choo via GitGitGadget
2022-06-30 13:20 ` Ævar Arnfjörð Bjarmason
2022-06-30 17:28 ` Glen Choo
2022-06-30 18:13 ` [PATCH v6 0/5] config: introduce discovery.bare and protected config Glen Choo via GitGitGadget
2022-06-30 18:13 ` [PATCH v6 1/5] Documentation/git-config.txt: add SCOPES section Glen Choo via GitGitGadget
2022-06-30 22:32 ` Taylor Blau
2022-07-06 17:44 ` Glen Choo
2022-06-30 18:13 ` [PATCH v6 2/5] Documentation: define protected configuration Glen Choo via GitGitGadget
2022-06-30 23:49 ` Taylor Blau
2022-07-06 18:21 ` Glen Choo
2022-06-30 18:13 ` [PATCH v6 3/5] config: learn `git_protected_config()` Glen Choo via GitGitGadget
2022-07-01 1:22 ` Taylor Blau
2022-07-06 22:42 ` Glen Choo
2022-06-30 18:13 ` [PATCH v6 4/5] safe.directory: use git_protected_config() Glen Choo via GitGitGadget
2022-06-30 18:13 ` [PATCH v6 5/5] setup.c: create `discovery.bare` Glen Choo via GitGitGadget
2022-07-01 1:30 ` Taylor Blau
2022-07-07 19:55 ` Glen Choo
2022-06-30 22:13 ` [PATCH v6 0/5] config: introduce discovery.bare and protected config Taylor Blau
2022-06-30 23:07 ` Ævar Arnfjörð Bjarmason
2022-07-01 17:37 ` Glen Choo
2022-07-08 21:58 ` Ævar Arnfjörð Bjarmason [this message]
2022-07-12 20:47 ` Glen Choo
2022-07-12 23:53 ` Ævar Arnfjörð Bjarmason
2022-07-07 23:01 ` [PATCH v7 " Glen Choo via GitGitGadget
2022-07-07 23:01 ` [PATCH v7 1/5] Documentation/git-config.txt: add SCOPES section Glen Choo via GitGitGadget
2022-07-07 23:43 ` Junio C Hamano
2022-07-08 17:01 ` Glen Choo
2022-07-08 19:01 ` Junio C Hamano
2022-07-08 21:38 ` Glen Choo
2022-07-07 23:01 ` [PATCH v7 2/5] Documentation: define protected configuration Glen Choo via GitGitGadget
2022-07-08 0:39 ` Junio C Hamano
2022-07-07 23:01 ` [PATCH v7 3/5] config: learn `git_protected_config()` Glen Choo via GitGitGadget
2022-07-07 23:01 ` [PATCH v7 4/5] safe.directory: use git_protected_config() Glen Choo via GitGitGadget
2022-07-07 23:01 ` [PATCH v7 5/5] setup.c: create `discovery.bare` Glen Choo via GitGitGadget
2022-07-08 1:07 ` [PATCH v7 0/5] config: introduce discovery.bare and protected config Junio C Hamano
2022-07-08 20:35 ` Glen Choo
2022-07-12 22:11 ` Glen Choo
2022-07-14 21:27 ` [PATCH v8 0/5] config: introduce safe.bareRepository " Glen Choo via GitGitGadget
2022-07-14 21:27 ` [PATCH v8 1/5] Documentation/git-config.txt: add SCOPES section Glen Choo via GitGitGadget
2022-07-14 21:27 ` [PATCH v8 2/5] Documentation: define protected configuration Glen Choo via GitGitGadget
2022-07-14 21:27 ` [PATCH v8 3/5] config: learn `git_protected_config()` Glen Choo via GitGitGadget
2022-07-25 18:26 ` SANITIZE=address failure on master (was: [PATCH v8 3/5] config: learn `git_protected_config()`) Ævar Arnfjörð Bjarmason
2022-07-25 20:15 ` Glen Choo
2022-07-25 20:41 ` Ævar Arnfjörð Bjarmason
2022-07-25 20:56 ` Glen Choo
2022-07-14 21:28 ` [PATCH v8 4/5] safe.directory: use git_protected_config() Glen Choo via GitGitGadget
2022-07-14 21:28 ` [PATCH v8 5/5] setup.c: create `safe.bareRepository` Glen Choo via GitGitGadget
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=220709.86zghj8d6i.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=chooglen@google.com \
--cc=derrickstolee@github.com \
--cc=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.com \
--cc=me@ttaylorr.com \
--cc=sandals@crustytoothpaste.net \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).