-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Rust: Add basic request forgery query #20381
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
Merged
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
eea11db
Rust: Relabel reqwest sinks as `request-url`
paldepind 63a2c9d
Rust: Add request forgery tests
paldepind e1047dc
Rust: Add request forgery query
paldepind 1c922f0
Rust: Add request forgery query qhelp
paldepind 387d08a
Rust: Update query suite expected files
paldepind 4f9d827
Rust: Add change node for request forgery query
paldepind 50cd200
Apply suggestions from code review
paldepind c73d081
Rust: Fix based on review
paldepind d1a3294
Merge branch 'main' into rust/request-forgery-query
paldepind 065388d
Rust: Fix formatting
paldepind 4d0635d
Update qhelp for request forgery query
paldepind b456a8c
Apply suggestions from code review
paldepind 50bdc65
Rust: Apply documentation tweaks
paldepind File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
49 changes: 49 additions & 0 deletions
49
rust/ql/lib/codeql/rust/security/RequestForgeryExtensions.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| /** | ||
| * Provides classes and predicates for reasoning about request forgery | ||
| * vulnerabilities. | ||
| */ | ||
|
|
||
| import rust | ||
| private import codeql.rust.dataflow.DataFlow | ||
| private import codeql.rust.dataflow.FlowSink | ||
| private import codeql.rust.dataflow.FlowSource | ||
| private import codeql.rust.Concepts | ||
|
|
||
| /** | ||
| * Provides default sources, sinks and barriers for detecting request forgery | ||
| * vulnerabilities, as well as extension points for adding your own. | ||
| */ | ||
| module RequestForgery { | ||
| /** | ||
| * A data flow source for request forgery vulnerabilities. | ||
| */ | ||
| abstract class Source extends DataFlow::Node { } | ||
|
|
||
| /** | ||
| * A data flow sink for request forgery vulnerabilities. | ||
| */ | ||
| abstract class Sink extends QuerySink::Range { | ||
| /** | ||
| * Gets the name of a part of the request that may be tainted by this sink, | ||
| * such as the URL or the host. | ||
| */ | ||
| override string getSinkType() { result = "RequestForgery" } | ||
| } | ||
|
|
||
| /** | ||
| * A barrier for request forgery vulnerabilities. | ||
| */ | ||
| abstract class Barrier extends DataFlow::Node { } | ||
|
|
||
| /** | ||
| * An active threat-model source, considered as a flow source. | ||
| */ | ||
| private class ActiveThreatModelSourceAsSource extends Source, ActiveThreatModelSource { } | ||
|
|
||
| /** | ||
| * A sink for request forgery from model data. | ||
| */ | ||
| private class ModelsAsDataSink extends Sink { | ||
| ModelsAsDataSink() { sinkNode(this, "request-url") } | ||
| } | ||
geoffw0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| --- | ||
| category: newQuery | ||
| --- | ||
| * Added a new query, `rust/request-forgery`, for detecting server-side request forgery vulnerabilities. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
|
|
||
| <overview> | ||
| <p> | ||
| 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 is connected to an internal network, attackers can bypass security | ||
| boundaries to target internal services. | ||
|
|
||
| Forged requests can execute unintended actions, leak data if redirected to an | ||
| external server, or compromise the server if responses are handled insecurely. | ||
| </p> | ||
| </overview> | ||
|
|
||
| <recommendation> | ||
| <p> | ||
| 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. | ||
| </p> | ||
| </recommendation> | ||
|
|
||
| <example> | ||
| <p> | ||
| 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. | ||
| </p> | ||
|
|
||
| <sample src="RequestForgery.rs" /> | ||
| </example> | ||
|
|
||
| <references> | ||
| <li> | ||
| <a href="https://owasp.org/www-community/attacks/Server_Side_Request_Forgery">OWASP Server Side Request Forgery</a>. | ||
| </li> | ||
| </references> | ||
|
|
||
| </qhelp> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| /** | ||
| * @name Server-side request forgery | ||
| * @description Making a network request with user-controlled data in the URL allows for request forgery attacks. | ||
| * @kind path-problem | ||
| * @problem.severity error | ||
| * @security-severity 9.1 | ||
| * @precision high | ||
| * @id rust/request-forgery | ||
| * @tags security | ||
| * external/cwe/cwe-918 | ||
| */ | ||
|
|
||
| private import rust | ||
| private import codeql.rust.dataflow.TaintTracking | ||
| private import codeql.rust.dataflow.DataFlow | ||
|
||
| private import codeql.rust.security.RequestForgeryExtensions | ||
geoffw0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /** | ||
| * A taint-tracking configuration for detecting request forgery vulnerabilities. | ||
| */ | ||
| module RequestForgeryConfig implements DataFlow::ConfigSig { | ||
| predicate isSource(DataFlow::Node source) { source instanceof RequestForgery::Source } | ||
|
|
||
| predicate isSink(DataFlow::Node sink) { sink instanceof RequestForgery::Sink } | ||
|
|
||
| predicate isBarrier(DataFlow::Node barrier) { barrier instanceof RequestForgery::Barrier } | ||
|
|
||
| predicate observeDiffInformedIncrementalMode() { any() } | ||
| } | ||
|
|
||
| module RequestForgeryFlow = TaintTracking::Global<RequestForgeryConfig>; | ||
|
|
||
| import RequestForgeryFlow::PathGraph | ||
|
|
||
| from RequestForgeryFlow::PathNode source, RequestForgeryFlow::PathNode sink | ||
| where RequestForgeryFlow::flowPath(source, sink) | ||
| select sink.getNode(), source, sink, "The URL of this request depends on a $@.", source.getNode(), | ||
| "user-provided value" | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| // 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") | ||
| } | ||
| } | ||
geoffw0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 changes: 3 additions & 0 deletions
3
rust/ql/test/query-tests/security/CWE-918/CONSISTENCY/PathResolutionConsistency.expected
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| multipleCallTargets | ||
| | request_forgery_tests.rs:30:36:30:51 | user_url.as_str() | | ||
| | request_forgery_tests.rs:30:36:30:52 | user_url.as_str() | |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good design, both queries use
request-urlbut we can still define a non-URLtransmissiononly sink if we want to (I don't think we have any right now, but we surely will at some point).