Skip to content

Commit

Permalink
fix: reject v1.1.0 deals whose deal label is not utf8
Browse files Browse the repository at this point in the history
  • Loading branch information
dirkmc committed Jun 21, 2022
1 parent fd8afb8 commit 5132179
Show file tree
Hide file tree
Showing 10 changed files with 215 additions and 57 deletions.
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ require (
github.com/multiformats/go-multihash v0.1.0
github.com/multiformats/go-varint v0.0.6
github.com/petar/GoLLRB v0.0.0-20210522233825-ae3b015fd3e9
github.com/polydawn/refmt v0.0.0-20201211092308-30ac6d18308e
github.com/stretchr/testify v1.7.0
github.com/whyrusleeping/cbor v0.0.0-20171005072247-63513f603b11
github.com/whyrusleeping/cbor-gen v0.0.0-20220302191723-37c43cae8e14
Expand Down
12 changes: 6 additions & 6 deletions shared_testutil/test_network_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ func StubbedDealPaymentReader(payment rm.DealPayment) DealPaymentReader {
}

// StorageDealProposalReader is a function to mock reading deal proposals.
type StorageDealProposalReader func() (smnet.Proposal, cid.Cid, error)
type StorageDealProposalReader func() (smnet.Proposal, error)

// StorageDealResponseReader is a function to mock reading deal responses.
type StorageDealResponseReader func() (smnet.SignedResponse, []byte, error)
Expand Down Expand Up @@ -460,7 +460,7 @@ func NewTestStorageDealStream(params TestStorageDealStreamParams) *TestStorageDe
}

