From 51321793cb85afdf25232213c9b4b287114a3ea9 Mon Sep 17 00:00:00 2001 From: Dirk McCormick Date: Tue, 21 Jun 2022 15:45:22 +0200 Subject: [PATCH] fix: reject v1.1.0 deals whose deal label is not utf8 --- go.mod | 1 + shared_testutil/test_network_types.go | 12 +- storagemarket/impl/provider.go | 17 +- storagemarket/impl/provider_test.go | 9 +- storagemarket/network/deal_stream.go | 15 +- storagemarket/network/deal_stream_v110.go | 32 ++-- .../network/deal_stream_v110_test.go | 164 ++++++++++++++++++ storagemarket/network/legacy_deal_stream.go | 15 +- storagemarket/network/libp2p_impl_test.go | 4 +- storagemarket/network/network.go | 3 +- 10 files changed, 215 insertions(+), 57 deletions(-) create mode 100644 storagemarket/network/deal_stream_v110_test.go diff --git a/go.mod b/go.mod index 9cc9fbf0..ba62db75 100644 --- a/go.mod +++ b/go.mod @@ -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 diff --git a/shared_testutil/test_network_types.go b/shared_testutil/test_network_types.go index bb1e3c46..89d0de03 100644 --- a/shared_testutil/test_network_types.go +++ b/shared_testutil/test_network_types.go @@ -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) @@ -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() } @@ -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. @@ -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 } } diff --git a/storagemarket/impl/provider.go b/storagemarket/impl/provider.go index 6ccad5ed..6dd567dd 100644 --- a/storagemarket/impl/provider.go +++ b/storagemarket/impl/provider.go @@ -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) } @@ -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) @@ -295,7 +300,7 @@ 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, @@ -303,15 +308,15 @@ func (p *Provider) receiveDeal(s network.StorageDealStream) error { 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 diff --git a/storagemarket/impl/provider_test.go b/storagemarket/impl/provider_test.go index cd769ef5..cf9e1381 100644 --- a/storagemarket/impl/provider_test.go +++ b/storagemarket/impl/provider_test.go @@ -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 diff --git a/storagemarket/network/deal_stream.go b/storagemarket/network/deal_stream.go index fcf54a21..fbcac2bf 100644 --- a/storagemarket/network/deal_stream.go +++ b/storagemarket/network/deal_stream.go @@ -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" @@ -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 { diff --git a/storagemarket/network/deal_stream_v110.go b/storagemarket/network/deal_stream_v110.go index 0eebba6e..76e12de8 100644 --- a/storagemarket/network/deal_stream_v110.go +++ b/storagemarket/network/deal_stream_v110.go @@ -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" @@ -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 { diff --git a/storagemarket/network/deal_stream_v110_test.go b/storagemarket/network/deal_stream_v110_test.go new file mode 100644 index 00000000..371f57f4 --- /dev/null +++ b/storagemarket/network/deal_stream_v110_test.go @@ -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"), + }, + } +} diff --git a/storagemarket/network/legacy_deal_stream.go b/storagemarket/network/legacy_deal_stream.go index 9eea8624..12a2bce3 100644 --- a/storagemarket/network/legacy_deal_stream.go +++ b/storagemarket/network/legacy_deal_stream.go @@ -3,9 +3,7 @@ package network import ( "bufio" "context" - "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" @@ -24,26 +22,19 @@ type dealStreamv101 struct { var _ StorageDealStream = (*dealStreamv101)(nil) -func (d *dealStreamv101) ReadDealProposal() (Proposal, cid.Cid, error) { +func (d *dealStreamv101) ReadDealProposal() (Proposal, error) { var ds migrations.Proposal0 if err := ds.UnmarshalCBOR(d.buffered); err != nil { log.Warn(err) - return ProposalUndefined, cid.Undef, err - } - - proposalNd, err := cborutil.AsIpld(ds.DealProposal) - if err != nil { - err = fmt.Errorf("getting v101 deal proposal as IPLD: %w", err) - log.Warnf(err.Error()) - return ProposalUndefined, cid.Undef, err + return ProposalUndefined, err } return Proposal{ DealProposal: ds.DealProposal, Piece: migrations.MigrateDataRef0To1(ds.Piece), FastRetrieval: ds.FastRetrieval, - }, proposalNd.Cid(), nil + }, nil } func (d *dealStreamv101) WriteDealProposal(dp Proposal) error { diff --git a/storagemarket/network/libp2p_impl_test.go b/storagemarket/network/libp2p_impl_test.go index 0602d373..7f89d955 100644 --- a/storagemarket/network/libp2p_impl_test.go +++ b/storagemarket/network/libp2p_impl_test.go @@ -265,7 +265,7 @@ func TestDealStreamSendReceiveDealProposal(t *testing.T) { tr2 := &testReceiver{ t: t, dealStreamHandler: func(s network.StorageDealStream) { - readD, _, err := s.ReadDealProposal() + readD, err := s.ReadDealProposal() require.NoError(t, err) dchan <- readD }, @@ -343,7 +343,7 @@ func TestDealStreamSendReceiveMultipleSuccessful(t *testing.T) { return nil, nil } tr2 := &testReceiver{t: t, dealStreamHandler: func(s network.StorageDealStream) { - _, _, err := s.ReadDealProposal() + _, err := s.ReadDealProposal() require.NoError(t, err) require.NoError(t, s.WriteDealResponse(dr, resigningFunc)) diff --git a/storagemarket/network/network.go b/storagemarket/network/network.go index 38d2eab6..83e506b8 100644 --- a/storagemarket/network/network.go +++ b/storagemarket/network/network.go @@ -3,7 +3,6 @@ package network import ( "context" - "github.com/ipfs/go-cid" "github.com/libp2p/go-libp2p-core/peer" ma "github.com/multiformats/go-multiaddr" @@ -29,7 +28,7 @@ type StorageAskStream interface { // StorageDealStream is a stream for reading and writing requests // and responses on the storage deal protocol type StorageDealStream interface { - ReadDealProposal() (Proposal, cid.Cid, error) + ReadDealProposal() (Proposal, error) WriteDealProposal(Proposal) error ReadDealResponse() (SignedResponse, []byte, error) WriteDealResponse(SignedResponse, ResigningFunc) error