filecoin-project / ref-fvm

Reference implementation of the Filecoin Virtual Machine
https://fvm.filecoin.io/
Other
384 stars 139 forks source link

Strict DAG-CBOR encoding/decoding #503

Closed vmx closed 1 year ago

vmx commented 2 years ago

DAG-CBOR only supports a subset of CBOR. Especially for the FVM we should be strict about (de)serializing only valid DAG-CBOR. DAG-CBOR supports:

The development will happen within the DAG-CBOR library, but I opened the issue here for tracking purpose.

Stebalien commented 2 years ago

Requirements & Risks:

  1. Messages must be deterministically accepted/rejected/executed by all implementations.
    • Canonical encoding of messages is enforced by the client (e.g., Lotus) by re-encoding.
    • Message parameters are stored as raw bytes and are only read by actors.
    • Given that actors are now compiled to WASM, we can be sure that all implementations will handle message parameters in the same way.
  2. Builtin-actor state must be reliably readable by all implementations (e.g., for mining, post dispute, power, etc.).
    • All builtin-actor state is re-encoded (minimally) by said built-in actors.
  3. No implementation may reject valid state in reachability analysis, flushing, etc.
    • This is only an issue in M2.
  4. No implementation may accept invalid state in reachability analysis, flushing, etc.
    • This is only an issue in M2.

Given this, the main issue we have is with deterministically accepting/rejecting CBOR-encoded objects. Deterministic encoding isn't an issue for us.

Current Rules:

Future Work:

Currently:

  1. We have no reachability analysis.
  2. We parse actor state on flush (to traverse the tree).

Instead, we should:

  1. On put, validate the block and extract and store the links using a CBOR codec compiled to WASM.
  2. Never parse the block again, relying on the stored links for garbage collection, flushing, reachability, etc.

This would move all consensus-critical IPLD encoding/decoding (with respect to the FVM, at least) into WASM, significantly reducing any risks.

Stebalien commented 2 years ago

Visiting for M2.1. Unfortunately, we do need to land at least some part of this because EVM actors will be able to handle arbitrary FVM messages, and send messages to native actors.

NOTE: If we restrict those "native" messages to using precompiles and don't add the "native send opcode" to M2.1, we can significantly reduce the risk. Otherwise, the native send opcode will effectively be writing an arbitrary cbor-encoded block and we'll need to validate it.

vmx commented 2 years ago

The encoding rules for https://github.com/ipld/serde_ipld_dagcbor are already pretty strict, e.g. I think infinite length lists error already. The one that tackles this issue, it would be great if you could add a section for serde_ipld_dagcbor to https://github.com/ipld/ipld/blob/2f010292da7e3d089f2512b175494abe114f1a2a/specs/codecs/dag-cbor/spec.md#implementations.

Stebalien commented 2 years ago

I'm punting this to m2.2, limiting to #1001 for now.

Stebalien commented 1 year ago

We've resolved this by not being strict and instead clearly defining what Filecoin cares about and what it doesn't. This reduces the risk of different implementations defining conflicting rules. If you're interested: