Limitations of ownership checking fox for CVE-2022-24765

Limitations of ownership checking fox for CVE-2022-24765

[Jeremy Maitin-Shepard]

The current fix for CVE-2022-24765 prevents unsafe command execution
in some cases but does not address all cases:

- Ownership by the current user should not be taken to mean "trusted":
the user may have retrieved a directory tree from an untrusted source,
including:
  - Another version control system (which won't prevent a .git directory)
  - Unpacking an archive
  - FUSE mounting a remote filesystem

Additionally, the current fix requires additional configuration to
support existing use cases, and does not provide uses a way to safely
execute commands like `git status` or `git log` on untrusted
repositories.

I think a better solution would be for git to support a `--safe`
option that only runs config-specified commands specifically added to
an allowed list, or if the repository itself has been added to
safe.directories.

Ideally git would default to running in `--safe` mode, but if that is
too disruptive at least the option would be available for use in
prompt commands, etc.

(Please CC me in replies as I'm not subscribed to the list.)

Re: Limitations of ownership checking fox for CVE-2022-24765

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 12 2022, Jeremy Maitin-Shepard wrote:

> The current fix for CVE-2022-24765 prevents unsafe command execution
> in some cases but does not address all cases:
>
> - Ownership by the current user should not be taken to mean "trusted":
> the user may have retrieved a directory tree from an untrusted source,
> including:
>   - Another version control system (which won't prevent a .git directory)
>   - Unpacking an archive
>   - FUSE mounting a remote filesystem

Those would be good to fix, but I don't think it's correct that it's
within the scope of CVE-2022-24765.

That CVE is specifically about the multi-user case where we'd previously
pick up another user's .git directory.

Whereas these cases aren't like that in that they:

1) Require the user to have set up that .git directory themselves, in
   one way or another.
2) ...or for the OS to enforce user permissions so loosely that others
  can chown files as you.

> Additionally, the current fix requires additional configuration to
> support existing use cases, and does not provide uses a way to safely
> execute commands like `git status` or `git log` on untrusted
> repositories.

Yes, I agree. I.e. that the method of fixing it is all-or-nothing, and
therefore creates escalation issues that wouldn't occur with a narrower approach.

There was extensive off-list discussion about this, my [1] touches some
of it. To quote my side of a discussion following-up [1] about the more
narrow approach:
	
	An implementation that asks the user to create an opt-in for any and all
	config will be much less secure for the core.sharedRepository case,
	which is an edge case my proposed change on top doesn't have.
	
	That's because we'll refuse to read *any* config (including just that
	created by "git init"), and then ask the user to opt-in to any *future*
	config to keep using the repository at all.
	
	So for the sort of staging environment I mentioned upthread it'll
	effectively create a vulnerability which we'll still have no practical
	mitigation for.
	
	Whereas if we only error on the specific exploitation vector(s) users
	who're using a repository without such config will never need to opt-in
	at all, so when someone sneaks up on them later and adds a
	core.fsmonitor=/path/to/exploit they'll know, and we can show a much
	more pointed and obvious error to that effect.

As noted in [1] the solution that got committed does suffer from that
edge case, but I think [1] also summarizes why that approach was taken.

A way of mitigating that case is to put something like this in one's
.bashrc:

	git () {
		command git -c core.fsmonitor=false "$@";
	}

Then, even if you need to use that safe.directory feature you won't be
vulnerable to someone sneaking up on you and adding this to the
.git/config of the (presumably core.sharedRepository) repo:

    [core]
    fsmonitor = rm -rf /

> I think a better solution would be for git to support a `--safe`
> option that only runs config-specified commands specifically added to
> an allowed list, or if the repository itself has been added to
> safe.directories.
>
> Ideally git would default to running in `--safe` mode, but if that is
> too disruptive at least the option would be available for use in
> prompt commands, etc.
>
> (Please CC me in replies as I'm not subscribed to the list.)

What you're suggesting would be nice, and has been discussed on-list
before.

But it's fundamentally the same case as "make inspecting an unpacked
.git [from a tarball] safe", i.e. that we'll currently pick up config
from it.

The CVE specifically exists because it's subtly different from that
long-known-about case.

1. https://lore.kernel.org/git/220412.86h76yglfe.gmgdl@evledraar.gmail.com/

Re: Limitations of ownership checking fox for CVE-2022-24765

[Jeremy Maitin-Shepard]

On Wed, Apr 13, 2022, 07:28 Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>
> On Tue, Apr 12 2022, Jeremy Maitin-Shepard wrote:
>
> > The current fix for CVE-2022-24765 prevents unsafe command execution
> > in some cases but does not address all cases:
> >
> > - Ownership by the current user should not be taken to mean "trusted":
> > the user may have retrieved a directory tree from an untrusted source,
> > including:
> >   - Another version control system (which won't prevent a .git directory)
> >   - Unpacking an archive
> >   - FUSE mounting a remote filesystem
>
> Those would be good to fix, but I don't think it's correct that it's
> within the scope of CVE-2022-24765.
>
> That CVE is specifically about the multi-user case where we'd previously
> pick up another user's .git directory.


I can see the distinction but I would say the CVE addresses a specific
case of the more general problem that commands like `status` for
inspecting a repository aren't safe by default.

By solving the more general problem we also solve the case described
in the CVE.  On the other hand if the more general problem is not
solved then it is still not safe to run `git status` automatically.
The fix for this CVE might give users the impression that it is safe,
when given how most users do things, it is probably not.

> As noted in [1] the solution that got committed does suffer from that
> edge case, but I think [1] also summarizes why that approach was taken.


The argument in the thread seemed to be that by broadly blocking the
multi-user issue, there is less risk of discovering another multi-user
vulnerability.  But since it does nothing to address the more general
issue of `git status` being unsafe to run from untrusted directories,
the real mitigation is to ensure `git status` isn't run automatically
(and carefully inspect any untrusted directories before running it).
And if users employ that mitigation, the additional ownership check
isn't necessary.

Other tools such as direnv and the Emacs dir-locals mechanism have
exactly the same issue of needing to decide whether to trust
configuration files in the current directory.  They solve the issue
with an allowlist.  I'm not aware of any tool that trusts based on
ownership.

>
> A way of mitigating that case is to put something like this in one's
> .bashrc:
>
>         git () {
>                 command git -c core.fsmonitor=false "$@";
>         }
>
> Then, even if you need to use that safe.directory feature you won't be
> vulnerable to someone sneaking up on you and adding this to the
> .git/config of the (presumably core.sharedRepository) repo:
>
>     [core]
>     fsmonitor = rm -rf /


It fixes the current known issue by disabling that feature altogether,
but unlike a hypothetical --safe option it doesn't protect against any
similar config options that may be added in the future (or existing
ones found in the future to be unsafe).

Additionally it doesn't provide a mechanism for allowing specific
commands or specific repositories.

On Wed, Apr 13, 2022 at 7:28 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Tue, Apr 12 2022, Jeremy Maitin-Shepard wrote:
>
> > The current fix for CVE-2022-24765 prevents unsafe command execution
> > in some cases but does not address all cases:
> >
> > - Ownership by the current user should not be taken to mean "trusted":
> > the user may have retrieved a directory tree from an untrusted source,
> > including:
> >   - Another version control system (which won't prevent a .git directory)
> >   - Unpacking an archive
> >   - FUSE mounting a remote filesystem
>
> Those would be good to fix, but I don't think it's correct that it's
> within the scope of CVE-2022-24765.
>
> That CVE is specifically about the multi-user case where we'd previously
> pick up another user's .git directory.
>
> Whereas these cases aren't like that in that they:
>
> 1) Require the user to have set up that .git directory themselves, in
>    one way or another.
> 2) ...or for the OS to enforce user permissions so loosely that others
>   can chown files as you.
>
> > Additionally, the current fix requires additional configuration to
> > support existing use cases, and does not provide uses a way to safely
> > execute commands like `git status` or `git log` on untrusted
> > repositories.
>
> Yes, I agree. I.e. that the method of fixing it is all-or-nothing, and
> therefore creates escalation issues that wouldn't occur with a narrower approach.
>
> There was extensive off-list discussion about this, my [1] touches some
> of it. To quote my side of a discussion following-up [1] about the more
> narrow approach:
>
>         An implementation that asks the user to create an opt-in for any and all
>         config will be much less secure for the core.sharedRepository case,
>         which is an edge case my proposed change on top doesn't have.
>
>         That's because we'll refuse to read *any* config (including just that
>         created by "git init"), and then ask the user to opt-in to any *future*
>         config to keep using the repository at all.
>
>         So for the sort of staging environment I mentioned upthread it'll
>         effectively create a vulnerability which we'll still have no practical
>         mitigation for.
>
>         Whereas if we only error on the specific exploitation vector(s) users
>         who're using a repository without such config will never need to opt-in
>         at all, so when someone sneaks up on them later and adds a
>         core.fsmonitor=/path/to/exploit they'll know, and we can show a much
>         more pointed and obvious error to that effect.
>
> As noted in [1] the solution that got committed does suffer from that
> edge case, but I think [1] also summarizes why that approach was taken.
>
> A way of mitigating that case is to put something like this in one's
> .bashrc:
>
>         git () {
>                 command git -c core.fsmonitor=false "$@";
>         }
>
> Then, even if you need to use that safe.directory feature you won't be
> vulnerable to someone sneaking up on you and adding this to the
> .git/config of the (presumably core.sharedRepository) repo:
>
>     [core]
>     fsmonitor = rm -rf /
>
> > I think a better solution would be for git to support a `--safe`
> > option that only runs config-specified commands specifically added to
> > an allowed list, or if the repository itself has been added to
> > safe.directories.
> >
> > Ideally git would default to running in `--safe` mode, but if that is
> > too disruptive at least the option would be available for use in
> > prompt commands, etc.
> >
> > (Please CC me in replies as I'm not subscribed to the list.)
>
> What you're suggesting would be nice, and has been discussed on-list
> before.
>
> But it's fundamentally the same case as "make inspecting an unpacked
> .git [from a tarball] safe", i.e. that we'll currently pick up config
> from it.
>
> The CVE specifically exists because it's subtly different from that
> long-known-about case.
>
> 1. https://lore.kernel.org/git/220412.86h76yglfe.gmgdl@evledraar.gmail.com/

Re: Limitations of ownership checking fox for CVE-2022-24765

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 13 2022, Jeremy Maitin-Shepard wrote:

> On Wed, Apr 13, 2022, 07:28 Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>>
>> On Tue, Apr 12 2022, Jeremy Maitin-Shepard wrote:
>>
>> > The current fix for CVE-2022-24765 prevents unsafe command execution
>> > in some cases but does not address all cases:
>> >
>> > - Ownership by the current user should not be taken to mean "trusted":
>> > the user may have retrieved a directory tree from an untrusted source,
>> > including:
>> >   - Another version control system (which won't prevent a .git directory)
>> >   - Unpacking an archive
>> >   - FUSE mounting a remote filesystem
>>
>> Those would be good to fix, but I don't think it's correct that it's
>> within the scope of CVE-2022-24765.
>>
>> That CVE is specifically about the multi-user case where we'd previously
>> pick up another user's .git directory.
>
> I can see the distinction but I would say the CVE addresses a specific
> case of the more general problem that commands like `status` for
> inspecting a repository aren't safe by default.
>
> By solving the more general problem we also solve the case described
> in the CVE.  On the other hand if the more general problem is not
> solved then it is still not safe to run `git status` automatically.
> The fix for this CVE might give users the impression that it is safe,
> when given how most users do things, it is probably not.

Yes, let's be clear. I'm not saying it doesn't suck, just that the git
project has maintained for approximate forever that thou shalt be
careful with a repo you unpack via a tarball, for exactly this reason.

So I think fixing that is needed, but that it would fall under future
feature development, not an immediate follow-up to the CVE.

I recognize that the CVE somewhat blurred the lines though, i.e. it's
really just a special-case of the old tarball case, for which we never
cared about ownership.

But that's also an artifact of git's *nix legacy, where you don't need
to worry about /.git or /home/.git, except if your root user creates it,
and then you're hosed anyway.

>> As noted in [1] the solution that got committed does suffer from that
>> edge case, but I think [1] also summarizes why that approach was taken.
>
>
> The argument in the thread seemed to be that by broadly blocking the
> multi-user issue, there is less risk of discovering another multi-user
> vulnerability.  But since it does nothing to address the more general
> issue of `git status` being unsafe to run from untrusted directories,
> the real mitigation is to ensure `git status` isn't run automatically
> (and carefully inspect any untrusted directories before running it).
> And if users employ that mitigation, the additional ownership check
> isn't necessary.
>
> Other tools such as direnv and the Emacs dir-locals mechanism have
> exactly the same issue of needing to decide whether to trust
> configuration files in the current directory.  They solve the issue
> with an allowlist.  I'm not aware of any tool that trusts based on
> ownership.

Yes, I think we're on the wrong path with considering ownership of the
config at all, and I've been advocating for adopting something more like
Emacs's model for years :) E.g.:

	https://lore.kernel.org/git/87zi6eakkt.fsf@evledraar.gmail.com/

It also came up in the git-security@ discussion...

But that's a somewhat larger change, so just because that's a sensible
eventual goal doesn't mean that a shorter term mitigation is the wrong
thing for now.

But I for one would be estatic to have someone add the Emacs-like model
to git *hint* *hint* :)

>>
>> A way of mitigating that case is to put something like this in one's
>> .bashrc:
>>
>>         git () {
>>                 command git -c core.fsmonitor=false "$@";
>>         }
>>
>> Then, even if you need to use that safe.directory feature you won't be
>> vulnerable to someone sneaking up on you and adding this to the
>> .git/config of the (presumably core.sharedRepository) repo:
>>
>>     [core]
>>     fsmonitor = rm -rf /
>
>
> It fixes the current known issue by disabling that feature altogether,
> but unlike a hypothetical --safe option it doesn't protect against any
> similar config options that may be added in the future (or existing
> ones found in the future to be unsafe).

Aside from anything else I think it's safe to say that we'll probably be
much more careful in adding these arbitrary execution options in the
future, so I'm not too worried about that.

> Additionally it doesn't provide a mechanism for allowing specific
> commands or specific repositories.

*nod*. I'm just saying that this should get you out of the woods when
doing "basic inspection", it would probably be a good idea to add -c
core.pager=less or whatever to that though.