Skip to content

Commit 8251ad5

Browse files
erik-krogherik-krogh
committed
add unsafe-html-construction query
1 parent 2e4f4c6 commit 8251ad5

11 files changed

+300
-0
lines changed

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

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for reasoning about
3+
* HTML constructed from library input vulnerabilities, as
4+
* well as extension points for adding your own.
5+
*/
6+
7+
private import ruby
8+
private import codeql.ruby.ApiGraphs
9+
private import codeql.ruby.frameworks.core.Gem::Gem as Gem
10+
11+
/**
12+
* Module containing sources, sinks, and sanitizers for HTML constructed from library input.
13+
*/
14+
module UnsafeHtmlConstruction {
15+
/** A source for HTML constructed from library input vulnerabilities. */
16+
abstract class Source extends DataFlow::Node { }
17+
18+
/** An input parameter to a gem seen as a source. */
19+
private class LibraryInputAsSource extends Source instanceof DataFlow::ParameterNode {
20+
LibraryInputAsSource() { this = Gem::getALibraryInput() }
21+
}
22+
23+
/** A sink for HTML constructed from library input vulnerabilities. */
24+
abstract class Sink extends DataFlow::Node {
25+
/**
26+
* Gets the node for the XSS sink the HTML flows to.
27+
*/
28+
abstract DataFlow::Node getXssSink();
29+
30+
/**
31+
* Gets the type of sink.
32+
*/
33+
abstract string getSinkType();
34+
}
35+
36+
private import codeql.ruby.security.XSS::ReflectedXss as ReflectedXss
37+
38+
/** Gets a node that eventually ends up in the XSS `sink`. */
39+
DataFlow::Node getANodeThatEndsInXssSink(ReflectedXss::Sink sink) {
40+
result = getANodeThatEndsInXssSink(TypeTracker::TypeBackTracker::end(), sink)
41+
}
42+
43+
private import codeql.ruby.typetracking.TypeTracker as TypeTracker
44+
45+
/** Gets a node that is eventually ends up in the XSS `sink`, type-tracked with `t`. */
46+
private DataFlow::LocalSourceNode getANodeThatEndsInXssSink(
47+
TypeTracker::TypeBackTracker t, ReflectedXss::Sink sink
48+
) {
49+
t.start() and
50+
result = sink.getALocalSource()
51+
or
52+
exists(TypeTracker::TypeBackTracker t2 |
53+
result = getANodeThatEndsInXssSink(t2, sink).backtrack(t2, t)
54+
)
55+
}
56+
57+
/**
58+
* A string constructed from a string-literal (e.g. `"foo #{sink}"`),
59+
* where the resulting string ends up being used in an XSS sink.
60+
*/
61+
class StringFormatAsSink extends Sink {
62+
ReflectedXss::Sink s;
63+
64+
StringFormatAsSink() {
65+
exists(Ast::StringlikeLiteral lit |
66+
any(DataFlow::Node n | n.asExpr().getExpr() = lit) = getANodeThatEndsInXssSink(s) and
67+
this.asExpr().getExpr() = lit.getComponent(_)
68+
)
69+
}
70+
71+
override DataFlow::Node getXssSink() { result = s }
72+
73+
override string getSinkType() { result = "string interpolation" }
74+
}
75+
76+
import codeql.ruby.security.TaintedFormatStringSpecific as TaintedFormat
77+
78+
/**
79+
* A string constructed from a printf-style call,
80+
* where the resulting string ends up being used in an XSS sink.
81+
*/
82+
class TaintedFormatStringAsSink extends Sink {
83+
ReflectedXss::Sink s;
84+
85+
TaintedFormatStringAsSink() {
86+
exists(TaintedFormat::PrintfStyleCall call |
87+
call = getANodeThatEndsInXssSink(s) and
88+
this = [call.getFormatArgument(_), call.getFormatString()]
89+
)
90+
}
91+
92+
override DataFlow::Node getXssSink() { result = s }
93+
94+
override string getSinkType() { result = "string format" }
95+
}
96+
}

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

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
/**
2+
* Provides a taint-tracking configuration for reasoning about HTML
3+
* constructed from library input vulnerabilities.
4+
*
5+
* Note, for performance reasons: only import this file if `Configuration` is needed,
6+
* otherwise `UnsafeHtmlConstructionCustomizations` should be imported instead.
7+
*/
8+
9+
import codeql.ruby.DataFlow
10+
import UnsafeHtmlConstructionCustomizations::UnsafeHtmlConstruction
11+
private import codeql.ruby.TaintTracking
12+
private import codeql.ruby.dataflow.BarrierGuards
13+
14+
/**
15+
* A taint-tracking configuration for detecting unsafe HTML construction.
16+
*/
17+
class Configuration extends TaintTracking::Configuration {
18+
Configuration() { this = "UnsafeShellCommandConstruction" }
19+
20+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
21+
22+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
23+
24+
override predicate isSanitizer(DataFlow::Node node) {
25+
node instanceof StringConstCompareBarrier or
26+
node instanceof StringConstArrayInclusionCallBarrier
27+
}
28+
29+
// override to require the path doesn't have unmatched return steps
30+
override DataFlow::FlowFeature getAFeature() {
31+
result instanceof DataFlow::FeatureHasSourceCallContext
32+
}
33+
34+
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
35+
// if an array element gets tainted, then we treat the entire array as tainted
36+
exists(DataFlow::CallNode call |
37+
call.getMethodName() = ["<<", "push", "append"] and
38+
call.getReceiver() = succ and
39+
pred = call.getArgument(0) and
40+
call.getNumberOfArguments() = 1
41+
)
42+
or
43+
exists(DataFlow::CallNode call |
44+
call.getMethodName() = "[]" and
45+
succ = call and
46+
pred = call.getArgument(_)
47+
)
48+
}
49+
}

