Skip to content

Commit 8cc6617

Browse files
atorralbaatorralba
committed
Add path injection query
1 parent 0f87eb4 commit 8cc6617

File tree

11 files changed

+453
-0
lines changed

11 files changed

+453
-0
lines changed

‎swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,10 +82,12 @@ private module Frameworks {
8282
private import codeql.swift.frameworks.StandardLibrary.Data
8383
private import codeql.swift.frameworks.StandardLibrary.InputStream
8484
private import codeql.swift.frameworks.StandardLibrary.NSData
85+
private import codeql.swift.frameworks.StandardLibrary.NsUrl
8586
private import codeql.swift.frameworks.StandardLibrary.String
8687
private import codeql.swift.frameworks.StandardLibrary.Url
8788
private import codeql.swift.frameworks.StandardLibrary.UrlSession
8889
private import codeql.swift.frameworks.StandardLibrary.WebView
90+
private import codeql.swift.security.PathInjection
8991
}
9092

9193
/**

‎swift/ql/lib/codeql/swift/frameworks/StandardLibrary/NsUrl.qll

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
import swift
2+
private import codeql.swift.dataflow.DataFlow
3+
private import codeql.swift.dataflow.ExternalFlow
4+
private import codeql.swift.dataflow.FlowSteps
5+
6+
/**
7+
* A model for `NSURL` members that permit taint flow.
8+
*/
9+
private class NsUrlSummaries extends SummaryModelCsv {
10+
override predicate row(string row) {
11+
row =
12+
[
13+
";NSURL;true;init(string:);(String);;Argument[0];ReturnValue;taint",
14+
// TODO
15+
]
16+
}
17+
}

