native/vm: add script check for deployed contracts

Refs. #1699.

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)
+}

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)

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

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
+}

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))

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
+}

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))
+	})
+}

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 {
+func getOffsetDesc(ctx *Context, parameter []byte) string {
-	offset, rOffset, err := v.calcJumpOffset(ctx, parameter)
+	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)
+		cOffset := getJumpOffset(ctx, catchP)
-		fOffset := v.getJumpOffset(ctx, finallyP)
+		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 {
+func getJumpOffset(ctx *Context, parameter []byte) int {
-	offset, _, err := v.calcJumpOffset(ctx, parameter)
+	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: