-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Disallow usage of unsafe hashes for reading and adding content #4751
Changes from all commits
903ecd1
c30422d
578cb24
bcedd38
9634991
f1125f0
a9417a1
51bf1b6
60018d3
4527279
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This too |
||
uio "github.com/ipfs/go-ipfs/unixfs/io" | ||
|
||
ds "gx/ipfs/QmPpegoMqhAEqjncrzArm7KVWAkCm78rqL2DPuNjhPrshg/go-datastore" | ||
|
@@ -170,7 +171,9 @@ func setupNode(ctx context.Context, n *IpfsNode, cfg *BuildCfg) error { | |
TempErrFunc: isTooManyFDError, | ||
} | ||
|
||
// hash security | ||
bs := bstore.NewBlockstore(rds) | ||
bs = &verifbs.VerifBS{bs} | ||
|
||
opts := bstore.DefaultCacheOpts() | ||
conf, err := n.Repo.Config() | ||
|
@@ -196,8 +199,10 @@ func setupNode(ctx context.Context, n *IpfsNode, cfg *BuildCfg) error { | |
n.Blockstore = bstore.NewGCBlockstore(cbs, n.GCLocker) | ||
|
||
if conf.Experimental.FilestoreEnabled { | ||
// hash security | ||
n.Filestore = filestore.NewFilestore(bs, n.Repo.FileManager()) | ||
n.Blockstore = bstore.NewGCBlockstore(n.Filestore, n.GCLocker) | ||
n.Blockstore = &verifbs.VerifBSGC{n.Blockstore} | ||
} | ||
|
||
rcfg, err := n.Repo.Config() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need the hash security comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the only place |
||
dserv := dag.NewDAGService(bserv) | ||
|
||
outChan := make(chan interface{}, adderOutChanSize) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ import ( | |
path "github.com/ipfs/go-ipfs/path" | ||
resolver "github.com/ipfs/go-ipfs/path/resolver" | ||
pin "github.com/ipfs/go-ipfs/pin" | ||
"github.com/ipfs/go-ipfs/thirdparty/verifcid" | ||
uio "github.com/ipfs/go-ipfs/unixfs/io" | ||
|
||
u "gx/ipfs/QmNiJuT8Ja3hMVpBHXv3Q6dwmperaQ6JjLtpMQgMCD7xvx/go-ipfs-util" | ||
|
@@ -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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this looks like its being changed to only emit a single output. Whats going on here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to know what is going on also. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (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). |
||
if err != nil { | ||
return nil, err | ||
} | ||
r, ok := out.(*PinVerifyRes) | ||
if !ok { | ||
return nil, u.ErrCast() | ||
return nil, e.TypeErr(r, out) | ||
} | ||
|
||
rdr, wtr := io.Pipe() | ||
go func() { | ||
defer wtr.Close() | ||
for r0 := range outChan { | ||
r := r0.(*PinVerifyRes) | ||
if quiet && !r.Ok { | ||
fmt.Fprintf(wtr, "%s\n", r.Cid) | ||
} else if !quiet { | ||
r.Format(wtr) | ||
} | ||
} | ||
}() | ||
buf := &bytes.Buffer{} | ||
if quiet && !r.Ok { | ||
fmt.Fprintf(buf, "%s\n", r.Cid) | ||
} else if !quiet { | ||
r.Format(buf) | ||
} | ||
|
||
return rdr, nil | ||
return buf, nil | ||
}, | ||
}, | ||
} | ||
|
@@ -610,6 +609,15 @@ func pinVerify(ctx context.Context, n *core.IpfsNode, opts pinVerifyOpts) <-chan | |
return status | ||
} | ||
|
||
if err := verifcid.ValidateCid(root); err != nil { | ||
status := PinStatus{Ok: false} | ||
if opts.explain { | ||
status.BadNodes = []BadNode{BadNode{Cid: key, Err: err.Error()}} | ||
} | ||
visited[key] = status | ||
return status | ||
} | ||
|
||
links, err := getLinks(ctx, root) | ||
if err != nil { | ||
status := PinStatus{Ok: false} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
testing |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
#!/bin/sh | ||
# | ||
# Copyright (c) 2017 Jakub Sztandera | ||
# MIT Licensed; see the LICENSE file in this repository. | ||
# | ||
|
||
test_description="Cid Security" | ||
|
||
. lib/test-lib.sh | ||
|
||
test_init_ipfs | ||
|
||
test_expect_success "adding using unsafe function fails with error" ' | ||
echo foo | test_must_fail ipfs add --hash murmur3 2>add_out | ||
' | ||
|
||
test_expect_success "error reason is pointed out" ' | ||
grep "insecure hash functions not allowed" add_out | ||
' | ||
|
||
test_expect_success "adding using too short of a hash function gives out an error" ' | ||
echo foo | test_must_fail ipfs block put --mhlen 19 2>block_out | ||
' | ||
|
||
test_expect_success "error reason is pointed out" ' | ||
grep "hashes must be at 20 least bytes long" block_out | ||
' | ||
|
||
|
||
test_cat_get() { | ||
|
||
test_expect_success "ipfs cat fails with unsafe hash function" ' | ||
test_must_fail ipfs cat zDvnoLcPKWR 2>ipfs_cat | ||
' | ||
|
||
|
||
test_expect_success "error reason is pointed out" ' | ||
grep "insecure hash functions not allowed" ipfs_cat | ||
' | ||
|
||
|
||
test_expect_success "ipfs get fails with too short function" ' | ||
test_must_fail ipfs get z2ba5YhCCFNFxLtxMygQwjBjYSD8nUeN 2>ipfs_get | ||
|
||
' | ||
|
||
test_expect_success "error reason is pointed out" ' | ||
grep "hashes must be at 20 least bytes long" ipfs_get | ||
' | ||
} | ||
|
||
|
||
test_gc() { | ||
test_expect_success "injecting insecure block" ' | ||
mkdir -p "$IPFS_PATH/blocks/JZ" && | ||
cp -f ../t0275-cid-security-data/AFKSEBCGPUJZE.data "$IPFS_PATH/blocks/JZ" | ||
' | ||
|
||
test_expect_success "gc works" 'ipfs repo gc > gc_out' | ||
test_expect_success "gc removed bad block" ' | ||
grep zDvnoGUyhEq gc_out | ||
' | ||
} | ||
|
||
|
||
# should work offline | ||
test_cat_get | ||
test_gc | ||
|
||
# should work online | ||
test_launch_ipfs_daemon | ||
test_cat_get | ||
test_gc | ||
test_kill_ipfs_daemon | ||
|
||
test_done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.