fix: refuse creating new DAGs with insecure hash functions (SHA1)

[finkbeca]

Closing #8703, added an optional forcing flag for ipfs add to discourage use of broken hashes like SHA-1 while still allowing it, given you pass in an optional forcing flag. Tested this manually due to the small scope of the changes.

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

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

[guseggert]

Thanks for this!

A more specific flag would make it more obvious that a less secure hash function is being used, like --allow-insecure-hash-function or something like that. Also can you add a sharness test for this? Should be really easy.

[finkbeca]

Thanks for the feedback. I fixed suggested the edits changing the flag name to --allow-insecure-hash-function and added a sharness test for the new command. Hope that cover the change requests. :)

[Jorropo]

Your patch seems to only work for SHA1:

$ ./ipfs add --hash=md5 --allow-insecure-hash-function <(echo test)
Error: potentially insecure hash functions not allowed

(it might be significantly harder to support all unsecure functions, don't do it if you can't, sha1 is a good start 🙂)

[Jorropo]

Thanks, this is definitely a feature we want to merge, however a few things needs some tweaking first. 🙂

[Jorropo]

You should add a test that tries to add a file with an unsecure function and check that it fails.

[finkbeca]

Fixed the syntax changes @Jorropo , also added failure tests specifically for SHA1. I'm aware that the Patch only works for SHA1 at this time, I can change the wording of the error if you think that would be better. My initial thought that generalizing the error statement will simplify future extensions.

EDIT: I've been racking my brain on why the CI tests failed, I only added a single 'fail' test to my sharness. It works locally, based my tests directly off existing tests. Can't figure out what is now failing, as I have changed nothing / how a single test will cause http_client tests to fail.

[lidel]

Thank you, @finkbeca ❤️

Would you have time to add the same flag to dag put and block put – see #8696 for prior art of (--allow-big-block)? If not, let us know, someone else will push this over the finish line.

[Jorropo]

Sorry to ask for more but this:

ipfs add --hash=md5 --allow-insecure-hash randomFileHere

Doesn't work.

I know this is a more complicated task, but I would like to avoid yet an other half working feature.
I don't know what other maintainers think of this.

[lidel]

@Jorropo Sounds sensible. Do you have more things we should block?

@finkbeca Mind replacing static "sha1" with a check against a list of insecure hashes ["sha1", "md5"]?
That way, we can add more as time goes by.

[Jorropo]

Mind replacing static "sha1" with a check against a list of insecure hashes ["sha1", "md5"]?
That way, we can add more as time goes by.

We already have that, it is https://github.com/ipfs/go-verifcid.

Do you have more things we should block?

• Refactor go-verify CID consumers, to work on the command level (instead of deep internals) so we can disable it more easly. (what I am already asking for)
• Remove SHA1 from go-verify CID's safe list. (or only allows the "safe" cryptoanalysis fix, see https://shattered.io/)

[aschmahmann]

(or only allows the "safe" cryptoanalysis fix, see https://shattered.io/)

Seems doable, see: #8703 (comment)

Refactor go-verify CID consumers, to work on the command level (instead of deep internals) so we can disable it more easly. (what I am already asking for)

I don't see a particularly good reason to bother with doing go-verifcid plumbing everywhere to allow sets of insecure hash functions. Note: you likely wouldn't want it to be all or nothing... allowing SHA-1 doesn't necessarily mean allowing a 1 bit SHA-2 hash or MD5.

Allowing the system to support more insecure hash functions seems like a bad idea for a widely deployed consumer binary, especially given that most other users of that binary won't be able to retrieve the data anyhow. Nothing at the library or protocol level stops you from using broken hash functions like MD4, but I don't see a reason why go-ipfs should support using it.

[Jorropo]

I don't see a particularly good reason to bother with doing go-verifcid plumbing everywhere to allow sets of insecure hash functions. Note: you likely wouldn't want it to be all or nothing... allowing SHA-1 doesn't necessarily mean allowing a 1 bit SHA-2 hash or MD5.

I'm saying this is what we currently more less have.
I want to scrape that, and only do the checking in commands handlers (where adding --allow-insecure-hash is trivial to implement).

Nothing at the library or protocol level stops you from using broken hash functions like MD4, but I don't see a reason why go-ipfs should support using it.

$ rgrep -I "verifcid" vendor/ | cut -f1 -d':' | while read f; do dirname ${f}; done | uniq | grep -v verifcid | grep -v "^vendor$"
vendor/github.com/ipfs/go-ipfs-provider/simple
vendor/github.com/ipfs/go-ipfs-provider/batched
vendor/github.com/ipfs/go-blockservice

[aschmahmann]

I want to scrape that, and only do the checking in commands handlers (where adding --allow-insecure-hash is trivial to implement).

This seems unlikely to work. For example ipfs cat bafysomesha2 internally has a pointer to an MD4 hash -> game over. You still need this pretty low down the stack.

Nothing at the library or protocol level stops you from using broken hash functions like MD4, but I don't see a reason why go-ipfs should support using it.

Ok fair enough there are a few library pieces where you'd have to mock out the interfaces or do a fork to remove the checks. IIRC those structs aren't explicitly used rather than their interfaces so not too bad to deal with. However, if there's actual demand for it it's probably fine to add some configurability.

[Jorropo]

This seems unlikely to work. For example ipfs cat bafysomesha2 internally has a pointer to an MD4 hash -> game over. You still need this pretty low down the stack.

Ok fair enough too.

We can put that value in the context (create a allow-insecure-hash bool key), and let verifcid take a context and use that to know if we allows it or not. (we can't use globals obviously since this parameter is request dependent)

[BigLep]

2022-06-16 conversation: @Jorropo will write up the next steps that we'd like to take here since we don't want to introduce something special that is only relevant for sha1.

[Jorropo]

Sorry but we are not intrested in this fix,
It's not actually doing what it claims, I can't use it to add files with MD4.

But mainly.
it only supports ipfs add --hash=sha1
Which is not what is important.

The danger is in ipfs get (and gateway, ipfs cat, ... whatever that read data), because some malicious attacker can create a SHA256 block linking a bad SHA1 hash and send you whatever data they want instead.

We will follow the same route as git, using the sha1 fix to make the function "safe" again. (the only reason sha1 is still there is for git anyway so using the same fix as it make sense).

The context for this is here: #8703 (comment)