Skip to content

Commit fd1314b

Browse files
geoffw0geoffw0
authored
Merge pull request #14888 from maikypedia/maikypedia/swift-zip
Swift: Add Unsafe Unpacking Query (CWE-022)
2 parents 7217dfa + ed030bc commit fd1314b

File tree

11 files changed

+346
-0
lines changed

11 files changed

+346
-0
lines changed

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

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for reasoning about
3+
* unsafe unpack vulnerabilities, as well as extension points for
4+
* adding your own.
5+
*/
6+
7+
import swift
8+
import codeql.swift.dataflow.DataFlow
9+
import codeql.swift.dataflow.ExternalFlow
10+
11+
/**
12+
* A dataflow source for unsafe unpack vulnerabilities.
13+
*/
14+
abstract class UnsafeUnpackSource extends DataFlow::Node { }
15+
16+
/**
17+
* A dataflow sink for unsafe unpack vulnerabilities.
18+
*/
19+
abstract class UnsafeUnpackSink extends DataFlow::Node { }
20+
21+
/**
22+
* A barrier for unsafe unpack vulnerabilities.
23+
*/
24+
abstract class UnsafeUnpackBarrier extends DataFlow::Node { }
25+
26+
/**
27+
* A unit class for adding additional flow steps.
28+
*/
29+
class UnsafeUnpackAdditionalFlowStep extends Unit {
30+
/**
31+
* Holds if the step from `node1` to `node2` should be considered a flow
32+
* step for paths related to unsafe unpack vulnerabilities.
33+
*/
34+
abstract predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo);
35+
}
36+
37+
/**
38+
* A sink defined in a CSV model.
39+
*/
40+
private class DefaultUnsafeUnpackSink extends UnsafeUnpackSink {
41+
DefaultUnsafeUnpackSink() { sinkNode(this, "unsafe-unpack") }
42+
}
43+
44+
private class UnsafeUnpackSinks extends SinkModelCsv {
45+
override predicate row(string row) {
46+
row =
47+
[
48+
";Zip;true;unzipFile(_:destination:overwrite:password:progress:fileOutputHandler:);;;Argument[0];unsafe-unpack",
49+
";FileManager;true;unzipItem(at:to:skipCRC32:progress:pathEncoding:);;;Argument[0];unsafe-unpack",
50+
]
51+
}
52+
}
53+
54+
/**
55+
* An additional taint step for unsafe unpack vulnerabilities.
56+
*/
57+
private class UnsafeUnpackAdditionalDataFlowStep extends UnsafeUnpackAdditionalFlowStep {
58+
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
59+
exists(CallExpr initCall, CallExpr call |
60+
// If a zip file is remotely downloaded the destination path is tainted
61+
call.getStaticTarget().(Method).hasQualifiedName("Data", "write(to:options:)") and
62+
call.getQualifier() = initCall and
63+
initCall.getStaticTarget().(Initializer).getEnclosingDecl().(TypeDecl).getName() = "Data" and
64+
nodeFrom.asExpr() = initCall and
65+
nodeTo.asExpr() = call.getAnArgument().getExpr()
66+
)
67+
}
68+
}
69+
70+
/**
71+
* A barrier for unsafe unpack vulnerabilities.
72+
*/
73+
private class UnsafeUnpackDefaultBarrier extends UnsafeUnpackBarrier {
74+
UnsafeUnpackDefaultBarrier() {
75+
// any numeric type
76+
this.asExpr().getType().getUnderlyingType().getABaseType*().getName() = "Numeric"
77+
}
78+
}

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for reasoning about
3+
* unsafe unpack vulnerabilities, as well as extension points for
4+
* adding your own.
5+
*/
6+
7+
import swift
8+
import codeql.swift.dataflow.TaintTracking
9+
import codeql.swift.dataflow.FlowSources
10+
import codeql.swift.security.UnsafeUnpackExtensions
11+
12+
/**
13+
* A taint configuration for tainted data that reaches a unsafe unpack sink.
14+
*/
15+
module UnsafeUnpackConfig implements DataFlow::ConfigSig {
16+
predicate isSource(DataFlow::Node node) {
17+
node instanceof FlowSource or node instanceof RemoteFlowSource
18+
}
19+
20+
predicate isSink(DataFlow::Node node) { node instanceof UnsafeUnpackSink }
21+
22+
predicate isBarrier(DataFlow::Node barrier) { barrier instanceof UnsafeUnpackBarrier }
23+
24+
predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
25+
any(UnsafeUnpackAdditionalFlowStep s).step(nodeFrom, nodeTo)
26+
}
27+
}
28+
29+
/**
30+
* Detect taint flow of tainted data that reaches a unsafe unpack sink.
31+
*/
32+
module UnsafeUnpackFlow = TaintTracking::Global<UnsafeUnpackConfig>;

