Re: When Will We See Collisions for SHA-1? (An interesting analysis by Bruce Schneier)
- From
Jeff King <peff@peff.net>
- Date
- Oct 16, 2012, 17:32 UTC
- Message-ID
- <20121016173254.GD27243@sigill.intra.peff.net>
- In-Reply-To
- <507D4651.6080207@lsrfire.ath.cx>
On Tue, Oct 16, 2012 at 01:34:41PM +0200, René Scharfe wrote:
Show 18 quoted lines
> FWIW, I couldn't measure a performance difference for git log with and
> without the following patch, which catches commits created with your
> hash collision trick, but might be too strict:
>
> diff --git a/commit.c b/commit.c
> index 213bc98..4cd1e83 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -262,6 +262,12 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s
> if (item->object.parsed)
> return 0;
> item->object.parsed = 1;
> +
> + if (memchr(buffer, '\0', size)) {
> + return error("bogus commit contains a NUL character: %s",
> + sha1_to_hex(item->object.sha1));
> + }
> +Hmm. Yeah, that should be relatively inexpensive, since we are about to read through most of the bytes anyway (we probably have just zlib inflated them all, so they would even be in cache). It might make more of a difference for a raw traversal that is not even going to look at below the header, like rev-list or merge-base. But I couldn't measure a difference doing "git rev-list HEAD >/dev/null" in either git.git or linux-2.6.git.
So maybe it is worth doing preemptively. Even without security concerns, we would be truncating the commit message, so it is probably better to let the user know (a warning is probably more appropriate, though, just in case somebody does have embedded NULs for historical reason).
-Peff