filecoin-project / ref-fvm

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

Wasm Validation Rules #1038

Open Stebalien opened 1 year ago

Stebalien commented 1 year ago
Stebalien commented 1 year ago

cc @Jacarte

Jacarte commented 1 year ago

Adding another rule:

Stebalien commented 1 year ago

Allocates at most X MiB of memory up-front (i.e., doesn't statically declare more than some amount of minimum memory).

Stebalien commented 1 year ago

Currently, code can run before invoke, but it'll run out of gas immediately and trap (leading to a fatal error).

We need to either:

  1. Properly account for this kind of code.
  2. Forbid it.

Ideally, we'd just forbid it. It looks like we can run wasm-ctor-eval (from binaryen)?

Specifically, we need to handle:

  1. The start function.
  2. Table initialization functions. See the exprs that appear in https://webassembly.github.io/spec/core/syntax/modules.html#syntax-elem.

Migrated from https://github.com/filecoin-project/ref-fvm/issues/607.

Jacarte commented 1 year ago

Another rule to add:

Apparently, cranelift is packaging the initial declared memory in the module. A simple Wasm binary of 600bytes can go to several Mb in the compiled obj.

Stebalien commented 1 year ago

I wonder if it's related to https://docs.wasmtime.dev/api/wasmtime/struct.Config.html#method.memory_init_cow?

Stebalien commented 1 year ago

Yeah, it looks like https://docs.wasmtime.dev/api/wasmtime/struct.Config.html#method.memory_guaranteed_dense_image_size. Which sets a maximum of 16MiB, which isn't terrible but something we may want to lower.

Stebalien commented 1 year ago

@aakoshh any reason to leave memory_init_cow enabled? IMO, we should consider disabling it because we'll need to charge for it anyways.

Jacarte commented 1 year ago

@aakoshh any reason to leave memory_init_cow enabled? IMO, we should consider disabling it because we'll need to charge for it anyways.

Can it be related to this https://github.com/advisories/GHSA-wh6w-3828-g9qf ?

Jacarte commented 1 year ago

BTW, https://github.com/advisories/GHSA-44mr-8vmm-wjhg and https://github.com/advisories/GHSA-wh6w-3828-g9qf are two security CVEs related to wasmtime version 1.0.2 used in the ref-fvm. We should start thinking on migrating.

aakoshh commented 1 year ago

Maybe I'm blind, but it looks to me like they zeroed out any non-empty image, if the next image was None: https://github.com/bytecodealliance/wasmtime/blob/v1.0.2/crates/runtime/src/cow.rs#L387

I don't see how the scenarios described in https://github.com/advisories/GHSA-wh6w-3828-g9qf can happen :thinking:

Jacarte commented 1 year ago

Maybe I'm blind, but it looks to me like they zeroed out any non-empty image, if the next image was None: https://github.com/bytecodealliance/wasmtime/blob/v1.0.2/crates/runtime/src/cow.rs#L387

I don't see how the scenarios described in GHSA-wh6w-3828-g9qf can happen 🤔

But the limits of the zeroed memory changes from one to other instantiation, right ? https://github.com/bytecodealliance/wasmtime/blob/15991947f485a60298e45d1d915125c0f3cc5a8a/crates/runtime/src/cow.rs#L390

I only see the issue if we reuse the same engine between actor initializations/calls.

(Disclaimer here, I am also struggling to see how this scenario can happen here :) )

Stebalien commented 1 year ago

We also need to account for size in the wasmtime pooling instance config. Specifically, We'll need to make sure we can fit the maximum number of imports/exports/functions/types/globals/etc. in the maximum instance size.

Jacarte commented 1 year ago

Currently, code can run before invoke, but it'll run out of gas immediately and trap (leading to a fatal error).

We need to either:

1. Properly account for this kind of code.

2. Forbid it.

Ideally, we'd just forbid it. It looks like we can run wasm-ctor-eval (from binaryen)?

Specifically, we need to handle:

1. The start function.

2. Table initialization functions. See the `expr`s that appear in https://webassembly.github.io/spec/core/syntax/modules.html#syntax-elem.

Migrated from #607.

It seems we can use wizer here, https://github.com/bytecodealliance/wizer

Jacarte commented 1 year ago

Addresssing MVP in https://github.com/filecoin-project/Fuzzing-FVM/pull/776