Skip to content

Commit 86da3c2

Browse files
nickrolfenickrolfe
committed
Add rb/path-injection query
1 parent 3f396ac commit 86da3c2

File tree

13 files changed

+386
-0
lines changed

13 files changed

+386
-0
lines changed

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -583,3 +583,21 @@ module OrmInstantiation {
583583
abstract predicate methodCallMayAccessField(string methodName);
584584
}
585585
}
586+
587+
/** Provides classes for modeling path-related APIs. */
588+
module Path {
589+
/**
590+
* A data-flow node that performs path sanitization. This is often needed in order
591+
* to safely access paths.
592+
*/
593+
class PathSanitization extends DataFlow::Node instanceof PathSanitization::Range { }
594+
595+
/** Provides a class for modeling new path sanitization APIs. */
596+
module PathSanitization {
597+
/**
598+
* A data-flow node that performs path sanitization. This is often needed in order
599+
* to safely access paths.
600+
*/
601+
abstract class Range extends DataFlow::Node { }
602+
}
603+
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
private import codeql.ruby.frameworks.ActionController
66
private import codeql.ruby.frameworks.ActiveRecord
7+
private import codeql.ruby.frameworks.ActiveStorage
78
private import codeql.ruby.frameworks.ActionView
89
private import codeql.ruby.frameworks.StandardLibrary
910
private import codeql.ruby.frameworks.Files

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,9 @@ predicate summaryElement(DataFlowCallable c, string input, string output, string
6464
SummaryComponent interpretComponentSpecific(string c) {
6565
c = "BlockArgument" and
6666
result = FlowSummary::SummaryComponent::block()
67+
or
68+
c = "Argument[_]" and
69+
result = FlowSummary::SummaryComponent::argument(any(int i | i >= 0))
6770
}
6871

6972
/** Gets the return kind corresponding to specification `"ReturnValue"`. */

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
private import codeql.ruby.AST
2+
private import codeql.ruby.ApiGraphs
3+
private import codeql.ruby.Concepts
4+
private import codeql.ruby.DataFlow
5+
6+
class ActiveStorageFilenameSanitizedCall extends Path::PathSanitization::Range, DataFlow::CallNode {
7+
ActiveStorageFilenameSanitizedCall() {
8+
this.getReceiver() =
9+
API::getTopLevelMember("ActiveStorage").getMember("Filename").getAnInstantiation() and
10+
this.asExpr().getExpr().(MethodCall).getMethodName() = "sanitized"
11+
}
12+
}

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

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ private import codeql.ruby.Concepts
77
private import codeql.ruby.ApiGraphs
88
private import codeql.ruby.DataFlow
99
private import codeql.ruby.frameworks.StandardLibrary
10+
private import codeql.ruby.dataflow.FlowSummary
1011

1112
private DataFlow::Node ioInstanceInstantiation() {
1213
result = API::getTopLevelMember("IO").getAnInstantiation() or
@@ -253,6 +254,47 @@ module File {
253254

254255
override DataFlow::Node getAPermissionNode() { result = permissionArg }
255256
}
257+
258+
/**
259+
* Flow summary for several methods on the `File` class that propagate taint
260+
* from their first argument to the return value.
261+
*/
262+
class FilePathConversionSummary extends SummarizedCallable {
263+
string methodName;
264+
265+
FilePathConversionSummary() {
266+
methodName = ["absolute_path", "dirname", "expand_path", "path", "realdirpath", "realpath"] and
267+
this = "File." + methodName
268+
}
269+
270+
override MethodCall getACall() {
271+
result = API::getTopLevelMember("File").getAMethodCall(methodName).asExpr().getExpr()
272+
}
273+
274+
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
275+
input = "Argument[0]" and
276+
output = "ReturnValue" and
277+
preservesValue = false
278+
}
279+
}
280+
281+
/**
282+
* Flow summary for `File.join`, which propagates taint from every argument to
283+
* its return value.
284+
*/
285+
class FileJoinSummary extends SummarizedCallable {
286+
FileJoinSummary() { this = "File.join" }
287+
288+
override MethodCall getACall() {
289+
result = API::getTopLevelMember("File").getAMethodCall("join").asExpr().getExpr()
290+
}
291+
292+
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
293+
input = "Argument[_]" and
294+
output = "ReturnValue" and
295+
preservesValue = false
296+
}
297+
}
256298
}
257299

