Skip to content

Commit 3eea3b2

Browse files
MathiasVPMathiasVP
authored
Merge pull request github#11446 from atorralba/atorralba/swift/path-injection
Swift: Add path injection query
2 parents 3b5b121 + 7dca1b4 commit 3eea3b2

File tree

12 files changed

+471
-0
lines changed

12 files changed

+471
-0
lines changed

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,13 +80,16 @@ private import internal.FlowSummaryImplSpecific
8080
private module Frameworks {
8181
private import codeql.swift.frameworks.StandardLibrary.CustomUrlSchemes
8282
private import codeql.swift.frameworks.StandardLibrary.Data
83+
private import codeql.swift.frameworks.StandardLibrary.FilePath
8384
private import codeql.swift.frameworks.StandardLibrary.InputStream
8485
private import codeql.swift.frameworks.StandardLibrary.NSData
86+
private import codeql.swift.frameworks.StandardLibrary.NsUrl
8587
private import codeql.swift.frameworks.StandardLibrary.String
8688
private import codeql.swift.frameworks.StandardLibrary.Url
8789
private import codeql.swift.frameworks.StandardLibrary.UrlSession
8890
private import codeql.swift.frameworks.StandardLibrary.WebView
8991
private import codeql.swift.frameworks.Alamofire.Alamofire
92+
private import codeql.swift.security.PathInjection
9093
}
9194

