Skip to content

Commit 4686718

Browse files
hmachmac
committed
Ruby: Add kind to Http::Server::RequestInputAccess
Like in JS, this describes whether the input came from the request URL, body, parameters, headers or cookie. Only some of these are relevant for UrlRedirect and ReflectedXSS queries.
1 parent 9eff493 commit 4686718

8 files changed

Lines changed: 99 additions & 15 deletions

File tree

‎ruby/ql/lib/codeql/ruby/Concepts.qll‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,26 @@ module Http {
290290
}
291291
}
292292

293+
/** A kind of request input. */
294+
class RequestInputKind extends string {
295+
RequestInputKind() { this = ["parameter", "header", "body", "url", "cookie"] }
296+
}
297+
298+
/** Input from the parameters of a request. */
299+
RequestInputKind parameterInputKind() { result = "parameter" }
300+
301+
/** Input from the headers of a request. */
302+
RequestInputKind headerInputKind() { result = "header" }
303+
304+
/** Input from the body of a request. */
305+
RequestInputKind bodyInputKind() { result = "body" }
306+
307+
/** Input from the URL of a request. */
308+
RequestInputKind urlInputKind() { result = "url" }
309+
310+
/** Input from the cookies of a request. */
311+
RequestInputKind cookieInputKind() { result = "cookie" }
312+
293313
/**
294314
* An access to a user-controlled HTTP request input. For example, the URL or body of a request.
295315
* Instances of this class automatically become `RemoteFlowSource`s.
@@ -304,6 +324,32 @@ module Http {
304324
* This is typically the name of the method that gives rise to this input.
305325
*/
306326
string getSourceType() { result = super.getSourceType() }
327+
328+
/**
329+
* Gets the kind of the accessed input,
330+
* Can be one of "parameter", "header", "body", "url", "cookie".
331+
*/
332+
RequestInputKind getKind() { result = super.getKind() }
333+
334+
/**
335+
* Holds if this part of the request may be controlled by a third party,
336+
* that is, an agent other than the one who sent the request.
337+
*
338+
* This is true for the URL, query parameters, and request body.
339+
* These can be controlled by a malicious third party in the following scenarios:
340+
*
341+
* - The user clicks a malicious link or is otherwise redirected to a malicious URL.
342+
* - The user visits a web site that initiates a form submission or AJAX request on their behalf.
343+
*
344+
* In these cases, the request is technically sent from the user's browser, but
345+
* the user is not in direct control of the URL or POST body.
346+
*
347+
* Headers are never considered third-party controllable by this predicate, although the
348+
* third party does have some control over the the Referer and Origin headers.
349+
*/
350+
predicate isThirdPartyControllable() {
351+
this.getKind() = [parameterInputKind(), urlInputKind(), bodyInputKind()]
352+
}
307353
}
308354

309355
/** Provides a class for modeling new HTTP request inputs. */
@@ -321,6 +367,12 @@ module Http {
321367
* This is typically the name of the method that gives rise to this input.
322368
*/
323369
abstract string getSourceType();
370+
371+
/**
372+
* Gets the kind of the accessed input,
373+
* Can be one of "parameter", "header", "body", "url", "cookie".
374+
*/
375+
abstract RequestInputKind getKind();
324376
}
325377
}
326378

@@ -387,6 +439,8 @@ module Http {
387439
RoutedParameter() { this.getParameter() = handler.getARoutedParameter() }
388440

389441
override string getSourceType() { result = handler.getFramework() + " RoutedParameter" }
442+
443+
override RequestInputKind getKind() { result = parameterInputKind() }
390444
}
391445

392446
/**

‎ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,8 @@ class ParamsSource extends Http::Server::RequestInputAccess::Range {
139139
ParamsSource() { this.asExpr().getExpr() instanceof Rails::ParamsCall }
140140

141141
override string getSourceType() { result = "ActionController::Metal#params" }
142+
143+
override Http::Server::RequestInputKind getKind() { result = Http::Server::parameterInputKind() }
142144
}
143145

144146
/**
@@ -149,6 +151,8 @@ class CookiesSource extends Http::Server::RequestInputAccess::Range {
149151
CookiesSource() { this.asExpr().getExpr() instanceof Rails::CookiesCall }
150152

151153
override string getSourceType() { result = "ActionController::Metal#cookies" }
154+
155+
override Http::Server::RequestInputKind getKind() { result = Http::Server::cookieInputKind() }
152156
}
153157

154158
/** A call to `cookies` from within a controller. */
@@ -199,11 +203,17 @@ private module Request {
199203
"filtered_parameters"
200204
]
201205
}
206+
207+
override Http::Server::RequestInputKind getKind() {
208+
result = Http::Server::parameterInputKind()
209+
}
202210
}
203211

204212
/** A method call on `request` which returns part or all of the request path. */
205213
private class PathCall extends RequestInputAccess {
206214
PathCall() { this.getMethodName() = ["path", "filtered_path"] }
215+
216+
override Http::Server::RequestInputKind getKind() { result = Http::Server::urlInputKind() }
207217
}
208218

209219
/** A method call on `request` which returns a specific request header. */
@@ -221,6 +231,8 @@ private module Request {
221231
this.getMethodName() = ["get_header", "fetch_header"] and
222232
this.getArgument(0).asExpr().getExpr().getConstantValue().getString().regexpMatch("^HTTP_.+")
223233
}
234+
235+
override Http::Server::RequestInputKind getKind() { result = Http::Server::headerInputKind() }
224236
}
225237

226238
// TODO: each_header
@@ -236,6 +248,8 @@ private module Request {
236248
"forwarded_host", "port", "forwarded_port"
237249
]
238250
}
251+
252+
override Http::Server::RequestInputKind getKind() { result = Http::Server::headerInputKind() }
239253
}
240254

241255
/**
@@ -247,11 +261,15 @@ private module Request {
247261
this.getMethodName() =
248262
["media_type", "media_type", "media_type_params", "content_charset", "base_url"]
249263
}
264+
265+
override Http::Server::RequestInputKind getKind() { result = Http::Server::headerInputKind() }
250266
}
251267

252268
/** A method call on `request` which returns the request body. */
253269
private class BodyCall extends RequestInputAccess {
254270
BodyCall() { this.getMethodName() = ["body", "raw_post"] }
271+
272+
override Http::Server::RequestInputKind getKind() { result = Http::Server::bodyInputKind() }
255273
}
256274

257275
/** A method call on `request` which returns the rack env. */
@@ -260,6 +278,8 @@ private module Request {
260278
this.getMethodName() = ["env", "filtered_env"] and
261279
this.getArgument(0).asExpr().getExpr().getConstantValue().getString().regexpMatch("^HTTP_.+")
262280
}
281+
282+
override Http::Server::RequestInputKind getKind() { result = Http::Server::headerInputKind() }
263283
}
264284
}
265285

‎ruby/ql/lib/codeql/ruby/security/UrlRedirectCustomizations.qll‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,9 @@ module UrlRedirect {
5050
/**
5151
* A source of remote user input, considered as a flow source.
5252
*/
53-
class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
53+
class HttpRequestInputAccessAsSource extends Source, Http::Server::RequestInputAccess {
54+
HttpRequestInputAccessAsSource() { this.isThirdPartyControllable() }
55+
}
5456

5557
/**
5658
* A HTTP redirect response, considered as a flow sink.

‎ruby/ql/lib/codeql/ruby/security/XSS.qll‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -312,9 +312,11 @@ module ReflectedXss {
312312
deprecated predicate isAdditionalXSSTaintStep = isAdditionalXssTaintStep/2;
313313

314314
/**
315-
* A source of remote user input, considered as a flow source.
315+
* A HTTP request input, considered as a flow source.
316316
*/
317-
class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
317+
class HttpRequestInputAccessAsSource extends Source, Http::Server::RequestInputAccess {
318+
HttpRequestInputAccessAsSource() { this.isThirdPartyControllable() }
319+
}
318320
}
319321

320322
/** DEPRECATED: Alias for ReflectedXss */

‎ruby/ql/test/query-tests/security/cwe-079/ReflectedXSS.expected‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,13 @@ edges
1010
| app/controllers/foo/bars_controller.rb:17:21:17:36 | ...[...] : | app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website |
1111
| app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params : | app/controllers/foo/bars_controller.rb:18:10:18:22 | ...[...] : |
1212
| app/controllers/foo/bars_controller.rb:18:10:18:22 | ...[...] : | app/controllers/foo/bars_controller.rb:19:22:19:23 | dt : |
13-
| app/controllers/foo/bars_controller.rb:18:10:18:22 | ...[...] : | app/controllers/foo/bars_controller.rb:23:53:23:54 | dt : |
13+
| app/controllers/foo/bars_controller.rb:18:10:18:22 | ...[...] : | app/controllers/foo/bars_controller.rb:24:53:24:54 | dt : |
1414
| app/controllers/foo/bars_controller.rb:19:22:19:23 | dt : | app/views/foo/bars/show.html.erb:41:3:41:16 | @instance_text |
15-
| app/controllers/foo/bars_controller.rb:23:53:23:54 | dt : | app/views/foo/bars/show.html.erb:5:9:5:20 | call to display_text |
16-
| app/controllers/foo/bars_controller.rb:23:53:23:54 | dt : | app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] |
17-
| app/controllers/foo/bars_controller.rb:23:53:23:54 | dt : | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] |
18-
| app/controllers/foo/bars_controller.rb:23:53:23:54 | dt : | app/views/foo/bars/show.html.erb:36:3:36:14 | call to display_text |
19-
| app/controllers/foo/bars_controller.rb:23:53:23:54 | dt : | app/views/foo/bars/show.html.erb:44:76:44:87 | call to display_text : |
15+
| app/controllers/foo/bars_controller.rb:24:53:24:54 | dt : | app/views/foo/bars/show.html.erb:5:9:5:20 | call to display_text |
16+
| app/controllers/foo/bars_controller.rb:24:53:24:54 | dt : | app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] |
17+
| app/controllers/foo/bars_controller.rb:24:53:24:54 | dt : | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] |
18+
| app/controllers/foo/bars_controller.rb:24:53:24:54 | dt : | app/views/foo/bars/show.html.erb:36:3:36:14 | call to display_text |
19+
| app/controllers/foo/bars_controller.rb:24:53:24:54 | dt : | app/views/foo/bars/show.html.erb:44:76:44:87 | call to display_text : |
2020
| app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... : | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text |
2121
| app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... : | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] |
2222
| app/views/foo/bars/show.html.erb:44:76:44:87 | call to display_text : | app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... : |
@@ -35,7 +35,7 @@ nodes
3535
| app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params : | semmle.label | call to params : |
3636
| app/controllers/foo/bars_controller.rb:18:10:18:22 | ...[...] : | semmle.label | ...[...] : |
3737
| app/controllers/foo/bars_controller.rb:19:22:19:23 | dt : | semmle.label | dt : |
38-
| app/controllers/foo/bars_controller.rb:23:53:23:54 | dt : | semmle.label | dt : |
38+
| app/controllers/foo/bars_controller.rb:24:53:24:54 | dt : | semmle.label | dt : |
3939
| app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | semmle.label | call to display_text |
4040
| app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | semmle.label | ...[...] |
4141
| app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website | semmle.label | @user_website |

‎ruby/ql/test/query-tests/security/cwe-079/app/controllers/foo/bars_controller.rb‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ def show
2020
@safe_foo = params[:text]
2121
@safe_foo = "safe_foo"
2222
@html_escaped = ERB::Util.html_escape(params[:text])
23+
@header_escaped = ERB::Util.html_escape(cookies[:foo]) # OK - cookies not controllable by 3rd party
2324
render "foo/bars/show", locals: { display_text: dt, safe_text: "hello" }
2425
end
2526
end

‎ruby/ql/test/query-tests/security/cwe-601/UrlRedirect.expected‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,14 @@ edges
33
| UrlRedirect.rb:14:17:14:22 | call to params : | UrlRedirect.rb:14:17:14:43 | call to fetch |
44
| UrlRedirect.rb:19:17:19:22 | call to params : | UrlRedirect.rb:19:17:19:37 | call to to_unsafe_hash |
55
| UrlRedirect.rb:24:31:24:36 | call to params : | UrlRedirect.rb:24:17:24:37 | call to filter_params |
6-
| UrlRedirect.rb:24:31:24:36 | call to params : | UrlRedirect.rb:88:21:88:32 | input_params : |
6+
| UrlRedirect.rb:24:31:24:36 | call to params : | UrlRedirect.rb:93:21:93:32 | input_params : |
77
| UrlRedirect.rb:34:20:34:25 | call to params : | UrlRedirect.rb:34:20:34:31 | ...[...] : |
88
| UrlRedirect.rb:34:20:34:31 | ...[...] : | UrlRedirect.rb:34:17:34:37 | "#{...}/foo" |
99
| UrlRedirect.rb:58:17:58:22 | call to params : | UrlRedirect.rb:58:17:58:28 | ...[...] |
1010
| UrlRedirect.rb:63:38:63:43 | call to params : | UrlRedirect.rb:63:38:63:49 | ...[...] |
1111
| UrlRedirect.rb:68:38:68:43 | call to params : | UrlRedirect.rb:68:38:68:49 | ...[...] |
1212
| UrlRedirect.rb:73:25:73:30 | call to params : | UrlRedirect.rb:73:25:73:36 | ...[...] |
13-
| UrlRedirect.rb:88:21:88:32 | input_params : | UrlRedirect.rb:89:5:89:29 | call to permit : |
13+
| UrlRedirect.rb:93:21:93:32 | input_params : | UrlRedirect.rb:94:5:94:29 | call to permit : |
1414
nodes
1515
| UrlRedirect.rb:4:17:4:22 | call to params | semmle.label | call to params |
1616
| UrlRedirect.rb:9:17:9:22 | call to params : | semmle.label | call to params : |
@@ -32,10 +32,10 @@ nodes
3232
| UrlRedirect.rb:68:38:68:49 | ...[...] | semmle.label | ...[...] |
3333
| UrlRedirect.rb:73:25:73:30 | call to params : | semmle.label | call to params : |
3434
| UrlRedirect.rb:73:25:73:36 | ...[...] | semmle.label | ...[...] |
35-
| UrlRedirect.rb:88:21:88:32 | input_params : | semmle.label | input_params : |
36-
| UrlRedirect.rb:89:5:89:29 | call to permit : | semmle.label | call to permit : |
35+
| UrlRedirect.rb:93:21:93:32 | input_params : | semmle.label | input_params : |
36+
| UrlRedirect.rb:94:5:94:29 | call to permit : | semmle.label | call to permit : |
3737
subpaths
38-
| UrlRedirect.rb:24:31:24:36 | call to params : | UrlRedirect.rb:88:21:88:32 | input_params : | UrlRedirect.rb:89:5:89:29 | call to permit : | UrlRedirect.rb:24:17:24:37 | call to filter_params |
38+
| UrlRedirect.rb:24:31:24:36 | call to params : | UrlRedirect.rb:93:21:93:32 | input_params : | UrlRedirect.rb:94:5:94:29 | call to permit : | UrlRedirect.rb:24:17:24:37 | call to filter_params |
3939
#select
4040
| UrlRedirect.rb:4:17:4:22 | call to params | UrlRedirect.rb:4:17:4:22 | call to params | UrlRedirect.rb:4:17:4:22 | call to params | Untrusted URL redirection depends on a $@. | UrlRedirect.rb:4:17:4:22 | call to params | user-provided value |
4141
| UrlRedirect.rb:9:17:9:28 | ...[...] | UrlRedirect.rb:9:17:9:22 | call to params : | UrlRedirect.rb:9:17:9:28 | ...[...] | Untrusted URL redirection depends on a $@. | UrlRedirect.rb:9:17:9:22 | call to params | user-provided value |

‎ruby/ql/test/query-tests/security/cwe-601/UrlRedirect.rb‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,11 @@ def route14
8383
redirect_back_or_to params[:key], allow_other_host: false
8484
end
8585

86+
# GOOD
87+
def route15
88+
redirect_to cookies[:foo]
89+
end
90+
8691
private
8792

8893
def filter_params(input_params)

0 commit comments

Comments
 (0)