‎swift/ql/lib/codeql/swift/security/PathInjection.qll

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
/** Provides classes and predicates to reason about path injection vulnerabilities. */
2+
3+
import swift
4+
private import codeql.swift.dataflow.DataFlow
5+
private import codeql.swift.dataflow.ExternalFlow
6+
7+
/** A data flow sink for path injection vulnerabilities. */
8+
abstract class PathInjectionSink extends DataFlow::Node { }
9+
10+
/** A sanitizer for path injection vulnerabilities. */
11+
abstract class PathInjectionSanitizer extends DataFlow::Node { }
12+
13+
/**
14+
* A unit class for adding additional taint steps.
15+
*
16+
* Extend this class to add additional taint steps that should apply to paths related to
17+
* path injection vulnerabilities.
18+
*/
19+
class PathInjectionAdditionalTaintStep extends Unit {
20+
/**
21+
* Holds if the step from `node1` to `node2` should be considered a taint
22+
* step for paths related to path injection vulnerabilities.
23+
*/
24+
abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
25+
}
26+
27+
private class DefaultPathInjectionSink extends PathInjectionSink {
28+
DefaultPathInjectionSink() { sinkNode(this, "path-injection") }
29+
}
30+
31+
private class DefaultPathInjectionSanitizer extends PathInjectionSanitizer {
32+
DefaultPathInjectionSanitizer() { none() } // TODO: Implement a proper path sanitizer
33+
}
34+
35+
private class PathInjectionSinks extends SinkModelCsv {
36+
override predicate row(string row) {
37+
row =
38+
[
39+
";Data;true;write(to:options:);;;Argument[0];path-injection",
40+
";NSData;true;write(to:atomically:);;;Argument[0];path-injection",
41+
";NSData;true;write(to:options:);;;Argument[0];path-injection",
42+
";NSData;true;write(toFile:atomically:);;;Argument[0];path-injection",
43+
";NSData;true;write(toFile:options:);;;Argument[0];path-injection",
44+
";FileManager;true;contentsOfDirectory(at:includingPropertiesForKeys:options:);;;Argument[0];path-injection",
45+
";FileManager;true;contentsOfDirectory(atPath:);;;Argument[0];path-injection",
46+
";FileManager;true;enumerator(at:includingPropertiesForKeys:options:errorHandler:);;;Argument[0];path-injection",
47+
";FileManager;true;enumerator(atPath:);;;Argument[0];path-injection",
48+
";FileManager;true;subpathsOfDirectory(atPath:);;;Argument[0];path-injection",
49+
";FileManager;true;subpaths(atPath:);;;Argument[0];path-injection",
50+
";FileManager;true;createDirectory(at:withIntermediateDirectories:attributes:);;;Argument[0];path-injection",
51+
";FileManager;true;createDirectory(atPath:withIntermediateDirectories:attributes:);;;Argument[0];path-injection",
52+
";FileManager;true;createFile(atPath:contents:attributes:);;;Argument[0];path-injection",
53+
";FileManager;true;removeItem(at:);;;Argument[0];path-injection",
54+
";FileManager;true;removeItem(atPath:);;;Argument[0];path-injection",
55+
";FileManager;true;trashItem(at:resultingItemURL:);;;Argument[0];path-injection",
56+
";FileManager;true;replaceItem(at:withItemAt:backupItemName:options:resultingItemURL:);;;Argument[0..1];path-injection",
57+
";FileManager;true;replaceItemAt(_:withItemAt:backupItemName:options:);;;Argument[0..1];path-injection",
58+
";FileManager;true;replaceItemAt(_:withItemAt:backupItemName:options:resultingItemURL:);;;Argument[0..1];path-injection",
59+
";FileManager;true;copyItem(at:to:);;;Argument[0..1];path-injection",
60+
";FileManager;true;copyItem(atPath:toPath:);;;Argument[0..1];path-injection",
61+
";FileManager;true;moveItem(at:to:);;;Argument[0..1];path-injection",
62+
";FileManager;true;moveItem(atPath:toPath:);;;Argument[0..1];path-injection",
63+
";FileManager;true;createSymbolicLink(at:withDestinationURL:);;;Argument[0..1];path-injection",
64+
";FileManager;true;createSymbolicLink(atPath:withDestinationPath:);;;Argument[0..1];path-injection",
65+
";FileManager;true;linkItem(at:to:);;;Argument[0..1];path-injection",
66+
";FileManager;true;linkItem(atPath:toPath:);;;Argument[0..1];path-injection",
67+
";FileManager;true;destinationOfSymbolicLink(atPath:);;;Argument[0];path-injection",
68+
";FileManager;true;fileExists(atPath:);;;Argument[0];path-injection",
69+
";FileManager;true;fileExists(atPath:isDirectory:);;;Argument[0];path-injection",
70+
";FileManager;true;isReadableFile(atPath:);;;Argument[0];path-injection",
71+
";FileManager;true;isWritableFile(atPath:);;;Argument[0];path-injection",
72+
";FileManager;true;isDeletableFile(atPath:);;;Argument[0];path-injection",
73+
";FileManager;true;componentsToDisplay(forPath:);;;Argument[0];path-injection",
74+
";FileManager;true;displayName(atPath:);;;Argument[0];path-injection",
75+
";FileManager;true;attributesOfItem(atPath:);;;Argument[0];path-injection",
76+
";FileManager;true;attributesOfFileSystem(forPath:);;;Argument[0];path-injection",
77+
";FileManager;true;setAttributes(_:ofItemAtPath:);;;Argument[1];path-injection",
78+
";FileManager;true;contents(atPath:);;;Argument[0];path-injection",
79+
";FileManager;true;contentsEqual(atPath:andPath:);;;Argument[0..1];path-injection",
80+
";FileManager;true;getRelationship(_:ofDirectoryAt:toItemAt:);;;Argument[1..2];path-injection",
81+
";FileManager;true;getRelationship(_:of:in:toItemAt:);;;Argument[3];path-injection",
82+
";FileManager;true;changeCurrentDirectoryPath(_:);;;Argument[0];path-injection",
83+
";FileManager;true;unmountVolume(at:options:completionHandler:);;;Argument[0];path-injection",
84+
";FileManager;true;NSHFSTypeOfFile(_:);;;Argument[0];path-injection",
85+
// Deprecated FileManager methods:
86+
";FileManager;true;changeFileAttributes(_:atPath:);;;Argument[1];path-injection",
87+
";FileManager;true;fileAttributes(atPath:traverseLink:);;;Argument[0];path-injection",
88+
";FileManager;true;fileSystemAttributes(atPath:);;;Argument[0];path-injection",
89+
";FileManager;true;directoryContents(atPath:);;;Argument[0];path-injection",
90+
";FileManager;true;createDirectory(atPath:attributes:);;;Argument[0];path-injection",
91+
";FileManager;true;createSymbolicLink(atPath:pathContent:);;;Argument[0..1];path-injection",
92+
";FileManager;true;pathContentOfSymbolicLink(atPath:);;;Argument[0];path-injection",
93+
";FileManager;true;replaceItemAtURL(originalItemURL:withItemAtURL:backupItemName:options:);;;Argument[0..1];path-injection",
94+
";NIOFileHandle;true;init(descriptor:);;;Argument[0];path-injection",
95+
";NIOFileHandle;true;init(path:mode:flags:);;;Argument[0];path-injection",
96+
";NIOFileHandle;true;init(path:);;;Argument[0];path-injection"
97+
]
98+
}
99+
}

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
/**
2+
* Provides a taint-tracking configuration for reasoning about path injection
3+
* vulnerabilities.
4+
*/
5+
6+
import swift
7+
private import codeql.swift.dataflow.DataFlow
8+
private import codeql.swift.dataflow.ExternalFlow
9+
private import codeql.swift.dataflow.FlowSources
10+
private import codeql.swift.dataflow.TaintTracking
11+
private import codeql.swift.security.PathInjection
12+
13+
/**
14+
* A taint-tracking configuration for path injection vulnerabilities.
15+
*/
16+
class PathInjectionConfiguration extends TaintTracking::Configuration {
17+
PathInjectionConfiguration() { this = "PathInjectionConfiguration" }
18+
19+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
20+
21+
override predicate isSink(DataFlow::Node sink) { sink instanceof PathInjectionSink }
22+
23+
override predicate isSanitizer(DataFlow::Node sanitizer) {
24+
sanitizer instanceof PathInjectionSanitizer
25+
}
26+
27+
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
28+
any(PathInjectionAdditionalTaintStep s).step(node1, node2)
29+
}
30+
}

