c4-bot-3 commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-03-phala-network/blob/a01ffbe992560d8d0f17deadfb9b9a2bed38377e/phala-blockchain/crates/pink/chain-extension/src/lib.rs#L100 https://github.com/code-423n4/2024-03-phala-network/blob/a01ffbe992560d8d0f17deadfb9b9a2bed38377e/phala-blockchain/crates/pink/chain-extension/src/lib.rs#L184

Vulnerability details

Impact

An attacker can perform a DoS attack on a worker.

Proof of Concept

Pink Extension provides two functions to make http requests: http_request batch_http_request

The default timeout of http_request is 10 * 1000 ms.

impl<T: PinkRuntimeEnv, E: From<&'static str>> PinkExtBackend for DefaultPinkExtension<'_, T, E> {
    .....
    fn http_request(&self, request: HttpRequest) -> Result<HttpResponse, Self::Error> {
        http_request(request, 10 * 1000).map_err(|err| err.display().into())
    }
    .....
}

The problem is that batch_http_request can pass in a timeout of any value.

batch_http_request sets the maximum number of http requests to 5, but there is no limit to the timeout period:

pub fn batch_http_request(requests: Vec<HttpRequest>, timeout_ms: u64) -> ext::BatchHttpResult {
    const MAX_CONCURRENT_REQUESTS: usize = 5;
    if requests.len() > MAX_CONCURRENT_REQUESTS {
        return Err(ext::HttpRequestError::TooManyRequests);
    }
    .....
}

This will lead to DoS attacks. The attacker can launch a large number of requests and specify an infinite timeout period. In this way, a large number of http requests in processing will appear in the worker, which may lead to problems such as memory overflow.

An attacker can set up a malicious web server to make the timeout period as long as possible, such as continuously writing data to the client to maintain the connection.

In addition, the Extension is ultimately run in the worker as a .so file, so the http request code is not executed in the VM, so the DoS cannot be prevented by paying gas.

Tools Used

vscode, manual

Recommended Mitigation Steps

pub fn batch_http_request(requests: Vec<HttpRequest>, timeout_ms: u64) -> ext::BatchHttpResult {
    const MAX_CONCURRENT_REQUESTS: usize = 5;
    if requests.len() > MAX_CONCURRENT_REQUESTS {
        return Err(ext::HttpRequestError::TooManyRequests);
    }
+    if timeout_ms > MAX_TIMEOUT_MS {
+        return Err(ext::HttpRequestError::TooLargeTimeout);
+    }
}

Assessed type

DoS

c4-pre-sort commented 3 months ago

141345 marked the issue as duplicate of #43

c4-judge commented 3 months ago

OpenCoreCH changed the severity to 2 (Med Risk)