Merge pull request #14888 from maikypedia/maikypedia/swift-zip

Swift: Add Unsafe Unpacking Query (CWE-022)

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

@@ -0,0 +1,78 @@
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about
+ * unsafe unpack vulnerabilities, as well as extension points for
+ * adding your own.
+ */
+
+import swift
+import codeql.swift.dataflow.DataFlow
+import codeql.swift.dataflow.ExternalFlow
+
+/**
+ * A dataflow source for unsafe unpack vulnerabilities.
+ */
+abstract class UnsafeUnpackSource extends DataFlow::Node { }
+
+/**
+ * A dataflow sink for unsafe unpack vulnerabilities.
+ */
+abstract class UnsafeUnpackSink extends DataFlow::Node { }
+
+/**
+ * A barrier for unsafe unpack vulnerabilities.
+ */
+abstract class UnsafeUnpackBarrier extends DataFlow::Node { }
+
+/**
+ * A unit class for adding additional flow steps.
+ */
+class UnsafeUnpackAdditionalFlowStep extends Unit {
+  /**
+   * Holds if the step from `node1` to `node2` should be considered a flow
+   * step for paths related to unsafe unpack vulnerabilities.
+   */
+  abstract predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo);
+}
+
+/**
+ * A sink defined in a CSV model.
+ */
+private class DefaultUnsafeUnpackSink extends UnsafeUnpackSink {
+  DefaultUnsafeUnpackSink() { sinkNode(this, "unsafe-unpack") }
+}
+
+private class UnsafeUnpackSinks extends SinkModelCsv {
+  override predicate row(string row) {
+    row =
+      [
+        ";Zip;true;unzipFile(_:destination:overwrite:password:progress:fileOutputHandler:);;;Argument[0];unsafe-unpack",
+        ";FileManager;true;unzipItem(at:to:skipCRC32:progress:pathEncoding:);;;Argument[0];unsafe-unpack",
+      ]
+  }
+}
+
+/**
+ * An additional taint step for unsafe unpack vulnerabilities.
+ */
+private class UnsafeUnpackAdditionalDataFlowStep extends UnsafeUnpackAdditionalFlowStep {
+  override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+    exists(CallExpr initCall, CallExpr call |
+      // If a zip file is remotely downloaded the destination path is tainted
+      call.getStaticTarget().(Method).hasQualifiedName("Data", "write(to:options:)") and
+      call.getQualifier() = initCall and
+      initCall.getStaticTarget().(Initializer).getEnclosingDecl().(TypeDecl).getName() = "Data" and
+      nodeFrom.asExpr() = initCall and
+      nodeTo.asExpr() = call.getAnArgument().getExpr()
+    )
+  }
+}
+
+/**
+ * A barrier for unsafe unpack vulnerabilities.
+ */
+private class UnsafeUnpackDefaultBarrier extends UnsafeUnpackBarrier {
+  UnsafeUnpackDefaultBarrier() {
+    // any numeric type
+    this.asExpr().getType().getUnderlyingType().getABaseType*().getName() = "Numeric"
+  }
+}

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

@@ -0,0 +1,32 @@
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about
+ * unsafe unpack vulnerabilities, as well as extension points for
+ * adding your own.
+ */
+
+import swift
+import codeql.swift.dataflow.TaintTracking
+import codeql.swift.dataflow.FlowSources
+import codeql.swift.security.UnsafeUnpackExtensions
+
+/**
+ * A taint configuration for tainted data that reaches a unsafe unpack sink.
+ */
+module UnsafeUnpackConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) {
+    node instanceof FlowSource or node instanceof RemoteFlowSource
+  }
+
+  predicate isSink(DataFlow::Node node) { node instanceof UnsafeUnpackSink }
+
+  predicate isBarrier(DataFlow::Node barrier) { barrier instanceof UnsafeUnpackBarrier }
+
+  predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+    any(UnsafeUnpackAdditionalFlowStep s).step(nodeFrom, nodeTo)
+  }
+}
+
+/**
+ * Detect taint flow of tainted data that reaches a unsafe unpack sink.
+ */
+module UnsafeUnpackFlow = TaintTracking::Global<UnsafeUnpackConfig>;

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

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* 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