9295
/**

‎swift/ql/lib/codeql/swift/frameworks/StandardLibrary/FilePath.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.ExternalFlow
3+
4+
/** The struct `FilePath`. */
5+
class FilePath extends StructDecl {
6+
FilePath() { this.getFullName() = "FilePath" }
7+
}
8+
9+
/**
10+
* A model for `FilePath` members that permit taint flow.
11+
*/
12+
private class FilePathSummaries extends SummaryModelCsv {
13+
override predicate row(string row) {
14+
// TODO: Properly model this class
15+
row = ";FilePath;true;init(stringLiteral:);(String);;Argument[0];ReturnValue;taint"
16+
}
17+
}

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

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

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

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
/** Provides classes and predicates to reason about path injection vulnerabilities. */
2+
3+
import swift
4+
private import codeql.swift.controlflow.BasicBlocks
5+
private import codeql.swift.controlflow.ControlFlowGraph
6+
private import codeql.swift.dataflow.DataFlow
7+
private import codeql.swift.dataflow.ExternalFlow
8+
private import codeql.swift.dataflow.TaintTracking
9+
private import codeql.swift.generated.ParentChild
10+
private import codeql.swift.frameworks.StandardLibrary.FilePath
11+
12+
/** A data flow sink for path injection vulnerabilities. */
13+
abstract class PathInjectionSink extends DataFlow::Node { }
14+
15+
/** A sanitizer for path injection vulnerabilities. */
16+
abstract class PathInjectionSanitizer extends DataFlow::Node { }
17+
18+
/**
19+
* A unit class for adding additional taint steps.
20+
*
21+
* Extend this class to add additional taint steps that should apply to paths related to
22+
* path injection vulnerabilities.
23+
*/
24+
class PathInjectionAdditionalTaintStep extends Unit {
25+
/**
26+
* Holds if the step from `node1` to `node2` should be considered a taint
27+
* step for paths related to path injection vulnerabilities.
28+
*/
29+
abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
30+
}
31+
32+
private class DefaultPathInjectionSink extends PathInjectionSink {
33+
DefaultPathInjectionSink() { sinkNode(this, "path-injection") }
34+
}
35+
36+
private class DefaultPathInjectionSanitizer extends PathInjectionSanitizer {
37+
DefaultPathInjectionSanitizer() {
38+
// This is a simplified implementation.
39+
// TODO: Implement a complete path sanitizer when Guards are available.
40+
exists(CallExpr starts, CallExpr normalize, DataFlow::Node validated |
41+
starts.getStaticTarget().getName() = "starts(with:)" and
42+
starts.getStaticTarget().getEnclosingDecl() instanceof FilePath and
43+
normalize.getStaticTarget().getName() = "lexicallyNormalized()" and
44+
normalize.getStaticTarget().getEnclosingDecl() instanceof FilePath
45+
|
46+
TaintTracking::localTaint(validated, DataFlow::exprNode(normalize.getQualifier())) and
47+
DataFlow::localExprFlow(normalize, starts.getQualifier()) and
48+
DataFlow::localFlow(validated, this) and
49+
exists(ConditionBlock bb, SuccessorTypes::BooleanSuccessor b |
50+
bb.getANode().getNode().asAstNode().(IfStmt).getACondition() = getImmediateParent*(starts) and
51+
b.getValue() = true
52+
|
53+
bb.controls(this.getCfgNode().getBasicBlock(), b)
54+
)
55+
)
56+
}
57+
}
58+
59+
private class PathInjectionSinks extends SinkModelCsv {
60+
override predicate row(string row) {
61+
row =
62+
[
63+
";Data;true;write(to:options:);;;Argument[0];path-injection",
64+
";NSData;true;write(to:atomically:);;;Argument[0];path-injection",
65+
";NSData;true;write(to:options:);;;Argument[0];path-injection",
66+
";NSData;true;write(toFile:atomically:);;;Argument[0];path-injection",
67+
";NSData;true;write(toFile:options:);;;Argument[0];path-injection",
68+
";FileManager;true;contentsOfDirectory(at:includingPropertiesForKeys:options:);;;Argument[0];path-injection",
69+
";FileManager;true;contentsOfDirectory(atPath:);;;Argument[0];path-injection",
70+
";FileManager;true;enumerator(at:includingPropertiesForKeys:options:errorHandler:);;;Argument[0];path-injection",
71+
";FileManager;true;enumerator(atPath:);;;Argument[0];path-injection",
72+
";FileManager;true;subpathsOfDirectory(atPath:);;;Argument[0];path-injection",
73+
";FileManager;true;subpaths(atPath:);;;Argument[0];path-injection",
74+
";FileManager;true;createDirectory(at:withIntermediateDirectories:attributes:);;;Argument[0];path-injection",
75+
";FileManager;true;createDirectory(atPath:withIntermediateDirectories:attributes:);;;Argument[0];path-injection",
76+
";FileManager;true;createFile(atPath:contents:attributes:);;;Argument[0];path-injection",
77+
";FileManager;true;removeItem(at:);;;Argument[0];path-injection",
78+
";FileManager;true;removeItem(atPath:);;;Argument[0];path-injection",
79+
";FileManager;true;trashItem(at:resultingItemURL:);;;Argument[0];path-injection",
80+
";FileManager;true;replaceItem(at:withItemAt:backupItemName:options:resultingItemURL:);;;Argument[0..1];path-injection",
81+
";FileManager;true;replaceItemAt(_:withItemAt:backupItemName:options:);;;Argument[0..1];path-injection",
82+
";FileManager;true;copyItem(at:to:);;;Argument[0..1];path-injection",
83+
";FileManager;true;copyItem(atPath:toPath:);;;Argument[0..1];path-injection",
84+
";FileManager;true;moveItem(at:to:);;;Argument[0..1];path-injection",
85+
";FileManager;true;moveItem(atPath:toPath:);;;Argument[0..1];path-injection",
86+
";FileManager;true;createSymbolicLink(at:withDestinationURL:);;;Argument[0..1];path-injection",
87+
";FileManager;true;createSymbolicLink(atPath:withDestinationPath:);;;Argument[0..1];path-injection",
88+
";FileManager;true;linkItem(at:to:);;;Argument[0..1];path-injection",
89+
";FileManager;true;linkItem(atPath:toPath:);;;Argument[0..1];path-injection",
90+
";FileManager;true;destinationOfSymbolicLink(atPath:);;;Argument[0];path-injection",
91+
";FileManager;true;fileExists(atPath:);;;Argument[0];path-injection",
92+
";FileManager;true;fileExists(atPath:isDirectory:);;;Argument[0];path-injection",
93+
";FileManager;true;setAttributes(_:ofItemAtPath:);;;Argument[1];path-injection",
94+
";FileManager;true;contents(atPath:);;;Argument[0];path-injection",
95+
";FileManager;true;contentsEqual(atPath:andPath:);;;Argument[0..1];path-injection",
96+
";FileManager;true;changeCurrentDirectoryPath(_:);;;Argument[0];path-injection",
97+
";FileManager;true;unmountVolume(at:options:completionHandler:);;;Argument[0];path-injection",
98+
// Deprecated FileManager methods:
99+
";FileManager;true;changeFileAttributes(_:atPath:);;;Argument[1];path-injection",
100+
";FileManager;true;directoryContents(atPath:);;;Argument[0];path-injection",
101+
";FileManager;true;createDirectory(atPath:attributes:);;;Argument[0];path-injection",
102+
";FileManager;true;createSymbolicLink(atPath:pathContent:);;;Argument[0..1];path-injection",
103+
";FileManager;true;pathContentOfSymbolicLink(atPath:);;;Argument[0];path-injection",
104+
";FileManager;true;replaceItemAtURL(originalItemURL:withItemAtURL:backupItemName:options:);;;Argument[0..1];path-injection",
105+
";NIOFileHandle;true;init(descriptor:);;;Argument[0];path-injection",
106+
";NIOFileHandle;true;init(path:mode:flags:);;;Argument[0];path-injection",
107+
";NIOFileHandle;true;init(path:);;;Argument[0];path-injection"
108+
]
109+
}
110+
}

