Re: Unaligned accesses in sha1dc

> On Fri, Jun 2, 2017 at 2:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > Martin Ågren <martin.agren@gmail.com> writes:
> >
> >> I looked into this some more. It turns out it is possible to trigger
> >> undefined behavior on "next". Here's what I did:
> >> ...
> >>
> >> This "fixes" the problem:
> >> ...
> >> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
> >> index 3dff80a..d6f4c44 100644
> >> --- a/sha1dc/sha1.c
> >> +++ b/sha1dc/sha1.c
> >> @@ -66,9 +66,9 @@
> >> ...
> >> With this diff, various tests which seem relevant for SHA-1 pass,
> >> including t0013, and the UBSan-error is gone. The second diff is just
> >> a monkey-patch. I have no reason to believe I will be able to come up
> >> with a proper and complete patch for sha1dc. And I guess such a thing
> >> would not really be Git's patch to carry, either. But at least Git
> >> could consider whether to keep relying on undefined behavior or not.
> >>
> >> There's a fair chance I've mangled the whitespace. I'm using gmail's
> >> web interface... Sorry about that.
> >
> > Thanks.  I see Marc Stevens is CC'ed in the thread, so I'd expect
> > that the final "fix" would come from his sha1collisiondetection
> > repository via Ævar.
> >
> > In the meantime, I am wondering if it makes sense to merge the
> > earlier update with #ifdef ALLOW_UNALIGNED_ACCESS and #ifdef
> > SHA1DC_FORCE_LITTLEENDIAN for the v2.13.x maintenance track, which
> > would at least unblock those on platforms v2.13.0 did not work
> > correctly at all.
> >
> > Ævar, thoughts?
> 
> I think we're mixing up several things here, which need to be untangled:
> 
> 1) The sha1dc works just fine on most platforms even with undefined
> behavior, as evidenced by 2.13.0 working.
> 
> 2) There was a bug in practice with unaligned access on SPARC. It's
> not clear to me whether anyone (Andreas, Liam?) still has any issues
> in practice on any platform without specifying compile flags like what
> Martin Ågren suggested above.
> 
> Andreas: Is your initial report of unaligned access here fixed in the
> next branch with my "sha1dc: update from upstream" commit? You didn't
> say what platform you were on.
> 
> Liam: How about your issue on SPARC?

> 
>     make -j8 CFLAGS="-fsanitize=undefined
> -fsanitize-recover=undefined" LDFLAGS="-fsanitize=undefined
> -fsanitize-recover=undefined" all
> 
> And then run e.g.:
> 
>     ./t0020-crlf.sh -v

> 
> To get spiel like:
> 
>     sha1dc/sha1.c:346:2: runtime error: load of misaligned address
> 0x5610bf16d005 for type 'const uint32_t', which requires 4 byte
> alignment
>     0x5610bf16d005: note: pointer points here
>      65 6e 74 20 66 30 34  66 61 39 37 36 36 64 62  62 38 65 34 63 37
> 33 38  34 37 30 61 31 36 63 61  62
> 
> I think that this is definitely something worth looking into /
> coordinating with upstream, but I haven't seen anything to suggest
> that we need to be rushing to get a patch in to fix this given 1) and
> nobody saying yet that 2) doesn't solve their issue as long as they're
> not supplying some -fsanitize=* flags.
> 
> Now, stepping a bit back from this whole thing: I didn't read the
> entire discussion back in February when sha1dc was integrated, but I
> really don't see given all this churn / bug reporting we're getting
> now why another acceptable solution wouldn't be to just revert
> e6b07da278 ("Makefile: make DC_SHA1 the default", 2017-03-17) &
> release 2.13.1 with that.
> 
> Clearly there are outstanding issues with it, and needing to do a
> memcpy() as my `next` patch does will harm performance on some
> platforms, and something like Martin's patch on top will slow it down
> even more.
> 
> It seems to me that we should give it more time to cook, and better
> understand the various trade-offs involved. The shattered attack is
> very unlikely to impact anything in practice, and users who are
> paranoid about it can opt-in to this extra protection.