request.headers has no limit

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact

An attacker can perform a DoS attack on a worker.

Proof of Concept

chain-extension/lib/async_http_reques titerates request.headers and adds the requested headers to another map:

async fn async_http_request(
    request: HttpRequest,
    timeout_ms: u64,
) -> Result<HttpResponse, HttpRequestError> {
    .....
    let mut headers = HeaderMap::new();
    for (key, value) in &request.headers {
        let key =
            HeaderName::from_str(key.as_str()).or(Err(HttpRequestError::InvalidHeaderName))?;
        let value = HeaderValue::from_str(value).or(Err(HttpRequestError::InvalidHeaderValue))?;
        headers.insert(key, value);
    }
    .....

The request is passed in by the user at the time of invocation and has no size limit.

Therefore, if a malicious user sends a large request.headers and sends a large number of requests at the same time, there may be a memory overflow problem, and the for loop will occupy cpu time, resulting in worker failure, network delay and other problems.

Another factor that can lead to a DoS attack is, http request code is not executed in the VM, so the DoS cannot be prevented by paying gas. The code for the http request is compiled into a native. so file.

Tools Used

vscode, manual

Recommended Mitigation Steps

Limit the size of request.headers

Assessed type

DoS

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[141345]

huge header size result in memory overflow

[kvinwang]

The guest contract can send a maximum of 2MB of headers to the runtime due to the VM memory limit. Therefore, memory should not be a problem. However, it may construct 2MB of headers with empty values to consume CPU resources. There is a scheduler in the worker that, if a query to a contract takes too much time, will decrease the priority of subsequent queries to the same contract.

Anyway, a header size check is good to have in the runtime. Might be QA level.

[c4-sponsor]

kvinwang (sponsor) confirmed

[c4-sponsor]

kvinwang marked the issue as disagree with severity

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[OpenCoreCH]

Agree with the sponsor's take, limit is a good idea to have, but it is not clear (and I doubt it because of the memory limit) that it could actually be used to cause a DoS.

[c4-judge]

OpenCoreCH marked the issue as grade-b