diff --git a/actors/builtin/init/init_actor.go b/actors/builtin/init/init_actor.go index ba7bba6b9..57f79aeed 100644 --- a/actors/builtin/init/init_actor.go +++ b/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} diff --git a/actors/builtin/init/init_test.go b/actors/builtin/init/init_test.go index c79e18fe2..3cc84e08c 100644 --- a/actors/builtin/init/init_test.go +++ b/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) { diff --git a/actors/builtin/multisig/multisig_actor.go b/actors/builtin/multisig/multisig_actor.go index b371d46b9..5965830d3 100644 --- a/actors/builtin/multisig/multisig_actor.go +++ b/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(). diff --git a/actors/builtin/multisig/multisig_test.go b/actors/builtin/multisig/multisig_test.go index 414b72cda..c09ec58f1 100644 --- a/actors/builtin/multisig/multisig_test.go +++ b/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} diff --git a/actors/builtin/power/power_actor.go b/actors/builtin/power/power_actor.go index b483d99de..a3cc5bc69 100644 --- a/actors/builtin/power/power_actor.go +++ b/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 diff --git a/actors/builtin/power/power_test.go b/actors/builtin/power/power_test.go index cd0e47d67..e9088852e 100644 --- a/actors/builtin/power/power_test.go +++ b/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) diff --git a/actors/runtime/runtime.go b/actors/runtime/runtime.go index 421d64844..adb03a70b 100644 --- a/actors/runtime/runtime.go +++ b/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 diff --git a/go.mod b/go.mod index 1392a713e..ab76ec13d 100644 --- a/go.mod +++ b/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 ) diff --git a/go.sum b/go.sum index 05c084278..e6975dab7 100644 --- a/go.sum +++ b/go.sum @@ -126,10 +126,9 @@ 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-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=