From c79d440c219503f8f0c50aabfcf734ed00772c57 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 2 Oct 2020 19:25:15 +0300 Subject: [PATCH 1/4] network: fix MerkleBlockPayload serialisation --- pkg/network/payload/merkleblock.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/network/payload/merkleblock.go b/pkg/network/payload/merkleblock.go index c208246de..a870450ec 100644 --- a/pkg/network/payload/merkleblock.go +++ b/pkg/network/payload/merkleblock.go @@ -26,7 +26,6 @@ func (m *MerkleBlock) DecodeBinary(br *io.BinReader) { // EncodeBinary implements Serializable interface. func (m *MerkleBlock) EncodeBinary(bw *io.BinWriter) { - m.Base = &block.Base{} m.Base.EncodeBinary(bw) bw.WriteVarUint(uint64(m.TxCount)) From e1e586f18be2452a5fdf395a8ebf812276d7a29c Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 2 Oct 2020 17:25:55 +0300 Subject: [PATCH 2/4] core: restrict the muximum number of contents per block --- pkg/consensus/prepare_request.go | 3 +- pkg/consensus/prepare_request_test.go | 31 ++++++++++++++ pkg/core/block/block.go | 18 ++++++++ pkg/core/block/block_test.go | 25 +++++++++++ pkg/network/payload/merkleblock.go | 9 +++- pkg/network/payload/merkleblock_test.go | 57 +++++++++++++++++++++++++ 6 files changed, 140 insertions(+), 3 deletions(-) create mode 100644 pkg/network/payload/merkleblock_test.go diff --git a/pkg/consensus/prepare_request.go b/pkg/consensus/prepare_request.go index cded41ed2..ad745f9c6 100644 --- a/pkg/consensus/prepare_request.go +++ b/pkg/consensus/prepare_request.go @@ -2,6 +2,7 @@ package consensus import ( "github.com/nspcc-dev/dbft/payload" + "github.com/nspcc-dev/neo-go/pkg/core/block" "github.com/nspcc-dev/neo-go/pkg/io" "github.com/nspcc-dev/neo-go/pkg/util" ) @@ -26,7 +27,7 @@ func (p *prepareRequest) EncodeBinary(w *io.BinWriter) { func (p *prepareRequest) DecodeBinary(r *io.BinReader) { p.timestamp = r.ReadU64LE() p.nonce = r.ReadU64LE() - r.ReadArray(&p.transactionHashes) + r.ReadArray(&p.transactionHashes, block.MaxTransactionsPerBlock) } // Timestamp implements payload.PrepareRequest interface. diff --git a/pkg/consensus/prepare_request_test.go b/pkg/consensus/prepare_request_test.go index 188e32d97..ef51a7d56 100644 --- a/pkg/consensus/prepare_request_test.go +++ b/pkg/consensus/prepare_request_test.go @@ -3,7 +3,9 @@ package consensus import ( "testing" + "github.com/nspcc-dev/neo-go/pkg/core/block" "github.com/nspcc-dev/neo-go/pkg/internal/random" + "github.com/nspcc-dev/neo-go/pkg/internal/testserdes" "github.com/nspcc-dev/neo-go/pkg/util" "github.com/stretchr/testify/require" ) @@ -30,3 +32,32 @@ func TestPrepareRequest_Setters(t *testing.T) { p.SetTransactionHashes(hashes[:]) require.Equal(t, hashes[:], p.TransactionHashes()) } + +func TestPrepareRequest_EncodeDecodeBinary(t *testing.T) { + t.Run("positive", func(t *testing.T) { + expected := &prepareRequest{ + timestamp: 112, + nonce: 1325, + transactionHashes: []util.Uint256{ + random.Uint256(), + random.Uint256(), + }, + } + testserdes.EncodeDecodeBinary(t, expected, new(prepareRequest)) + }) + + t.Run("bad hashes count", func(t *testing.T) { + hashes := make([]util.Uint256, block.MaxTransactionsPerBlock+1) + for i := range hashes { + hashes[i] = random.Uint256() + } + expected := &prepareRequest{ + timestamp: 112, + nonce: 1325, + transactionHashes: hashes, + } + data, err := testserdes.EncodeBinary(expected) + require.NoError(t, err) + require.Error(t, testserdes.DecodeBinary(data, new(prepareRequest))) + }) +} diff --git a/pkg/core/block/block.go b/pkg/core/block/block.go index d06e44575..650df993e 100644 --- a/pkg/core/block/block.go +++ b/pkg/core/block/block.go @@ -3,6 +3,7 @@ package block import ( "encoding/json" "errors" + "math" "github.com/Workiva/go-datastructures/queue" "github.com/nspcc-dev/neo-go/pkg/config/netmode" @@ -12,6 +13,16 @@ import ( "github.com/nspcc-dev/neo-go/pkg/util" ) +const ( + // MaxContentsPerBlock is the maximum number of contents (transactions + consensus data) per block. + MaxContentsPerBlock = math.MaxUint16 + // MaxTransactionsPerBlock is the maximum number of transactions per block. + MaxTransactionsPerBlock = MaxContentsPerBlock - 1 +) + +// ErrMaxContentsPerBlock is returned when the maximum number of contents per block is reached. +var ErrMaxContentsPerBlock = errors.New("the number of contents exceeds the maximum number of contents per block") + // Block represents one block in the chain. type Block struct { // The base of the block. @@ -82,6 +93,9 @@ func NewBlockFromTrimmedBytes(network netmode.Magic, b []byte) (*Block, error) { block.Script.DecodeBinary(br) lenHashes := br.ReadVarUint() + if lenHashes > MaxContentsPerBlock { + return nil, ErrMaxContentsPerBlock + } if lenHashes > 0 { var consensusDataHash util.Uint256 consensusDataHash.DecodeBinary(br) @@ -142,6 +156,10 @@ func (b *Block) DecodeBinary(br *io.BinReader) { br.Err = errors.New("invalid block format") return } + if contentsCount > MaxContentsPerBlock { + br.Err = ErrMaxContentsPerBlock + return + } b.ConsensusData.DecodeBinary(br) txes := make([]*transaction.Transaction, contentsCount-1) for i := 0; i < int(contentsCount)-1; i++ { diff --git a/pkg/core/block/block_test.go b/pkg/core/block/block_test.go index 02df2403e..ab1052944 100644 --- a/pkg/core/block/block_test.go +++ b/pkg/core/block/block_test.go @@ -3,6 +3,7 @@ package block import ( "encoding/base64" "encoding/hex" + "errors" "strings" "testing" @@ -218,3 +219,27 @@ func TestBlockCompare(t *testing.T) { assert.Equal(t, 0, b2.Compare(&b2)) assert.Equal(t, -1, b2.Compare(&b3)) } + +func TestBlockEncodeDecode(t *testing.T) { + t.Run("positive", func(t *testing.T) { + b := newDumbBlock() + b.Transactions = []*transaction.Transaction{} + _ = b.Hash() + testserdes.EncodeDecodeBinary(t, b, new(Block)) + }) + + t.Run("bad contents count", func(t *testing.T) { + b := newDumbBlock() + b.Transactions = make([]*transaction.Transaction, MaxContentsPerBlock) + for i := range b.Transactions { + b.Transactions[i] = &transaction.Transaction{ + Script: []byte("my_pretty_script"), + } + } + _ = b.Hash() + data, err := testserdes.EncodeBinary(b) + require.NoError(t, err) + + require.True(t, errors.Is(testserdes.DecodeBinary(data, new(Block)), ErrMaxContentsPerBlock)) + }) +} diff --git a/pkg/network/payload/merkleblock.go b/pkg/network/payload/merkleblock.go index a870450ec..0fef31742 100644 --- a/pkg/network/payload/merkleblock.go +++ b/pkg/network/payload/merkleblock.go @@ -19,8 +19,13 @@ func (m *MerkleBlock) DecodeBinary(br *io.BinReader) { m.Base = &block.Base{} m.Base.DecodeBinary(br) - m.TxCount = int(br.ReadVarUint()) - br.ReadArray(&m.Hashes) + txCount := int(br.ReadVarUint()) + if txCount > block.MaxContentsPerBlock { + br.Err = block.ErrMaxContentsPerBlock + return + } + m.TxCount = txCount + br.ReadArray(&m.Hashes, m.TxCount) m.Flags = br.ReadVarBytes() } diff --git a/pkg/network/payload/merkleblock_test.go b/pkg/network/payload/merkleblock_test.go new file mode 100644 index 000000000..807367744 --- /dev/null +++ b/pkg/network/payload/merkleblock_test.go @@ -0,0 +1,57 @@ +package payload + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/nspcc-dev/neo-go/pkg/core/block" + "github.com/nspcc-dev/neo-go/pkg/core/transaction" + "github.com/nspcc-dev/neo-go/pkg/crypto/hash" + "github.com/nspcc-dev/neo-go/pkg/internal/testserdes" + "github.com/nspcc-dev/neo-go/pkg/util" +) + +func newDumbBlock() *block.Base { + return &block.Base{ + Version: 0, + PrevHash: hash.Sha256([]byte("a")), + MerkleRoot: hash.Sha256([]byte("b")), + Timestamp: 100500, + Index: 1, + NextConsensus: hash.Hash160([]byte("a")), + Script: transaction.Witness{ + VerificationScript: []byte{0x51}, // PUSH1 + InvocationScript: []byte{0x61}, // NOP + }, + } +} + +func TestMerkleBlock_EncodeDecodeBinary(t *testing.T) { + t.Run("positive", func(t *testing.T) { + b := newDumbBlock() + _ = b.Hash() + expected := &MerkleBlock{ + Base: b, + TxCount: 0, + Hashes: []util.Uint256{}, + Flags: []byte{}, + } + testserdes.EncodeDecodeBinary(t, expected, new(MerkleBlock)) + }) + + t.Run("bad contents count", func(t *testing.T) { + b := newDumbBlock() + _ = b.Hash() + expected := &MerkleBlock{ + Base: b, + TxCount: block.MaxContentsPerBlock + 1, + Hashes: make([]util.Uint256, block.MaxContentsPerBlock), + Flags: []byte{}, + } + data, err := testserdes.EncodeBinary(expected) + require.NoError(t, err) + require.True(t, errors.Is(block.ErrMaxContentsPerBlock, testserdes.DecodeBinary(data, new(MerkleBlock)))) + }) +} From 560e470484ab7711369818b94100324ff3f5bebd Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 2 Oct 2020 18:15:16 +0300 Subject: [PATCH 3/4] core: add validation to native Policy methods close #1442 --- pkg/core/native/policy.go | 29 +++++++++++++++++++---------- pkg/core/native_policy_test.go | 32 +++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/pkg/core/native/policy.go b/pkg/core/native/policy.go index 5a172fbbb..80fa90c3e 100644 --- a/pkg/core/native/policy.go +++ b/pkg/core/native/policy.go @@ -8,6 +8,7 @@ import ( "sort" "sync" + "github.com/nspcc-dev/neo-go/pkg/core/block" "github.com/nspcc-dev/neo-go/pkg/core/dao" "github.com/nspcc-dev/neo-go/pkg/core/interop" "github.com/nspcc-dev/neo-go/pkg/core/interop/runtime" @@ -30,6 +31,8 @@ const ( defaultMaxBlockSystemFee = 9000 * GASFactor // minBlockSystemFee is the minimum allowed system fee per block. minBlockSystemFee = 4007600 + // maxFeePerByte is the maximum allowed fee per byte value. + maxFeePerByte = 100_000_000 ) var ( @@ -311,6 +314,10 @@ func (p *Policy) GetBlockedAccountsInternal(dao dao.DAO) (BlockedAccounts, error // setMaxTransactionsPerBlock is Policy contract method and sets the upper limit // of transactions per block. func (p *Policy) setMaxTransactionsPerBlock(ic *interop.Context, args []stackitem.Item) stackitem.Item { + value := uint32(toBigInt(args[0]).Int64()) + if value > block.MaxTransactionsPerBlock { + panic(fmt.Errorf("MaxTransactionsPerBlock cannot exceed the maximum allowed transactions per block = %d", block.MaxTransactionsPerBlock)) + } ok, err := p.checkValidators(ic) if err != nil { panic(err) @@ -318,7 +325,6 @@ func (p *Policy) setMaxTransactionsPerBlock(ic *interop.Context, args []stackite if !ok { return stackitem.NewBool(false) } - value := uint32(toBigInt(args[0]).Int64()) p.lock.Lock() defer p.lock.Unlock() err = p.setUint32WithKey(ic.DAO, maxTransactionsPerBlockKey, value) @@ -331,6 +337,10 @@ func (p *Policy) setMaxTransactionsPerBlock(ic *interop.Context, args []stackite // setMaxBlockSize is Policy contract method and sets maximum block size. func (p *Policy) setMaxBlockSize(ic *interop.Context, args []stackitem.Item) stackitem.Item { + value := uint32(toBigInt(args[0]).Int64()) + if value > payload.MaxSize { + panic(fmt.Errorf("MaxBlockSize cannot be more than the maximum payload size = %d", payload.MaxSize)) + } ok, err := p.checkValidators(ic) if err != nil { panic(err) @@ -338,10 +348,6 @@ func (p *Policy) setMaxBlockSize(ic *interop.Context, args []stackitem.Item) sta if !ok { return stackitem.NewBool(false) } - value := uint32(toBigInt(args[0]).Int64()) - if payload.MaxSize <= value { - return stackitem.NewBool(false) - } p.lock.Lock() defer p.lock.Unlock() err = p.setUint32WithKey(ic.DAO, maxBlockSizeKey, value) @@ -354,6 +360,10 @@ func (p *Policy) setMaxBlockSize(ic *interop.Context, args []stackitem.Item) sta // setFeePerByte is Policy contract method and sets transaction's fee per byte. func (p *Policy) setFeePerByte(ic *interop.Context, args []stackitem.Item) stackitem.Item { + value := toBigInt(args[0]).Int64() + if value < 0 || value > maxFeePerByte { + panic(fmt.Errorf("FeePerByte shouldn't be negative or greater than %d", maxFeePerByte)) + } ok, err := p.checkValidators(ic) if err != nil { panic(err) @@ -361,7 +371,6 @@ func (p *Policy) setFeePerByte(ic *interop.Context, args []stackitem.Item) stack if !ok { return stackitem.NewBool(false) } - value := toBigInt(args[0]).Int64() p.lock.Lock() defer p.lock.Unlock() err = p.setInt64WithKey(ic.DAO, feePerByteKey, value) @@ -374,6 +383,10 @@ func (p *Policy) setFeePerByte(ic *interop.Context, args []stackitem.Item) stack // setMaxBlockSystemFee is Policy contract method and sets the maximum system fee per block. func (p *Policy) setMaxBlockSystemFee(ic *interop.Context, args []stackitem.Item) stackitem.Item { + value := toBigInt(args[0]).Int64() + if value <= minBlockSystemFee { + panic(fmt.Errorf("MaxBlockSystemFee cannot be less then %d", minBlockSystemFee)) + } ok, err := p.checkValidators(ic) if err != nil { panic(err) @@ -381,10 +394,6 @@ func (p *Policy) setMaxBlockSystemFee(ic *interop.Context, args []stackitem.Item if !ok { return stackitem.NewBool(false) } - value := toBigInt(args[0]).Int64() - if value <= minBlockSystemFee { - return stackitem.NewBool(false) - } p.lock.Lock() defer p.lock.Unlock() err = p.setInt64WithKey(ic.DAO, maxBlockSystemFeeKey, value) diff --git a/pkg/core/native_policy_test.go b/pkg/core/native_policy_test.go index 2fb56c8c9..9a5c9f777 100644 --- a/pkg/core/native_policy_test.go +++ b/pkg/core/native_policy_test.go @@ -5,11 +5,13 @@ import ( "sort" "testing" + "github.com/nspcc-dev/neo-go/pkg/core/block" "github.com/nspcc-dev/neo-go/pkg/core/native" "github.com/nspcc-dev/neo-go/pkg/core/state" "github.com/nspcc-dev/neo-go/pkg/core/transaction" "github.com/nspcc-dev/neo-go/pkg/encoding/bigint" "github.com/nspcc-dev/neo-go/pkg/io" + "github.com/nspcc-dev/neo-go/pkg/network/payload" "github.com/nspcc-dev/neo-go/pkg/util" "github.com/nspcc-dev/neo-go/pkg/vm" "github.com/nspcc-dev/neo-go/pkg/vm/emit" @@ -41,6 +43,12 @@ func TestMaxTransactionsPerBlock(t *testing.T) { n := chain.contracts.Policy.GetMaxTransactionsPerBlockInternal(chain.dao) require.Equal(t, 1024, int(n)) }) + + t.Run("set, too big value", func(t *testing.T) { + res, err := invokeNativePolicyMethod(chain, "setMaxTransactionsPerBlock", bigint.ToBytes(big.NewInt(block.MaxContentsPerBlock))) + require.NoError(t, err) + checkFAULTState(t, res) + }) } func TestMaxBlockSize(t *testing.T) { @@ -69,6 +77,12 @@ func TestMaxBlockSize(t *testing.T) { checkResult(t, res, stackitem.NewBigInteger(big.NewInt(102400))) require.NoError(t, chain.persist()) }) + + t.Run("set, too big value", func(t *testing.T) { + res, err := invokeNativePolicyMethod(chain, "setMaxBlockSize", bigint.ToBytes(big.NewInt(payload.MaxSize+1))) + require.NoError(t, err) + checkFAULTState(t, res) + }) } func TestFeePerByte(t *testing.T) { @@ -95,6 +109,18 @@ func TestFeePerByte(t *testing.T) { n := chain.contracts.Policy.GetFeePerByteInternal(chain.dao) require.Equal(t, 1024, int(n)) }) + + t.Run("set, negative value", func(t *testing.T) { + res, err := invokeNativePolicyMethod(chain, "setFeePerByte", bigint.ToBytes(big.NewInt(-1))) + require.NoError(t, err) + checkFAULTState(t, res) + }) + + t.Run("set, too big value", func(t *testing.T) { + res, err := invokeNativePolicyMethod(chain, "setFeePerByte", bigint.ToBytes(big.NewInt(100_000_000+1))) + require.NoError(t, err) + checkFAULTState(t, res) + }) } func TestBlockSystemFee(t *testing.T) { @@ -116,7 +142,7 @@ func TestBlockSystemFee(t *testing.T) { t.Run("set, too low fee", func(t *testing.T) { res, err := invokeNativePolicyMethod(chain, "setMaxBlockSystemFee", bigint.ToBytes(big.NewInt(4007600))) require.NoError(t, err) - checkResult(t, res, stackitem.NewBool(false)) + checkFAULTState(t, res) }) t.Run("set, success", func(t *testing.T) { @@ -251,3 +277,7 @@ func checkResult(t *testing.T, result *state.AppExecResult, expected stackitem.I require.Equal(t, 1, len(result.Stack)) require.Equal(t, expected, result.Stack[0]) } + +func checkFAULTState(t *testing.T, result *state.AppExecResult) { + require.Equal(t, vm.FaultState, result.VMState) +} From 6cff35c927a912b56e00453834da77f45aa52fc4 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 2 Oct 2020 19:30:29 +0300 Subject: [PATCH 4/4] network: restrict flags size in MerkleBlockPayload --- pkg/network/payload/merkleblock.go | 2 +- pkg/network/payload/merkleblock_test.go | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/pkg/network/payload/merkleblock.go b/pkg/network/payload/merkleblock.go index 0fef31742..9867e4ab0 100644 --- a/pkg/network/payload/merkleblock.go +++ b/pkg/network/payload/merkleblock.go @@ -26,7 +26,7 @@ func (m *MerkleBlock) DecodeBinary(br *io.BinReader) { } m.TxCount = txCount br.ReadArray(&m.Hashes, m.TxCount) - m.Flags = br.ReadVarBytes() + m.Flags = br.ReadVarBytes((txCount + 7) / 8) } // EncodeBinary implements Serializable interface. diff --git a/pkg/network/payload/merkleblock_test.go b/pkg/network/payload/merkleblock_test.go index 807367744..30362bb40 100644 --- a/pkg/network/payload/merkleblock_test.go +++ b/pkg/network/payload/merkleblock_test.go @@ -54,4 +54,18 @@ func TestMerkleBlock_EncodeDecodeBinary(t *testing.T) { require.NoError(t, err) require.True(t, errors.Is(block.ErrMaxContentsPerBlock, testserdes.DecodeBinary(data, new(MerkleBlock)))) }) + + t.Run("bad flags size", func(t *testing.T) { + b := newDumbBlock() + _ = b.Hash() + expected := &MerkleBlock{ + Base: b, + TxCount: 0, + Hashes: []util.Uint256{}, + Flags: []byte{1, 2, 3, 4, 5}, + } + data, err := testserdes.EncodeBinary(expected) + require.NoError(t, err) + require.Error(t, testserdes.DecodeBinary(data, new(MerkleBlock))) + }) }