Rust: Add basic request forgery query

[paldepind]

This PR adds a basic request forgery query. I say "basic" because I think there's a bit more to do in order for the query to be production quality:

• Add sources for more web framework. This will benefit this query as well as others.
• Add sinks for more HTTP libraries.
• Add barriers. For instance for something like format!("https://safe-url.com/{user_path}"). There is a test for this.

A few notes for reviewers:

• The text in the qhelp is taken from the same query for other languages.
• Since there's a whole bunch of web frameworks for Rust, I've made the example in the qhelp generic. I think using a specific web framework would be jarring for users who are using a different one.
• The sinks for this query are places where a value is used as the URL of a web request. These places are also sinks for the "plaintext transmission" query and we had such sinks labeled as transmission in MaD yaml. I'd like to reuse those for this query, so I've changed the label to request-url and this label is picked up by both queries. I'm wondering if "request URL" is something that we could make a concept? But if so, it seems like out of scope for this PR.
• I got 0 results on MRVA top 100. Looking at the repositories I don't think we can expect to find anything there anyway as it's mostly libraries. Once the above mentioned items are done it would make sense to look at top 1000 and see what shows up there.

[github-actions]

QHelp previews:

rust/ql/src/queries/security/CWE-918/RequestForgery.qhelp

Server-side request forgery

Directly incorporating user input into an HTTP request without validating the input can facilitate server-side request forgery (SSRF) attacks. In these attacks, the server may be tricked into making a request to an unintended API endpoint or resource. If the server performing the request is connected to an internal network, this can give an attacker the means to bypass the network boundary and make requests against internal services. A forged request may perform an unintended action on behalf of the attacker, or cause information leak if redirected to an external server or if the request response is fed back to the user. It may also compromise the server making the request, if the request response is handled in an unsafe way.

Recommendation

To guard against SSRF attacks, you should avoid putting user-provided input directly into a request URL. Instead, maintain a list of authorized URLs on the server; then choose from that list based on the input provided. Alternatively, ensure requests constructed from user input are limited to a particular host or a more restrictive URL prefix.

Example

The following example shows an HTTP request parameter being used directly to form a new request without validating the input, which facilitates SSRF attacks. It also shows how to remedy the problem by validating the user input against a known fixed string.

// BAD: Endpoint handler that makes requests based on user input
async fn vulnerable_endpoint_handler(req: Request) -> Result<Response> {
    // This request is vulnerable to SSRF attacks as the user controls the
    // entire URL
    let response = reqwest::get(&req.user_url).await;
    
    match response {
        Ok(resp) => {
            let body = resp.text().await.unwrap_or_default();
            Ok(Response {
                message: "Success".to_string(),
                data: body,
            })
        }
        Err(_) => Err("Request failed")
    }
}

// GOOD: Validate user input against an allowlist
async fn secure_endpoint_handler(req: Request) -> Result<Response> {
    // Allow list of specific, known-safe URLs
    let allowed_hosts = ["api.example.com", "trusted-service.com"];
    
    if !allowed_hosts.contains(&req.user_url) {
        return Err("Untrusted domain");
    }
    // This request is safe as the user input has been validated
    let response = reqwest::get(&req.user_url).await;
    match response {
        Ok(resp) => {
            let body = resp.text().await.unwrap_or_default();
            Ok(Response {
                message: "Success".to_string(),
                data: body,
            })
        }
        Err(_) => Err("Request failed")
    }
}

References

• OWASP Server Side Request Forgery.
• Common Weakness Enumeration: CWE-918.

[Copilot]

Pull Request Overview

This PR adds a basic Server-Side Request Forgery (SSRF) query for Rust that detects vulnerabilities where user-controlled data flows into HTTP request URLs. The query is built on the existing taint tracking infrastructure and reuses HTTP request sinks.

• Introduces new RequestForgery query and supporting extensions
• Reuses existing HTTP sinks by changing the label from "transmission" to "request-url"
• Adds comprehensive test cases covering various vulnerability patterns and edge cases

Reviewed Changes

Copilot reviewed 16 out of 18 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
rust/ql/src/queries/security/CWE-918/RequestForgery.ql	Main query implementation using taint tracking to detect SSRF vulnerabilities
rust/ql/src/queries/security/CWE-918/RequestForgery.qhelp	Documentation explaining SSRF vulnerabilities with examples and recommendations
rust/ql/src/queries/security/CWE-918/RequestForgery.rs	Code examples for the documentation showing vulnerable and secure patterns
rust/ql/lib/codeql/rust/security/RequestForgeryExtensions.qll	Extension module defining sources, sinks and barriers for request forgery detection
rust/ql/lib/codeql/rust/security/CleartextTransmissionExtensions.qll	Updated to also use "request-url" sinks for transmission queries
rust/ql/lib/codeql/rust/frameworks/reqwest.model.yml	Changed sink labels from "transmission" to "request-url" for HTTP request methods
rust/ql/test/query-tests/security/CWE-918/*	Comprehensive test suite with various SSRF scenarios and expected results

[geoffw0]

Looks great, I have a couple of small / cleanup suggestions and of course we'll want to look at a DCA run.

I think there's a bit more to do in order for the query to be production quality

Yes but I think we can merge this PR without necessarily containing any of those improvements - then prioritize some to do as immediate follow-up, and others to put into longer term planning (I usually write up one "improvements" issue per query listing these kinds of thoughts).

I'm wondering if "request URL" is something that we could make a concept? But if so, it seems like out of scope for this PR.

I agree this is out of scope, but we're always looking for ways to bring CodeQL for different target languages closer together - and a shared concept for URL use could help do that.

Once the above mentioned items are done it would make sense to look at top 1000 and see what shows up there.

Yep, it could also be worth doing MRVA runs querying the sources and sinks specifically, to see if they make sense + prioritize further modelling for the weakest end.

[geoffw0]

These places are also sinks for the "plaintext transmission" query and we had such sinks labeled as transmission in MaD yaml. I'd like to reuse those for this query, so I've changed the label to request-url and this label is picked up by both queries.

This looks like a good design, both queries use request-url but we can still define a non-URL transmission only sink if we want to (I don't think we have any right now, but we surely will at some point).

[geoffw0]

Good tests.

[paldepind]

Thanks for the review. I've applied the suggestions 👍

[isaacmbrown]

Docs FR here, it doesn't seem like the ready-for-doc-review label added this to our review board, but I've just added it manually

[mchammer01]

@paldepind 👋🏻 - Carried out an editorial review on behalf of Docs.
This LGTM. Left a few comments and suggestions. Spotted a potential readability issue with 2 consecutive sentences that are a bit convoluted.
Also, wondering if we need to add this query to a table/list of released queries somewhere 🤔

[mchammer01]

Sentences on lines 13 and 17 are a bit difficult to parse because they are a bit long. Can we simplify them, or use smaller sentences instead? Let me know if you'd like me to make some suggestions. Thanks!

[paldepind]

I can see what you mean. Some suggestions would be appreciated 🙏

[mchammer01]

@paldepind - what about something like:

If a server is connected to an internal network, attackers can bypass security boundaries to target internal services.

Forged requests can execute unauthorized actions, leak data through external redirects, or compromise the server if responses are handled insecurely.

Let me know what you think.

[aibaars]

@paldepind I guess you copied this sentence from Javascript, right?

[geoffw0]

Approved from a code point-of-view. Can be merged as soon as you have 👍 from docs. :)