// ReadDealProposal calls the mocked deal proposal reader function.
func (tsds *TestStorageDealStream) ReadDealProposal() (smnet.Proposal, cid.Cid, error) {
func (tsds *TestStorageDealStream) ReadDealProposal() (smnet.Proposal, error) {
return tsds.proposalReader()
}

Expand Down Expand Up @@ -489,8 +489,8 @@ func (tsds *TestStorageDealStream) Close() error {
}

// TrivialStorageDealProposalReader succeeds trivially, returning an empty proposal.
func TrivialStorageDealProposalReader() (smnet.Proposal, cid.Cid, error) {
return smnet.Proposal{}, cid.Undef, nil
func TrivialStorageDealProposalReader() (smnet.Proposal, error) {
return smnet.Proposal{}, nil
}

// TrivialStorageDealResponseReader succeeds trivially, returning an empty deal response.
Expand All @@ -510,8 +510,8 @@ func TrivialStorageDealResponseWriter(smnet.SignedResponse, smnet.ResigningFunc)

// StubbedStorageProposalReader returns the given proposal when called
func StubbedStorageProposalReader(proposal smnet.Proposal) StorageDealProposalReader {
return func() (smnet.Proposal, cid.Cid, error) {
return proposal, cid.Undef, nil
return func() (smnet.Proposal, error) {
return proposal, nil
}
}

Expand Down
17 changes: 11 additions & 6 deletions storagemarket/impl/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ func (p *Provider) HandleDealStream(s network.StorageDealStream) {
}

func (p *Provider) receiveDeal(s network.StorageDealStream) error {
proposal, propCid, err := s.ReadDealProposal()
proposal, err := s.ReadDealProposal()
if err != nil {
return xerrors.Errorf("failed to read proposal message: %w", err)
}
Expand All @@ -265,9 +265,14 @@ func (p *Provider) receiveDeal(s network.StorageDealStream) error {
return xerrors.Errorf("failed to get deal proposal from proposal message")
}

proposalNd, err := cborutil.AsIpld(proposal.DealProposal)
if err != nil {
return fmt.Errorf("getting deal proposal as IPLD: %w", err)
}

// Check if we are already tracking this deal
var md storagemarket.MinerDeal
if err := p.deals.Get(propCid).Get(&md); err == nil {
if err := p.deals.Get(proposalNd.Cid()).Get(&md); err == nil {
// We are already tracking this deal, for some reason it was re-proposed, perhaps because of a client restart
// this is ok, just send a response back.
return p.resendProposalResponse(s, &md)
Expand Down Expand Up @@ -295,23 +300,23 @@ func (p *Provider) receiveDeal(s network.StorageDealStream) error {
Client: s.RemotePeer(),
Miner: p.net.ID(),
ClientDealProposal: *proposal.DealProposal,
ProposalCid: propCid,
ProposalCid: proposalNd.Cid(),
State: storagemarket.StorageDealUnknown,
Ref: proposal.Piece,
FastRetrieval: proposal.FastRetrieval,
CreationTime: curTime(),
InboundCAR: path,
}

err = p.deals.Begin(propCid, deal)
err = p.deals.Begin(proposalNd.Cid(), deal)
if err != nil {
return err
}
err = p.conns.AddStream(propCid, s)
err = p.conns.AddStream(proposalNd.Cid(), s)
if err != nil {
return err
}
return p.deals.Send(propCid, storagemarket.ProviderEventOpen)
return p.deals.Send(proposalNd.Cid(), storagemarket.ProviderEventOpen)
}

// Stop terminates processing of deals on a StorageProvider
Expand Down
9 changes: 2 additions & 7 deletions storagemarket/impl/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -306,17 +306,12 @@ func TestHandleDealStream(t *testing.T) {

var responseWriteCount int
s := shared_testutil.NewTestStorageDealStream(shared_testutil.TestStorageDealStreamParams{
ProposalReader: func() (network.Proposal, cid.Cid, error) {
propNd, err := cborutil.AsIpld(proposal)
if err != nil {
return network.Proposal{}, cid.Undef, err
}

ProposalReader: func() (network.Proposal, error) {
return network.Proposal{
DealProposal: proposal,
Piece: dataRef,
FastRetrieval: false,
}, propNd.Cid(), nil
}, nil
},
ResponseWriter: func(response network.SignedResponse, resigningFunc network.ResigningFunc) error {
responseWriteCount += 1
Expand Down
15 changes: 3 additions & 12 deletions storagemarket/network/deal_stream.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@ package network

import (
"bufio"
"fmt"

"github.com/ipfs/go-cid"
"github.com/libp2p/go-libp2p-core/host"
"github.com/libp2p/go-libp2p-core/mux"
"github.com/libp2p/go-libp2p-core/peer"
Expand All @@ -24,22 +22,15 @@ type dealStreamv111 struct {

var _ StorageDealStream = (*dealStreamv111)(nil)

func (d *dealStreamv111) ReadDealProposal() (Proposal, cid.Cid, error) {
func (d *dealStreamv111) ReadDealProposal() (Proposal, error) {
var ds Proposal

if err := ds.UnmarshalCBOR(d.buffered); err != nil {
log.Warn(err)
return ProposalUndefined, cid.Undef, err
return ProposalUndefined, err
}

proposalNd, err := cborutil.AsIpld(ds.DealProposal)
if err != nil {
err = fmt.Errorf("getting v111 deal proposal as IPLD: %w", err)
log.Warnf(err.Error())
return ProposalUndefined, cid.Undef, err
}

return ds, proposalNd.Cid(), nil
return ds, nil
}

func (d *dealStreamv111) WriteDealProposal(dp Proposal) error {
Expand Down
32 changes: 22 additions & 10 deletions storagemarket/network/deal_stream_v110.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@ package network

import (
"bufio"
"encoding/hex"
"fmt"
"unicode/utf8"

"github.com/ipfs/go-cid"
"github.com/libp2p/go-libp2p-core/host"
"github.com/libp2p/go-libp2p-core/mux"
"github.com/libp2p/go-libp2p-core/peer"
Expand All @@ -23,33 +24,44 @@ type dealStreamv110 struct {

var _ StorageDealStream = (*dealStreamv110)(nil)

func (d *dealStreamv110) ReadDealProposal() (Proposal, cid.Cid, error) {
func (d *dealStreamv110) ReadDealProposal() (Proposal, error) {
var ds migrations.Proposal1

if err := ds.UnmarshalCBOR(d.buffered); err != nil {
err = fmt.Errorf("unmarshalling v110 deal proposal: %w", err)
log.Warnf(err.Error())
return ProposalUndefined, cid.Undef, err
return ProposalUndefined, err
}

proposalNd, err := cborutil.AsIpld(ds.DealProposal)
if err != nil {
err = fmt.Errorf("getting v110 deal proposal as IPLD: %w", err)
log.Warnf(err.Error())
return ProposalUndefined, cid.Undef, err
// The signature over the deal proposal will be different between a v1.1.0
// deal proposal and higher versions if the deal label cannot be parsed as
// a utf8 string.
// The signature is checked when submitting the Publish Storage Deals
// message, so we reject the deal proposal here to avoid that scenario.
if err := checkDealLabel(ds.DealProposal.Proposal.Label); err != nil {
return ProposalUndefined, err
}

// Migrate the deal proposal to the new format
prop, err := migrations.MigrateClientDealProposal0To1(*ds.DealProposal)
if err != nil {
err = fmt.Errorf("migrating v110 deal proposal to current version: %w", err)
log.Warnf(err.Error())
return ProposalUndefined, cid.Undef, err
return ProposalUndefined, err
}
return Proposal{
DealProposal: prop,
Piece: ds.Piece,
FastRetrieval: ds.FastRetrieval,
}, proposalNd.Cid(), nil
}, nil
}

func checkDealLabel(label string) error {
labelBytes := []byte(label)
if !utf8.Valid(labelBytes) {
return fmt.Errorf("cannot parse deal label 0x%s as string", hex.EncodeToString(labelBytes))
}
return nil
}

func (d *dealStreamv110) WriteDealProposal(dp Proposal) error {
Expand Down
164 changes: 164 additions & 0 deletions storagemarket/network/deal_stream_v110_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
package network

import (
"bufio"
"bytes"
"testing"
"unicode/utf8"

"github.com/ipfs/go-cid"
"github.com/polydawn/refmt/cbor"
"github.com/stretchr/testify/require"

"github.com/filecoin-project/go-address"
cborutil "github.com/filecoin-project/go-cbor-util"
"github.com/filecoin-project/go-state-types/abi"
"github.com/filecoin-project/go-state-types/builtin/v8/market"
"github.com/filecoin-project/go-state-types/crypto"
marketOld "github.com/filecoin-project/specs-actors/actors/builtin/market"

"github.com/filecoin-project/go-fil-markets/storagemarket/migrations"
)

// TestReceivev110DealProposal verifies that the provider will reject a v110
// deal that has a non-utf8 deal label
func TestReceivev110DealProposal(t *testing.T) {
runTest := func(label string, validUtf8 bool) {
dp := makeOldDealProposal()
dp.Proposal.Label = label

dpReq := migrations.Proposal1{
DealProposal: &dp,
}

var buff bytes.Buffer
err := dpReq.MarshalCBOR(&buff)
require.NoError(t, err)

ds := &dealStreamv110{
buffered: bufio.NewReader(&buff),
}
prop, err := ds.ReadDealProposal()
if validUtf8 {
require.NoError(t, err)
require.True(t, prop.DealProposal.Proposal.Label.IsString())
} else {
require.Error(t, err)
}
}

t.Run("empty label", func(t *testing.T) {
runTest("", true)
})
t.Run("string label", func(t *testing.T) {
runTest("label", true)
})
t.Run("byte label", func(t *testing.T) {
label := []byte{66, 250}
require.False(t, utf8.Valid(label))
runTest(string(label), false)
})
}

func TestDealLabelCheck(t *testing.T) {
err := checkDealLabel("")
require.NoError(t, err)
err = checkDealLabel("label")
require.NoError(t, err)
err = checkDealLabel(string([]byte{66, 250}))
require.Error(t, err)
}

// Expect that CBOR marshaling a string will give the same result as marshaling
// a DealLabel with that string.
func TestLabelMatchingString(t *testing.T) {
str := "testing"
marshaledStr, err := cbor.Marshal(str)
require.NoError(t, err)

l, err := market.NewLabelFromString(str)
require.NoError(t, err)
var marshaledLabel bytes.Buffer
err = l.MarshalCBOR(&marshaledLabel)
require.NoError(t, err)

require.Equal(t, marshaledLabel.Bytes(), marshaledStr)
}

// Expect that CBOR marshaling a string with bytes that are not valid utf8
// will give a different result than marshaling a DealLabel with those bytes.
func TestLabelMatchingBytes(t *testing.T) {
bz := []byte{66, 250}
require.False(t, utf8.Valid(bz))
marshaledStr, err := cbor.Marshal(string(bz))
require.NoError(t, err)

l, err := market.NewLabelFromBytes(bz)
require.NoError(t, err)
var marshaledLabelFromBytes bytes.Buffer
err = l.MarshalCBOR(&marshaledLabelFromBytes)
require.NoError(t, err)

require.NotEqual(t, marshaledLabelFromBytes.Bytes(), marshaledStr)
}

// TestSignedProposalCidMatching verifies that the ipld-marshaled signed deal
// proposal cid matches between the old deal proposal format and the new one
// for strings, but not for non-utf8 bytes
func TestSignedProposalCidMatching(t *testing.T) {
runTest := func(label string, expectEqual bool) {
oldDealProp := makeOldDealProposal()
oldDealProp.Proposal.Label = label
oldDealPropNd, err := cborutil.AsIpld(&oldDealProp)
require.NoError(t, err)

//t.Logf("testing label %s", oldDealProp.Proposal.Label)

newDealProp, err := migrations.MigrateClientDealProposal0To1(oldDealProp)
require.NoError(t, err)
newDealPropNd, err := cborutil.AsIpld(newDealProp)
require.NoError(t, err)

require.Equal(t, expectEqual, oldDealPropNd.Cid() == newDealPropNd.Cid())
}

t.Run("empty label", func(t *testing.T) {
runTest("", true)
})
t.Run("string label", func(t *testing.T) {
runTest("label", true)
})
t.Run("byte label", func(t *testing.T) {
label := []byte{66, 250}
require.False(t, utf8.Valid(label))
runTest(string(label), false)
})
}

func makeOldDealProposal() marketOld.ClientDealProposal {
pieceCid, err := cid.Parse("bafkqaaa")
if err != nil {
panic(err)
}
return marketOld.ClientDealProposal{
Proposal: marketOld.DealProposal{
PieceCID: pieceCid,
PieceSize: abi.PaddedPieceSize(2048),

Client: address.TestAddress,
Provider: address.TestAddress2,
Label: "label",

StartEpoch: abi.ChainEpoch(10),
EndEpoch: abi.ChainEpoch(20),

StoragePricePerEpoch: abi.NewTokenAmount(1),
ProviderCollateral: abi.NewTokenAmount(2),
ClientCollateral: abi.NewTokenAmount(3),
},
ClientSignature: crypto.Signature{
Type: crypto.SigTypeSecp256k1,
Data: []byte("signature data"),
},
}
}
Loading

0 comments on commit 5132179

Please sign in to comment.