Skip to content
This repository has been archived by the owner on Jun 6, 2023. It is now read-only.

CBORBytes does not validate - change Send() API to accept []byte #972

Open
Stebalien opened this issue Aug 13, 2020 · 6 comments
Open

CBORBytes does not validate - change Send() API to accept []byte #972

Stebalien opened this issue Aug 13, 2020 · 6 comments
Labels
change-behaviour Changes behaviour or state interpretation, necessitating a network version upgrade enhancement New feature or request robustness Related to correctness

Comments

@Stebalien
Copy link
Member

Unlike cbg.Deferred, runtime.CBORBytes does not validate the input CBOR. It looks like we may be fine at the moment, but anything like the following would be really dangerous:

type Params struct {
    ...
    Params runtime.CBORBytes
    ...
}
...
p = Params{..., Params: runtime.CBORBytes(userDefinedBytes), ...}

I'd feel more comfortable if we used deferred (always checks on deserialize), but this would be a pretty large refactor.

@Stebalien Stebalien added the enhancement New feature or request label Aug 13, 2020
@Stebalien
Copy link
Member Author

My current solution is to"

  1. Fix Check deferred method "params" when proposed #973.
  2. Use a CheckCBOR([]byte) function (ensures the input is a single cbor object) instead of casting byte arrays to CBORBytes.

Stebalien added a commit that referenced this issue Aug 14, 2020
This change validates CBOR whenever accepted from the user as an arbitrary byte
array.

Note:

1. For the multisig actor, we could _technically_ skip validating the CBOR on
approval because it has already been validated on propose. But this shouldn't be
that expensive.
2. We don't actually need to validate the CBOR in the cron actor. I'm happy to
remove it, I just wanted to start with something paranoid and walk back from there.

fixes #972

depends on whyrusleeping/cbor-gen#39
Stebalien added a commit that referenced this issue Aug 14, 2020
This change validates CBOR whenever accepted from the user as an arbitrary byte
array.

Note:

1. For the multisig actor, we could _technically_ skip validating the CBOR on
approval because it has already been validated on propose. But this shouldn't be
that expensive.
2. We don't actually need to validate the CBOR in the cron actor. I'm happy to
remove it, I just wanted to start with something paranoid and walk back from there.

fixes #972

depends on whyrusleeping/cbor-gen#39
@anorth
Copy link
Member

anorth commented Aug 17, 2020

I do not understand what's dangerous here. Perhaps a concrete example would help. What would go wrong if something isn't valid CBOR, besides a call failing with ErrSerialization?

@anorth
Copy link
Member

anorth commented Aug 17, 2020

See discussion in #973, but I don't think this is a problem or that we should do anything.

@Stebalien
Copy link
Member Author

Concrete example here: #974 (comment)

@Stebalien
Copy link
Member Author

This is effectively a buffer overflow.

@Stebalien
Copy link
Member Author

Resolution: In theory, actor "methods" take bytes and return bytes. However, for convenience, we encode to/from CBOR in the runtime.

This means that, in theory, we shouldn't have to validate CBOR before calling Send. Unfortunately, Send takes a CBORMarshaller and we generally assume that CBORMarshaler produces valid CBOR.

To be specific, the following would be problematic:

type Invocation struct {
    Method abi.MethodNum
    Params CBORMarshaller // Note: we don't do this in practice, we use bytes.
    Amount abi.TokenAmount
}

If Params produced two concatenated CBOR objects, a valid "params" object, and a valid "amount", it would be able to overwrite the amount in the Invocation struct.

So, to be extra-careful, we validate when calling Send.


Future proposal:

  1. Change the Send API to:
func Send(target Address, method MethodNum, params []bytes, amount TokenAmount) (ret []byte, code ExitCode)
  1. Implement a SendCBOR helper:
func SendCBOR(rt Runtime, target Address, method MethodNum, params CBORMarshaler, amount TokenAmount, out CBORUnmarshaler) ExitCode

Stebalien added a commit that referenced this issue Aug 17, 2020
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
Stebalien added a commit that referenced this issue Aug 20, 2020
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
Stebalien added a commit that referenced this issue Aug 20, 2020
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
Stebalien added a commit that referenced this issue Aug 20, 2020
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
@anorth anorth changed the title CBORBytes does not validate CBORBytes does not validate - change Send() API to accept []byte Aug 27, 2020
@ZenGround0 ZenGround0 added change-behaviour Changes behaviour or state interpretation, necessitating a network version upgrade robustness Related to correctness labels Oct 28, 2020
@ZenGround0 ZenGround0 added this to the 🧡 v3 milestone Oct 28, 2020
@anorth anorth removed this from the 🧡 v3 milestone Jan 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
change-behaviour Changes behaviour or state interpretation, necessitating a network version upgrade enhancement New feature or request robustness Related to correctness
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants