builtin-actors icon indicating copy to clipboard operation
builtin-actors copied to clipboard

Validate proof types on decode

Open Stebalien opened this issue 2 years ago • 4 comments

Currently, we validate proof types manually inside our actors to match go's behavior. It would be safer to simply reject invalid proof types when we attempt to decode state/messages.

Doing this for NV16 (M1) is pretty easy: we'd just remove the Invalid cases from our enums. E.g.:

https://github.com/filecoin-project/ref-fvm/blob/0692d5bbb4d7ca588278a3544a48504a7f5ed0ee/shared/src/sector/registered_proof.rs#L30

Doing this after Nv16 is possible, but slightly tricker as these types tend to be "shared" between versions. Although it shouldn't be too bad as we won't re-compile nv16 actors in nv17.

Stebalien avatar Mar 09 '22 18:03 Stebalien

@Stebalien mind pasting a link to the code that validates manually, to illustrate the case here? 🙏

raulk avatar Mar 09 '22 18:03 raulk

Hm. That's actually interesting. Usually, when we "check" the proof types, we check if the proof type is within some set of "allowed" proof types, not if the proof type is "valid".

For example: https://github.com/filecoin-project/builtin-actors/blob/1a4287f1452e0a1a226ccfcaec105b370af9cc3d/actors/miner/src/lib.rs#L4069-L4079

The invalid checks are things like:

  • https://github.com/filecoin-project/builtin-actors/blob/1a4287f1452e0a1a226ccfcaec105b370af9cc3d/actors/runtime/src/builtin/sector.rs#L29
  • https://github.com/filecoin-project/ref-fvm/blob/0692d5bbb4d7ca588278a3544a48504a7f5ed0ee/shared/src/sector/registered_proof.rs#L85
  • https://github.com/filecoin-project/ref-fvm/blob/0692d5bbb4d7ca588278a3544a48504a7f5ed0ee/shared/src/sector/registered_proof.rs#L158

I guess the primary benefit of checking earlier is taht we'd be able to get rid of all the "results" like https://github.com/filecoin-project/ref-fvm/blob/0692d5bbb4d7ca588278a3544a48504a7f5ed0ee/shared/src/sector/registered_proof.rs#L145, because those methods would be infallible.

Stebalien avatar Mar 09 '22 18:03 Stebalien

@Stebalien is this still relevant?

anorth avatar Aug 11 '22 23:08 anorth

Yes. Basically, it involves getting rid of statements things like https://github.com/filecoin-project/ref-fvm/blob/b30538700be419b42719ecf75350ef6836be770e/shared/src/sector/registered_proof.rs#L30.

That would mean we simply wouldn't decode messages that reference invalid proof types.

Stebalien avatar Aug 11 '22 23:08 Stebalien