Skip to content

Ruby: Minor change of SSRF concept #8524

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.

Sign up for GitHub

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 7 commits into from
Mar 24, 2022
Merged

Ruby: Minor change of SSRF concept #8524

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9c2fc1b
Ruby: client request: getUrl => getAUrlPart
RasmusWL Mar 9, 2022
68de6a4
Ruby: Add change-note
RasmusWL Mar 9, 2022
945b52f
Ruby: autoformat
RasmusWL Mar 22, 2022
9254b29
Ruby: Adjust HttpClients test
RasmusWL Mar 22, 2022
64a5c5d
Ruby: Keep `getURL` predicate for easier deprecation
RasmusWL Mar 22, 2022
e66932c
Ruby: Make deprecated `getURL` work with new modeling
RasmusWL Mar 23, 2022
6718893
Ruby: Update wording of change-note
RasmusWL Mar 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ruby/ql/lib/change-notes/2022-03-09-http-client-getaurlpart.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: breaking
---
* The `getURL` member-predicate of the `HTTP::Client::Request` and `HTTP::Client::Request::Range` classes from `Concepts.qll` have been renamed to `getAUrlPart`.
20 changes: 18 additions & 2 deletions ruby/ql/lib/codeql/ruby/Concepts.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,10 +485,18 @@ module HTTP {
DataFlow::Node getResponseBody() { result = super.getResponseBody() }

/**
* DEPRECATED: Use `getAUrlPart` instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't entirely related to this PR, but I was wondering what you thought about adding (2022-03-24) or similar to this comment so it's clear when the predicate was deprecated. I know we can estimate this using git blame, but that can become inaccurate if code gets moved around.

Even if it's a good idea, it might be something better added to all the deprecated predicates as a separate PR. I'm mostly just interested in thoughts.

Copy link
Member Author

@RasmusWL RasmusWL Mar 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's an excellent idea 👍 (although git blame should have our back)

*
* Gets a node that contributes to the URL of the request.
* Depending on the framework, a request may have multiple nodes which contribute to the URL.
*/
DataFlow::Node getURL() { result = super.getURL() }
deprecated DataFlow::Node getURL() { result = super.getURL() or result = super.getAUrlPart() }

/**
* Gets a data-flow node that contributes to the URL of the request.
* Depending on the framework, a request may have multiple nodes which contribute to the URL.
*/
DataFlow::Node getAUrlPart() { result = super.getAUrlPart() }

/** Gets a string that identifies the framework used for this request. */
string getFramework() { result = super.getFramework() }
Expand Down Expand Up @@ -516,10 +524,18 @@ module HTTP {
abstract DataFlow::Node getResponseBody();

/**
* DEPRECATED: overwrite `getAUrlPart` instead.
*
* Gets a node that contributes to the URL of the request.
* Depending on the framework, a request may have multiple nodes which contribute to the URL.
*/
abstract DataFlow::Node getURL();
deprecated DataFlow::Node getURL() { none() }

/**
* Gets a data-flow node that contributes to the URL of the request.
* Depending on the framework, a request may have multiple nodes which contribute to the URL.
*/
abstract DataFlow::Node getAUrlPart();

/** Gets a string that identifies the framework used for this request. */
abstract string getFramework();
Expand Down
2 changes: 1 addition & 1 deletion ruby/ql/lib/codeql/ruby/frameworks/http_clients/Excon.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class ExconHttpRequest extends HTTP::Client::Request::Range {

override DataFlow::Node getResponseBody() { result = requestNode.getAMethodCall("body") }

override DataFlow::Node getURL() {
override DataFlow::Node getAUrlPart() {
// For one-off requests, the URL is in the first argument of the request method call.
// For connection re-use, the URL is split between the first argument of the `new` call
// and the `path` keyword argument of the request method call.
Expand Down
2 changes: 1 addition & 1 deletion ruby/ql/lib/codeql/ruby/frameworks/http_clients/Faraday.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class FaradayHttpRequest extends HTTP::Client::Request::Range {

override DataFlow::Node getResponseBody() { result = requestNode.getAMethodCall("body") }

override DataFlow::Node getURL() {
override DataFlow::Node getAUrlPart() {
result = requestUse.getArgument(0) or
result = connectionUse.(DataFlow::CallNode).getArgument(0) or
result = connectionUse.(DataFlow::CallNode).getKeywordArgument("url")
Expand Down
2 changes: 1 addition & 1 deletion ruby/ql/lib/codeql/ruby/frameworks/http_clients/HttpClient.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class HttpClientRequest extends HTTP::Client::Request::Range {
this = requestUse.asExpr().getExpr()
}

override DataFlow::Node getURL() { result = requestUse.getArgument(0) }
override DataFlow::Node getAUrlPart() { result = requestUse.getArgument(0) }

override DataFlow::Node getResponseBody() {
// The `get_content` and `post_content` methods return the response body as
Expand Down
2 changes: 1 addition & 1 deletion ruby/ql/lib/codeql/ruby/frameworks/http_clients/Httparty.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class HttpartyRequest extends HTTP::Client::Request::Range {
this = requestUse.asExpr().getExpr()
}

override DataFlow::Node getURL() { result = requestUse.getArgument(0) }
override DataFlow::Node getAUrlPart() { result = requestUse.getArgument(0) }

override DataFlow::Node getResponseBody() {
// If HTTParty can recognise the response type, it will parse and return it
Expand Down
2 changes: 1 addition & 1 deletion ruby/ql/lib/codeql/ruby/frameworks/http_clients/NetHttp.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class NetHttpRequest extends HTTP::Client::Request::Range {
* Gets the node representing the URL of the request.
* Currently unused, but may be useful in future, e.g. to filter out certain requests.
*/
override DataFlow::Node getURL() { result = request.getArgument(0) }
override DataFlow::Node getAUrlPart() { result = request.getArgument(0) }

override DataFlow::Node getResponseBody() { result = responseBody }

Expand Down
4 changes: 2 additions & 2 deletions ruby/ql/lib/codeql/ruby/frameworks/http_clients/OpenURI.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class OpenUriRequest extends HTTP::Client::Request::Range {
this = requestUse.asExpr().getExpr()
}

override DataFlow::Node getURL() { result = requestUse.getArgument(0) }
override DataFlow::Node getAUrlPart() { result = requestUse.getArgument(0) }

override DataFlow::Node getResponseBody() {
result = requestNode.getAMethodCall(["read", "readlines"])
Expand Down Expand Up @@ -65,7 +65,7 @@ class OpenUriKernelOpenRequest extends HTTP::Client::Request::Range {
this = requestUse.asExpr().getExpr()
}

override DataFlow::Node getURL() { result = requestUse.getArgument(0) }
override DataFlow::Node getAUrlPart() { result = requestUse.getArgument(0) }

override DataFlow::CallNode getResponseBody() {
result.asExpr().getExpr().(MethodCall).getMethodName() in ["read", "readlines"] and
Expand Down
2 changes: 1 addition & 1 deletion ruby/ql/lib/codeql/ruby/frameworks/http_clients/RestClient.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class RestClientHttpRequest extends HTTP::Client::Request::Range {
)
}

override DataFlow::Node getURL() {
override DataFlow::Node getAUrlPart() {
result = requestUse.getKeywordArgument("url")
or
result = requestUse.getArgument(0) and
Expand Down
2 changes: 1 addition & 1 deletion ruby/ql/lib/codeql/ruby/frameworks/http_clients/Typhoeus.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class TyphoeusHttpRequest extends HTTP::Client::Request::Range {
this = requestUse.asExpr().getExpr()
}

override DataFlow::Node getURL() { result = requestUse.getArgument(0) }
override DataFlow::Node getAUrlPart() { result = requestUse.getArgument(0) }

override DataFlow::Node getResponseBody() { result = requestNode.getAMethodCall("body") }

Expand Down
2 changes: 1 addition & 1 deletion ruby/ql/lib/codeql/ruby/security/ServerSideRequestForgeryCustomizations.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ module ServerSideRequestForgery {

/** The URL of an HTTP request, considered as a sink. */
class HttpRequestAsSink extends Sink {
HttpRequestAsSink() { exists(HTTP::Client::Request req | req.getURL() = this) }
HttpRequestAsSink() { exists(HTTP::Client::Request req | req.getAUrlPart() = this) }
}

/** A string interpolation with a fixed prefix, considered as a flow sanitizer. */
Expand Down
4 changes: 2 additions & 2 deletions ruby/ql/test/library-tests/frameworks/http_clients/HttpClients.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ import codeql.ruby.Concepts
import codeql.ruby.DataFlow

query predicate httpRequests(
HTTP::Client::Request r, string framework, DataFlow::Node url, DataFlow::Node responseBody
HTTP::Client::Request r, string framework, DataFlow::Node urlPart, DataFlow::Node responseBody
) {
r.getFramework() = framework and
r.getURL() = url and
r.getAUrlPart() = urlPart and
r.getResponseBody() = responseBody
}