ecall::dispatch is vulnerable to panic events which can be used to cause panic to the worker

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-03-phala-network/blob/a01ffbe992560d8d0f17deadfb9b9a2bed38377e/phala-blockchain/crates/pink/macro/src/macro_xcall.rs#L190

Vulnerability details

Impact

pink_runtime::capi::ecall() serves as the central hub for all ecall functions. Upon invocation, it forwards function calls by invoking the ecall::dispatch.

ecall::dispatch function is generated by macro_xcall

    parse_quote! {
        pub fn dispatch(
            executor: &mut (impl Executing + ?Sized),
            srv: &mut (impl #trait_name + ?Sized),
            id: u32,
            input: &[u8],
        ) -> Vec<u8> {
            match id {
                #(#calls)*
                _ => panic!("Unknown call id {}", id),
            }
        }
    }
}

macro_xcall.rs#L199

Here is where the execution methods are generated

        calls.push(parse_quote! {
            #id => {
                let (#(#args),*) = Decode::decode(&mut &input[..]).expect("Failed to decode args");
                executor.#exec_fn(move || srv.#name(#(#args),*)).encode()
            }
        });

macro_xcall.rs#L190

As you can see, the args are decoded before passing it to the executor. There are two problems here:

• There is no limit enforced for the depth of decoding. If the limit is hit, it will stack overflow which cause a panic to the worker. That's a common issue when using parity_scale_codec crate decode feature. Because of this, decode_with_depth_limit function was introduced. Check: https://docs.rs/parity-scale-codec/latest/parity_scale_codec/trait.DecodeLimit.html
• There is expect, if an error occured, it will panic with the message "Failed to decode args". So, even if decode_with_depth_limit was used, panic will occur, just caused by a different reason.

The following code won't panic. Instead, it will return an error.

fn test_limit_decode() {
    use scale::{Decode,DecodeLimit, Encode};

    type NestedVec = Vec<Vec<Vec<Vec<u8>>>>;
    let nested: NestedVec = vec![vec![vec![vec![1]]]];
    let encoded = nested.encode();

    let decoded = NestedVec::decode_with_depth_limit(3, &mut encoded.as_slice()).unwrap();
    assert_eq!(decoded, nested);
    assert!(NestedVec::decode_with_depth_limit(2, &mut encoded.as_slice()).is_err());
}

Proof of Concept

Included above within the impact for simplicity.

Tools Used

Manual analysis

Recommended Mitigation Steps

• Set a reasonable depth limit for decoding objects to avoid stack overflow issue
• Handle the error differently. For example, return the error as an output instead of panicking.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as insufficient quality report

[141345]

Out of scope

[OpenCoreCH]

Was OOS for this audit, tagging @kvinwang in case you want to take a look anyways

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Out of scope

[kvinwang]

The macro does not only generate the fn dispatch but also generate the caller side implementation which would ensure the panic never happen. As for the depth_limit, the input data structure is defined in pink runtime itself. There isn't such data structure that would be too deep.