258300
/**

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

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for reasoning about
3+
* path injection vulnerabilities, as well as extension points for
4+
* adding your own.
5+
*/
6+
7+
private import ruby
8+
private import codeql.ruby.ApiGraphs
9+
private import codeql.ruby.CFG
10+
private import codeql.ruby.Concepts
11+
private import codeql.ruby.DataFlow
12+
private import codeql.ruby.dataflow.BarrierGuards
13+
private import codeql.ruby.dataflow.RemoteFlowSources
14+
15+
module PathInjection {
16+
/**
17+
* A data flow source for path injection vulnerabilities.
18+
*/
19+
abstract class Source extends DataFlow::Node { }
20+
21+
/**
22+
* A data flow sink for path injection vulnerabilities.
23+
*/
24+
abstract class Sink extends DataFlow::Node { }
25+
26+
/**
27+
* A sanitizer guard for path injection vulnerabilities.
28+
*/
29+
abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
30+
31+
/**
32+
* A source of remote user input, considered as a flow source.
33+
*/
34+
class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
35+
36+
/**
37+
* A file system access, considered as a flow sink.
38+
*/
39+
class FileSystemAccessAsSink extends Sink {
40+
FileSystemAccessAsSink() { this = any(FileSystemAccess e).getAPathArgument() }
41+
}
42+
43+
/**
44+
* A comparison with a constant string, considered as a sanitizer-guard.
45+
*/
46+
class StringConstCompareAsSanitizerGuard extends SanitizerGuard, StringConstCompare { }
47+
48+
/**
49+
* An inclusion check against an array of constant strings, considered as a
50+
* sanitizer-guard.
51+
*/
52+
class StringConstArrayInclusionCallAsSanitizerGuard extends SanitizerGuard,
53+
StringConstArrayInclusionCall { }
54+
}

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/**
2+
* Provides a taint tracking configuration for reasoning about
3+
* path injection vulnerabilities.
4+
*
5+
* Note, for performance reasons: only import this file if
6+
* `PathInjection::Configuration` is needed, otherwise
7+
* `PathInjectionCustomizations` should be imported instead.
8+
*/
9+
10+
import PathInjectionCustomizations
11+
private import codeql.ruby.Concepts
12+
private import codeql.ruby.DataFlow
13+
private import codeql.ruby.TaintTracking
14+
15+
/**
16+
* A taint-tracking configuration for reasoning about path injection
17+
* vulnerabilities.
18+
*/
19+
class Configuration extends TaintTracking::Configuration {
20+
Configuration() { this = "PathInjection" }
21+
22+
override predicate isSource(DataFlow::Node source) { source instanceof PathInjection::Source }
23+
24+
override predicate isSink(DataFlow::Node sink) { sink instanceof PathInjection::Sink }
25+
26+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Path::PathSanitization }
27+
28+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
29+
guard instanceof PathInjection::SanitizerGuard
30+
}
31+
}

‎ruby/ql/src/queries/security/cwe-022/PathInjection.qhelp

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
Accessing files using paths constructed from user-controlled data can allow an
9+
attacker to access unexpected resources. This can result in sensitive
10+
information being revealed or deleted, or an attacker being able to influence
11+
behavior by modifying unexpected files.
12+
</p>
13+
</overview>
14+
15+
<recommendation>
16+
<p>
17+
Validate user input before using it to construct a file path, either using an
18+
off-the-shelf library like <code>ActiveStorage::Filename#sanitized</code> in
19+
Rails, or by performing custom validation.
20+
</p>
21+
22+
<p>
23+
Ideally, follow these rules:
24+
</p>
25+
26+
<ul>
27+
<li>Do not allow more than a single "." character.</li>
28+
<li>Do not allow directory separators such as "/" or "\" (depending on the file
29+
system).</li>
30+
<li>Do not rely on simply replacing problematic sequences such as "../". For
31+
example, after applying this filter to ".../...//", the resulting string would
32+
still be "../".</li>
33+
<li>Use a whitelist of known good patterns.</li>
34+
</ul>
35+
</recommendation>
36+
37+
<example>
38+
<p>
39+
In the first example, a file name is read from an HTTP request and then used to
40+
access a file. However, a malicious user could enter a file name which is an
41+
absolute path, such as <code>"/etc/passwd"</code>.
42+
</p>
43+
44+
<p>
45+
In the second example, it appears that the user is restricted to opening a file
46+
within the <code>"user"</code> home directory. However, a malicious user could
47+
enter a file name containing special characters. For example, the string
48+
<code>"../../etc/passwd"</code> will result in the code reading the file located
49+
at <code>"/home/user/../../etc/passwd"</code>, which is the system's password
50+
file. This file would then be sent back to the user, giving them access to all
51+
the system's passwords.
52+
</p>
53+
54+
<sample src="examples/tainted_path.rb" />
55+
</example>
56+
57+
<references>
58+
<li>OWASP: <a href="https://owasp.org/www-community/attacks/Path_Traversal">Path Traversal</a>.</li>
59+
<li>Rails: <a href="https://api.rubyonrails.org/classes/ActiveStorage/Filename.html#method-i-sanitized">ActiveStorage::Filename#sanitized</a>.</li>
60+
</references>
61+
</qhelp>

‎ruby/ql/src/queries/security/cwe-022/PathInjection.ql

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
/**
2+
* @name Uncontrolled data used in path expression
3+
* @description Accessing paths influenced by users can allow an attacker to access
4+
* unexpected resources.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @security-severity 7.5
8+
* @precision high
9+
* @id rb/path-injection
10+
* @tags security
11+
* external/cwe/cwe-022
12+
* external/cwe/cwe-023
13+
* external/cwe/cwe-036
14+
* external/cwe/cwe-073
15+
* external/cwe/cwe-099
16+
*/
17+
18+
import ruby
19+
import codeql.ruby.security.PathInjectionQuery
20+
import codeql.ruby.DataFlow
21+
import DataFlow::PathGraph
22+
23+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
24+
where cfg.hasFlowPath(source, sink)
25+
select sink.getNode(), source, sink, "This path depends on $@.", source.getNode(),
26+
"a user-provided value"

‎ruby/ql/src/queries/security/cwe-022/examples/tainted_path.rb

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
class FilesController < ActionController::Base
2+
def first_example
3+
# BAD: This could read any file on the file system
4+
@content = File.read params[:path]
5+
end
6+
7+
def second_example
8+
# BAD: This could still read any file on the file system
9+
@content = File.read "/home/user/#{ params[:path] }"
10+
end
11+
end

0 commit comments

Comments
 (0)