@@ -0,0 +1,43 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+
+Unpacking files from a malicious zip without properly validating that the destination file path 
+is within the destination directory, or allowing symlinks to point to files outside the extraction directory, 
+allows an attacker to extract files to arbitrary locations outside the extraction directory. This helps 
+overwrite sensitive user data and, in some cases, can lead to code execution if an 
+attacker overwrites an application's shared object file.
+</p>
+</overview>
+
+<recommendation>
+<p>Consider using a safer module, such as: <code>ZIPArchive</code></p>
+</recommendation>
+
+<example>
+<p>
+The following examples unpacks a remote zip using `Zip.unzipFile()` which is vulnerable to path traversal.
+</p>
+<sample src="ZipBad.swift" />
+
+<p>
+The following examples unpacks a remote zip using `fileManager.unzipItem()` which is vulnerable to symlink path traversal.
+</p>
+<sample src="ZIPFoundationBad.swift" />
+
+
+<p>Consider using a safer module, such as: <code>ZIPArchive</code></p>
+<sample src="ZipArchiveGood.swift" />
+</example>
+
+<references>
+<li>
+Ostorlab:
+<a href="https://blog.ostorlab.co/zip-packages-exploitation.html">Zip Packages Exploitation</a>.
+</li>
+</references>
+</qhelp>

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

@@ -0,0 +1,23 @@
+/**
+ * @name Arbitrary file write during a zip extraction from a user controlled source
+ * @description Unpacking user controlled zips without validating whether the
+ *              destination file path is within the destination directory can cause files
+ *              outside the destination directory to be overwritten.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 7.5
+ * @precision high
+ * @id swift/unsafe-unpacking
+ * @tags security
+ *       experimental
+ *       external/cwe/cwe-022
+ */
+
+import swift
+import codeql.swift.security.UnsafeUnpackQuery
+import UnsafeUnpackFlow::PathGraph
+
+from UnsafeUnpackFlow::PathNode sourceNode, UnsafeUnpackFlow::PathNode sinkNode
+where UnsafeUnpackFlow::flowPath(sourceNode, sinkNode)
+select sinkNode.getNode(), sourceNode, sinkNode,
+  "Unsafe unpacking from a malicious zip retrieved from a remote location."

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

@@ -0,0 +1,28 @@
+import Foundation
+import ZIPFoundation
+
+
+func unzipFile(at sourcePath: String, to destinationPath: String) {
+    do {
+        let remoteURL = URL(string: "https://example.com/")!
+
+        let source  = URL(fileURLWithPath: sourcePath)
+        let destination = URL(fileURLWithPath: destinationPath)
+
+        // Malicious zip is downloaded 
+        try Data(contentsOf: remoteURL).write(to: source)
+
+        let fileManager = FileManager()
+        // Malicious zip is unpacked
+        try fileManager.unzipItem(at:source, to: destination)
+        } catch {
+    }
+}
+
+func main() {
+    let sourcePath = "/sourcePath" 
+    let destinationPath = "/destinationPath" 
+    unzipFile(at: sourcePath, to: destinationPath)
+}
+
+main()

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

@@ -0,0 +1,25 @@
+import Foundation
+import ZipArchive
+
+func unzipFile(at sourcePath: String, to destinationPath: String) {
+    do {
+        let remoteURL = URL(string: "https://example.com/")!
+
+        let source  = URL(fileURLWithPath: sourcePath)
+
+        // Malicious zip is downloaded 
+        try Data(contentsOf: remoteURL).write(to: source)
+
+        // ZipArchive is safe
+        try SSZipArchive.unzipFile(atPath: sourcePath, toDestination: destinationPath, delegate: self)
+        } catch {
+    }
+}
+
+func main() {
+    let sourcePath = "/sourcePath" 
+    let destinationPath = "/destinationPath" 
+    unzipFile(at: sourcePath, to: destinationPath)
+}
+
+main()

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

