Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lack of upper limit of timeout for batch HTTP request could be utilized to DoS the worker #67

Closed
c4-bot-10 opened this issue Mar 22, 2024 · 3 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Warden finding downgraded by judge Judge downgraded the risk level of this issue duplicate-43 🤖_13_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@c4-bot-10
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

Pink Extension provides the possibility to make HTTP requests as a feature through a query. To prevent DoS related attacks, the HTTP request is set with a timeout of 10 seconds. This way, this feature can not be abused to overload the worker.

However, Pink Extension provides the possibility to make batch HTTP requests as well, when doing so, the timeout is an external input (i.e. dynamic value), and there is no upper limit for the timeout here.

pub fn batch_http_request(requests: Vec<HttpRequest>, timeout_ms: u64) -> ext::BatchHttpResult {
.
.
    block_on(async move {
        let futs = requests
            .into_iter()
            .map(|request| async_http_request(request, timeout_ms));	
.
.
.
fn batch_http_request(
	&self,
	requests: Vec<HttpRequest>,
	timeout_ms: u64,
) -> Result<ext::BatchHttpResult, Self::Error> {
	Ok(batch_http_request(requests, timeout_ms))
}

chain-extension/src/lib.rs#L64
chain-extension/src/lib.rs#L194

This can be abused to set too high value for the timeout (u64::MAX=18446744073709551615).

Proof of Concept

The attack path as follows:

  • The maclious actor deploys a Phala contract which makes 5 HTTP request (5 is maximum) as a batch to a server that's created by the actor.

     const MAX_CONCURRENT_REQUESTS: usize = 5;  
     if requests.len() > MAX_CONCURRENT_REQUESTS {
         return Err(ext::HttpRequestError::TooManyRequests);
     }

    chain-extension/src/lib.rs#L57

  • This server receives HTTP requests but it delays responses for some time (e.g 1 or 2 hours).

  • The maclious actor sends a query to the worker RPC for the Phala contract.

  • The contract gets executed by Pink Runtime via the worker.

  • This execution will last for 1 hour (assuming the timeout set in the contract is 1 hour, can be even more).

As a consequence, the maclious actor can send too many queries to the same worker for this contract, flooding the worker, leading to a DoS eventually or even a crash since the worker could hit the hard limit of the resources set.

Tools Used

Manual analysis

Recommended Mitigation Steps

Add a upper limit for the timeout for the batch HTTP request feature.

Assessed type

DoS

@c4-bot-10 c4-bot-10 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Warden finding labels Mar 22, 2024
c4-bot-10 added a commit that referenced this issue Mar 22, 2024
@c4-bot-12 c4-bot-12 added the 🤖_13_group AI based duplicate group recommendation label Mar 22, 2024
@c4-pre-sort
Copy link

141345 marked the issue as duplicate of #43

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Mar 28, 2024
@c4-judge
Copy link

OpenCoreCH changed the severity to 2 (Med Risk)

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Mar 28, 2024
@c4-judge
Copy link

OpenCoreCH marked the issue as satisfactory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Warden finding downgraded by judge Judge downgraded the risk level of this issue duplicate-43 🤖_13_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

4 participants