Introduce (optional) SHA1 collision attack detection#4136
Introduce (optional) SHA1 collision attack detection#4136
Conversation
|
The imported code is not ISO C90 compatible, there's a declaration after code in Otherwise this looks fine, I guess (even though I obviously didn't skim through the SHA1DC implementation). |
|
Dang, thanks @pks-t , I missed that. (Seriously, you would have thought somebody at Microsoft would have gotten that one right. 😛 ) |
|
AppVeyor's mingw builds are busted. Sigh. 👀 |
f5234b0 to
d65e408
Compare
|
All fixed up! |
tests/core/sha1.c
Outdated
| git_oid __expected; \ | ||
| git_oid_fromstr(&__expected, (idstr)); \ | ||
| cl_assert_equal_oid(&__expected, (oid)); \ | ||
| } |
There was a problem hiding this comment.
Yep, good 👁 . I've dropped this.
| ELSEIF (WIN32 AND NOT MINGW AND NOT SHA1_TYPE STREQUAL "builtin") | ||
| ADD_DEFINITIONS(-DGIT_SHA1_WIN32) | ||
| FILE(GLOB SRC_SHA1 src/hash/hash_win32.c) | ||
| ELSEIF (${CMAKE_SYSTEM_NAME} MATCHES "Darwin") |
There was a problem hiding this comment.
By the way, is it correct that OS X does not check for SHA1_TYPE=="builtin"? Sounds as if it wasn't possible for OS X systems to build with our generic hashing algorithm.
There was a problem hiding this comment.
Good question... digging in a bit deeper, it looks like one can't actually set SHA1_TYPE. So I'm removing these tests for builtin entirely.
08fd219 to
fb2b98b
Compare
carlosmn
left a comment
There was a problem hiding this comment.
Just some thoughts, feel free to tell me I'm petty.
CMakeLists.txt
Outdated
| OPTION( ENABLE_TRACE "Enables tracing support" OFF ) | ||
| OPTION( LIBGIT2_FILENAME "Name of the produced binary" OFF ) | ||
|
|
||
| OPTION( USE_SHA1DC "Use sha1 with collision detection" OFF ) |
|
|
||
| #ifdef GIT_SHA1_COLLISIONDETECT | ||
| GIT_UNUSED(expected); | ||
| cl_git_fail(sha1_file(&oid, FIXTURE_DIR "/shattered-1.pdf")); |
There was a problem hiding this comment.
Do we want to have a specific error code for this as well, so we can do cl_git_fail_with(GIT_ESHA1ATTACK, ...)? I'm not sure if this adds that much value, just a thought.
There was a problem hiding this comment.
So... my thinking here is that this is only useful if it's something that you want to act on. I think that if you were a hosting provider that you would actually not want to bother logging these, because once this becomes reasonably cheap and you can create two git objects that have colliding hashes -- or when a research org decides to extend their corpus of files with colliding hashes to git objects and not just PDFs -- then people are going to go crazy uploading them to poke at you. And if you're a client, then this isn't useful knowledge to have at all.
But it's quite easy to implement, so I'm happy to add it if you're passionate about it.
There was a problem hiding this comment.
I just figured a client might want to throw a particular prompt, but it's so far beyond what we have control over that it's not really worth it.
Include the SHA1 collision attack detection library from https://github.com/cr-marcstevens/sha1collisiondetection
We never set `SHA1_TYPE` to `builtin`. Don't bother testing for it.
fb2b98b to
9f128d2
Compare
|
It looks like the git authors and the authors of this SHA-1 collision detection library are interested in optimizing it; we'll revisit this if/when they settle on something more efficient. |
Introduce (optional) SHA1 collision attack detection
Introduce (optional) SHA1 collision attack detection
Introduce (optional) SHA1 collision attack detection
Introduce (optional) SHA1 collision attack detection
Include SHA1 collision attack detection from https://github.com/cr-marcstevens/sha1collisiondetection when configured with
-DUSE_SHA1DC=1.This is the same implementation being proposed for git core; see https://public-inbox.org/git/20170223230536.tdmtsn46e4lnrimx@sigill.intra.peff.net/