Re: [PATCH] fsckObjects tests: show how v2.17.1 can exploit downstream

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

On Tue, May 29 2018, Jeff King wrote:

> On Tue, May 29, 2018 at 09:19:50PM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> Something that's known but not explicitly discussed in the v2.17.1
>> release notes, or tested for, is that v2.17.1 will still happily pass
>> on evil .gitmodules objects by default to vulnerable downstream
>> clients.
>>
>> This could happen e.g. if an in-house git hosting site is mirroring a
>> remote repository that doesn't have transfer.fsckObjects turned on.
>> Someone can remotely push evil data to that remote hosting site
>> knowing that it's mirrored downstream, and the in-house mirror without
>> transfer.fsckObjects will happily pass those evil objects along, even
>> though it's been updated to v2.17.1.
>
> Sure, I agree with all of the above, but...
>
>> It's worth testing for this explicitly. So let's amend the tests added
>> in 73c3f0f704 ("index-pack: check .gitmodules files with --strict",
>> 2018-05-04) to show how this can result in a v2.17.1 client passing
>> along the evil objects.
>
> I'm not sure what testing this buys us. [...]

Half of what I'm trying to do here is clarifying the v2.17.1 release
notes. The initial version Junio had & my proposed amendment on
git-security was:

      * In addition to the above fix that also appears in maintenance
    -   releases v2.13.7, v2.14.4, v2.15.2 and v2.16.4, this has support on
    -   the server side to reject pushes to repositories that attempt to
    -   create such problematic .gitmodules file etc. as tracked contents,
    -   to help hosting sites protect their customers by preventing
    -   malicious contents from spreading.
    +   releases v2.13.7, v2.14.4, v2.15.2 and v2.16.4, this release
    +   extends transfer.fsckObjects (off by default) to reject fetches or
    +   pushes to repositories that attempt to create such problematic
    +   .gitmodules file etc. as tracked contents, to help hosting sites
    +   protect their customers by preventing malicious contents from
    +   spreading, or to protect clients that fetch from passing on a bad
    +   repository to their downstream fetchers.

But Junio's final version of that in
https://public-inbox.org/git/xmqqy3g2flb6.fsf@gitster-ct.c.googlers.com/
rewrote that suggestion of transfer.fsckObjects to receive.fsckObjects.

That means that for someone who doesn't know how this stuff works in
detail and is just following along with our release notes (and say
enabling just receive.fsckObjects globally on their site) they might not
be covered at all.

The receive.fsckObjects variable only kicks in when someone pushes to
you, not when you fetch something malicious and someone then fetches
from you.

So with this and my https://news.ycombinator.com/item?id=17183044 (in
reply to your comment) I'm trying to do my small bit to inform people
about this angle of it being exploitable, since this issue of git mirror
chains seems to have not been noticed so far, and lots of sites run this
sort of setup to mirror & re-serve arbitrary public Git repos in-house.

The other half, which is why I think this patch is needed, is making
this aspect of it clearer to future maintainers. Before I started
hacking on my recent fsck series[1] I didn't realize the intricacies of
how *.fsckObjects worked in various situations, and I think explicitly
calling this case out in code helps.

Unlike documentation, when we change something in the code we're forced
to take notice that the test suite changes, and are thus more likely to
notice this important but apparently forgotten use-case. If the security
issue of promiscuously re-serving bad objects you've fetched was
forgotten this time around, this will help us remember it if say we ever
turn on *.fsckObjects by default in the future.

1. https://public-inbox.org/git/20180525192811.25680-6-avarab@gmail.com/