‎swift/ql/src/queries/Security/CWE-022/PathInjection.qhelp

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>Accessing paths controlled by users can allow an attacker to access unexpected resources. This
8+
can result in sensitive information being revealed or deleted, or an attacker being able to influence
9+
behavior by modifying unexpected files.</p>
10+
11+
<p>Paths that are naively constructed from data controlled by a user may contain unexpected special characters,
12+
such as "..". Such a path may potentially point to any directory on the file system.</p>
13+
</overview>
14+
15+
<recommendation>
16+
17+
<p>Validate user input before using it to construct a file path. Ideally, follow these rules:</p>
18+
19+
<ul>
20+
<li>Do not allow more than a single "." character.</li>
21+
<li>Do not allow directory separators such as "/" or "\" (depending on the file system).</li>
22+
<li>Do not rely on simply replacing problematic sequences such as "../". For example, after applying this filter to
23+
".../...//" the resulting string would still be "../".</li>
24+
<li>Ideally use a whitelist of known good patterns.</li>
25+
</ul>
26+
27+
</recommendation>
28+
29+
<example>
30+
<p>
31+
In the first example, a file name is read from an HTTP request and then used to access a file.
32+
However, a malicious response could include a file name that is an absolute path, such as
33+
<code>"/Applications/(current_application)/Documents/sensitive.data"</code>.
34+
</p>
35+
36+
<p>
37+
In the second example, it appears that the user is restricted to opening a file within the
38+
<code>"/Library/Caches"</code> home directory. However, a malicious response could contain a file name containing
39+
special characters. For example, the string <code>"../../Documents/sensitive.data"</code> will result in the code
40+
reading the file located at <code>"/Applications/(current_application)/Library/Caches/../../Documents/sensitive.data"</code>,
41+
which contains users' sensitive data. This file may then be made accesible to an attacker, giving them access to all this data.
42+
</p>
43+
44+
<sample src="PathInjectionBad.swift" />
45+
46+
<p>
47+
In the third example, the path used to access the file system is normalized <em>before</em> being checked against a
48+
known prefix. This ensures that regardless of the user input, the resulting path is safe.
49+
</p>
50+
51+
<sample src="PathInjectionGood.swift" />
52+
53+
</example>
54+
55+
<references>
56+
<li>OWASP: <a href="https://owasp.org/www-community/attacks/Path_Traversal">Path Traversal</a>.</li>
57+
</references>
58+
</qhelp>

‎swift/ql/src/queries/Security/CWE-022/PathInjection.ql

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

‎swift/ql/src/queries/Security/CWE-022/PathInjectionBad.swift

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
let fm = FileManager.default
2+
let path = try String(contentsOf: URL(string: "http://example.com/")!)
3+
4+
// BAD
5+
return fm.contents(atPath: path)
6+
7+
// BAD
8+
if (path.hasPrefix(NSHomeDirectory() + "/Library/Caches")) {
9+
return fm.contents(atPath: path)
10+
}

‎swift/ql/src/queries/Security/CWE-022/PathInjectionGood.swift

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
let fm = FileManager.default
2+
let path = try String(contentsOf: URL(string: "http://example.com/")!)
3+
4+
// GOOD
5+
let filePath = FilePath(stringLiteral: path)
6+
if (filePath.lexicallyNormalized().starts(with: FilePath(stringLiteral: NSHomeDirectory() + "/Library/Caches"))) {
7+
return fm.contents(atPath: path)
8+
}

‎swift/ql/test/query-tests/Security/CWE-022/PathInjectionTest.expected

Whitespace-only changes.

‎swift/ql/test/query-tests/Security/CWE-022/PathInjectionTest.ql

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import swift
2+
import codeql.swift.dataflow.DataFlow
3+
import codeql.swift.dataflow.FlowSources
4+
import codeql.swift.security.PathInjectionQuery
5+
import TestUtilities.InlineExpectationsTest
6+
7+
class PathInjectionTest extends InlineExpectationsTest {
8+
PathInjectionTest() { this = "PathInjectionTest" }
9+
10+
override string getARelevantTag() { result = "hasPathInjection" }
11+
12+
override predicate hasActualResult(Location location, string element, string tag, string value) {
13+
exists(
14+
PathInjectionConfiguration config, DataFlow::Node source, DataFlow::Node sink, Expr sinkExpr
15+
|
16+
config.hasFlow(source, sink) and
17+
sinkExpr = sink.asExpr() and
18+
location = sinkExpr.getLocation() and
19+
element = sinkExpr.toString() and
20+
tag = "hasPathInjection" and
21+
value = source.asExpr().getLocation().getStartLine().toString()
22+
)
23+
}
24+
}

0 commit comments

Comments
 (0)