Skip to content

Commit f166880

Browse files
erik-krogherik-krogh
committed
add a rb/unsafe-code-construction query
rebase
1 parent 5f6cb16 commit f166880

File tree

10 files changed

+267
-0
lines changed

10 files changed

+267
-0
lines changed

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

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

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/**
2+
* Provides a taint-tracking configuration for reasoning about code
3+
* constructed from library input vulnerabilities.
4+
*
5+
* Note, for performance reasons: only import this file if `Configuration` is needed,
6+
* otherwise `UnsafeCodeConstructionCustomizations` should be imported instead.
7+
*/
8+
9+
import codeql.ruby.DataFlow
10+
import UnsafeCodeConstructionCustomizations::UnsafeCodeConstruction
11+
private import codeql.ruby.TaintTracking
12+
private import codeql.ruby.dataflow.BarrierGuards
13+
14+
/**
15+
* A taint-tracking configuration for detecting shell command constructed from library input vulnerabilities.
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+
}

‎ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
When a library function dynamically constructs code in a potentially unsafe way, then
9+
it's important to document to clients of the library that the function should only be
10+
used with trusted inputs.
11+
12+
If the function is not documented as being potentially unsafe, then a client may
13+
incorrectly use inputs containing unsafe code fragments, and thereby leave the
14+
client vulnerable to code-injection attacks.
15+
</p>
16+
17+
</overview>
18+
19+
<recommendation>
20+
<p>
21+
Properly document library functions that construct code from unsanitized
22+
inputs, or avoid constructing code in the first place.
23+
</p>
24+
</recommendation>
25+
26+
<example>
27+
<p>
28+
The following example shows two methods implemented using `eval`: a simple
29+
deserialization routine and a getter method.
30+
If untrusted inputs are used with these methods,
31+
then an attacker might be able to execute arbitrary code on the system.
32+
</p>
33+
34+
<sample src="examples/UnsafeCodeConstruction.rb" />
35+
36+
<p>
37+
To avoid this problem, either properly document that the function is potentially
38+
unsafe, or use an alternative solution such as `JSON.parse` or another library, like in the examples below,
39+
that does not allow arbitrary code to be executed.
40+
</p>
41+
42+
<sample src="examples/UnsafeCodeConstructionSafe.rb" />
43+
44+
</example>
45+
46+
<references>
47+
<li>
48+
OWASP:
49+
<a href="https://www.owasp.org/index.php/Code_Injection">Code Injection</a>.
50+
</li>
51+
<li>
52+
Wikipedia: <a href="https://en.wikipedia.org/wiki/Code_injection">Code Injection</a>.
53+
</li>
54+
</references>
55+
</qhelp>

‎ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.ql

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* @name Unsafe code constructed from library input
3+
* @description Using externally controlled strings to construct code may allow a malicious
4+
* user to execute arbitrary code.
5+
* @kind path-problem
6+
* @problem.severity warning
7+
* @security-severity 6.1
8+
* @precision medium
9+
* @id rb/unsafe-code-construction
10+
* @tags security
11+
* external/cwe/cwe-094
12+
* external/cwe/cwe-079
13+
* external/cwe/cwe-116
14+
*/
15+
16+
import codeql.ruby.security.UnsafeCodeConstructionQuery
17+
import DataFlow::PathGraph
18+
19+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode
20+
where cfg.hasFlowPath(source, sink) and sinkNode = sink.getNode()
21+
select sink.getNode(), source, sink,
22+
"This " + sinkNode.getSinkType() + " which depends on $@ is later $@.", source.getNode(),
23+
"library input", sinkNode.getCodeSink(), "interpreted as code"

‎ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction.rb

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
module MyLib
2+
def unsafeDeserialize(value)
3+
eval("foo = #{value}")
4+
foo
5+
end
6+
7+
def unsafeGetter(obj, path)
8+
eval("obj.#{path}")
9+
end
10+
end

‎ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstructionSafe.rb

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
require 'json'
2+
3+
module MyLib
4+
def safeDeserialize(value)
5+
JSON.parse(value)
6+
end
7+
8+
def safeGetter(obj, path)
9+
obj.dig(*path.split("."))
10+
end
11+
end

‎ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
edges
2+
| impl/unsafeCode.rb:2:12:2:17 | target : | impl/unsafeCode.rb:3:17:3:25 | #{...} |
3+
| impl/unsafeCode.rb:7:12:7:12 | x : | impl/unsafeCode.rb:8:30:8:30 | x |
4+
| impl/unsafeCode.rb:12:12:12:12 | x : | impl/unsafeCode.rb:13:33:13:33 | x |
5+
nodes
6+
| impl/unsafeCode.rb:2:12:2:17 | target : | semmle.label | target : |
7+
| impl/unsafeCode.rb:3:17:3:25 | #{...} | semmle.label | #{...} |
8+
| impl/unsafeCode.rb:7:12:7:12 | x : | semmle.label | x : |
9+
| impl/unsafeCode.rb:8:30:8:30 | x | semmle.label | x |
10+
| impl/unsafeCode.rb:12:12:12:12 | x : | semmle.label | x : |
11+
| impl/unsafeCode.rb:13:33:13:33 | x | semmle.label | x |
12+
subpaths
13+
#select
14+
| impl/unsafeCode.rb:3:17:3:25 | #{...} | impl/unsafeCode.rb:2:12:2:17 | target : | impl/unsafeCode.rb:3:17:3:25 | #{...} | This string interpolation which depends on $@ is later $@. | impl/unsafeCode.rb:2:12:2:17 | target | library input | impl/unsafeCode.rb:3:5:3:27 | call to eval | interpreted as code |
15+
| impl/unsafeCode.rb:8:30:8:30 | x | impl/unsafeCode.rb:7:12:7:12 | x : | impl/unsafeCode.rb:8:30:8:30 | x | This string format which depends on $@ is later $@. | impl/unsafeCode.rb:7:12:7:12 | x | library input | impl/unsafeCode.rb:8:5:8:32 | call to eval | interpreted as code |
16+
| impl/unsafeCode.rb:13:33:13:33 | x | impl/unsafeCode.rb:12:12:12:12 | x : | impl/unsafeCode.rb:13:33:13:33 | x | This string format which depends on $@ is later $@. | impl/unsafeCode.rb:12:12:12:12 | x | library input | impl/unsafeCode.rb:13:5:13:35 | call to eval | interpreted as code |

‎ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.qlref

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

‎ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
class Foobar
2+
def foo1(target)
3+
eval("foo = #{target}") # NOT OK
4+
end
5+
6+
# sprintf
7+
def foo2(x)
8+
eval(sprintf("foo = %s", x)) # NOT OK
9+
end
10+
11+
# String#%
12+
def foo3(x)
13+
eval("foo = %{foo}" % {foo: x}) # NOT OK
14+
end
15+
16+
def indirect_eval(x)
17+
eval(x) # OK - no construction.
18+
end
19+
end

‎ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/unsafe-code.gemspec

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Gem::Specification.new do |s|
2+
s.name = 'unsafe-code'
3+
s.require_path = "impl"
4+
end
5+

0 commit comments

Comments
 (0)