From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Michael McClimon <michael@mcclimon.org>, git@vger.kernel.org
Subject: Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking
Date: Mon, 24 Oct 2022 12:57:29 +0200 [thread overview]
Message-ID: <221024.861qqxeah5.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <Y1RY38RULkqd9pBN@coredump.intra.peff.net>
On Sat, Oct 22 2022, Jeff King wrote:
> On Sat, Oct 22, 2022 at 09:45:14PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> The vulnerability safe.directory was supposed to address was one where
>> you'd set your fsmonitor hook via a config variable, so running "diff",
>> "status" etc. would unexpectedly execute arbitrary code.
>>
>> Especially on Windows where apparently the equivalent of the root of a
>> shared mounted volume routinely has global write permissions (user's
>> subdirectories being less permissive).
>>
>> An alternative I raised on the security list was to narrowly target just
>> the fsmonitor config, since that was the vulnerability.
>>
>> [...]
>>
>> I'm unaware of any other variable(s) that provide a similar vector, and
>> safe.directory is encouraging users (especially in core.sharedRepository
>> settings) to mark a dir as "safe", and we'd then later have an exploit
>> from a user with rw access who'd use the fsmonitor hook vector.
>
> Here are a few off the top of my head that you can trigger via git-diff:
>
> - core.pager will run an arbitrary program
>
> - pager.diff will run an arbitrary program
>
> - diff.*.textconv runs an arbitrary program; you need matching
> .gitattributes, but those are under the control of the repository.
> (not diff.*.command, though, as you need to pass --ext-diff)
>
> - browser/man paths if you run "git diff --help"
>
> And of course as you expand the set of commands there are more options.
> E.g., credential helper commands if you do anything that wants auth,
> interactive diff-filter for "add -p", hooks for git-commit, git-push,
> etc. Those commands are less likely to be run in an untrusted repository
> than inspection commands like "status" or "diff", but the boundary is
> getting quite fuzzy.
>
> fsmonitor _might_ be the only one that is triggered by git-prompt.sh,
> because it has a limited command palette, generally reads (or sends to
> /dev/null) the stdout of commands (preventing pager invocation), and
> doesn't do text diffs (so no textconv). Even if true, I'm not sure if
> that's a good place to draw the line, though. People do tend to run "git
> log" themselves.
Right, by "a similar" vector I meant the unexpected execution of
fsmonitor as there was software in the wild that was running "status"
for the user.
These are also a problem, but my understanding of that issue is that if
it wasn't for the fsmonitor issue we'd have put that in the bucket of
not running arbitrary commands on a .git you just copied to somewhere,
i.e. that issue was already known.
The difference being that users might have that implicit "status"
running if they cd'd to /mnt/$USER/subdir and there was a hostile
/mnt/.git, but would be much less likely to run "git diff" or whatever
in such a subdir, unless they'd initialized a .git in say
/mnt/$USER/subdir, at which point we'd ignore the /mnt/.git.
Anyway, that's more into the "would it have been a CVE?" territory, so
let's leave that for now.
The important point/question I have is whether we can think of any such
config variable understood by the code that uses Git.pm.
The only ones I can think are the "sendemail.{to,cc}Cmd" variables.
I'm just pointing out that the reason we ended up with the facility (per
my understand) was among other things:
* A. There were too many config variables to exhaustively audit on the
security list and be sure we caught them all, and ...
* B. ...such a change would probably be larger, which ...
* C. ...given the embargo & desire to keep security patches minimal
warranted the more general safe.directory approach.
* D. You can talk about on the public list, or not have a zero-day, but
not both :)
Now, we may come up with a reason "E" for extending this at this point,
e.g. maybe just being consistent is reason enough.
But in this case "A" doesn't apply, it's maybe 20-30 config variables,
and it's easy to skim the git-{send-email,svn} docs to see what they
are. "B" might be the case, but taht's OK since we're past "D" here,
ditto "C" not applying.
next prev parent reply other threads:[~2022-10-24 11:18 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-16 21:22 [PATCH 0/1] Git.pm: add semicolon after catch statement Michael McClimon
2022-10-16 21:22 ` [PATCH 1/1] " Michael McClimon
2022-10-16 23:18 ` Jeff King
2022-10-17 2:17 ` Michael McClimon
2022-10-17 17:34 ` Jeff King
2022-10-18 1:39 ` Michael McClimon
2022-11-10 15:10 ` Johannes Schindelin
2022-11-10 21:41 ` Jeff King
2022-10-22 1:19 ` [PATCH v2 0/2] Fix behavior of Git.pm in unsafe bare repositories Michael McClimon
2022-10-22 1:19 ` [PATCH v2 1/2] Git.pm: add semicolon after catch statement Michael McClimon
2022-10-22 1:19 ` [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking Michael McClimon
2022-10-22 5:29 ` Junio C Hamano
2022-10-22 21:18 ` Jeff King
2022-10-22 23:17 ` Junio C Hamano
2022-10-22 19:45 ` Ævar Arnfjörð Bjarmason
2022-10-22 20:55 ` Jeff King
2022-10-24 10:57 ` Ævar Arnfjörð Bjarmason [this message]
2022-10-24 23:38 ` Jeff King
2022-10-22 21:16 ` Jeff King
2022-10-22 22:08 ` Jeff King
2022-10-22 23:19 ` Michael McClimon
2022-10-24 23:33 ` Jeff King
2022-10-22 23:14 ` 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=221024.861qqxeah5.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=michael@mcclimon.org \
--cc=peff@peff.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.
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).