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

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

Merged
merged 10 commits into from
Mar 5, 2018
33 changes: 29 additions & 4 deletions blockservice/blockservice.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"io"

exchange "github.com/ipfs/go-ipfs/exchange"
"github.com/ipfs/go-ipfs/thirdparty/verifcid"
Copy link
Contributor

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...

Copy link
Member Author

@Kubuxu Kubuxu Mar 5, 2018

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.

Copy link
Contributor

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.

Copy link
Member

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"

Copy link
Contributor

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.


logging "gx/ipfs/QmRb5jh8z2E8hMGN2tkvs1yHynUanqnZ3UeKwgN1i9P1F8/go-log"
blockstore "gx/ipfs/QmTVDM4LCSUMFNQzbDLL9zQwp8usE6QHymFdh3h8vL9v6b/go-ipfs-blockstore"
Expand Down Expand Up @@ -130,6 +131,11 @@ func NewSession(ctx context.Context, bs BlockService) *Session {
// TODO pass a context into this if the remote.HasBlock is going to remain here.
func (s *blockService) AddBlock(o blocks.Block) error {
c := o.Cid()
// hash security
err := verifcid.ValidateCid(c)
if err != nil {
return err
}
if s.checkFirst {
if has, err := s.blockstore.Has(c); has || err != nil {
return err
Expand All @@ -149,6 +155,13 @@ func (s *blockService) AddBlock(o blocks.Block) error {
}

func (s *blockService) AddBlocks(bs []blocks.Block) error {
// hash security
for _, b := range bs {
err := verifcid.ValidateCid(b.Cid())
if err != nil {
return err
}
}
var toput []blocks.Block
if s.checkFirst {
toput = make([]blocks.Block, 0, len(bs))
Expand Down Expand Up @@ -189,10 +202,15 @@ func (s *blockService) GetBlock(ctx context.Context, c *cid.Cid) (blocks.Block,
f = s.exchange
}

return getBlock(ctx, c, s.blockstore, f)
return getBlock(ctx, c, s.blockstore, f) // hash security
}

func getBlock(ctx context.Context, c *cid.Cid, bs blockstore.Blockstore, f exchange.Fetcher) (blocks.Block, error) {
err := verifcid.ValidateCid(c) // hash security
if err != nil {
return nil, err
}

block, err := bs.Get(c)
if err == nil {
return block, nil
Expand Down Expand Up @@ -224,11 +242,18 @@ func getBlock(ctx context.Context, c *cid.Cid, bs blockstore.Blockstore, f excha
// the returned channel.
// NB: No guarantees are made about order.
func (s *blockService) GetBlocks(ctx context.Context, ks []*cid.Cid) <-chan blocks.Block {
return getBlocks(ctx, ks, s.blockstore, s.exchange)
return getBlocks(ctx, ks, s.blockstore, s.exchange) // hash security
}

func getBlocks(ctx context.Context, ks []*cid.Cid, bs blockstore.Blockstore, f exchange.Fetcher) <-chan blocks.Block {
out := make(chan blocks.Block)
for _, c := range ks {
// hash security
if err := verifcid.ValidateCid(c); err != nil {
log.Errorf("unsafe CID (%s) passed to blockService.GetBlocks: %s", c, err)
}
}

go func() {
defer close(out)
var misses []*cid.Cid
Expand Down Expand Up @@ -285,12 +310,12 @@ type Session struct {

// GetBlock gets a block in the context of a request session
func (s *Session) GetBlock(ctx context.Context, c *cid.Cid) (blocks.Block, error) {
return getBlock(ctx, c, s.bs, s.ses)
return getBlock(ctx, c, s.bs, s.ses) // hash security
}

// GetBlocks gets blocks in the context of a request session
func (s *Session) GetBlocks(ctx context.Context, ks []*cid.Cid) <-chan blocks.Block {
return getBlocks(ctx, ks, s.bs, s.ses)
return getBlocks(ctx, ks, s.bs, s.ses) // hash security
}

var _ BlockGetter = (*Session)(nil)
5 changes: 5 additions & 0 deletions core/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The 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"
Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion core/commands/add.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need the hash security comment?

Copy link
Member

@magik6k magik6k Mar 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dserv := dag.NewDAGService(bserv)

outChan := make(chan interface{}, adderOutChanSize)
Expand Down
38 changes: 23 additions & 15 deletions core/commands/pin.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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())
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to know what is going on also.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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
},
},
}
Expand Down Expand Up @@ -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}
Expand Down
26 changes: 25 additions & 1 deletion pin/gc/gc.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ import (
"context"
"errors"
"fmt"
"strings"

bserv "github.com/ipfs/go-ipfs/blockservice"
offline "github.com/ipfs/go-ipfs/exchange/offline"
dag "github.com/ipfs/go-ipfs/merkledag"
pin "github.com/ipfs/go-ipfs/pin"
"github.com/ipfs/go-ipfs/thirdparty/verifcid"

dstore "gx/ipfs/QmPpegoMqhAEqjncrzArm7KVWAkCm78rqL2DPuNjhPrshg/go-datastore"
logging "gx/ipfs/QmRb5jh8z2E8hMGN2tkvs1yHynUanqnZ3UeKwgN1i9P1F8/go-log"
Expand Down Expand Up @@ -129,12 +131,34 @@ func GC(ctx context.Context, bs bstore.GCBlockstore, dstor dstore.Datastore, pn
// adds them to the given cid.Set, using the provided dag.GetLinks function
// to walk the tree.
func Descendants(ctx context.Context, getLinks dag.GetLinks, set *cid.Set, roots []*cid.Cid) error {
verifyGetLinks := func(ctx context.Context, c *cid.Cid) ([]*ipld.Link, error) {
err := verifcid.ValidateCid(c)
if err != nil {
return nil, err
}

return getLinks(ctx, c)
}

verboseCidError := func(err error) error {
if strings.Contains(err.Error(), verifcid.ErrBelowMinimumHashLength.Error()) ||
strings.Contains(err.Error(), verifcid.ErrPossiblyInsecureHashFunction.Error()) {
err = fmt.Errorf("\"%s\"\nPlease run 'ipfs pin verify'"+
" to list insecure hashes. If you want to read them,"+
" please downgrade your go-ipfs to 0.4.13\n", err)
log.Error(err)
}
return err
}

for _, c := range roots {
set.Add(c)

// EnumerateChildren recursively walks the dag and adds the keys to the given set
err := dag.EnumerateChildren(ctx, getLinks, c, set.Visit)
err := dag.EnumerateChildren(ctx, verifyGetLinks, c, set.Visit)

if err != nil {
err = verboseCidError(err)
return err
}
}
Expand Down
4 changes: 2 additions & 2 deletions test/sharness/t0050-block.sh
Original file line number Diff line number Diff line change
Expand Up @@ -185,11 +185,11 @@ test_expect_success "block get output looks right" '
'

test_expect_success "can set multihash type and length on block put" '
HASH=$(echo "foooo" | ipfs block put --format=raw --mhtype=sha3 --mhlen=16)
HASH=$(echo "foooo" | ipfs block put --format=raw --mhtype=sha3 --mhlen=20)
'

test_expect_success "output looks good" '
test "z25ScPysKoxJBcPxczn9NvuHiZU5" = "$HASH"
test "z83bYcqyBkbx5fuNAcvbdv4pr5RYQiEpK" = "$HASH"
'

test_expect_success "can read block with different hash" '
Expand Down
9 changes: 9 additions & 0 deletions test/sharness/t0085-pins.sh
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,15 @@ test_pins() {
cat hashes | ipfs pin add $EXTRA_ARGS
'

test_expect_success "see if verify works" '
ipfs pin verify
'

test_expect_success "see if verify --verbose works" '
ipfs pin verify --verbose > verify_out &&
test $(cat verify_out | wc -l) > 8
'

test_expect_success "unpin those hashes" '
cat hashes | ipfs pin rm
'
Expand Down
61 changes: 61 additions & 0 deletions test/sharness/t0275-cid-security.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
#!/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
'
}


# should work offline
test_cat_get

# should work online
test_launch_ipfs_daemon
test_cat_get
test_kill_ipfs_daemon
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


test_done
63 changes: 63 additions & 0 deletions thirdparty/verifbs/verifbs.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package verifbs

import (
"github.com/ipfs/go-ipfs/thirdparty/verifcid"

bstore "gx/ipfs/QmTVDM4LCSUMFNQzbDLL9zQwp8usE6QHymFdh3h8vL9v6b/go-ipfs-blockstore"
cid "gx/ipfs/QmcZfnkapfECQGcLZaf9B79NRg7cRa9EnZh4LSbkCzwNvY/go-cid"
blocks "gx/ipfs/Qmej7nf81hi2x2tvjRBF3mcp74sQyuDH4VMYDGd1YtXjb2/go-block-format"
)

type VerifBSGC struct {
bstore.GCBlockstore
}

func (bs *VerifBSGC) Put(b blocks.Block) error {
if err := verifcid.ValidateCid(b.Cid()); err != nil {
return err
}
return bs.GCBlockstore.Put(b)
}

func (bs *VerifBSGC) PutMany(blks []blocks.Block) error {
for _, b := range blks {
if err := verifcid.ValidateCid(b.Cid()); err != nil {
return err
}
}
return bs.GCBlockstore.PutMany(blks)
}

func (bs *VerifBSGC) Get(c *cid.Cid) (blocks.Block, error) {
if err := verifcid.ValidateCid(c); err != nil {
return nil, err
}
return bs.GCBlockstore.Get(c)
}

type VerifBS struct {
bstore.Blockstore
}

func (bs *VerifBS) Put(b blocks.Block) error {
if err := verifcid.ValidateCid(b.Cid()); err != nil {
return err
}
return bs.Blockstore.Put(b)
}

func (bs *VerifBS) PutMany(blks []blocks.Block) error {
for _, b := range blks {
if err := verifcid.ValidateCid(b.Cid()); err != nil {
return err
}
}
return bs.Blockstore.PutMany(blks)
}

func (bs *VerifBS) Get(c *cid.Cid) (blocks.Block, error) {
if err := verifcid.ValidateCid(c); err != nil {
return nil, err
}
return bs.Blockstore.Get(c)
}
Loading