Validate CBOR

This change validates CBOR when "sending" it to other actor methods.
Technically, actor message params are arbitrary bytes. Ideally, we'd just pass
the raw bytes in.

However, for convenience, the runtime accepts any object implementing
`CBORMarshaler`. Unfortunately, passing raw bytes as a `CBORMarshaler` without
_checking_ them could lead to bugs down the road if we decide to treat this
`CBORMarshaler` object as a valid CBOR object, so we validate them.

fixes #972

actors/builtin/init/init_actor.go

@@ -57,6 +57,13 @@ func (a Actor) Exec(rt runtime.Runtime, params *ExecParams) *ExecReturn {
 		rt.Abortf(exitcode.ErrForbidden, "caller type %v cannot exec actor type %v", callerCodeCID, params.CodeCID)
 	}
 
+	// Technically, actors take _bytes_ as their method params. However,
+	// given that we pass them around as a cbor-able object, we need to
+	// ensure it correctly encodes to CBOR.
+	// See: https://github.com/filecoin-project/specs-actors/issues/972
+	ctorParams, err := runtime.CheckCBOR(params.ConstructorParams)
+	builtin.RequireNoErr(rt, err, exitcode.ErrIllegalArgument, "params must be valid CBOR")
+
 	// Compute a re-org-stable address.
 	// This address exists for use by messages coming from outside the system, in order to
 	// stably address the newly created actor even if a chain re-org causes it to end up with
@@ -77,7 +84,7 @@ func (a Actor) Exec(rt runtime.Runtime, params *ExecParams) *ExecReturn {
 	rt.CreateActor(params.CodeCID, idAddr)
 
 	// Invoke constructor.
-	_, code := rt.Send(idAddr, builtin.MethodConstructor, runtime.CBORBytes(params.ConstructorParams), rt.Message().ValueReceived())
+	_, code := rt.Send(idAddr, builtin.MethodConstructor, ctorParams, rt.Message().ValueReceived())
 	builtin.RequireSuccess(rt, code, "constructor failed")
 
 	return &ExecReturn{idAddr, uniqueAddress}

actors/builtin/init/init_test.go

@@ -6,6 +6,7 @@ import (
 
 	cid "github.com/ipfs/go-cid"
 	assert "github.com/stretchr/testify/assert"
+	cbg "github.com/whyrusleeping/cbor-gen"
 
 	addr "github.com/filecoin-project/go-address"
 	abi "github.com/filecoin-project/specs-actors/actors/abi"
@@ -55,7 +56,7 @@ func TestExec(t *testing.T) {
 		})
 	})
 
