Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 29, 2017, 20:02 UTC
Message-ID: <alpine.DEB.2.20.1703291509450.48202@virtualbox>
In-Reply-To: <xmqq7f3d6ev1.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Fri, 24 Mar 2017, Junio C Hamano wrote:

Show 11 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > - the most important part will be the patch turning core.enableSHA1DC
> > into a tristate: "externalOnly" or "smart" or "auto" or something
> > indicating that it switches on collision detection only for commands
> > that accept objects from an outside source into the local repository,
> > such as fetch, fast-import, etc
> 
> There are different uses of SHA-1 hashing in Git, and I do agree
> that depending on the use, some of them do not need the overhead for
> the collision-attack detection.

Indeed.

I guess I should have clarified very clearly what I intended to accomplish with this preview.

Let me first summarize the background:

- Git originally used SHA-1 as a convenient catch-all hashing algorithm,
  with the security of the hash merely being a nice afterthought.
  Unfortunately, SHA-1 was hardcoded. Mistakes were made. They always are.

- After the SHAttered blog post became public, Linus first made the case
  that it matters not all that much: the development of the Linux kernel
  is based on trust, and nobody would pull from a person they do not trust.
  This approach does obviously not extend to most other projects.

- By switching the default to DC_SHA1 already in `master`, you now took
  the exact opposite position: it *always* matters, even when you trust
  people, and the 6x slowdown demonstrated by my perf test is something that
  everybody needs to accept, even if it is spent for no benefit in return.

Between these two extremes ("collision attacks do not matter if you run your project based on trust" vs "we always mistrust everybody, including the user's own source code that they stage for commit"), I think there are many shades of green, and I think it would be delusional to believe that we can predict the trust model for each and every Git user [*1*], baking it into a single compile time setting.

That is why I wanted to implement a tristate config so that users can adapt Git to their particular scenario. That way, maintainers of precompiled Git packages do not have to dictate to Git users what trust model they should use.

One scenario seems to be common, and it is certainly one that I have a direct interest in supporting: inside a company, where the server as well as the developers are implicitly trusted not to fiddle with collision attacks (because there are much easier ways to hide malicious code, let's be frank). And in this scenario, slowing down the SHA-1 computation half an order of magnitude by trying to detect collision attacks is simply unacceptable, because there is no benefit, only cost to it.

An even more common scenario is when a developer works on a local repository, adds a couple of files, then runs rebase, then merges, etc. In *none* of these cases does the developer distrust any of the objects flying about. Like above, forcing the developer to accept half an order of magnitude slow down of the SHA-1 computation is something I would consider disrespectful of those developers' time. Note: in this scenario, any object coming from elsewhere would most likely be subject to the collision detection, as the developer may not trust anybody but themselves.

In other words: I disagree that, say, `git add` should use the collision detecting SHA-1.

I also suspect that you had a much more elaborate (maybe even fragile) strategy than mine in mind when you tried to determine which code paths would need collision detection and which ones would not: we have *no* context in the object-oriented sense whenever we call the object hashing functions. Meaning that you would have to introduce such a context, or to add some sort of thread local state. I have to admit that I do not like either way.

The approach I chose instead was to make the switch global, per command. Obviously, the next step is to identify the Git commands which accept objects from external sources (`clone`, `fetch`, `fast-import` and all the remote helpers come to mind) and let them set a global flag before asking git_default_config() to parse the core.enableSHA1DC setting, so that the special value `external` could trigger the collision detecting code for those, and only those, commands. That would still err on the safe side if these commands are used to hash objects originating from the same machine, but that is a price I would be willing to pay for the simplicity of this approach.

Does my explanation manage to illustrate in a plausible way why I chose the approach that I did?

Ciao, Johannes

Footnote *1*: It is equally delusional, of course, to claim that every Git user can and should configure and compile Git for themselves.