From 1f9b92c295e3c81856545963b4a7ee39519c5ee8 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 28 Sep 2020 16:09:42 +0300 Subject: [PATCH 1/4] vm: restrict comparable ByteArray length MaxByteArrayComparableSize should equals to 65535. --- pkg/vm/stackitem/item.go | 36 +++++++++++++++++++++++++---------- pkg/vm/stackitem/item_test.go | 28 ++++++++++++++++++++++++--- 2 files changed, 51 insertions(+), 13 deletions(-) diff --git a/pkg/vm/stackitem/item.go b/pkg/vm/stackitem/item.go index d928c1a62..33292e3f6 100644 --- a/pkg/vm/stackitem/item.go +++ b/pkg/vm/stackitem/item.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "fmt" + "math" "math/big" "reflect" "unicode/utf8" @@ -16,14 +17,17 @@ import ( "github.com/nspcc-dev/neo-go/pkg/util" ) -// MaxBigIntegerSizeBits is the maximum size of BigInt item in bits. -const MaxBigIntegerSizeBits = 32 * 8 - -// MaxArraySize is the maximum array size allowed in the VM. -const MaxArraySize = 1024 - -// MaxSize is the maximum item size allowed in the VM. -const MaxSize = 1024 * 1024 +const ( + // MaxBigIntegerSizeBits is the maximum size of BigInt item in bits. + MaxBigIntegerSizeBits = 32 * 8 + // MaxArraySize is the maximum array size allowed in the VM. + MaxArraySize = 1024 + // MaxSize is the maximum item size allowed in the VM. + MaxSize = 1024 * 1024 + // MaxByteArrayComparableSize is the maximum allowed length of ByteArray for Equals method. + // It is set to be the maximum uint16 value. + MaxByteArrayComparableSize = math.MaxUint16 +) // Item represents the "real" value that is pushed on the stack. type Item interface { @@ -46,7 +50,10 @@ type Item interface { Convert(Type) (Item, error) } -var errInvalidConversion = errors.New("invalid conversion type") +var ( + errInvalidConversion = errors.New("invalid conversion type") + errExceedingMaxComparableSize = errors.New("the operand exceeds the maximum comparable size") +) // Make tries to make appropriate stack item from provided value. // It will panic if it's not possible. @@ -536,13 +543,22 @@ func (i *ByteArray) TryInteger() (*big.Int, error) { // Equals implements Item interface. func (i *ByteArray) Equals(s Item) bool { + if len(i.value) > MaxByteArrayComparableSize { + panic(errExceedingMaxComparableSize) + } if i == s { return true } else if s == nil { return false } val, ok := s.(*ByteArray) - return ok && bytes.Equal(i.value, val.value) + if !ok { + return false + } + if len(val.value) > MaxByteArrayComparableSize { + panic(errExceedingMaxComparableSize) + } + return bytes.Equal(i.value, val.value) } // Dup implements Item interface. diff --git a/pkg/vm/stackitem/item_test.go b/pkg/vm/stackitem/item_test.go index 43c2a03dd..14e378dad 100644 --- a/pkg/vm/stackitem/item_test.go +++ b/pkg/vm/stackitem/item_test.go @@ -143,6 +143,7 @@ var equalsTestCases = map[string][]struct { item1 Item item2 Item result bool + panics bool }{ "struct": { { @@ -251,6 +252,21 @@ var equalsTestCases = map[string][]struct { item2: Make([]byte{1, 2, 3}), result: true, }, + { + item1: NewByteArray(make([]byte, MaxByteArrayComparableSize+1)), + item2: NewByteArray([]byte{1, 2, 3}), + panics: true, + }, + { + item1: NewByteArray([]byte{1, 2, 3}), + item2: NewByteArray(make([]byte, MaxByteArrayComparableSize+1)), + panics: true, + }, + { + item1: NewByteArray(make([]byte, MaxByteArrayComparableSize+1)), + item2: NewByteArray(make([]byte, MaxByteArrayComparableSize+1)), + panics: true, + }, }, "array": { { @@ -350,9 +366,15 @@ func TestEquals(t *testing.T) { for name, testBatch := range equalsTestCases { for _, testCase := range testBatch { t.Run(name, func(t *testing.T) { - assert.Equal(t, testCase.result, testCase.item1.Equals(testCase.item2)) - // Reference equals - assert.Equal(t, true, testCase.item1.Equals(testCase.item1)) + if testCase.panics { + assert.Panics(t, func() { + testCase.item1.Equals(testCase.item2) + }) + } else { + assert.Equal(t, testCase.result, testCase.item1.Equals(testCase.item2)) + // Reference equals + assert.Equal(t, true, testCase.item1.Equals(testCase.item1)) + } }) } } From 543fd58e9397a3185955d06117f62549402dbd34 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Tue, 29 Sep 2020 10:56:57 +0300 Subject: [PATCH 2/4] vm: restrict map key size --- docs/compiler.md | 1 + pkg/vm/stackitem/item.go | 20 ++++++++++++++------ pkg/vm/stackitem/json.go | 4 ++-- pkg/vm/stackitem/json_test.go | 2 +- pkg/vm/vm.go | 4 ++-- 5 files changed, 20 insertions(+), 11 deletions(-) diff --git a/docs/compiler.md b/docs/compiler.md index ebee25c9d..d3166c73b 100644 --- a/docs/compiler.md +++ b/docs/compiler.md @@ -24,6 +24,7 @@ a dialect of Go rather than a complete port of the language: overhead for all contracts. This can easily be mitigated by first storing values in variables and returning the result. * lambdas are supported, but closures are not. + * maps are supported, but valid map keys are booleans, integers and strings with length <= 64 ## VM API (interop layer) Compiler translates interop function calls into NEO VM syscalls or (for custom diff --git a/pkg/vm/stackitem/item.go b/pkg/vm/stackitem/item.go index 33292e3f6..ca33f111e 100644 --- a/pkg/vm/stackitem/item.go +++ b/pkg/vm/stackitem/item.go @@ -27,6 +27,8 @@ const ( // MaxByteArrayComparableSize is the maximum allowed length of ByteArray for Equals method. // It is set to be the maximum uint16 value. MaxByteArrayComparableSize = math.MaxUint16 + // MaxKeySize is the maximum size of map key. + MaxKeySize = 64 ) // Item represents the "real" value that is pushed on the stack. @@ -773,8 +775,8 @@ func (i *Map) Convert(typ Type) (Item, error) { // Add adds key-value pair to the map. func (i *Map) Add(key, value Item) { - if !IsValidMapKey(key) { - panic("wrong key type") + if err := IsValidMapKey(key); err != nil { + panic(err) } index := i.Index(key) if index >= 0 { @@ -792,12 +794,18 @@ func (i *Map) Drop(index int) { // IsValidMapKey checks whether it's possible to use given Item as a Map // key. -func IsValidMapKey(key Item) bool { +func IsValidMapKey(key Item) error { switch key.(type) { - case *Bool, *BigInteger, *ByteArray: - return true + case *Bool, *BigInteger: + return nil + case *ByteArray: + size := len(key.Value().([]byte)) + if size > MaxKeySize { + return fmt.Errorf("invalid map key size: %d", size) + } + return nil default: - return false + return fmt.Errorf("invalid map key of type %s", key.Type()) } } diff --git a/pkg/vm/stackitem/json.go b/pkg/vm/stackitem/json.go index 4a4b359a7..a1886ef30 100644 --- a/pkg/vm/stackitem/json.go +++ b/pkg/vm/stackitem/json.go @@ -360,8 +360,8 @@ func FromJSONWithTypes(data []byte) (Item, error) { key, err := FromJSONWithTypes(arr[i].Key) if err != nil { return nil, err - } else if !IsValidMapKey(key) { - return nil, fmt.Errorf("invalid map key of type %s", key.Type()) + } else if err = IsValidMapKey(key); err != nil { + return nil, err } value, err := FromJSONWithTypes(arr[i].Value) if err != nil { diff --git a/pkg/vm/stackitem/json_test.go b/pkg/vm/stackitem/json_test.go index bb9e17acd..c8e15dbed 100644 --- a/pkg/vm/stackitem/json_test.go +++ b/pkg/vm/stackitem/json_test.go @@ -88,7 +88,7 @@ func TestFromToJSON(t *testing.T) { t.Run("BigNestedArray", getTestDecodeFunc(`[[[[[[[[[[[]]]]]]]]]]]`, nil)) t.Run("EncodeRecursive", func(t *testing.T) { // add this item to speed up test a bit - item := NewByteArray(make([]byte, MaxSize/100)) + item := NewByteArray(make([]byte, MaxKeySize)) t.Run("Array", func(t *testing.T) { arr := NewArray([]Item{item}) arr.Append(arr) diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index 17d6521e6..0aa99207b 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -1668,8 +1668,8 @@ func validateMapKey(key *Element) { if key == nil { panic("no key found") } - if !stackitem.IsValidMapKey(key.Item()) { - panic("key can't be a collection") + if err := stackitem.IsValidMapKey(key.Item()); err != nil { + panic(err) } } From 66ca654b0704c7d3bd5b1932cb6bf73f8a3fb1f8 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Tue, 29 Sep 2020 12:21:51 +0300 Subject: [PATCH 3/4] vm: refactor ISNULL opcode handling --- pkg/vm/vm.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index 0aa99207b..ffc9332e6 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -531,8 +531,8 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro v.estack.PushVal(stackitem.Null{}) case opcode.ISNULL: - res := v.estack.Pop().value.Equals(stackitem.Null{}) - v.estack.PushVal(res) + _, ok := v.estack.Pop().value.(stackitem.Null) + v.estack.PushVal(ok) case opcode.ISTYPE: res := v.estack.Pop().Item() From 6761c297b5e0f20cb6cc258bd3fd7f861325ca07 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 28 Sep 2020 17:50:25 +0300 Subject: [PATCH 4/4] vm: add test to check reference counter after PACK-UNPACK Related issue: https://github.com/neo-project/neo-vm/pull/370 --- pkg/vm/vm_test.go | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/pkg/vm/vm_test.go b/pkg/vm/vm_test.go index c32424ec2..81aa8ff18 100644 --- a/pkg/vm/vm_test.go +++ b/pkg/vm/vm_test.go @@ -2031,6 +2031,52 @@ func TestPACKGood(t *testing.T) { assert.Equal(t, int64(1), vm.estack.Peek(1).BigInt().Int64()) } +func TestPACK_UNPACK_MaxSize(t *testing.T) { + prog := makeProgram(opcode.PACK, opcode.UNPACK) + elements := make([]int, stackitem.MaxArraySize) + vm := load(prog) + // canary + vm.estack.PushVal(1) + for i := len(elements) - 1; i >= 0; i-- { + vm.estack.PushVal(elements[i]) + } + vm.estack.PushVal(len(elements)) + runVM(t, vm) + // check reference counter = 1+1+1024 + assert.Equal(t, 1+1+len(elements), vm.refs.size) + assert.Equal(t, 1+1+len(elements), vm.estack.Len()) // canary + length + elements + assert.Equal(t, int64(len(elements)), vm.estack.Peek(0).Value().(*big.Int).Int64()) + for i := 0; i < len(elements); i++ { + e, ok := vm.estack.Peek(i + 1).Value().(*big.Int) + assert.True(t, ok) + assert.Equal(t, int64(elements[i]), e.Int64()) + } + assert.Equal(t, int64(1), vm.estack.Peek(1+len(elements)).BigInt().Int64()) +} + +func TestPACK_UNPACK_PACK_MaxSize(t *testing.T) { + prog := makeProgram(opcode.PACK, opcode.UNPACK, opcode.PACK) + elements := make([]int, stackitem.MaxArraySize) + vm := load(prog) + // canary + vm.estack.PushVal(1) + for i := len(elements) - 1; i >= 0; i-- { + vm.estack.PushVal(elements[i]) + } + vm.estack.PushVal(len(elements)) + runVM(t, vm) + // check reference counter = 1+1+1024 + assert.Equal(t, 1+1+len(elements), vm.refs.size) + assert.Equal(t, 2, vm.estack.Len()) + a := vm.estack.Peek(0).Array() + assert.Equal(t, len(elements), len(a)) + for i := 0; i < len(elements); i++ { + e := a[i].Value().(*big.Int) + assert.Equal(t, int64(elements[i]), e.Int64()) + } + assert.Equal(t, int64(1), vm.estack.Peek(1).BigInt().Int64()) +} + func TestUNPACKBadNotArray(t *testing.T) { prog := makeProgram(opcode.UNPACK) runWithArgs(t, prog, nil, 1)