Introduce (optional) SHA1 collision attack detection by ethomson · Pull Request #4136 · libgit2/libgit2 · GitHub

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/

The imported code is not ISO C90 compatible, there's a declaration after code in src/hash/sha1dc/sha1.c:55.

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

All fixed up!

This #define is currently unused?

Yep, good 👁 . I've dropped this.

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.

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.

Just some thoughts, feel free to tell me I'm petty.

s/sha1/SHA-1/ ?

Done

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.

🤔

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.

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.

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

ethomson force-pushed the ethomson/sha1dc branch from f5234b0 to d65e408 Compare February 25, 2017 12:47

pks-t reviewed Mar 1, 2017

View reviewed changes

ethomson force-pushed the ethomson/sha1dc branch from 08fd219 to fb2b98b Compare March 2, 2017 20:59

carlosmn approved these changes Mar 3, 2017

View reviewed changes

Edward Thomson added 4 commits March 3, 2017 10:50

hash: rename implementation selection constants

a371a8a

hash: include sha1collisiondetection

2dfd129

Include the SHA1 collision attack detection library from https://github.com/cr-marcstevens/sha1collisiondetection

hash: test for sha1 collision attack detection

d087c8f

cmake: drop unset SHA1_TYPE

9f128d2

We never set `SHA1_TYPE` to `builtin`. Don't bother testing for it.

ethomson force-pushed the ethomson/sha1dc branch from fb2b98b to 9f128d2 Compare March 3, 2017 10:50

carlosmn merged commit 3348570 into master Mar 3, 2017

gnawhleinad pushed a commit that referenced this pull request Mar 28, 2017

Merge pull request #4136 from libgit2/ethomson/sha1dc

704bb61

Introduce (optional) SHA1 collision attack detection

gnawhleinad pushed a commit that referenced this pull request Mar 29, 2017

Merge pull request #4136 from libgit2/ethomson/sha1dc

ea36f55

Introduce (optional) SHA1 collision attack detection

gnawhleinad pushed a commit that referenced this pull request Mar 29, 2017

Merge pull request #4136 from libgit2/ethomson/sha1dc

554b281

Introduce (optional) SHA1 collision attack detection

gnawhleinad pushed a commit that referenced this pull request Mar 29, 2017

Merge pull request #4136 from libgit2/ethomson/sha1dc

46dc7ab

Introduce (optional) SHA1 collision attack detection

johnhaley81 mentioned this pull request Apr 20, 2017

Bump libgit2 to 9e76f2c7 nodegit/nodegit#1276

Merged

ethomson deleted the ethomson/sha1dc branch January 9, 2019 10:18

+              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")

+              	git_oid __expected; \
+              	git_oid_fromstr(&__expected, (idstr)); \
+              	cl_assert_equal_oid(&__expected, (oid)); \
+              }

               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"));

Introduce (optional) SHA1 collision attack detection by ethomson · Pull Request #4136 · libgit2/libgit2 · GitHub

Repository navigation

Introduce (optional) SHA1 collision attack detection - #4136

Introduce (optional) SHA1 collision attack detection#4136carlosmn merged 4 commits intomasterlibgit2/libgit2:masterfrom ethomson/sha1dclibgit2/libgit2:ethomson/sha1dcCopy head branch name to clipboard

Conversation

ethomson commented Feb 24, 2017

Uh oh!

pks-t commented Feb 24, 2017

Uh oh!

ethomson commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ethomson commented Feb 24, 2017

Uh oh!

ethomson commented Feb 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosmn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethomson commented Mar 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Introduce (optional) SHA1 collision attack detection#4136
carlosmn merged 4 commits into
masterfrom
ethomson/sha1dc

ethomson commented Feb 24, 2017 •
edited
Loading