Skip to content

Commit 2ec8673

Browse files
asgerfasgerf
authored
Merge pull request #22677 from asgerf/unified/path-injection
unified: Add path injection query
2 parents f1d3f1d + 8247086 commit 2ec8673

11 files changed

Lines changed: 1072 additions & 0 deletions

File tree

‎unified/ql/lib/ext/legacy-swift.model.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,7 @@ extensions:
235235
- ["", "Connection", true, "scalar(_:_:)", "", "", "Argument[0]", "database-store", "manual"]
236236
- ["", "Connection", true, "scalar(_:_:)", "", "", "Argument[1]", "database-store", "manual"]
237237
- ["", "Connection", true, "init(_:readonly:)", "", "", "Argument[0]", "path-injection", "manual"]
238+
- ["", "Connection.Location", true, "uri(_:parameters:)", "", "", "Argument[0]", "path-injection", "manual"]
238239
- ["", "Database", true, "changePassphrase(_:)", "", "", "Argument[0]", "encryption-key", "manual"]
239240
- ["", "Database", true, "usePassphrase(_:)", "", "", "Argument[0]", "encryption-key", "manual"]
240241
- ["", "Database", true, "allStatements(arguments:sql:)", "", "", "Argument[arguments:]", "database-store", "manual"]

‎unified/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>

‎unified/ql/src/queries/security/CWE-022/PathInjection.ql‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
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 unified/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 unified
18+
19+
/**
20+
* A string that might be a label for a path argument.
21+
*/
22+
pragma[inline]
23+
private predicate pathLikeHeuristic(string label) {
24+
label =
25+
[
26+
"atFile", "atPath", "atDirectory", "toFile", "toPath", "toDirectory", "inFile", "inPath",
27+
"inDirectory", "contentsOfFile", "contentsOfPath", "contentsOfDirectory", "filePath",
28+
"directory", "directoryPath"
29+
]
30+
}
31+
32+
predicate heuristicSink(DataFlow::Node node) {
33+
node.isIncomingValue(any(Identifier id | id.getValue() = "sqlite3_temp_directory"))
34+
or
35+
exists(Argument arg |
36+
pathLikeHeuristic(arg.getName()) and
37+
node.asExpr() = arg.getValue()
38+
)
39+
}
40+
41+
module PathInjectionConfig implements DataFlow::ConfigSig {
42+
predicate isSource(DataFlow::Node node) { Models::isSource(node, _) }
43+
44+
predicate isSink(DataFlow::Node node) {
45+
Models::isSink(node, "path-injection") or
46+
heuristicSink(node)
47+
}
48+
49+
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { none() }
50+
51+
predicate isBarrier(DataFlow::Node node) {
52+
// TODO: add barriers
53+
none()
54+
}
55+
}
56+
57+
module PathInjectionFlow = DataFlow::Global<PathInjectionConfig>;
58+
59+
import PathInjectionFlow::PathGraph
60+
61+
from PathInjectionFlow::PathNode source, PathInjectionFlow::PathNode sink
62+
where PathInjectionFlow::flowPath(source, sink)
63+
select sink.getNode(), source, sink, "This path depends on a $@.", source.getNode(),
64+
"user-provided value"

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

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

‎unified/ql/test/library-tests/mad/test.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ isSink
1515
| test.swift:100:24:100:36 | encryptionKey | encryption-key |
1616
| test.swift:101:18:101:24 | fileURL | path-injection |
1717
| test.swift:107:23:107:34 | seedFilePath | path-injection |
18+
| test.swift:119:29:119:34 | string | path-injection |
1819
isSource
1920
| test.swift:4:5:4:27 | String(...) | remote |
2021
| test.swift:5:5:5:44 | String(...) | remote |

‎unified/ql/test/library-tests/mad/test.swift‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,3 +108,13 @@ func testQualifiedConstructors(
108108
shouldCompactOnLaunch: nil,
109109
syncConfiguration: nil)
110110
}
111+
112+
class Connection {
113+
enum Location {
114+
static func uri(_ path: String, parameters: String) {}
115+
}
116+
}
117+
118+
func testConnectionLocation(string: String) {
119+
Connection.Location.uri(string, parameters: "") // $ isSink=path-injection
120+
}

‎unified/ql/test/query-tests/security/CWE-022/PathInjection/CONSISTENCY/CfgConsistency.expected‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
consistencyOverview
2+
| deadEnd | 1 |
3+
deadEnd
4+
| testPathInjection.swift:330:14:330:62 | Entry |

0 commit comments

Comments
 (0)