Metering does not work in presence of trapping instructions

[georgemitenkov]

Describe the bug

Metering instrumentation does not work in presence of trapping instructions. Right now the algorithm would place instrumentation at each branch source / target, e.g. loop, call, br, etc. - essentially at the end of a basic block. If the basic block contains an instruction that traps, the execution is not metered. This allows to create malicious programs with a single computationally-expensive basic block with an instruction that traps in the end, which is not metered.

Steps to reproduce

Example of a program which would not be metered if $x == 0.

func $bomb (param $x i32) (result i32)
    i32.const 10
    local.get $x
    i32.div_u)

To reproduce, you can add this code to tests in metering instrumentation folder.

fn bug() {
        fn test_cost_function(operator: &Operator) -> u64 {
            match operator {
                Operator::LocalGet { .. } | Operator::I32Const { .. } => 1,
                Operator::I32DivU { .. } => 2,
                _ => 0,
            }
        }

        let metering = Arc::new(Metering::new(10, test_cost_function));
        let mut compiler_config = Cranelift::default();
        compiler_config.push_middleware(metering);
        let mut store = Store::new(EngineBuilder::new(compiler_config));

        let bytecode: Vec<u8> = wat2wasm(
            br#"
            (module
              (type $bomb_t (func (param i32) (result i32)))
              (func $bomb (type $bomb_t) (param $x i32) (result i32)
                i32.const 10
                local.get $x
                i32.div_u)
              (export "bomb" (func $bomb)))
            "#,
        ).unwrap().to_vec();
        let module = Module::new(&store, bytecode).unwrap();

        // Before we execute: we have 10 points.
        let instance = Instance::new(&mut store, &module, &imports! {}).unwrap();
        assert_eq!(
            get_remaining_points(&mut store, &instance),
            MeteringPoints::Remaining(10)
        );

        let bomb: TypedFunction<i32, i32> = instance
            .exports
            .get_function("bomb")
            .unwrap()
            .typed(&store)
            .unwrap();

        // Divide 10 / 2 = 5. Total cost is 1 + 1 + 2 = 4.
        // The remaining balance is 10 - 4 = 6.
        let r = bomb.call(&mut store, 2);
        assert_eq!(r.unwrap(), 5);
        assert_eq!(
            get_remaining_points(&mut store, &instance),
            MeteringPoints::Remaining(6)
        );

        // Divide 10 / 0 traps. Total cost should still be 1 + 1 + 2 = 4.
        // The remaining balance still has to be 6 - 4 = 2.
        let r = bomb.call(&mut store, 0);
        assert!(r.is_err());
        assert_eq!(
            get_remaining_points(&mut store, &instance),
            MeteringPoints::Remaining(2)
        );
}

Expected behavior

Process finished with exit code 0

Actual behavior

Left:  Remaining(6)
Right: Remaining(2)
<Click to see difference>

thread 'metering::tests::my_get_remaining_points_works' panicked at 'assertion failed: `(left == right)`
  left: `Remaining(6)`,
 right: `Remaining(2)`', lib/middlewares/src/metering.rs:504:9
stack backtrace:
...

The instrumentation with an accumulated cost of metering (4) is added after division instruction. Because division traps, it is never reached.

Additional context

To fix the issue, the metering has to be done before every trapping instruction, or a different algorithm should be used.

[Michael-F-Bryan]

Thanks for reporting this @georgemitenkov!

I'm guessing there is some sort of conditional testing in the trap handler. For example when we trigger a trap, maybe we don't check metering is active and therefore we don't count those instructions. This is particularly relevant to Wasmer Edge and blockchains because it is a potential DOS vector.

[rachel-bousfield]

Wanted to comment that we solved this in our fork here by charging for compute before executing the basic block.

These changes may be valuable to upstream since they make middlewares more powerful generally :)