Skip to content

Commit 3851a27

Browse files
nickrolfenickrolfe
authored
Merge pull request #358 from github/external-control-file-path
Add rb/path-injection query
2 parents 4f72d08 + 61d7cde commit 3851a27

File tree

16 files changed

+436
-3
lines changed

16 files changed

+436
-3
lines changed

‎ruby/change-notes/2021-10-20-path-injection.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* A new query (`rb/path-injection`) has been added. The query finds file operations using paths that derive from user input without being sanitized.

‎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/FlowSummary.qll‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@ private import internal.FlowSummaryImpl as Impl
66
private import internal.DataFlowDispatch
77

88
// import all instances below
9-
private module Summaries { }
9+
private module Summaries {
10+
private import codeql.ruby.Frameworks
11+
}
1012

1113
class SummaryComponent = Impl::Public::SummaryComponent;
1214

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -398,7 +398,7 @@ private module ParameterNodes {
398398

399399
override DataFlowCallable getEnclosingCallable() { result = sc }
400400

401-
override Location getLocationImpl() { none() }
401+
override EmptyLocation getLocationImpl() { any() }
402402

403403
override string toStringImpl() { result = "parameter " + pos + " of " + sc }
404404
}
@@ -417,7 +417,7 @@ private class SummaryNode extends NodeImpl, TSummaryNode {
417417

418418
override DataFlowCallable getEnclosingCallable() { result = c }
419419

420-
override Location getLocationImpl() { none() }
420+
override EmptyLocation getLocationImpl() { any() }
421421

422422
override string toStringImpl() { result = "[summary] " + state + " in " + c }
423423
}

‎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: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
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+
private import codeql.ruby.dataflow.FlowSummary
6+
7+
/** Defines calls to `ActiveStorage::Filename#sanitized` as path sanitizers. */
8+
class ActiveStorageFilenameSanitizedCall extends Path::PathSanitization::Range, DataFlow::CallNode {
9+
ActiveStorageFilenameSanitizedCall() {
10+
this.getReceiver() =
11+
API::getTopLevelMember("ActiveStorage").getMember("Filename").getAnInstantiation() and
12+
this.asExpr().getExpr().(MethodCall).getMethodName() = "sanitized"
13+
}
14+
}
15+
16+
/** Taint summary for `ActiveStorage::Filename.new`. */
17+
class ActiveStorageFilenameNewSummary extends SummarizedCallable {
18+
ActiveStorageFilenameNewSummary() { this = "ActiveStorage::Filename.new" }
19+
20+
override MethodCall getACall() {
21+
result =
22+
API::getTopLevelMember("ActiveStorage")
23+
.getMember("Filename")
24+
.getAnInstantiation()
25+
.asExpr()
26+
.getExpr()
27+
}
28+
29+
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
30+
input = "Argument[0]" and
31+
output = "ReturnValue" and
32+
preservesValue = false
33+
}
34+
}
35+
36+
/** Taint summary for `ActiveStorage::Filename#sanitized`. */
37+
class ActiveStorageFilenameSanitizedSummary extends SummarizedCallable {
38+
ActiveStorageFilenameSanitizedSummary() { this = "ActiveStorage::Filename#sanitized" }
39+
40+
override MethodCall getACall() {
41+
result =
42+
API::getTopLevelMember("ActiveStorage")
43+
.getMember("Filename")
44+
.getInstance()
45+
.getAMethodCall("sanitized")
46+
.asExpr()
47+
.getExpr()
48+
}
49+
50+
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
51+
input = "Argument[-1]" and
52+
output = "ReturnValue" and
53+
preservesValue = false
54+
}
55+
}

‎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+
}

0 commit comments

Comments
 (0)