From 158f0d9d9c9bc33f1f78df057bbf4babfdbadd41 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Tue, 9 Feb 2021 21:42:39 +0300 Subject: [PATCH] native/vm: add script check for deployed contracts Refs. #1699. --- pkg/core/native/management.go | 22 +++++ pkg/core/native/management_test.go | 3 +- pkg/core/native_management_test.go | 50 ++++++++++ pkg/util/bitfield/bitfield.go | 15 +++ pkg/util/bitfield/bitfield_test.go | 4 + pkg/vm/contract_checks.go | 62 ++++++++++++ pkg/vm/contract_checks_test.go | 153 +++++++++++++++++++++++++++++ pkg/vm/vm.go | 26 ++--- 8 files changed, 321 insertions(+), 14 deletions(-) diff --git a/pkg/core/native/management.go b/pkg/core/native/management.go index e6ffcf9b2..6aded9512 100644 --- a/pkg/core/native/management.go +++ b/pkg/core/native/management.go @@ -22,6 +22,8 @@ import ( "github.com/nspcc-dev/neo-go/pkg/smartcontract/manifest" "github.com/nspcc-dev/neo-go/pkg/smartcontract/nef" "github.com/nspcc-dev/neo-go/pkg/util" + "github.com/nspcc-dev/neo-go/pkg/util/bitfield" + "github.com/nspcc-dev/neo-go/pkg/vm" "github.com/nspcc-dev/neo-go/pkg/vm/stackitem" ) @@ -273,6 +275,10 @@ func (m *Management) Deploy(d dao.DAO, sender util.Uint160, neff *nef.File, mani if err != nil { return nil, fmt.Errorf("invalid manifest: %w", err) } + err = checkScriptAndMethods(neff.Script, manif.ABI.Methods) + if err != nil { + return nil, err + } newcontract := &state.Contract{ ID: id, Hash: h, @@ -334,6 +340,10 @@ func (m *Management) Update(d dao.DAO, hash util.Uint160, neff *nef.File, manif m.markUpdated(hash) contract.Manifest = *manif } + err = checkScriptAndMethods(contract.NEF.Script, contract.Manifest.ABI.Methods) + if err != nil { + return nil, err + } contract.UpdateCounter++ err = m.PutContractState(d, contract) if err != nil { @@ -551,3 +561,15 @@ func (m *Management) emitNotification(ic *interop.Context, name string, hash uti } ic.Notifications = append(ic.Notifications, ne) } + +func checkScriptAndMethods(script []byte, methods []manifest.Method) error { + l := len(script) + offsets := bitfield.New(l) + for i := range methods { + if methods[i].Offset >= l { + return errors.New("out of bounds method offset") + } + offsets.Set(methods[i].Offset) + } + return vm.IsScriptCorrect(script, offsets) +} diff --git a/pkg/core/native/management_test.go b/pkg/core/native/management_test.go index ffa6a0c88..7dfa4c054 100644 --- a/pkg/core/native/management_test.go +++ b/pkg/core/native/management_test.go @@ -12,6 +12,7 @@ import ( "github.com/nspcc-dev/neo-go/pkg/smartcontract/manifest" "github.com/nspcc-dev/neo-go/pkg/smartcontract/nef" "github.com/nspcc-dev/neo-go/pkg/util" + "github.com/nspcc-dev/neo-go/pkg/vm/opcode" "github.com/stretchr/testify/require" ) @@ -19,7 +20,7 @@ func TestDeployGetUpdateDestroyContract(t *testing.T) { mgmt := newManagement() d := dao.NewCached(dao.NewSimple(storage.NewMemoryStore(), netmode.UnitTestNet, false)) mgmt.Initialize(&interop.Context{DAO: d}) - script := []byte{1} + script := []byte{byte(opcode.RET)} sender := util.Uint160{1, 2, 3} ne, err := nef.NewFile(script) require.NoError(t, err) diff --git a/pkg/core/native_management_test.go b/pkg/core/native_management_test.go index 972a97dd9..b0ba0082a 100644 --- a/pkg/core/native_management_test.go +++ b/pkg/core/native_management_test.go @@ -163,6 +163,17 @@ func TestContractDeploy(t *testing.T) { require.NoError(t, err) checkFAULTState(t, res) }) + t.Run("bad script in NEF", func(t *testing.T) { + nf, err := nef.FileFromBytes(nef1b) // make a full copy + require.NoError(t, err) + nf.Script[0] = 0xff + nf.CalculateChecksum() + nefbad, err := nf.Bytes() + require.NoError(t, err) + res, err := invokeContractMethod(bc, 11_00000000, mgmtHash, "deploy", nefbad, manif1) + require.NoError(t, err) + checkFAULTState(t, res) + }) t.Run("int for manifest", func(t *testing.T) { res, err := invokeContractMethod(bc, 11_00000000, mgmtHash, "deploy", nef1b, int64(1)) require.NoError(t, err) @@ -198,6 +209,32 @@ func TestContractDeploy(t *testing.T) { require.NoError(t, err) checkFAULTState(t, res) }) + t.Run("bad methods in manifest 1", func(t *testing.T) { + var badManifest = cs1.Manifest + badManifest.ABI.Methods = make([]manifest.Method, len(cs1.Manifest.ABI.Methods)) + copy(badManifest.ABI.Methods, cs1.Manifest.ABI.Methods) + badManifest.ABI.Methods[0].Offset = 100500 // out of bounds + + manifB, err := json.Marshal(badManifest) + require.NoError(t, err) + res, err := invokeContractMethod(bc, 11_00000000, mgmtHash, "deploy", nef1b, manifB) + require.NoError(t, err) + checkFAULTState(t, res) + }) + + t.Run("bad methods in manifest 2", func(t *testing.T) { + var badManifest = cs1.Manifest + badManifest.ABI.Methods = make([]manifest.Method, len(cs1.Manifest.ABI.Methods)) + copy(badManifest.ABI.Methods, cs1.Manifest.ABI.Methods) + badManifest.ABI.Methods[0].Offset = len(cs1.NEF.Script) - 2 // Ends with `CALLT(X,X);RET`. + + manifB, err := json.Marshal(badManifest) + require.NoError(t, err) + res, err := invokeContractMethod(bc, 11_00000000, mgmtHash, "deploy", nef1b, manifB) + require.NoError(t, err) + checkFAULTState(t, res) + }) + t.Run("not enough GAS", func(t *testing.T) { res, err := invokeContractMethod(bc, 1_00000000, mgmtHash, "deploy", nef1b, manif1) require.NoError(t, err) @@ -382,6 +419,19 @@ func TestContractUpdate(t *testing.T) { require.NoError(t, err) checkFAULTState(t, res) }) + t.Run("manifest and script mismatch", func(t *testing.T) { + nf, err := nef.FileFromBytes(nef1b) // Make a full copy. + require.NoError(t, err) + nf.Script = append(nf.Script, byte(opcode.RET)) + copy(nf.Script[1:], nf.Script) // Now all method offsets are wrong. + nf.Script[0] = byte(opcode.RET) // Even though the script is correct. + nf.CalculateChecksum() + nefnew, err := nf.Bytes() + require.NoError(t, err) + res, err := invokeContractMethod(bc, 10_00000000, cs1.Hash, "update", nefnew, manif1) + require.NoError(t, err) + checkFAULTState(t, res) + }) t.Run("change name", func(t *testing.T) { var badManifest = cs1.Manifest diff --git a/pkg/util/bitfield/bitfield.go b/pkg/util/bitfield/bitfield.go index 26344c4c8..8766c8d60 100644 --- a/pkg/util/bitfield/bitfield.go +++ b/pkg/util/bitfield/bitfield.go @@ -62,3 +62,18 @@ func (f Field) Equals(o Field) bool { } return true } + +// IsSubset returns true when f is a subset of o (only has bits set that are +// set in o). +func (f Field) IsSubset(o Field) bool { + if len(f) > len(o) { + return false + } + for i := range f { + r := f[i] & o[i] + if r != f[i] { + return false + } + } + return true +} diff --git a/pkg/util/bitfield/bitfield_test.go b/pkg/util/bitfield/bitfield_test.go index 38c4ab1af..f71bd4e71 100644 --- a/pkg/util/bitfield/bitfield_test.go +++ b/pkg/util/bitfield/bitfield_test.go @@ -17,6 +17,7 @@ func TestFields(t *testing.T) { b.Set(100) require.True(t, a.IsSet(42)) require.False(t, b.IsSet(43)) + require.True(t, a.IsSubset(b)) v := uint64(1<<10 | 1<<42) require.Equal(t, v, a[0]) @@ -28,14 +29,17 @@ func TestFields(t *testing.T) { require.True(t, c.Equals(b)) z := New(128) + require.True(t, z.IsSubset(c)) c.And(a) require.True(t, c.Equals(b)) c.And(z) require.True(t, c.Equals(z)) c = New(64) + require.False(t, z.IsSubset(c)) c[0] = a[0] require.False(t, c.Equals(a)) + require.True(t, c.IsSubset(a)) b.And(c) require.False(t, b.Equals(a)) diff --git a/pkg/vm/contract_checks.go b/pkg/vm/contract_checks.go index 1e8905663..00b425032 100644 --- a/pkg/vm/contract_checks.go +++ b/pkg/vm/contract_checks.go @@ -2,9 +2,12 @@ package vm import ( "encoding/binary" + "errors" + "fmt" "github.com/nspcc-dev/neo-go/pkg/core/interop/interopnames" "github.com/nspcc-dev/neo-go/pkg/encoding/bigint" + "github.com/nspcc-dev/neo-go/pkg/util/bitfield" "github.com/nspcc-dev/neo-go/pkg/vm/opcode" "github.com/nspcc-dev/neo-go/pkg/vm/stackitem" ) @@ -127,3 +130,62 @@ func IsSignatureContract(script []byte) bool { func IsStandardContract(script []byte) bool { return IsSignatureContract(script) || IsMultiSigContract(script) } + +// IsScriptCorrect checks script for errors and mask provided for correctness wrt +// instruction boundaries. Normally it returns nil, but can return some specific +// error if there is any. +func IsScriptCorrect(script []byte, methods bitfield.Field) error { + var ( + l = len(script) + instrs = bitfield.New(l) + jumps = bitfield.New(l) + ) + ctx := NewContext(script) + for ctx.nextip < l { + op, param, err := ctx.Next() + if err != nil { + return err + } + instrs.Set(ctx.ip) + switch op { + case opcode.JMP, opcode.JMPIF, opcode.JMPIFNOT, opcode.JMPEQ, opcode.JMPNE, + opcode.JMPGT, opcode.JMPGE, opcode.JMPLT, opcode.JMPLE, + opcode.CALL, opcode.ENDTRY, opcode.JMPL, opcode.JMPIFL, + opcode.JMPIFNOTL, opcode.JMPEQL, opcode.JMPNEL, + opcode.JMPGTL, opcode.JMPGEL, opcode.JMPLTL, opcode.JMPLEL, + opcode.ENDTRYL, opcode.CALLL, opcode.PUSHA: + off, _, err := calcJumpOffset(ctx, param) // It does bounds checking. + if err != nil { + return err + } + jumps.Set(off) + case opcode.TRY, opcode.TRYL: + catchP, finallyP := getTryParams(op, param) + off, _, err := calcJumpOffset(ctx, catchP) + if err != nil { + return err + } + jumps.Set(off) + off, _, err = calcJumpOffset(ctx, finallyP) + if err != nil { + return err + } + jumps.Set(off) + case opcode.NEWARRAYT, opcode.ISTYPE, opcode.CONVERT: + typ := stackitem.Type(param[0]) + if !typ.IsValid() { + return fmt.Errorf("invalid type specification at offset %d", ctx.ip) + } + if typ == stackitem.AnyT && op != opcode.NEWARRAYT { + return fmt.Errorf("using type ANY is incorrect at offset %d", ctx.ip) + } + } + } + if !jumps.IsSubset(instrs) { + return errors.New("some jumps are done to wrong offsets (not to instruction boundary)") + } + if methods != nil && !methods.IsSubset(instrs) { + return errors.New("some methods point to wrong offsets (not to instruction boundary)") + } + return nil +} diff --git a/pkg/vm/contract_checks_test.go b/pkg/vm/contract_checks_test.go index 4f5f83530..88eb1aeab 100644 --- a/pkg/vm/contract_checks_test.go +++ b/pkg/vm/contract_checks_test.go @@ -5,8 +5,12 @@ import ( "testing" "github.com/nspcc-dev/neo-go/pkg/crypto/keys" + "github.com/nspcc-dev/neo-go/pkg/io" "github.com/nspcc-dev/neo-go/pkg/smartcontract" + "github.com/nspcc-dev/neo-go/pkg/util/bitfield" + "github.com/nspcc-dev/neo-go/pkg/vm/emit" "github.com/nspcc-dev/neo-go/pkg/vm/opcode" + "github.com/nspcc-dev/neo-go/pkg/vm/stackitem" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -115,3 +119,152 @@ func TestIsMultiSigContract(t *testing.T) { assert.False(t, IsMultiSigContract(prog)) }) } + +func TestIsScriptCorrect(t *testing.T) { + w := io.NewBufBinWriter() + emit.String(w.BinWriter, "something") + + jmpOff := w.Len() + emit.Opcodes(w.BinWriter, opcode.JMP, opcode.Opcode(-jmpOff)) + + retOff := w.Len() + emit.Opcodes(w.BinWriter, opcode.RET) + + jmplOff := w.Len() + emit.Opcodes(w.BinWriter, opcode.JMPL, opcode.Opcode(0xff), opcode.Opcode(0xff), opcode.Opcode(0xff), opcode.Opcode(0xff)) + + tryOff := w.Len() + emit.Opcodes(w.BinWriter, opcode.TRY, opcode.Opcode(3), opcode.Opcode(0xfb)) // -5 + + trylOff := w.Len() + emit.Opcodes(w.BinWriter, opcode.TRYL, opcode.Opcode(0xfd), opcode.Opcode(0xff), opcode.Opcode(0xff), opcode.Opcode(0xff), + opcode.Opcode(9), opcode.Opcode(0), opcode.Opcode(0), opcode.Opcode(0)) + + istypeOff := w.Len() + emit.Opcodes(w.BinWriter, opcode.ISTYPE, opcode.Opcode(stackitem.IntegerT)) + + pushOff := w.Len() + emit.String(w.BinWriter, "else") + + good := w.Bytes() + + getScript := func() []byte { + s := make([]byte, len(good)) + copy(s, good) + return s + } + + t.Run("good", func(t *testing.T) { + require.NoError(t, IsScriptCorrect(good, nil)) + }) + + t.Run("bad instruction", func(t *testing.T) { + bad := getScript() + bad[retOff] = 0xff + require.Error(t, IsScriptCorrect(bad, nil)) + }) + + t.Run("out of bounds JMP 1", func(t *testing.T) { + bad := getScript() + bad[jmpOff+1] = 0x80 // -128 + require.Error(t, IsScriptCorrect(bad, nil)) + }) + + t.Run("out of bounds JMP 2", func(t *testing.T) { + bad := getScript() + bad[jmpOff+1] = 0x7f + require.Error(t, IsScriptCorrect(bad, nil)) + }) + + t.Run("bad JMP offset 1", func(t *testing.T) { + bad := getScript() + bad[jmpOff+1] = 0xff // into "something" + require.Error(t, IsScriptCorrect(bad, nil)) + }) + + t.Run("bad JMP offset 2", func(t *testing.T) { + bad := getScript() + bad[jmpOff+1] = byte(pushOff - jmpOff + 1) + require.Error(t, IsScriptCorrect(bad, nil)) + }) + + t.Run("out of bounds JMPL 1", func(t *testing.T) { + bad := getScript() + bad[jmplOff+1] = byte(-jmplOff - 1) + require.Error(t, IsScriptCorrect(bad, nil)) + }) + + t.Run("out of bounds JMPL 1", func(t *testing.T) { + bad := getScript() + bad[jmplOff+1] = byte(len(bad) - jmplOff) + bad[jmplOff+2] = 0 + bad[jmplOff+3] = 0 + bad[jmplOff+4] = 0 + require.Error(t, IsScriptCorrect(bad, nil)) + }) + + t.Run("bad JMPL offset", func(t *testing.T) { + bad := getScript() + bad[jmplOff+1] = 0xfe // into JMP + require.Error(t, IsScriptCorrect(bad, nil)) + }) + + t.Run("out of bounds TRY 1", func(t *testing.T) { + bad := getScript() + bad[tryOff+1] = byte(-tryOff - 1) + require.Error(t, IsScriptCorrect(bad, nil)) + }) + + t.Run("out of bounds TRY 2", func(t *testing.T) { + bad := getScript() + bad[tryOff+2] = byte(len(bad) - tryOff) + require.Error(t, IsScriptCorrect(bad, nil)) + }) + + t.Run("bad TRYL offset 1", func(t *testing.T) { + bad := getScript() + bad[trylOff+1] = byte(-(trylOff - jmpOff) - 1) // into "something" + require.Error(t, IsScriptCorrect(bad, nil)) + }) + + t.Run("bad TRYL offset 2", func(t *testing.T) { + bad := getScript() + bad[trylOff+5] = byte(len(bad) - trylOff - 1) + require.Error(t, IsScriptCorrect(bad, nil)) + }) + + t.Run("bad ISTYPE type", func(t *testing.T) { + bad := getScript() + bad[istypeOff+1] = byte(0xff) + require.Error(t, IsScriptCorrect(bad, nil)) + }) + + t.Run("bad ISTYPE type (Any)", func(t *testing.T) { + bad := getScript() + bad[istypeOff+1] = byte(stackitem.AnyT) + require.Error(t, IsScriptCorrect(bad, nil)) + }) + + t.Run("good NEWARRAY_T type", func(t *testing.T) { + bad := getScript() + bad[istypeOff] = byte(opcode.NEWARRAYT) + bad[istypeOff+1] = byte(stackitem.AnyT) + require.NoError(t, IsScriptCorrect(bad, nil)) + }) + + t.Run("good methods", func(t *testing.T) { + methods := bitfield.New(len(good)) + methods.Set(retOff) + methods.Set(tryOff) + methods.Set(pushOff) + require.NoError(t, IsScriptCorrect(good, methods)) + }) + + t.Run("bad methods", func(t *testing.T) { + methods := bitfield.New(len(good)) + methods.Set(retOff) + methods.Set(tryOff) + methods.Set(pushOff + 1) + require.Error(t, IsScriptCorrect(good, methods)) + }) +} diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index 67f3b1f0c..3320da5eb 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -185,11 +185,11 @@ func (v *VM) PrintOps(out io.Writer) { opcode.JMPEQL, opcode.JMPNEL, opcode.JMPGTL, opcode.JMPGEL, opcode.JMPLEL, opcode.JMPLTL, opcode.PUSHA, opcode.ENDTRY, opcode.ENDTRYL: - desc = v.getOffsetDesc(ctx, parameter) + desc = getOffsetDesc(ctx, parameter) case opcode.TRY, opcode.TRYL: catchP, finallyP := getTryParams(instr, parameter) desc = fmt.Sprintf("catch %s, finally %s", - v.getOffsetDesc(ctx, catchP), v.getOffsetDesc(ctx, finallyP)) + getOffsetDesc(ctx, catchP), getOffsetDesc(ctx, finallyP)) case opcode.INITSSLOT: desc = fmt.Sprint(parameter[0]) case opcode.CONVERT, opcode.ISTYPE: @@ -226,8 +226,8 @@ func (v *VM) PrintOps(out io.Writer) { w.Flush() } -func (v *VM) getOffsetDesc(ctx *Context, parameter []byte) string { - offset, rOffset, err := v.calcJumpOffset(ctx, parameter) +func getOffsetDesc(ctx *Context, parameter []byte) string { + offset, rOffset, err := calcJumpOffset(ctx, parameter) if err != nil { return fmt.Sprintf("ERROR: %v", err) } @@ -552,7 +552,7 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro v.estack.PushVal(parameter) case opcode.PUSHA: - n := v.getJumpOffset(ctx, parameter) + n := getJumpOffset(ctx, parameter) ptr := stackitem.NewPointerWithHash(n, ctx.prog, ctx.ScriptHash()) v.estack.PushVal(ptr) @@ -1249,7 +1249,7 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro opcode.JMPEQ, opcode.JMPEQL, opcode.JMPNE, opcode.JMPNEL, opcode.JMPGT, opcode.JMPGTL, opcode.JMPGE, opcode.JMPGEL, opcode.JMPLT, opcode.JMPLTL, opcode.JMPLE, opcode.JMPLEL: - offset := v.getJumpOffset(ctx, parameter) + offset := getJumpOffset(ctx, parameter) cond := true switch op { case opcode.JMP, opcode.JMPL: @@ -1268,7 +1268,7 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro case opcode.CALL, opcode.CALLL: // Note: jump offset must be calculated regarding to new context, // but it is cloned and thus has the same script and instruction pointer. - v.call(ctx, v.getJumpOffset(ctx, parameter)) + v.call(ctx, getJumpOffset(ctx, parameter)) case opcode.CALLA: ptr := v.estack.Pop().Item().(*stackitem.Pointer) @@ -1406,8 +1406,8 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro if ctx.tryStack.Len() >= MaxTryNestingDepth { panic("maximum TRY depth exceeded") } - cOffset := v.getJumpOffset(ctx, catchP) - fOffset := v.getJumpOffset(ctx, finallyP) + cOffset := getJumpOffset(ctx, catchP) + fOffset := getJumpOffset(ctx, finallyP) if cOffset == ctx.ip && fOffset == ctx.ip { panic("invalid offset for TRY*") } else if cOffset == ctx.ip { @@ -1423,7 +1423,7 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro if eCtx.State == eFinally { panic("invalid exception handling state during ENDTRY*") } - eOffset := v.getJumpOffset(ctx, parameter) + eOffset := getJumpOffset(ctx, parameter) if eCtx.HasFinally() { eCtx.State = eFinally eCtx.EndOffset = eOffset @@ -1527,8 +1527,8 @@ func (v *VM) call(ctx *Context, offset int) { // to a which JMP should be performed. // parameter should have length either 1 or 4 and // is interpreted as little-endian. -func (v *VM) getJumpOffset(ctx *Context, parameter []byte) int { - offset, _, err := v.calcJumpOffset(ctx, parameter) +func getJumpOffset(ctx *Context, parameter []byte) int { + offset, _, err := calcJumpOffset(ctx, parameter) if err != nil { panic(err) } @@ -1537,7 +1537,7 @@ func (v *VM) getJumpOffset(ctx *Context, parameter []byte) int { // calcJumpOffset returns absolute and relative offset of JMP/CALL/TRY instructions // either in short (1-byte) or long (4-byte) form. -func (v *VM) calcJumpOffset(ctx *Context, parameter []byte) (int, int, error) { +func calcJumpOffset(ctx *Context, parameter []byte) (int, int, error) { var rOffset int32 switch l := len(parameter); l { case 1: