Skip to content

Commit 36289aa

Browse files
alexrfordalexrford
authored
Merge pull request #255 from github/reflected-xss
rb/reflected-xss query
2 parents 3c05101 + e89d485 commit 36289aa

File tree

15 files changed

+548
-6
lines changed

15 files changed

+548
-6
lines changed

‎ql/lib/codeql/ruby/controlflow/CfgNodes.qll

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,13 @@ module ExprNodes {
355355
final override VariableReadAccess getExpr() { result = ExprCfgNode.super.getExpr() }
356356
}
357357

358+
/** A control-flow node that wraps a `InstanceVariableWriteAccess` AST expression. */
359+
class InstanceVariableWriteAccessCfgNode extends ExprCfgNode {
360+
override InstanceVariableWriteAccess e;
361+
362+
final override InstanceVariableWriteAccess getExpr() { result = ExprCfgNode.super.getExpr() }
363+
}
364+
358365
/** A control-flow node that wraps a `StringInterpolationComponent` AST expression. */
359366
class StringInterpolationComponentCfgNode extends StmtSequenceCfgNode {
360367
StringInterpolationComponentCfgNode() { this.getNode() instanceof StringInterpolationComponent }

‎ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,3 +314,16 @@ predicate mayBenefitFromCallContext(DataFlowCall call, Callable c) { none() }
314314
* restricted to those `call`s for which a context might make a difference.
315315
*/
316316
DataFlowCallable viableImplInCallContext(DataFlowCall call, DataFlowCall ctx) { none() }
317+
318+
/**
319+
* Holds if `e` is an `ExprNode` that may be returned by a call to `c`.
320+
*/
321+
predicate exprNodeReturnedFrom(DataFlow::ExprNode e, DataFlowCallable c) {
322+
exists(ReturnNode r |
323+
r.getEnclosingCallable() = c and
324+
(
325+
r.(ExplicitReturnNode).getReturningNode().getReturnedValueNode() = e.asExpr() or
326+
r.(ExprReturnNode) = e
327+
)
328+
)
329+
}

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,13 @@ private class ActionControllerHtmlSafeCall extends HtmlSafeCall {
134134
}
135135
}
136136

137+
// A call to `html_escape` from within a controller.
138+
private class ActionControllerHtmlEscapeCall extends HtmlEscapeCall {
139+
ActionControllerHtmlEscapeCall() {
140+
this.getEnclosingModule() instanceof ActionControllerControllerClass
141+
}
142+
}
143+
137144
/**
138145
* A call to the `redirect_to` method, used in an action to redirect to a
139146
* specific URL/path or to a different action in this controller.

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

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,34 @@ abstract class HtmlSafeCall extends MethodCall {
2020
HtmlSafeCall() { this.getMethodName() = "html_safe" }
2121
}
2222

23-
// A call to `html_safe` from within a template or view component.
23+
// A call to `html_safe` from within a template.
2424
private class ActionViewHtmlSafeCall extends HtmlSafeCall {
2525
ActionViewHtmlSafeCall() { inActionViewContext(this) }
2626
}
2727

28+
/**
29+
* A call to a method named "html_escape", "html_escape_once", or "h".
30+
*/
31+
abstract class HtmlEscapeCall extends MethodCall {
32+
// "h" is aliased to "html_escape" in ActiveSupport
33+
HtmlEscapeCall() { this.getMethodName() = ["html_escape", "html_escape_once", "h"] }
34+
}
35+
36+
class RailsHtmlEscaping extends Escaping::Range, DataFlow::CallNode {
37+
RailsHtmlEscaping() { this.asExpr().getExpr() instanceof HtmlEscapeCall }
38+
39+
override DataFlow::Node getAnInput() { result = this.getArgument(0) }
40+
41+
override DataFlow::Node getOutput() { result = this }
42+
43+
override string getKind() { result = Escaping::getHtmlKind() }
44+
}
45+
46+
// A call to `html_escape` from within a template.
47+
private class ActionViewHtmlEscapeCall extends HtmlEscapeCall {
48+
ActionViewHtmlEscapeCall() { inActionViewContext(this) }
49+
}
50+
2851
// A call in a context where some commonly used `ActionView` methods are available.
2952
private class ActionViewContextCall extends MethodCall {
3053
ActionViewContextCall() {
@@ -40,7 +63,7 @@ class RawCall extends ActionViewContextCall {
4063
RawCall() { this.getMethodName() = "raw" }
4164
}
4265

43-
// A call to the `params` method within the context of a template or view component.
66+
// A call to the `params` method within the context of a template.
4467
private class ActionViewParamsCall extends ActionViewContextCall, ParamsCall { }
4568

4669
/**
@@ -100,7 +123,7 @@ abstract class RenderCall extends MethodCall {
100123
// TODO: implicit renders in controller actions
101124
}
102125

103-
// A call to the `render` method within the context of a template or view component.
126+
// A call to the `render` method within the context of a template.
104127
private class ActionViewRenderCall extends RenderCall, ActionViewContextCall { }
105128

106129
/**
@@ -110,7 +133,7 @@ abstract class RenderToCall extends MethodCall {
110133
RenderToCall() { this.getMethodName() = ["render_to_body", "render_to_string"] }
111134
}
112135

113-
// A call to `render_to` from within a template or view component.
136+
// A call to `render_to` from within a template.
114137
private class ActionViewRenderToCall extends ActionViewContextCall, RenderToCall { }
115138

116139
/**
@@ -122,7 +145,12 @@ private class ActionViewRenderToCall extends ActionViewContextCall, RenderToCall
122145
class LinkToCall extends ActionViewContextCall {
123146
LinkToCall() { this.getMethodName() = "link_to" }
124147

125-
// TODO: the path can also be specified through other optional arguments
126-
Expr getPathArgument() { result = this.getArgument(1) }
148+
Expr getPathArgument() {
149+
// When `link_to` is called with a block, it uses the first argument as the
150+
// path, and otherwise the second argument.
151+
exists(this.getBlock()) and result = this.getArgument(0)
152+
or
153+
not exists(this.getBlock()) and result = this.getArgument(1)
154+
}
127155
}
128156
// TODO: model flow in/out of template files properly,

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

Lines changed: 200 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,200 @@
1+
private import ruby
2+
private import codeql.ruby.DataFlow
3+
private import codeql.ruby.CFG
4+
private import codeql.ruby.Concepts
5+
private import codeql.ruby.Frameworks
6+
private import codeql.ruby.frameworks.ActionController
7+
private import codeql.ruby.frameworks.ActionView
8+
private import codeql.ruby.dataflow.RemoteFlowSources
9+
private import codeql.ruby.dataflow.BarrierGuards
10+
import codeql.ruby.dataflow.internal.DataFlowDispatch
11+
private import codeql.ruby.typetracking.TypeTracker
12+
13+
/**
14+
* Provides default sources, sinks and sanitizers for detecting
15+
* "reflected server-side cross-site scripting"
16+
* vulnerabilities, as well as extension points for adding your own.
17+
*/
18+
module ReflectedXSS {
19+
/**
20+
* A data flow source for "reflected server-side cross-site scripting" vulnerabilities.
21+
*/
22+
abstract class Source extends DataFlow::Node { }
23+
24+
/**
25+
* A data flow sink for "reflected server-side cross-site scripting" vulnerabilities.
26+
*/
27+
abstract class Sink extends DataFlow::Node { }
28+
29+
/**
30+
* A sanitizer for "reflected server-side cross-site scripting" vulnerabilities.
31+
*/
32+
abstract class Sanitizer extends DataFlow::Node { }
33+
34+
/**
35+
* A sanitizer guard for "reflected server-side cross-site scripting" vulnerabilities.
36+
*/
37+
abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
38+
39+
/**
40+
* A source of remote user input, considered as a flow source.
41+
*/
42+
class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
43+
44+
private class ErbOutputMethodCallArgumentNode extends DataFlow::Node {
45+
private MethodCall call;
46+
47+
ErbOutputMethodCallArgumentNode() {
48+
exists(ErbOutputDirective d |
49+
call = d.getTerminalStmt() and
50+
this.asExpr().getExpr() = call.getAnArgument()
51+
)
52+
}
53+
54+
MethodCall getCall() { result = call }
55+
}
56+
57+
/**
58+
* An `html_safe` call marking the output as not requiring HTML escaping,
59+
* considered as a flow sink.
60+
*/
61+
class HtmlSafeCallAsSink extends Sink {
62+
HtmlSafeCallAsSink() {
63+
exists(HtmlSafeCall c, ErbOutputDirective d |
64+
this.asExpr().getExpr() = c.getReceiver() and
65+
c = d.getTerminalStmt()
66+
)
67+
}
68+
}
69+
70+
/**
71+
* An argument to a call to the `raw` method, considered as a flow sink.
72+
*/
73+
class RawCallArgumentAsSink extends Sink, ErbOutputMethodCallArgumentNode {
74+
RawCallArgumentAsSink() { this.getCall() instanceof RawCall }
75+
}
76+
77+
/**
78+
* A argument to a call to the `link_to` method, which does not expect
79+
* unsanitized user-input, considered as a flow sink.
80+
*/
81+
class LinkToCallArgumentAsSink extends Sink, ErbOutputMethodCallArgumentNode {
82+
LinkToCallArgumentAsSink() {
83+
this.asExpr().getExpr() = this.getCall().(LinkToCall).getPathArgument()
84+
}
85+
}
86+
87+
/**
88+
* An HTML escaping, considered as a sanitizer.
89+
*/
90+
class HtmlEscapingAsSanitizer extends Sanitizer {
91+
HtmlEscapingAsSanitizer() { this = any(HtmlEscaping esc).getOutput() }
92+
}
93+
94+
/**
95+
* A comparison with a constant string, considered as a sanitizer-guard.
96+
*/
97+
class StringConstCompareAsSanitizerGuard extends SanitizerGuard, StringConstCompare { }
98+
99+
/**
100+
* An inclusion check against an array of constant strings, considered as a sanitizer-guard.
101+
*/
102+
class StringConstArrayInclusionCallAsSanitizerGuard extends SanitizerGuard,
103+
StringConstArrayInclusionCall { }
104+
105+
/**
106+
* A `VariableWriteAccessCfgNode` that is not succeeded (locally) by another
107+
* write to that variable.
108+
*/
109+
private class FinalInstanceVarWrite extends CfgNodes::ExprNodes::InstanceVariableWriteAccessCfgNode {
110+
private InstanceVariable var;
111+
112+
FinalInstanceVarWrite() {
113+
var = this.getExpr().getVariable() and
114+
not exists(CfgNodes::ExprNodes::InstanceVariableWriteAccessCfgNode succWrite |
115+
succWrite.getExpr().getVariable() = var
116+
|
117+
succWrite = this.getASuccessor+()
118+
)
119+
}
120+
121+
InstanceVariable getVariable() { result = var }
122+
123+
AssignExpr getAnAssignExpr() { result.getLeftOperand() = this.getExpr() }
124+
}
125+
126+
/**
127+
* An additional step that is taint-preserving in the context of reflected XSS.
128+
*/
129+
predicate isAdditionalXSSTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
130+
// node1 is a `locals` argument to a render call...
131+
exists(RenderCall call, Pair kvPair, string hashKey |
132+
call.getLocals().getAKeyValuePair() = kvPair and
133+
kvPair.getValue() = node1.asExpr().getExpr() and
134+
kvPair.getKey().(StringlikeLiteral).getValueText() = hashKey and
135+
// `node2` appears in the rendered template file
136+
call.getTemplateFile() = node2.getLocation().getFile() and
137+
(
138+
// node2 is an element reference against `local_assigns`
139+
exists(
140+
CfgNodes::ExprNodes::ElementReferenceCfgNode refNode, DataFlow::Node argNode,
141+
CfgNodes::ExprNodes::StringlikeLiteralCfgNode strNode
142+
|
143+
refNode = node2.asExpr() and
144+
argNode.asExpr() = refNode.getArgument(0) and
145+
refNode.getReceiver().getExpr().(MethodCall).getMethodName() = "local_assigns" and
146+
argNode.getALocalSource() = DataFlow::exprNode(strNode) and
147+
strNode.getExpr().getValueText() = hashKey
148+
)
149+
or
150+
// ...node2 is a "method call" to a "method" with `hashKey` as its name
151+
// TODO: This may be a variable read in reality that we interpret as a method call
152+
exists(MethodCall varAcc |
153+
varAcc = node2.asExpr().(CfgNodes::ExprNodes::MethodCallCfgNode).getExpr() and
154+
varAcc.getMethodName() = hashKey
155+
)
156+
)
157+
)
158+
or
159+
// instance variables in the controller
160+
exists(
161+
ActionControllerActionMethod action, VariableReadAccess viewVarRead, AssignExpr ae,
162+
FinalInstanceVarWrite controllerVarWrite
163+
|
164+
viewVarRead = node2.asExpr().(CfgNodes::ExprNodes::VariableReadAccessCfgNode).getExpr() and
165+
action.getDefaultTemplateFile() = viewVarRead.getLocation().getFile() and
166+
// match read to write on variable name
167+
viewVarRead.getVariable().getName() = controllerVarWrite.getVariable().getName() and
168+
// propagate taint from assignment RHS expr to variable read access in view
169+
ae = controllerVarWrite.getAnAssignExpr() and
170+
node1.asExpr().getExpr() = ae.getRightOperand() and
171+
ae.getParent+() = action
172+
)
173+
or
174+
// flow from template into controller helper method
175+
exists(
176+
ErbFile template, ActionControllerHelperMethod helperMethod,
177+
CfgNodes::ExprNodes::MethodCallCfgNode helperMethodCall, int argIdx
178+
|
179+
template = node1.getLocation().getFile() and
180+
helperMethod.getName() = helperMethodCall.getExpr().getMethodName() and
181+
helperMethod.getControllerClass() = getAssociatedControllerClass(template) and
182+
helperMethodCall.getArgument(argIdx) = node1.asExpr() and
183+
helperMethod.getParameter(argIdx) = node2.asExpr().getExpr()
184+
)
185+
or
186+
// flow out of controller helper method into template
187+
exists(
188+
ErbFile template, ActionControllerHelperMethod helperMethod,
189+
CfgNodes::ExprNodes::MethodCallCfgNode helperMethodCall
190+
|
191+
template = node2.getLocation().getFile() and
192+
helperMethod.getName() = helperMethodCall.getExpr().getMethodName() and
193+
helperMethod.getControllerClass() = getAssociatedControllerClass(template) and
194+
// `node1` is an expr node that may be returned by the helper method
195+
exprNodeReturnedFrom(node1, helperMethod) and
196+
// `node2` is a call to the helper method
197+
node2.asExpr() = helperMethodCall
198+
)
199+
}
200+
}

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

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
/**
2+
* Provides a taint-tracking configuration for detecting "reflected server-side cross-site scripting" vulnerabilities.
3+
*
4+
* Note, for performance reasons: only import this file if
5+
* `ReflectedXSS::Configuration` is needed, otherwise
6+
* `ReflectedXSSCustomizations` should be imported instead.
7+
*/
8+
9+
private import ruby
10+
import codeql.ruby.DataFlow
11+
import codeql.ruby.TaintTracking
12+
13+
/**
14+
* Provides a taint-tracking configuration for detecting "reflected server-side cross-site scripting" vulnerabilities.
15+
*/
16+
module ReflectedXSS {
17+
import ReflectedXSSCustomizations::ReflectedXSS
18+
19+
/**
20+
* A taint-tracking configuration for detecting "reflected server-side cross-site scripting" vulnerabilities.
21+
*/
22+
class Configuration extends TaintTracking::Configuration {
23+
Configuration() { this = "ReflectedXSS" }
24+
25+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
26+
27+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
28+
29+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
30+
31+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
32+
guard instanceof SanitizerGuard
33+
}
34+
35+
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
36+
isAdditionalXSSTaintStep(node1, node2)
37+
}
38+
}
39+
}

0 commit comments

Comments
 (0)