ipfs / kubo

An IPFS implementation in Go
https://docs.ipfs.tech/how-to/command-line-quick-start/
Other
16.18k stars 3.02k forks source link

Discourage use of broken hashes like SHA1 #8703

Open lidel opened 2 years ago

lidel commented 2 years ago

Problem

Suggestion extracted from https://github.com/ipfs/go-ipfs/issues/8650

SHA1 is considered to be broken, and systems like git moved away from it.

IPFS makes migration wa easy thanks to abstraction provided by Multihash. go-ipfs switched the defaults in ipfs add to SHA2-256, but in theory, one can still use SHA1 to add new files if they opt-in.

We should have a mechanism to discourage users from using broken legacy functions like sha1 for adding new files.

Solution

There should be a list of "legacy/deprecated" hash functions like SHA1 which would be discouraged by requiring explicit --force flag:

$ ipfs add --hash=sha1 file
Error: selected hash function is no longer secure; use --hash=sha2-256 or pass --force

$ ipfs add --hash=sha1 file
Added ...

Note: this could be a part of a bigger refactor related to https://github.com/ipfs/go-ipfs/issues/4371

aschmahmann commented 2 years ago

Basically all the insecure hash functions are already forbidden by https://github.com/ipfs/go-verifcid/blob/46876f413195d36227ee5c7d6f16e995b62f754d/validate.go#L15. The only exception here is SHA1 because at the time it was deemed valuable (and I assume insufficiently broken) https://github.com/ipfs/go-verifcid/blob/46876f413195d36227ee5c7d6f16e995b62f754d/validate.go#L30.

This means that at the moment this really only applies to SHA1.


We could also just make using SHA-1 safer by utilizing the cryptoanalytic "fix" used in places like Git https://github.com/cr-marcstevens/sha1collisiondetection.

To do this there are two pieces:

  1. Make https://github.com/ipfs/distributions be able to build with CGO
  2. Have go-ipfs use something like https://github.com/aschmahmann/go-sha1collisiondetection (CGO wrapper around the C library with the cryptoanalytic fix). Work on the wrapper started in a PR to that repo.

Related: https://github.com/multiformats/go-multihash/issues/57

Jorropo commented 2 years ago

Other option:

I guess it's mainly just doing arithmetic on byte buffers, this should be trivial to machine translate (and lift up manually).

We should avoid relying on CGO for core features.

aschmahmann commented 2 years ago

We should avoid relying on CGO for core features.

I agree it'd be nice to, but I also suspect this is something we're going to run into wanting anyway. There is so much code written with FFI that restricting ourselves from using any of it seems rough, as opposed to evaluating the code and seeing if the CGO costs are worthwhile.

Rewrite or machine translate the cryptoanalytic fix in pure Go.

Not sure how painful this is. When I was looking around at this I didn't find native implementations in any other language which makes me think maybe it's painful to do given how important it is to have the implementation correct.

If we can get someone to write and review the SHA-1 code in Go that's fine, but IMO blocking on this seems like a bad idea that's prevented this from being integrated earlier (i.e. the linked go-multihash issue is from 2017)

hsanjuan commented 2 weeks ago

This is marked as P1, so probably something should be done. I think we can just remove the SHA1 exception in verify-cid and forget. I don't see why we would spend time integrating collisioncheck tbh. Probably no one is using sha1 in ipfs land anyways, and wouldn't feel sorry for those that do.