‎swift/ql/src/change-notes/2024-02-07-unsafe-unpacking.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new query, `swift/unsafe-unpacking`, that detects unpacking user controlled zips without validating the destination file path is within the destination directory.

‎swift/ql/src/experimental/Security/CWE-022/UnsafeUnpack.qhelp

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
9+
Unpacking files from a malicious zip without properly validating that the destination file path
10+
is within the destination directory, or allowing symlinks to point to files outside the extraction directory,
11+
allows an attacker to extract files to arbitrary locations outside the extraction directory. This helps
12+
overwrite sensitive user data and, in some cases, can lead to code execution if an
13+
attacker overwrites an application's shared object file.
14+
</p>
15+
</overview>
16+
17+
<recommendation>
18+
<p>Consider using a safer module, such as: <code>ZIPArchive</code></p>
19+
</recommendation>
20+
21+
<example>
22+
<p>
23+
The following examples unpacks a remote zip using `Zip.unzipFile()` which is vulnerable to path traversal.
24+
</p>
25+
<sample src="ZipBad.swift" />
26+
27+
<p>
28+
The following examples unpacks a remote zip using `fileManager.unzipItem()` which is vulnerable to symlink path traversal.
29+
</p>
30+
<sample src="ZIPFoundationBad.swift" />
31+
32+
33+
<p>Consider using a safer module, such as: <code>ZIPArchive</code></p>
34+
<sample src="ZipArchiveGood.swift" />
35+
</example>
36+
37+
<references>
38+
<li>
39+
Ostorlab:
40+
<a href="https://blog.ostorlab.co/zip-packages-exploitation.html">Zip Packages Exploitation</a>.
41+
</li>
42+
</references>
43+
</qhelp>

‎swift/ql/src/experimental/Security/CWE-022/UnsafeUnpack.ql

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* @name Arbitrary file write during a zip extraction from a user controlled source
3+
* @description Unpacking user controlled zips without validating whether the
4+
* destination file path is within the destination directory can cause files
5+
* outside the destination directory to be overwritten.
6+
* @kind path-problem
7+
* @problem.severity error
8+
* @security-severity 7.5
9+
* @precision high
10+
* @id swift/unsafe-unpacking
11+
* @tags security
12+
* experimental
13+
* external/cwe/cwe-022
14+
*/
15+
16+
import swift
17+
import codeql.swift.security.UnsafeUnpackQuery
18+
import UnsafeUnpackFlow::PathGraph
19+
20+
from UnsafeUnpackFlow::PathNode sourceNode, UnsafeUnpackFlow::PathNode sinkNode
21+
where UnsafeUnpackFlow::flowPath(sourceNode, sinkNode)
22+
select sinkNode.getNode(), sourceNode, sinkNode,
23+
"Unsafe unpacking from a malicious zip retrieved from a remote location."

‎swift/ql/src/experimental/Security/CWE-022/ZIPFoundationBad.swift

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
import Foundation
2+
import ZIPFoundation
3+
4+
5+
func unzipFile(at sourcePath: String, to destinationPath: String) {
6+
do {
7+
let remoteURL = URL(string: "https://example.com/")!
8+
9+
let source = URL(fileURLWithPath: sourcePath)
10+
let destination = URL(fileURLWithPath: destinationPath)
11+
12+
// Malicious zip is downloaded
13+
try Data(contentsOf: remoteURL).write(to: source)
14+
15+
let fileManager = FileManager()
16+
// Malicious zip is unpacked
17+
try fileManager.unzipItem(at:source, to: destination)
18+
} catch {
19+
}
20+
}
21+
22+
func main() {
23+
let sourcePath = "/sourcePath"
24+
let destinationPath = "/destinationPath"
25+
unzipFile(at: sourcePath, to: destinationPath)
26+
}
27+
28+
main()

