Disallow usage of unsafe hashes for reading and adding content #4751

Kubuxu · 2018-03-01T21:02:54Z

Should be done

Kubuxu · 2018-03-01T21:14:39Z

Short observations:
node.Exchange is not used directly to get blocks

it is passed to blockservice
commands/add.go uses it directly but passes it to blockservice. //hash security 001
thus blocked the blockservice

Kubuxu · 2018-03-01T21:23:03Z

Random failure in bitswap:

21:15:20.663 ERROR    bitswap: nil cid in GetBlock get.go:18
--- FAIL: TestBasicBitswap (0.01s)
	bitswap_test.go:318: Test a one node trying to get one block from another
	bitswap_test.go:361: stat node 0
	bitswap_test.go:363: stat node 1
	bitswap_test.go:296: mismatch in blocks sent: 0 vs 1
	bitswap_test.go:300: mismatch in blocks recvd: 1 vs 2
	bitswap_test.go:304: mismatch in data sent: 0 vs 1
	bitswap_test.go:308: mismatch in data recvd: 1 vs 2
	bitswap_test.go:365: stat node 2
	bitswap_test.go:372: [Block QmTTA2daxGqo5denp6SwLzzkLJm3fuisYEi9CoWsuHpzfb]
FAIL
FAIL	github.com/ipfs/go-ipfs/exchange/bitswap	4.173s

Passes locally

Kubuxu · 2018-03-01T21:23:43Z

Sharness has passed. Restarting tests to make them green.
Ignoring the codeclimate, most of this code should be reverted when go-cid can be propagated.

whyrusleeping · 2018-03-01T22:28:25Z

Lets not call the package removeme. Lets just put it in thirdparty (Which is already synonymous for removeme at this point).

whyrusleeping · 2018-03-01T22:28:45Z

blockservice/blockservice.go

@@ -149,6 +155,13 @@ func (s *blockService) AddBlock(o blocks.Block) error {
 }

 func (s *blockService) AddBlocks(bs []blocks.Block) error {
+	//hash security


for consistency, add a space before the word hash

magik6k

Code LGTM, could use some tests.

Travis fail looks random

magik6k · 2018-03-02T20:51:14Z

@whyrusleeping
Kubuxu here (I'm at Magik's atm) thus I'm using his account.
I've added some sharness tests.

I think there might be one issue with this whole code, GC might be failing when there are insecure blocks in the bockstore.
I won't have time to fix it today, I can try fixing it tomorrow morning.

kevina · 2018-03-02T22:01:31Z

Regarding GC, if a block can not be read it is unsafe to continue as that block could potentially point to other blocks that should not be removed.

There are two solutions (1) somehow allow reading of blocks with insecure hashes, (2) force a repo migration, have the the tool check for insecure blocks and then refuse to upgrade until those blocks are dealt with by the user, the tool should also check if the insure blocks point to secure blocks and inform the user of the situation. We could also also provide a separate tool that will automatically convert insecure hashes to a secure hash of there choosing.

Stebalien

As far as a stop-gap, this looks good to me (modulo a few more comments for grepability).

Stebalien · 2018-03-02T22:12:32Z

blockservice/blockservice.go

 }

 func getBlock(ctx context.Context, c *cid.Cid, bs blockstore.Blockstore, f exchange.Fetcher) (blocks.Block, error) {
+	err := verifcid.ValidateCid(c)


Add // hash security comment for future grep.

Stebalien · 2018-03-02T22:13:47Z

core/commands/add.go

@@ -258,7 +258,7 @@ You can now check what blocks have been created by:
 			exch = offline.Exchange(addblockstore)
 		}

-		bserv := blockservice.New(addblockstore, exch)
+		bserv := blockservice.New(addblockstore, exch) // hash security 001


Does this need the hash security comment?

This is the only place n.Exchnage is used standalone. ~Kubuxu

kevina · 2018-03-02T23:31:27Z

As this code still allows the use of CID's with insecure hashes, a potentially easy solution to the GC problem allow the reading of insecure hashes, but disallow the publishing or writing of insecure hashes. This will also provide a gentler upgrade path for the rare case when a user has a repo with insecure hashes.

Another solution to just the GC problem is to allow the reading of insecure hashes in the local blockstore, but not the blockservice. This may also allow the user to read them locally, but I am not sure as I think the blockservice is always used even in offline mode. I am also unsure if this will prevent the publishing of insecure hashes.

A third solution is to put a special case in the GC code that will dig out the underlying blockstore and use that directly. I like this solution the least.

kevina

Things will break badly in the unlikely event that there are blocks with an insecure hash in the repo. See my other comments.

kevina · 2018-03-03T06:24:05Z

@Kubuxu @whyrusleeping as I said in IRC I can take over this P.R. if it will help.

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

whyrusleeping · 2018-03-04T20:47:24Z

core/commands/pin.go

