Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discourage use of broken hashes like SHA1 #8703

Open
lidel opened this issue Jan 28, 2022 · 4 comments
Open

Discourage use of broken hashes like SHA1 #8703

lidel opened this issue Jan 28, 2022 · 4 comments
Labels
exp/intermediate Prior experience is likely helpful help wanted Seeking public contribution on this issue kind/enhancement A net-new feature or improvement to an existing feature P1 High: Likely tackled by core team if no one steps up topic/commands:add Topic commands:add topic/security Topic security
Milestone

Comments

@lidel
Copy link
Member

lidel commented Jan 28, 2022

Problem

Suggestion extracted from #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 #4371

@lidel lidel added kind/enhancement A net-new feature or improvement to an existing feature help wanted Seeking public contribution on this issue topic/security Topic security topic/commands:add Topic commands:add P3 Low: Not priority right now exp/intermediate Prior experience is likely helpful labels Jan 28, 2022
@lidel lidel mentioned this issue Jan 28, 2022
3 tasks
@lidel lidel changed the title Error on ipfs add --hash=sha1, but allow with --force Discourage use of broken hashes like SHA1 Jan 28, 2022
@BigLep BigLep added this to the Best Effort Track milestone Mar 10, 2022
@BigLep BigLep moved this to 🥞 Todo in IPFS Shipyard Team Mar 10, 2022
@lidel lidel added P1 High: Likely tackled by core team if no one steps up and removed P3 Low: Not priority right now labels Jun 9, 2022
@lidel lidel modified the milestones: Best Effort Track, go-ipfs 0.14 Jun 9, 2022
@lidel lidel moved this from 🥞 Todo to 🔎 In Review in IPFS Shipyard Team Jun 9, 2022
@aschmahmann
Copy link
Contributor

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: multiformats/go-multihash#57

@Jorropo
Copy link
Contributor

Jorropo commented Jun 14, 2022

Other option:

  • Rewrite or machine translate the cryptoanalytic fix in pure Go.

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
Copy link
Contributor

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
Copy link
Contributor

hsanjuan commented Nov 8, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/intermediate Prior experience is likely helpful help wanted Seeking public contribution on this issue kind/enhancement A net-new feature or improvement to an existing feature P1 High: Likely tackled by core team if no one steps up topic/commands:add Topic commands:add topic/security Topic security
Projects
No open projects
Status: 🔎 In Review
Development

No branches or pull requests

5 participants