‎swift/ql/src/experimental/Security/CWE-022/ZipArchiveGood.swift

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
import Foundation
2+
import ZipArchive
3+
4+
func unzipFile(at sourcePath: String, to destinationPath: String) {
5+
do {
6+
let remoteURL = URL(string: "https://example.com/")!
7+
8+
let source = URL(fileURLWithPath: sourcePath)
9+
10+
// Malicious zip is downloaded
11+
try Data(contentsOf: remoteURL).write(to: source)
12+
13+
// ZipArchive is safe
14+
try SSZipArchive.unzipFile(atPath: sourcePath, toDestination: destinationPath, delegate: self)
15+
} catch {
16+
}
17+
}
18+
19+
func main() {
20+
let sourcePath = "/sourcePath"
21+
let destinationPath = "/destinationPath"
22+
unzipFile(at: sourcePath, to: destinationPath)
23+
}
24+
25+
main()

‎swift/ql/src/experimental/Security/CWE-022/ZipBad.swift

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
import Foundation
2+
import Zip
3+
4+
5+
func unzipFile(at sourcePath: String, to destinationPath: String) {
6+
do {
7+
let remoteURL = URL(string: "https://example.com/")!
8+
9+
let source = URL(fileURLWithPath: sourcePath)
10+
let destination = URL(fileURLWithPath: destinationPath)
11+
12+
// Malicious zip is downloaded
13+
try Data(contentsOf: remoteURL).write(to: source)
14+
15+
let fileManager = FileManager()
16+
// Malicious zip is unpacked
17+
try Zip.unzipFile(source, destination: destination, overwrite: true, password: nil)
18+
} catch {
19+
}
20+
}
21+
22+
func main() {
23+
let sourcePath = "/sourcePath"
24+
let destinationPath = "/destinationPath"
25+
unzipFile(at: sourcePath, to: destinationPath)
26+
}
27+
28+
main()

‎swift/ql/test/query-tests/Security/CWE-022-Unsafe-Unpack/UnsafeUnpack.expected

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
edges
2+
| UnsafeUnpack.swift:62:9:62:48 | call to Data.init(contentsOf:options:) | UnsafeUnpack.swift:62:60:62:60 | source |
3+
| UnsafeUnpack.swift:62:60:62:60 | source | UnsafeUnpack.swift:64:27:64:27 | source |
4+
| UnsafeUnpack.swift:62:60:62:60 | source | UnsafeUnpack.swift:67:39:67:39 | source |
5+
nodes
6+
| UnsafeUnpack.swift:62:9:62:48 | call to Data.init(contentsOf:options:) | semmle.label | call to Data.init(contentsOf:options:) |
7+
| UnsafeUnpack.swift:62:60:62:60 | source | semmle.label | source |
8+
| UnsafeUnpack.swift:64:27:64:27 | source | semmle.label | source |
9+
| UnsafeUnpack.swift:67:39:67:39 | source | semmle.label | source |
10+
subpaths
11+
#select
12+
| UnsafeUnpack.swift:64:27:64:27 | source | UnsafeUnpack.swift:62:9:62:48 | call to Data.init(contentsOf:options:) | UnsafeUnpack.swift:64:27:64:27 | source | Unsafe unpacking from a malicious zip retrieved from a remote location. |
13+
| UnsafeUnpack.swift:67:39:67:39 | source | UnsafeUnpack.swift:62:9:62:48 | call to Data.init(contentsOf:options:) | UnsafeUnpack.swift:67:39:67:39 | source | Unsafe unpacking from a malicious zip retrieved from a remote location. |

‎swift/ql/test/query-tests/Security/CWE-022-Unsafe-Unpack/UnsafeUnpack.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/Security/CWE-022/UnsafeUnpack.ql

0 commit comments

Comments
 (0)