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

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

[Ævar Arnfjörð Bjarmason]

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.

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

I guess this test is technically a bit redundant, but I think it's
worth adding anyway since we're short in general on the subtle
semantics of how *.fsckObjects acts in various situations, and so
anyone reading the tests realizes that even a patched v2.17.1 can
still be fooled to collude with evil in its default configuration.

 t/t7415-submodule-names.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
index a770d92a55..f35f98e956 100755
--- a/t/t7415-submodule-names.sh
+++ b/t/t7415-submodule-names.sh
@@ -93,6 +93,15 @@ test_expect_success 'transfer.fsckObjects detects evil superproject (index)' '
 	test_must_fail git push dst.git HEAD
 '
 
+test_expect_success 'transfer.fsckObjects needs to be on to protect downstream' '
+	git init --bare intermediary.git &&
+	git -C intermediary.git config transfer.fsckObjects false &&
+	git -C intermediary.git fetch ../ master:master &&
+	git init --bare downstream.git &&
+	git -C downstream.git fetch ../intermediary.git &&
+	test_must_fail git -C downstream.git fsck
+'
+
 # Normally our packs contain commits followed by trees followed by blobs. This
 # reverses the order, which requires backtracking to find the context of a
 # blob. We'll start with a fresh gitmodules-only tree to make it simpler.
-- 
2.17.0.290.gded63e768a

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

[Jeff King]

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. If it stopped passing them
along, would we consider that a regression? This seems more like a
documentation issue than something that needs to be tested.

-Peff

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

[Ævar Arnfjörð Bjarmason]

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/

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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

Yes, that is what was described in the release notes as the server
side support.  If you want to avoid fetching from contaminated
sources, that protection applies to both leaf clients and
intermediate relays, and I tend to agree that it is worth helping
those who want to use fetch.fsckObjects (or the blanket transfer.*
variant) the same way.

> Unlike documentation, when we change something in the code we're forced
> to take notice that the test suite changes, ...

But then the test you want to have is not the one you posted, which
is "when disabled, the feature should not kick in and should not
protect you".  That, even together with hot-sounding word "exploit"
in the title, does not have enough sensational value to grab people's
attension as you seem to be hoping to do here.

A test that checks "when enabled, the feature kicks in as expected
and protects you" does make sense.  So is (maybe) additional
description around fetch.fsckObjects if we currently lack one.

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

[Jeff King]

On Tue, May 29, 2018 at 11:59:07PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > 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:

I think that's a fine goal, but I doubt that adding a test is going to
help much. That's why I say this seems like it should be a documentation
patch and not a test one. People are much less likely to crawl through
our tests than they are to crawl through the documentation.

> 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.

I agree we should be testing that, but I don't think it should be tied
into this test that is specific to one particular fsck check. Don't we
already check the behavior of the various fsckObjects options elsewhere,
like in t5504?

-Peff

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

[Ævar Arnfjörð Bjarmason]

On Thu, May 31 2018, Jeff King wrote:

> On Tue, May 29, 2018 at 11:59:07PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > 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:
>
> I think that's a fine goal, but I doubt that adding a test is going to
> help much. That's why I say this seems like it should be a documentation
> patch and not a test one. People are much less likely to crawl through
> our tests than they are to crawl through the documentation.
>
>> 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.
>
> I agree we should be testing that, but I don't think it should be tied
> into this test that is specific to one particular fsck check. Don't we
> already check the behavior of the various fsckObjects options elsewhere,
> like in t5504?

Okey, I'll turn this into some documentation in my re-rolled
fetch.fsck.* series.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> 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. If it stopped passing them
> along, would we consider that a regression? This seems more like a
> documentation issue than something that needs to be tested.

I tend to agree.  The recommendation of the release notes is about
protecting the place downstream would pull from, by telling Git to
vet the pushes into that place and that is why receive.fsckObjects
is mentioned there.  Nobody sane would expect receive.fsckObjects to
do anything when that central place fetches from elsewhere, and I am
not sure what additional value it buys us(over the tests we already
have) to test that diabled transfer.fsckObjects are truly disabled.

If we want to also encourage people to vet their own "fetches", I am
not against extending documentation.  It just is different from "we
extended the mechanism to help server side protect their clients"
that was the focus of (updated, relative to what is in the tarball)
the description in the release notes.

Thanks.

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

[Jeff King]

On Wed, May 30, 2018 at 10:32:19AM +0900, Junio C Hamano wrote:

> If we want to also encourage people to vet their own "fetches", I am
> not against extending documentation.  It just is different from "we
> extended the mechanism to help server side protect their clients"
> that was the focus of (updated, relative to what is in the tarball)
> the description in the release notes.

I haven't tested it, but I suspect that doing multiple fetches could
result in passing bad objects through a fetch.fsckObjects filter.
Because the objects aren't quarantined on fetch, and because
fsck_finish() requires the objects to be installed into place, they may
still exist in the repository even if we end up rejecting them. Would a
subsequent fetch hit the quickfetch() code and update without actually
sending the objects again?

This problem is specific to the .gitmodules thing, I think, because the
other fsck checks are able to die much earlier (before fsck_finish).

I think in the long run fetch should implement a similar quarantine
procedure to what happens on push.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I haven't tested it, but I suspect that doing multiple fetches could
> result in passing bad objects through a fetch.fsckObjects filter.
> Because the objects aren't quarantined on fetch, and because
> fsck_finish() requires the objects to be installed into place, they may
> ...
> I think in the long run fetch should implement a similar quarantine
> procedure to what happens on push.

Interesting.

I wonder if we can teach quickfetch codepath to notice the presence
of fsckObjects, instead of doing a full quarantine.  We can easily
enumerate those objects that were already in the object database but
outside of the reachability chain before we pretend that we fetched
them and make them reachable, and check the content integrity of
them, no?

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

[Jeff King]

On Fri, Jun 01, 2018 at 10:42:00AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I haven't tested it, but I suspect that doing multiple fetches could
> > result in passing bad objects through a fetch.fsckObjects filter.
> > Because the objects aren't quarantined on fetch, and because
> > fsck_finish() requires the objects to be installed into place, they may
> > ...
> > I think in the long run fetch should implement a similar quarantine
> > procedure to what happens on push.
> 
> Interesting.
> 
> I wonder if we can teach quickfetch codepath to notice the presence
> of fsckObjects, instead of doing a full quarantine.  We can easily
> enumerate those objects that were already in the object database but
> outside of the reachability chain before we pretend that we fetched
> them and make them reachable, and check the content integrity of
> them, no?

Yes, we could. But it kind of feels like plugging holes in the dike.
That saves "fetch" from referencing them accidentally, but other git
programs may see and react to them. E.g., you're just an "update-ref"
away from referencing the bad history. I don't expect that most
attackers can then convince a victim to reference the rejected objects,
but it feels a lot more hand-wavy than just saying "we don't let these
objects into the repository in the first place".

-Peff