Re: [PATCH] fsck: check skiplist for object in fsck_blob()

[Jeff King <peff@peff.net>]

On Wed, Jul 04, 2018 at 01:12:40AM +0100, Ramsay Jones wrote:

> > True that we don't even bother doing the parsing with your patch. But I
> > think I talked myself out of that part being a significant savings
> > elsewhere.
> 
> [Sorry for the late reply - watching football again!]
> 
> I'm not interested in any savings - it would have to be a pretty
> wacky repo for there to be much in the way of savings!
> 
> Simply, I have found (for many different reasons) that, if there
> is no good reason to execute some code, it is _far_ better to not
> do so! ;-)

Heh. I also agree with that as a guiding principle. But I _also_ like
the principle of "if you do not need to do add this code, do not add
it". So the two are a little at odds here. :)

> > I'm not sure. This has been running on every push to GitHub for the past
> > 6 weeks, and this is the first report. It's hard to say what that means,
> > and technically speaking of course this _is_ a regression.
> 
> Hmm, are you concerned about old clients 'transmitting' the
> bad data via large hosting sites? (New clients would complain
> about a dodgy .gitmodules file, no matter were it came from,
> right?)

I just meant above that anybody with a broken .gitmodules would have had
their push rejected, and we haven't gotten any such reports beyond yours
and Mike's. So that's some evidence that it's relatively rare.

As far as why those fsck checks are there in the first place, yes, it's
about transmitting bad data to unpatched clients. Ultimately the
responsibility for not being vulnerable is on the clients themselves.
But in practice large hosting sites can help by not being vectors.

> Has the definition of the config file syntax changed recently?
> If not, then old client versions will see the same syntax errors
> as recently 'fixed' versions. So they should error out without
> 'looking' at the bad data, right? (ignoring the 'lets fix the
> obvious syntax error' idea).

No, it hasn't change. So as far as I know, loosening the syntax check
does not impact _this_ vulnerability, because it requires a file that
can be parsed, and the parser is the same for both cases. It might
affect future vulnerabilities. We could also tighten when those come to
light, or tighten in a way that blocks the specific bug. But there's
potentially some value in putting our foot down _now_ and saying that
we're going to enforce certain properties of special files, before it
gets any further out-of-hand.

And of course I could be wrong. We do occasionally fix bugs in the
parser, so I won't be surprised if there's some obscure case where git
<2.0 would not be protected or something like that. But frankly,
anything that old is probably already vulnerable to other ancient bugs,
too.

> > Thanks. If we're going to do any loosening, I think we may want to
> > address that _first_, so it can go directly on top of the patches in
> > v2.17.1 (because it's a bigger issue than the stray message, IMHO).
> 
> Agreed. I probably haven't given it sufficient thought, but my
> immediate reaction is to loosen the check - I don't see how
> this could be exploited to significantly reduce security. (My lack
> of imagination has been noted several times in the past, however!)
> 
> Having said that, I am no security expert, so I will let those who
> have security expertise decide what is best to do in this situation.

I think you have a grasp on the situation from what you wrote above.

What next? I was kind of leaning towards loosening, but it sounded like
Junio thought the opposite. One thing I didn't like about the patch I
sent earlier is that it removes the option for the admin to say "no, I
really do want to enforce this". I don't know if that's of value or not.

In an ideal world, I think we'd detect the problem and then react
according to the receiver's receive.fsck.* config. And we could just
flip the default for receive.fsck.gitmodulesParse to "warning". But
IIRC, the fsck code in index-pack does not bother distinguishing between
warnings and errors. I think it ought to, but that's too big a change to
go on maint.

It _might_ work to just flip the default to "ignore". That leaves the
paranoid admin with a way to re-enable it if they want, and I _think_ it
would be respected by index-pack.

[goes and looks at the code]

Hmm, we seem to have "info" these days, so maybe that would do what I
want. I.e., I wonder if the patch below does everything we'd want. It's
late here and I probably won't get back to this until Monday, but you
may want to play with it in the meantime.

diff --git a/fsck.c b/fsck.c
index 48e7e36869..0b0003055e 100644
--- a/fsck.c
+++ b/fsck.c
@@ -61,7 +61,6 @@ static struct oidset gitmodules_done = OIDSET_INIT;
 	FUNC(ZERO_PADDED_DATE, ERROR) \
 	FUNC(GITMODULES_MISSING, ERROR) \
 	FUNC(GITMODULES_BLOB, ERROR) \
-	FUNC(GITMODULES_PARSE, ERROR) \
 	FUNC(GITMODULES_NAME, ERROR) \
 	FUNC(GITMODULES_SYMLINK, ERROR) \
 	/* warnings */ \
@@ -76,6 +75,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;
 	FUNC(NUL_IN_COMMIT, WARN) \
 	/* infos (reported as warnings, but ignored by default) */ \
 	FUNC(BAD_TAG_NAME, INFO) \
+	FUNC(GITMODULES_PARSE, INFO) \
 	FUNC(MISSING_TAGGER_ENTRY, INFO)
 
 #define MSG_ID(id, msg_type) FSCK_MSG_##id,