‎ruby/ql/src/change-notes/2022-11-28-unsafe-html-construction.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new query, `rb/html-constructed-from-input`, to detect libraries that unsafely construct HTML from their inputs.

‎ruby/ql/src/queries/security/cwe-079/UnsafeHtmlConstruction.qhelp

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
When a library function dynamically constructs HTML in a potentially unsafe
8+
way, then it's important to document to clients of the library that the function
9+
should only be used with trusted inputs.
10+
11+
If the function is not documented as being potentially unsafe, then a client
12+
may inadvertently use inputs containing unsafe HTML fragments, and thereby leave
13+
the client vulnerable to cross-site scripting attacks.
14+
</p>
15+
</overview>
16+
<recommendation>
17+
18+
<p>
19+
Document all library functions that can lead to cross-site scripting
20+
attacks, and guard against unsafe inputs where dynamic HTML
21+
construction is not intended.
22+
</p>
23+
</recommendation>
24+
<example>
25+
26+
<p>
27+
The following example has a library function that renders a boldface name
28+
by creating a string containing a <code>&lt;b&gt;</code> with the name
29+
embedded in it.
30+
</p>
31+
32+
<sample src="examples/unsafe-html-construction.rb" />
33+
34+
<p>
35+
This library function, however, does not escape unsafe HTML, and a client
36+
that calls the function with user-supplied input may be vulnerable to
37+
cross-site scripting attacks.
38+
</p>
39+
40+
<p>
41+
The library could either document that this function should not be used
42+
with unsafe inputs, or escape the input before embedding it in the
43+
HTML fragment.
44+
</p>
45+
46+
<sample src="examples/unsafe-html-construction_safe.rb" />
47+
48+
</example>
49+
<references>
50+
<li>
51+
OWASP:
52+
<a href="https://www.owasp.org/index.php/DOM_based_XSS_Prevention_Cheat_Sheet">DOM based
53+
XSS Prevention Cheat Sheet</a>.
54+
</li>
55+
<li>
56+
OWASP:
57+
<a href="https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet">XSS
58+
(Cross Site Scripting) Prevention Cheat Sheet</a>.
59+
</li>
60+
<li>
61+
OWASP
62+
<a href="https://www.owasp.org/index.php/DOM_Based_XSS">DOM Based XSS</a>.
63+
</li>
64+
<li>
65+
OWASP
66+
<a href="https://www.owasp.org/index.php/Types_of_Cross-Site_Scripting">Types of Cross-Site
67+
Scripting</a>.
68+
</li>
69+
<li>
70+
Wikipedia: <a href="http://en.wikipedia.org/wiki/Cross-site_scripting">Cross-site scripting</a>.
71+
</li>
72+
</references>
73+
</qhelp>

‎ruby/ql/src/queries/security/cwe-079/UnsafeHtmlConstruction.ql

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/**
2+
* @name Unsafe HTML constructed from library input
3+
* @description Using externally controlled strings to construct HTML might allow a malicious
4+
* user to perform a cross-site scripting attack.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @security-severity 6.1
8+
* @precision high
9+
* @id rb/html-constructed-from-input
10+
* @tags security
11+
* external/cwe/cwe-079
12+
* external/cwe/cwe-116
13+
*/
14+
15+
import codeql.ruby.security.UnsafeHtmlConstructionQuery
16+
import DataFlow::PathGraph
17+
18+
from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode
19+
where cfg.hasFlowPath(source, sink) and sink.getNode() = sinkNode
20+
select sinkNode, source, sink,
21+
"This " + sinkNode.getSinkType() + " which depends on $@ might later allow $@.", source.getNode(),
22+
"library input", sinkNode.getXssSink(), "cross-site scripting"

‎ruby/ql/src/queries/security/cwe-079/examples/unsafe-html-construction.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
class UsersController < ActionController::Base
2+
# BAD - create a user description, where the name is not escaped
3+
def create_user_description (name)
4+
"<h2>#{name}</h2>".html_safe
5+
end
6+
end

‎ruby/ql/src/queries/security/cwe-079/examples/unsafe-html-construction_safe.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
class UsersController < ActionController::Base
2+
# Good - create a user description, where the name is escaped
3+
def create_user_description (name)
4+
"<h2>#{ERB::Util.html_escape(name)}</h2>".html_safe
5+
end
6+
end

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
edges
2+
| lib/unsafeHtml.rb:2:31:2:34 | name : | lib/unsafeHtml.rb:3:10:3:16 | #{...} |
3+
| lib/unsafeHtml.rb:9:27:9:30 | name : | lib/unsafeHtml.rb:11:13:11:19 | #{...} |
4+
| lib/unsafeHtml.rb:16:19:16:22 | name : | lib/unsafeHtml.rb:17:28:17:31 | name |
5+
nodes
6+
| lib/unsafeHtml.rb:2:31:2:34 | name : | semmle.label | name : |
7+
| lib/unsafeHtml.rb:3:10:3:16 | #{...} | semmle.label | #{...} |
8+
| lib/unsafeHtml.rb:9:27:9:30 | name : | semmle.label | name : |
9+
| lib/unsafeHtml.rb:11:13:11:19 | #{...} | semmle.label | #{...} |
10+
| lib/unsafeHtml.rb:16:19:16:22 | name : | semmle.label | name : |
11+
| lib/unsafeHtml.rb:17:28:17:31 | name | semmle.label | name |
12+
subpaths
13+
#select
14+
| lib/unsafeHtml.rb:3:10:3:16 | #{...} | lib/unsafeHtml.rb:2:31:2:34 | name : | lib/unsafeHtml.rb:3:10:3:16 | #{...} | This string interpolation which depends on $@ might later allow $@. | lib/unsafeHtml.rb:2:31:2:34 | name | library input | lib/unsafeHtml.rb:3:5:3:22 | "<h2>#{...}</h2>" | cross-site scripting |
15+
| lib/unsafeHtml.rb:11:13:11:19 | #{...} | lib/unsafeHtml.rb:9:27:9:30 | name : | lib/unsafeHtml.rb:11:13:11:19 | #{...} | This string interpolation which depends on $@ might later allow $@. | lib/unsafeHtml.rb:9:27:9:30 | name | library input | lib/unsafeHtml.rb:13:5:13:5 | h | cross-site scripting |
16+
| lib/unsafeHtml.rb:17:28:17:31 | name | lib/unsafeHtml.rb:16:19:16:22 | name : | lib/unsafeHtml.rb:17:28:17:31 | name | This string format which depends on $@ might later allow $@. | lib/unsafeHtml.rb:16:19:16:22 | name | library input | lib/unsafeHtml.rb:17:5:17:32 | call to sprintf | cross-site scripting |

‎ruby/ql/test/query-tests/security/cwe-079/UnsafeHtmlConstruction.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
queries/security/cwe-079/UnsafeHtmlConstruction.ql

‎ruby/ql/test/query-tests/security/cwe-079/lib/unsafeHtml.rb

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
class Foobar
2+
def create_user_description(name)
3+
"<h2>#{name}</h2>".html_safe # NOT OK - the parameter is not escaped
4+
5+
# escape
6+
"<h2>#{ERB::Util.html_escape(name)}</h2>".html_safe # OK - the parameter is escaped
7+
end
8+
9+
def string_like_literal name
10+
h = <<-HTML
11+
<h2>#{name}</h2>
12+
HTML
13+
h.html_safe # NOT OK - the parameter is not escaped
14+
end
15+
16+
def sprintf_use name
17+
sprintf("<h2>%s</h2>", name).html_safe # NOT OK - the parameter is not escaped
18+
19+
# escape
20+
sprintf("<h2>%s</h2>", ERB::Util.html_escape(name)).html_safe # OK - the parameter is escaped
21+
end
22+
end

0 commit comments

Comments
 (0)