-	var fakeParams = runtime.CBORBytes([]byte{'D', 'E', 'A', 'D', 'B', 'E', 'E', 'F'})
+	var fakeParams = runtime.CBORBytes(cbg.CborNull)
 	var balance = abi.NewTokenAmount(100)
 
 	t.Run("happy path exec create 2 payment channels", func(t *testing.T) {

actors/builtin/multisig/multisig_actor.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 
 	addr "github.com/filecoin-project/go-address"
+
 	abi "github.com/filecoin-project/specs-actors/actors/abi"
 	builtin "github.com/filecoin-project/specs-actors/actors/builtin"
 	vmr "github.com/filecoin-project/specs-actors/actors/runtime"
@@ -471,19 +472,26 @@ func executeTransactionIfApproved(rt vmr.Runtime, st State, txnID TxnID, txn *Tr
 			rt.Abortf(exitcode.ErrInsufficientFunds, "insufficient funds unlocked: %v", err)
 		}
 
+		// Technically, actors take _bytes_ as their method params. However,
+		// given that we pass them around as a cbor-able object, we need to
+		// ensure it correctly encodes to CBOR.
+		// See: https://github.com/filecoin-project/specs-actors/issues/972
+		params, err := vmr.CheckCBOR(txn.Params)
+		builtin.RequireNoErr(rt, err, exitcode.ErrIllegalArgument, "invalid transaction parameters")
+
 		var ret vmr.SendReturn
 		// A sufficient number of approvals have arrived and sufficient funds have been unlocked: relay the message and delete from pending queue.
 		ret, code = rt.Send(
 			txn.To,
 			txn.Method,
-			vmr.CBORBytes(txn.Params),
+			params,
 			txn.Value,
 		)
 		applied = true
 
 		// Pass the return value through uninterpreted with the expectation that serializing into a CBORBytes never fails
 		// since it just copies the bytes.
-		err := ret.Into(&out)
+		err = ret.Into(&out)
 		builtin.RequireNoErr(rt, err, exitcode.ErrSerialization, "failed to deserialize result")
 
 		// This could be rearranged to happen inside the first state transaction, before the send().

actors/builtin/multisig/multisig_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/minio/blake2b-simd"
 	assert "github.com/stretchr/testify/assert"
 	require "github.com/stretchr/testify/require"
+	cbg "github.com/whyrusleeping/cbor-gen"
 
 	abi "github.com/filecoin-project/specs-actors/actors/abi"
 	big "github.com/filecoin-project/specs-actors/actors/abi/big"
@@ -197,7 +198,7 @@ func TestVesting(t *testing.T) {
 
 	const unlockDuration = 10
 	var multisigInitialBalance = abi.NewTokenAmount(100)
-	var fakeParams = runtime.CBORBytes([]byte{1, 2, 3, 4})
+	var fakeParams = runtime.CBORBytes(cbg.CborNull)
 
 	builder := mock.NewBuilder(context.Background(), receiver).
 		WithCaller(builtin.InitActorAddr, builtin.InitActorCodeID).
@@ -326,7 +327,7 @@ func TestPropose(t *testing.T) {
 
 	const noUnlockDuration = int64(0)
 	var sendValue = abi.NewTokenAmount(10)
-	var fakeParams = runtime.CBORBytes([]byte{1, 2, 3, 4})
+	var fakeParams = runtime.CBORBytes(cbg.CborNull)
 	var signers = []addr.Address{anne, bob}
 
 	builder := mock.NewBuilder(context.Background(), receiver).WithCaller(builtin.InitActorAddr, builtin.InitActorCodeID)
@@ -445,7 +446,7 @@ func TestApprove(t *testing.T) {
 	const txnID = int64(0)
 	const fakeMethod = abi.MethodNum(42)
 	var sendValue = abi.NewTokenAmount(10)
-	var fakeParams = runtime.CBORBytes([]byte{1, 2, 3, 4})
+	var fakeParams = runtime.CBORBytes(cbg.CborNull)
 	var signers = []addr.Address{anne, bob}
 
 	builder := mock.NewBuilder(context.Background(), receiver).
@@ -815,7 +816,7 @@ func TestCancel(t *testing.T) {
 	const numApprovals = uint64(2)
 	const txnID = int64(0)
 	const fakeMethod = abi.MethodNum(42)
-	var fakeParams = []byte{1, 2, 3, 4, 5}
+	var fakeParams = runtime.CBORBytes(cbg.CborNull)
 	var sendValue = abi.NewTokenAmount(10)
 	var signers = []addr.Address{anne, bob}
 

actors/builtin/power/power_actor.go

@@ -401,10 +401,19 @@ func (a Actor) processDeferredCronEvents(rt Runtime) {
 	})
 	failedMinerCrons := make([]addr.Address, 0)
 	for _, event := range cronEvents {
+		// Technically, actors take _bytes_ as their method params. However,
+		// given that we pass them around as a cbor-able object, we need to
+		// ensure it correctly encodes to CBOR.
+		// See: https://github.com/filecoin-project/specs-actors/issues/972
+		params, err := vmr.CheckCBOR(event.CallbackPayload)
+		if err != nil {
+			rt.Log(vmr.WARN, "OnDeferredCronEvent failed for due to invalid callback payload: %s", err)
+			continue
+		}
 		_, code := rt.Send(
 			event.MinerAddr,
 			builtin.MethodsMiner.OnDeferredCronEvent,
-			vmr.CBORBytes(event.CallbackPayload),
+			params,
 			abi.NewTokenAmount(0),
 		)
 		// If a callback fails, this actor continues to invoke other callbacks

actors/builtin/power/power_test.go

@@ -11,6 +11,7 @@ import (
 	cid "github.com/ipfs/go-cid"
 	assert "github.com/stretchr/testify/assert"
 	require "github.com/stretchr/testify/require"
+	cbg "github.com/whyrusleeping/cbor-gen"
 
 	abi "github.com/filecoin-project/specs-actors/actors/abi"
 	big "github.com/filecoin-project/specs-actors/actors/abi/big"
@@ -474,14 +475,14 @@ func TestCron(t *testing.T) {
 		//  4 - block - has event
 
 		rt.SetEpoch(1)
-		actor.enrollCronEvent(rt, miner1, 2, []byte{0x1, 0x3})
-		actor.enrollCronEvent(rt, miner2, 4, []byte{0x2, 0x3})
+		actor.enrollCronEvent(rt, miner1, 2, cbg.CborBoolFalse)
+		actor.enrollCronEvent(rt, miner2, 4, cbg.CborBoolTrue)
 
 		expectedRawBytePower := big.NewInt(0)
 		rt.SetEpoch(4)
 		rt.ExpectValidateCallerAddr(builtin.CronActorAddr)
-		rt.ExpectSend(miner1, builtin.MethodsMiner.OnDeferredCronEvent, vmr.CBORBytes([]byte{0x1, 0x3}), big.Zero(), nil, exitcode.Ok)
-		rt.ExpectSend(miner2, builtin.MethodsMiner.OnDeferredCronEvent, vmr.CBORBytes([]byte{0x2, 0x3}), big.Zero(), nil, exitcode.Ok)
+		rt.ExpectSend(miner1, builtin.MethodsMiner.OnDeferredCronEvent, vmr.CBORBytes(cbg.CborBoolFalse), big.Zero(), nil, exitcode.Ok)
+		rt.ExpectSend(miner2, builtin.MethodsMiner.OnDeferredCronEvent, vmr.CBORBytes(cbg.CborBoolTrue), big.Zero(), nil, exitcode.Ok)
 		rt.ExpectSend(builtin.RewardActorAddr, builtin.MethodsReward.UpdateNetworkKPI, &expectedRawBytePower, big.Zero(), nil, exitcode.Ok)
 		rt.SetCaller(builtin.CronActorAddr, builtin.CronActorCodeID)
 		rt.ExpectBatchVerifySeals(nil, nil, nil)
@@ -507,12 +508,12 @@ func TestCron(t *testing.T) {
 		rt.Verify()
 
 		// enroll a cron task at epoch 2 (which is in the past)
-		actor.enrollCronEvent(rt, miner1, 2, []byte{0x1, 0x3})
+		actor.enrollCronEvent(rt, miner1, 2, cbg.CborBoolTrue)
 
 		// run cron again in the future
 		rt.SetEpoch(6)
 		rt.ExpectValidateCallerAddr(builtin.CronActorAddr)
-		rt.ExpectSend(miner1, builtin.MethodsMiner.OnDeferredCronEvent, vmr.CBORBytes([]byte{0x1, 0x3}), big.Zero(), nil, exitcode.Ok)
+		rt.ExpectSend(miner1, builtin.MethodsMiner.OnDeferredCronEvent, vmr.CBORBytes(cbg.CborBoolTrue), big.Zero(), nil, exitcode.Ok)
 		rt.ExpectSend(builtin.RewardActorAddr, builtin.MethodsReward.UpdateNetworkKPI, &expectedRawBytePower, big.Zero(), nil, exitcode.Ok)
 		rt.SetCaller(builtin.CronActorAddr, builtin.CronActorCodeID)
 		rt.ExpectBatchVerifySeals(nil, nil, nil)
@@ -549,8 +550,8 @@ func TestCron(t *testing.T) {
 		actor.constructAndVerify(rt)
 
 		rt.SetEpoch(1)
-		actor.enrollCronEvent(rt, miner1, 2, []byte{})
-		actor.enrollCronEvent(rt, miner2, 2, []byte{})
+		actor.enrollCronEvent(rt, miner1, 2, cbg.CborNull)
+		actor.enrollCronEvent(rt, miner2, 2, cbg.CborNull)
 
 		actor.createMinerBasic(rt, owner, owner, miner1)
 		actor.createMinerBasic(rt, owner, owner, miner2)
@@ -564,11 +565,11 @@ func TestCron(t *testing.T) {
 		rt.SetEpoch(2)
 		rt.ExpectValidateCallerAddr(builtin.CronActorAddr)
 		// First send fails
-		rt.ExpectSend(miner1, builtin.MethodsMiner.OnDeferredCronEvent, vmr.CBORBytes(nil), big.Zero(), nil, exitcode.ErrIllegalState)
+		rt.ExpectSend(miner1, builtin.MethodsMiner.OnDeferredCronEvent, vmr.CBORBytes(cbg.CborNull), big.Zero(), nil, exitcode.ErrIllegalState)
 		rt.ExpectBatchVerifySeals(nil, nil, nil)
 
 		// Subsequent one still invoked
-		rt.ExpectSend(miner2, builtin.MethodsMiner.OnDeferredCronEvent, vmr.CBORBytes(nil), big.Zero(), nil, exitcode.Ok)
+		rt.ExpectSend(miner2, builtin.MethodsMiner.OnDeferredCronEvent, vmr.CBORBytes(cbg.CborNull), big.Zero(), nil, exitcode.Ok)
 		// Reward actor still invoked
 		rt.ExpectSend(builtin.RewardActorAddr, builtin.MethodsReward.UpdateNetworkKPI, &expectedPower, big.Zero(), nil, exitcode.Ok)
 		rt.SetCaller(builtin.CronActorAddr, builtin.CronActorCodeID)

actors/runtime/runtime.go

@@ -8,6 +8,7 @@ import (
 	"github.com/filecoin-project/go-address"
 	addr "github.com/filecoin-project/go-address"
 	cid "github.com/ipfs/go-cid"
+	cbg "github.com/whyrusleeping/cbor-gen"
 
 	abi "github.com/filecoin-project/specs-actors/actors/abi"
 	crypto "github.com/filecoin-project/specs-actors/actors/crypto"
@@ -286,6 +287,13 @@ type CBORer interface {
 	CBORUnmarshaler
 }
 
+func CheckCBOR(inp []byte) (CBORBytes, error) {
+	if err := cbg.ValidateCBOR(inp); err != nil {
+		return nil, err
+	}
+	return inp, nil
+}
+
 // Wraps already-serialized bytes as CBOR-marshalable.
 type CBORBytes []byte
 

go.mod

@@ -20,7 +20,7 @@ require (
 	github.com/smartystreets/goconvey v0.0.0-20190731233626-505e41936337 // indirect
 	github.com/stretchr/testify v1.6.1
 	github.com/warpfork/go-wish v0.0.0-20190328234359-8b3e70f8e830 // indirect
-	github.com/whyrusleeping/cbor-gen v0.0.0-20200812213548-958ddffe352c
+	github.com/whyrusleeping/cbor-gen v0.0.0-20200814232421-c568d328ad9d
 	github.com/xorcare/golden v0.6.0
 	golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1
 )

go.sum

@@ -126,10 +126,11 @@ github.com/whyrusleeping/cbor-gen v0.0.0-20200123233031-1cdf64d27158/go.mod h1:X
 github.com/whyrusleeping/cbor-gen v0.0.0-20200414195334-429a0b5e922e/go.mod h1:Xj/M2wWU+QdTdRbu/L/1dIZY8/Wb2K9pAhtroQuxJJI=
 github.com/whyrusleeping/cbor-gen v0.0.0-20200504204219-64967432584d/go.mod h1:W5MvapuoHRP8rz4vxjwCK1pDqF1aQcWsV5PZ+AHbqdg=
 github.com/whyrusleeping/cbor-gen v0.0.0-20200715143311-227fab5a2377/go.mod h1:fgkXqYy7bV2cFeIEOkVTZS/WjXARfBqSH6Q2qHL33hQ=
-github.com/whyrusleeping/cbor-gen v0.0.0-20200810223238-211df3b9e24c h1:BMg3YUwLEUIYBJoYZVhA4ZDTciXRj6r7ffOCshWrsoE=
 github.com/whyrusleeping/cbor-gen v0.0.0-20200810223238-211df3b9e24c/go.mod h1:fgkXqYy7bV2cFeIEOkVTZS/WjXARfBqSH6Q2qHL33hQ=
-github.com/whyrusleeping/cbor-gen v0.0.0-20200812213548-958ddffe352c h1:otRnI08JoahNBxUFqX3372Ab9GnTj8L5J9iP5ImyxGU=
-github.com/whyrusleeping/cbor-gen v0.0.0-20200812213548-958ddffe352c/go.mod h1:fgkXqYy7bV2cFeIEOkVTZS/WjXARfBqSH6Q2qHL33hQ=
+github.com/whyrusleeping/cbor-gen v0.0.0-20200814005402-534767cc4b7a h1:BCPdO+6J0hNjvyTxGaNrPKbaLXKOqiZoKifnt+vkS58=
+github.com/whyrusleeping/cbor-gen v0.0.0-20200814005402-534767cc4b7a/go.mod h1:fgkXqYy7bV2cFeIEOkVTZS/WjXARfBqSH6Q2qHL33hQ=
+github.com/whyrusleeping/cbor-gen v0.0.0-20200814232421-c568d328ad9d h1:KK78rRDcb1C+jfxYlkN7KZyqWxS/Lx6zkWYPLoinu+Y=
+github.com/whyrusleeping/cbor-gen v0.0.0-20200814232421-c568d328ad9d/go.mod h1:fgkXqYy7bV2cFeIEOkVTZS/WjXARfBqSH6Q2qHL33hQ=
 github.com/xorcare/golden v0.6.0 h1:E8emU8bhyMIEpYmgekkTUaw4vtcrRE+Wa0c5wYIcgXc=
 github.com/xorcare/golden v0.6.0/go.mod h1:7T39/ZMvaSEZlBPoYfVFmsBLmUl3uz9IuzWj/U6FtvQ=
 go.uber.org/atomic v1.6.0 h1:Ezj3JGmsOnG1MoRWQkPBsKLe9DwWD9QeXzTRzzldNVk=