Only allow crypto-safe hashes to be used in CIDs #40

whyrusleeping · 2018-02-22T21:38:03Z

This is a WIP solution to ipfs/kubo#4371

Stebalien · 2018-02-22T22:32:01Z

validate.go

+	mh.ID:           true,
+
+	// insecure
+	mh.SHA1:    false,


This will, of course, break the git plugin.

mmm, youre right.

As suggested by @magik6k we should probably set SHA1 as secure, but add the check described here: multiformats/go-multihash#57 to detect insecure hashes.

@whyrusleeping this would still break the git plugin, but only in the very rare case when the content so happens to hash to one of the insure hashes, or am I missing something?

@whyrusleeping yep to what? Am I missing something or will this still break the git plugin in the very rare case?

yes. This will still break the git plugin in that case.

From IRC (slightly edited for clarity):

whyrusleeping: juan wants this to make it into the next release
kevina: whyrusleeping: I vote we allow "sha1" for now to avoid breaking the git plugin until we think of a better solution
whyrusleeping: yeah, maybe.
whyrusleeping: I think most of the concern we want to fix here is preimage attacks
whyrusleeping: and theres no preimage attack for sha1
whyrusleeping: just a collision
kevina: whyrusleeping, stebalien: so let's allow it with a note that it will eventually be marked insure once we implement something like /ipfs-insecure/

Stebalien · 2018-02-22T22:33:53Z

validate.go

+		return ErrPossiblyInsecureHashFunction
+	}
+
+	if pref.MhType != mh.ID && pref.MhLength < minimumHashLength {


I'd rather check if the hash length >= the default. Alternatively, make goodset map hash function to minimum length (where we'd use -1 for ID).

Hrm... I still want to allow some truncation. Ethereum truncates a lot of things down to 20 bytes (from 32). I can go with a mapping, but maybe default 20 and have a special cases for anything else?

I think we should set this per hash as @Stebalien suggested to allow a little more flexibility.

@whyrusleeping WTF? That's 80 bits of security! No!

@Stebalien hold up, 8 * 20 is 160.

Hash functions have bits/2 security for collision attacks (I assumed this was a general property and applied to preimage attacks but it turns out this is due to the birthday attack and only applies to collisions; thanks for catching me on that).

However, unlike eth, we do care about collisions attacks. Eth doesn't because it only uses 160 bit hashes for account IDs so, at most, you can steal money from yourself by creating two colliding keys.

Now, whether or not a birthday attack against an 160 bit hash will likely be feasible in the foreseeable future is an interesting question (there are time/memory tradeoffs one can make to reduce the otherwise obscene memory complexity) but, if our answer is no, we should just truncate all of our CIDs.

@Stebalien the whole point of multiformats in general is to allow users to use what fits their needs best. We should continue using 256bit hashes for our important things, but shouldnt begrudge users who want to use 160bit hashes. I'm primarily concerned about preventing pre-image attacks, general collision birthday problem attacks are still a concern, but less so for now.

So, I've come to agree on this. It still annoys me, but we don't have much of a choice.

If we ever need to set different minimum lengths for different hashes, we can change that later.

Also, users can implement custom DAGServices for validating additional properties (we may want won for unixfs that has stricter requirements).

Kubuxu · 2018-02-24T19:48:47Z

@whyrusleeping I can work with this API if in future we want to swap this out for multihash-secruity-bits.

See: ipfs/kubo#4371

whyrusleeping · 2018-03-01T18:56:58Z

cid.go

