Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.