@@ -0,0 +1,28 @@
+import Foundation
+import Zip
+
+
+func unzipFile(at sourcePath: String, to destinationPath: String) {
+    do {
+        let remoteURL = URL(string: "https://example.com/")!
+
+        let source  = URL(fileURLWithPath: sourcePath)
+        let destination = URL(fileURLWithPath: destinationPath)
+
+        // Malicious zip is downloaded 
+        try Data(contentsOf: remoteURL).write(to: source)
+
+        let fileManager = FileManager()
+        // Malicious zip is unpacked
+        try Zip.unzipFile(source, destination: destination, overwrite: true, password: nil)
+        } catch {
+    }
+}
+
+func main() {
+    let sourcePath = "/sourcePath" 
+    let destinationPath = "/destinationPath" 
+    unzipFile(at: sourcePath, to: destinationPath)
+}
+
+main()

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

@@ -0,0 +1,13 @@
+edges
+| UnsafeUnpack.swift:62:9:62:48 | call to Data.init(contentsOf:options:) | UnsafeUnpack.swift:62:60:62:60 | source |
+| UnsafeUnpack.swift:62:60:62:60 | source | UnsafeUnpack.swift:64:27:64:27 | source |
+| UnsafeUnpack.swift:62:60:62:60 | source | UnsafeUnpack.swift:67:39:67:39 | source |
+nodes
+| UnsafeUnpack.swift:62:9:62:48 | call to Data.init(contentsOf:options:) | semmle.label | call to Data.init(contentsOf:options:) |
+| UnsafeUnpack.swift:62:60:62:60 | source | semmle.label | source |
+| UnsafeUnpack.swift:64:27:64:27 | source | semmle.label | source |
+| UnsafeUnpack.swift:67:39:67:39 | source | semmle.label | source |
+subpaths
+#select
+| 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. |
+| 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

@@ -0,0 +1 @@
+experimental/Security/CWE-022/UnsafeUnpack.ql

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

@@ -0,0 +1,71 @@
+
+// --- stubs ---
+struct URL
+{
+	init?(string: String) {}
+	init(fileURLWithPath: String) {}
+}
+
+class Zip {
+    class func unzipFile(_ zipFilePath: URL, destination: URL, overwrite: Bool, password: String?, progress: ((_ progress: Double) -> ())? = nil, fileOutputHandler: ((_ unzippedFile: URL) -> Void)? = nil) throws {}
+}
+
+
+class NSObject {
+}
+
+class Progress : NSObject {
+
+}
+
+class FileManager : NSObject {
+    func unzipItem(at sourceURL: URL, to destinationURL: URL, skipCRC32: Bool = false,
+                         progress: Progress? = nil, pathEncoding: String.Encoding? = nil) throws  {}
+}
+
+protocol DataProtocol { }
+class Data : DataProtocol {
+    struct ReadingOptions : OptionSet { let rawValue: Int }
+    struct WritingOptions : OptionSet { let rawValue: Int }
+
+    init<S>(_ elements: S) { count = 0 }
+	init(contentsOf: URL, options: ReadingOptions) { count = 0 }
+    func write(to: URL, options: Data.WritingOptions = []) {}
+
+	var count: Int
+}
+
+extension String {
+
+    struct Encoding {
+		var rawValue: UInt
+
+		init(rawValue: UInt) { self.rawValue = rawValue }
+
+		static let ascii = Encoding(rawValue: 1)
+	}
+    init(contentsOf url: URL) throws {
+        self.init("")
+    }
+}
+
+// --- tests ---
+
+func testCommandInjectionQhelpExamples() {
+    guard let remoteURL = URL(string: "https://example.com/") else {
+        return
+    }
+
+    let source  = URL(fileURLWithPath: "/sourcePath")
+    let destination = URL(fileURLWithPath: "/destination")
+
+    try Data(contentsOf: remoteURL, options: []).write(to: source) 
+    do {
+        try Zip.unzipFile(source, destination: destination, overwrite: true, password: nil) // BAD
+
+        let fileManager = FileManager()
+        try fileManager.unzipItem(at: source, to: destination) // BAD
+    } catch {
+        print("Error: \(error)")
+    }
+}