-	return &Cid{
+func NewCidV0(mhash mh.Multihash) (*Cid, error) {
+	if mhash[0] != mh.SHA2_256 && mhash[1] != byte(mh.DefaultLengths[mh.SHA2_256]) {
+		return nil, errors.New("NewCidV0 accepts only SHA256 hashes of standard length")


I would pull this error message out into a global

magik6k

1 minor nitpick, LGTM

magik6k · 2018-03-01T21:49:53Z

cid.go

@@ -51,6 +51,8 @@ var (
 	// ErrInvalidEncoding means that selected encoding is not supported
 	// by this Cid version
 	ErrInvalidEncoding = errors.New("invalid base encoding")
+
+	ErrCid0OnlySHA256 = errors.New("cidv0 accepts only SHA256 hashes of standard length")


ErrCidV0Format?

kevina · 2018-04-18T19:56:15Z

This will likely require a repo migration, to cleanly upgrade as I suggested in ipfs/kubo#4766.

hsanjuan · 2018-05-31T12:23:58Z

Is this going to be merged, or is it blocked by something?

whyrusleeping · 2018-05-31T12:27:57Z

we implemented a workaround in go-ipfs for now. This change could brick peoples repos if they already had bad hashes in them, so we removed the ability to create bad hashes, we're working on a migration to remove any existing bad hashes, and then once that is done, we can merge this.

kevina · 2018-06-16T02:20:28Z

@whyrusleeping @Stebalien is the only hold of this my repo migration? Or is this blocked on anything else. In particular how quickly can go-ipfs update a new go-cid version considering that it is used almost everywhere and will require a lot of package updates?

Stebalien · 2018-06-16T04:37:30Z

@kevina as far as I know, nothing is blocking a go-cid update. I can do it in a few hours with gx-workspace and a stress ball.

Stebalien · 2018-06-16T04:48:03Z

validate.go

+		return ErrPossiblyInsecureHashFunction
+	}
+
+	if pref.MhType != mh.ID && pref.MhLength < minimumHashLength {


So, I've come to agree on this. It still annoys me, but we don't have much of a choice.

If we ever need to set different minimum lengths for different hashes, we can change that later.

Stebalien · 2018-06-16T04:50:51Z

validate.go

+	mh.KECCAK_512:   true,
+	mh.ID:           true,
+
+	mh.SHA1: true, // not really secure but still useful


Could we restrict this to git only somehow?

I don't necessary agree with this unless we are 100% sure SHA-1 are not used anywhere outside of git. Otherwise we could break things, badly.

The only reason we want to call SHA1 a "secure" hash because we want git compatibility. SHA1 is not a secure hash function. I know there are no other existing IPLD formats using SHA1 by default. If users have chosen to use SHA1 with IPLD, well, I'm not exactly sympathetic.

If we find some existing merkledag format that uses SHA1 and want to add compatibility, we can add that to the list later.

Note: I'm fine pushing this patch through as-is and fixing this later as this patch is a strict improvement.

Stebalien · 2018-06-16T04:53:45Z

validate.go

+		return ErrPossiblyInsecureHashFunction
+	}
+
+	if pref.MhType != mh.ID && pref.MhLength < minimumHashLength {


Also, users can implement custom DAGServices for validating additional properties (we may want won for unixfs that has stricter requirements).

kevina · 2018-06-24T22:34:30Z

@whyrusleeping @Kubuxu is it okay if I take over this p.r.? For now I pushed my work on #50.

kevina

Blocking as I have outstanding changing on #50. Once I get the all clear to take over this P.R. I will unblock.

lidel · 2021-09-15T14:26:50Z

This PR never landed and got out of date,
I am closing it as it is most likely easier to create new one if we ever want to move beyond current solution (blocking at bockstore instead in go-cid – continued in ipfs/kubo#4371 (comment) ++

add function to check if a given cid is safe

881ca1a

ghost assigned whyrusleeping Feb 22, 2018

ghost added the status/in-progress In progress label Feb 22, 2018

whyrusleeping mentioned this pull request Feb 22, 2018

Forbid non-cryptographic hashes within /ipfs namespace ipfs/kubo#4371

Open

Stebalien reviewed Feb 22, 2018

View reviewed changes

For now allow SHA1

5bb572a

ghost assigned Kubuxu Feb 27, 2018

Kubuxu added 2 commits February 27, 2018 13:06

Add testing for ValidateCid

7c94d54

Enforce the security rules of crypto-safe hashes and min length

2166e30

See: ipfs/kubo#4371

Kubuxu requested review from Stebalien, kevina and magik6k March 1, 2018 16:26

This comment has been minimized.

Sign in to view

whyrusleeping commented Mar 1, 2018

View reviewed changes

Kubuxu added 2 commits March 1, 2018 20:20

Define cidv0 error

71aa36b

gx publish 0.7.21

2f731f0

magik6k approved these changes Mar 1, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

kevina mentioned this pull request Mar 5, 2018

Bump Repo Version and Provide Migration before strict Cid code lands ipfs/kubo#4766

Open

kevina changed the title ~~add function to check if a given cid is safe~~ Only allow crypto-safe hashes to be used in CIDs Apr 21, 2018

kevina mentioned this pull request Apr 21, 2018

[WIP] Next round of extractions (blockservice, merkledag, path) ipfs/kubo#4960

Closed

Stebalien reviewed Jun 16, 2018

View reviewed changes

kevina mentioned this pull request Jun 24, 2018

[WIP] Continuration of: Only allow crypto-safe hashes to be used in CIDs #50

Closed

kevina added the status/blocked Unable to be worked further until needs are met label Jun 25, 2018

kevina suggested changes Jun 25, 2018

View reviewed changes

kevina mentioned this pull request Jun 28, 2018

Tweak parameters of NewPrefix functions. #51

Closed

kevina mentioned this pull request Sep 11, 2018

continuation of use string instead of struct backing #71

Merged

lidel closed this Sep 15, 2021

lidel deleted the feat/safe-hash-whitelist branch September 15, 2021 14:27

Only allow crypto-safe hashes to be used in CIDs #40

Only allow crypto-safe hashes to be used in CIDs #40

Conversation

whyrusleeping commented Feb 22, 2018 • edited by kevina Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevina Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevina Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu commented Feb 24, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

magik6k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

kevina commented Apr 18, 2018 • edited Loading

hsanjuan commented May 31, 2018

whyrusleeping commented May 31, 2018

kevina commented Jun 16, 2018

Stebalien commented Jun 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevina commented Jun 24, 2018

kevina left a comment

Choose a reason for hiding this comment

lidel commented Sep 15, 2021

whyrusleeping commented Feb 22, 2018 •

edited by kevina

Loading

kevina Feb 23, 2018 •

edited

Loading

kevina Feb 23, 2018 •

edited

Loading

kevina commented Apr 18, 2018 •

edited

Loading