@@ -462,25 +463,23 @@ var verifyPinCmd = &cmds.Command{
 		cmds.Text: func(res cmds.Response) (io.Reader, error) {
 			quiet, _, _ := res.Request().Option("quiet").Bool()

-			outChan, ok := res.Output().(<-chan interface{})
+			out, err := unwrapOutput(res.Output())


this looks like its being changed to only emit a single output. Whats going on here?

I would like to know what is going on also.

The marshaller is called for every message pushed down the channel. I am not sure why the legacy layer is doing this but it prints out all the results.

(this is one of the many reasons I've given up on fixing issues with the compat layer and have started just converting commands to use the new system).

whyrusleeping · 2018-03-04T21:07:37Z

test/sharness/t0275-cid-security.sh

+# should work online
+test_launch_ipfs_daemon
+test_cat_get
+test_kill_ipfs_daemon


Could you add a test in here that manually puts a bad hash into the datastore, and then runs a gc?

whyrusleeping · 2018-03-04T21:19:20Z

This tenatively looks good to me. I'd like to see a few more tests before moving forward though.

whyrusleeping · 2018-03-04T23:00:22Z

@Stebalien want to give the gc changes another lookover here?

Kubuxu · 2018-03-04T23:42:21Z

@whyrusleeping I've added test for general GC but I am unable to test pin set cases. This is because there is no way to add insecure block to pinset.

kevina · 2018-03-04T23:47:24Z

@Kubuxu if there are insecure hashes will those end up being published?

Kubuxu · 2018-03-05T01:51:02Z

@kevina you mean by reprovider? Yes but it will continue to work.

Kubuxu · 2018-03-05T01:58:40Z

@kevina reprovider won't be providing insecure hashes anymore

kevina

[Please Ignore]

I am not thrilled with the idea of forcing a user to downgrade to read insure hashes, but as a stop-gap measure this will work, especially since it is unlikely that the insure hashes are used much, if at all.

kevina · 2018-03-05T02:31:36Z

@Kubuxu okay, We should be sure and mark any places where Cid's are verified so that they can cleanly be removed once the strict Cid code lands.

hsanjuan

Please don't grow thirdparty, I have to extract these modules and it puts more work on me.

hsanjuan · 2018-03-05T16:18:04Z

blockservice/blockservice.go

@@ -10,6 +10,7 @@ import (
 	"io"

 	exchange "github.com/ipfs/go-ipfs/exchange"
+	"github.com/ipfs/go-ipfs/thirdparty/verifcid"


We should not be putting more stuff in thirdparty. If it needs to be re-used across submodules or has interest for other places put it in a different repo, if not, place it in the submodule directly. It complicates extractions afterwards...

This is temporary measure, it will be removed as we are able to propagate go-cid (code comes from PR to go-cid).
The temporary measure was applied because we want to release ASAP and have this behaviour fixed in this release.

Then place that code as submodules of blockservice? That way the temporary measure can go together with the temporary code and be fixed with it when it's extracted. Otherwise I will have to do exactly that.

@hsanjuan the reason I want it in thirdparty is explicitly because it needs to be moved to the cid package as soon as possible. But (as you know) we have a bunch of stuff in-flight that prevents us from bubbling up the cid package without other complications, so its going here for now as a "please remove me"

@whyrusleeping sure, I understand the reasons, but it still introduces a blocker for me.

I'm trying to communicate that if it's placed under blockservice with a "please-remove-me" note it doesn't become a blocker while solving your problem at the same time.

If you say this will be removed from thirdparty right away after the release, OK. I'm just scared that things will take longer though.

hsanjuan · 2018-03-05T16:19:41Z

core/builder.go

@@ -17,6 +17,7 @@ import (
 	pin "github.com/ipfs/go-ipfs/pin"
 	repo "github.com/ipfs/go-ipfs/repo"
 	cfg "github.com/ipfs/go-ipfs/repo/config"
+	"github.com/ipfs/go-ipfs/thirdparty/verifbs"


Stebalien · 2018-03-05T18:33:31Z

core/commands/pin.go

@@ -462,25 +463,23 @@ var verifyPinCmd = &cmds.Command{
 		cmds.Text: func(res cmds.Response) (io.Reader, error) {
 			quiet, _, _ := res.Request().Option("quiet").Bool()

-			outChan, ok := res.Output().(<-chan interface{})
+			out, err := unwrapOutput(res.Output())


(this is one of the many reasons I've given up on fixing issues with the compat layer and have started just converting commands to use the new system).

Stebalien · 2018-03-05T18:36:19Z

Failure:

  
cp: cannot stat '../t0275-cid-security-data/AFKSEBCGPUJZE.data': No such file or directory
not ok 23 - injecting insecure block
#	
#	    mkdir -p "$IPFS_PATH/blocks/JZ" &&
#	    cp -f ../t0275-cid-security-data/AFKSEBCGPUJZE.data "$IPFS_PATH/blocks/JZ"
#

Kubuxu · 2018-03-05T18:37:08Z

missed file from commit, fixing (I didn't notice the CI failures for some reason).

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

Kubuxu · 2018-03-05T18:38:40Z

Fixed

whyrusleeping · 2018-03-05T20:25:21Z

Alright, here goes.

This mostly reverts commit 13887ff, reversing changes made to a544026. It keeps the fixed to "ipfs pin verify" and a fix to t0050-block.sh to use a longer block size.

ghost assigned Kubuxu Mar 1, 2018

ghost added the status/in-progress In progress label Mar 1, 2018

Kubuxu force-pushed the feat/safe-cid branch 2 times, most recently from 27eb665 to e643269 Compare March 1, 2018 21:13

Kubuxu requested a review from whyrusleeping March 1, 2018 21:14

Kubuxu requested review from Stebalien and magik6k March 1, 2018 21:55

whyrusleeping reviewed Mar 1, 2018

View reviewed changes

Kubuxu force-pushed the feat/safe-cid branch from a9184fd to c8acde6 Compare March 1, 2018 22:51

magik6k reviewed Mar 1, 2018

View reviewed changes

ghost assigned magik6k Mar 2, 2018

Stebalien approved these changes Mar 2, 2018

View reviewed changes

magik6k force-pushed the feat/safe-cid branch 2 times, most recently from edd60bf to 6e613d3 Compare March 2, 2018 22:59

kevina previously requested changes Mar 3, 2018

View reviewed changes

whyrusleeping mentioned this pull request Mar 3, 2018

Feat: remove circular dependencies in merkledag package tests #4704

Merged

Kubuxu requested review from Stebalien, whyrusleeping and kevina March 4, 2018 00:31

Kubuxu added 3 commits March 4, 2018 01:38

Include validatecid from new cid package

903ecd1

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

Enforce Cid security rules for getting and adding blocks

c30422d

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

Move the temporary packages to thirdparty

578cb24

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

Significanly improve GC UX with verifcid

51bf1b6

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

Kubuxu force-pushed the feat/safe-cid branch from 8629cb2 to 51bf1b6 Compare March 4, 2018 00:43

whyrusleeping reviewed Mar 4, 2018

View reviewed changes

whyrusleeping mentioned this pull request Mar 4, 2018

migrate repo verify to new cmds library. #4764

Closed

kevina reviewed Mar 5, 2018

View reviewed changes

kevina mentioned this pull request Mar 5, 2018

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

Open

hsanjuan requested changes Mar 5, 2018

View reviewed changes

Stebalien approved these changes Mar 5, 2018

View reviewed changes

Kubuxu added 2 commits March 5, 2018 19:38

cid_secuirty: add test for gc

60018d3

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

don't reprovide inssecure hashes

4527279

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

Kubuxu force-pushed the feat/safe-cid branch from d62859a to 4527279 Compare March 5, 2018 18:38

whyrusleeping merged commit 13887ff into master Mar 5, 2018

whyrusleeping deleted the feat/safe-cid branch March 5, 2018 20:25

ghost removed the status/in-progress In progress label Mar 5, 2018

Kubuxu mentioned this pull request Apr 18, 2018

Forbid non-cryptographic hashes within /ipfs namespace #4371

Open

kevina mentioned this pull request Jun 24, 2018

[WIP] Strict Cids #5150

Closed

Disallow usage of unsafe hashes for reading and adding content #4751

Disallow usage of unsafe hashes for reading and adding content #4751

Conversation

Kubuxu commented Mar 1, 2018 • edited Loading

Kubuxu commented Mar 1, 2018

Kubuxu commented Mar 1, 2018 • edited Loading

Kubuxu commented Mar 1, 2018 • edited Loading

whyrusleeping commented Mar 1, 2018

Choose a reason for hiding this comment

magik6k left a comment

Choose a reason for hiding this comment

magik6k commented Mar 2, 2018

kevina commented Mar 2, 2018

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magik6k Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

kevina commented Mar 2, 2018 • edited Loading

kevina left a comment • edited Loading

Choose a reason for hiding this comment

kevina commented Mar 3, 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

whyrusleeping commented Mar 4, 2018

whyrusleeping commented Mar 4, 2018

Kubuxu commented Mar 4, 2018

kevina commented Mar 4, 2018

Kubuxu commented Mar 5, 2018 • edited Loading

Kubuxu commented Mar 5, 2018

kevina left a comment • edited Loading

Choose a reason for hiding this comment

kevina commented Mar 5, 2018

hsanjuan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu Mar 5, 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

Stebalien commented Mar 5, 2018

Kubuxu commented Mar 5, 2018

Kubuxu commented Mar 5, 2018

whyrusleeping commented Mar 5, 2018

Kubuxu commented Mar 1, 2018 •

edited

Loading

Kubuxu commented Mar 1, 2018 •

edited

Loading

Kubuxu commented Mar 1, 2018 •

edited

Loading

magik6k Mar 2, 2018 •

edited

Loading

kevina commented Mar 2, 2018 •

edited

Loading

kevina left a comment •

edited

Loading

Kubuxu commented Mar 5, 2018 •

edited

Loading

kevina left a comment •

edited

Loading

Kubuxu Mar 5, 2018 •

edited

Loading