‎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 expose resources to attackers.</p>
8+
9+
<p>Paths that are naively constructed from data controlled by a user may contain unexpected special characters,
10+
such as <code>..</code>. Such a path could point to any directory on the file system.</p>
11+
</overview>
12+
13+
<recommendation>
14+
15+
<p>Validate user input before using it to construct a file path. Ideally, follow these rules:</p>
16+
17+
<ul>
18+
<li>Do not allow more than a single <code>.</code> character.</li>
19+
<li>Do not allow directory separators such as <code>/</code> or <code>\</code> (depending on the file system).</li>
20+
<li>Do not rely on simply replacing problematic sequences such as <code>../</code>. For example, after applying this filter to
21+
<code>.../...//</code> the resulting string would still be <code>../</code>.</li>
22+
<li>Use a whitelist of known good patterns.</li>
23+
</ul>
24+
25+
</recommendation>
26+
27+
<example>
28+
<p>
29+
The following code shows two bad examples.
30+
</p>
31+
32+
<sample src="PathInjectionBad.swift" />
33+
34+
<p>
35+
In the first, a file name is read from an HTTP request and then used to access a file. In this case, a malicious response could include a file name that is an absolute path, such as
36+
<code>"/Applications/(current_application)/Documents/sensitive.data"</code>.
37+
</p>
38+
39+
<p>
40+
In the second bad example, it appears that the user is restricted to opening a file within the
41+
<code>"/Library/Caches"</code> home directory. In this case, a malicious response could contain a file name containing
42+
special characters. For example, the string <code>"../../Documents/sensitive.data"</code> will result in the code
43+
reading the file located at <code>"/Applications/(current_application)/Library/Caches/../../Documents/sensitive.data"</code>,
44+
which contains users' sensitive data. This file may then be made accessible to an attacker, giving them access to all this data.
45+
</p>
46+
47+
<p>
48+
In the following (good) example, the path used to access the file system is normalized <em>before</em> being checked against a
49+
known prefix. This ensures that regardless of the user input, the resulting path is safe.
50+
</p>
51+
52+
<sample src="PathInjectionGood.swift" />
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.

0 commit comments

Comments
 (0)