Re: SHA1 collisions found

[Jeff King <peff@peff.net>]

On Thu, Feb 23, 2017 at 11:47:16AM -0800, Linus Torvalds wrote:

> On Thu, Feb 23, 2017 at 11:32 AM, Jeff King <peff@peff.net> wrote:
> >
> > Yeah, they're not expensive. We've discussed enabling them by default.
> > The sticking point is that there is old history with minor bugs which
> > triggers some warnings (e.g., malformed committer names), and it would
> > be annoying to start rejecting that unconditionally.
> >
> > So I think we would need a good review of what is a "warning" versus an
> > "error", and to only reject on errors (right now the NUL thing is a
> > warning, and it should probably upgraded).
> 
> I think even a warning (as opposed to failing the operation) is
> already a big deal.
> 
> If people start saying "why do I get this odd warning", and start
> looking into it, that's going to be a pretty strong defense against
> bad behavior. SCM attacks depend on flying under the radar.

Sorry, I conflated two things there. I agree a warning is better than
nothing. But right now transfer.fsck croaks even for warnings, and there
are some warnings that it is not worth croaking for. So before we turn
it on, we need to stop croaking on warnings (and possibly bump up some
warnings to errors).

I think it _is_ important to have dangerous things as errors, though.
Because it helps an unattended server (where nobody would see the
warning) avoid being a vector for spreading malicious objects to older
clients which do not do the fsck.

> There's actually already code for that, pointed to by the shattered project:
> 
>   https://github.com/cr-marcstevens/sha1collisiondetection
> 
> the "meat" of that check is in lib/ubc_check.c.

Thanks, I hadn't seen that yet. That doesn't look like it should be hard
to integrate into Git.

-Peff