Merge pull request #304 from weichsel/bugfix/pathEscape

Fix Path Escape Vulerability

Sources/ZIPFoundation/FileManager+ZIP.swift

@@ -347,9 +347,25 @@ extension CocoaError {
 }
 
 public extension URL {
+
     func isContained(in parentDirectoryURL: URL) -> Bool {
         // Ensure this URL is contained in the passed in URL
         let parentDirectoryURL = URL(fileURLWithPath: parentDirectoryURL.path, isDirectory: true).standardized
-        return self.standardized.absoluteString.hasPrefix(parentDirectoryURL.absoluteString)
+        // Maliciously crafted ZIP files can contain entries using a prepended path delimiter `/` in combination
+        // with the parent directory shorthand `..` to bypass our containment check.
+        // When a malicious entry path like e.g. `/../secret.txt` gets appended to the destination 
+        // directory URL (e.g. `file:///tmp/`), the resulting URL `file:///tmp//../secret.txt` gets expanded
+        // to `file:///tmp/secret` when using `URL.standardized`. This URL would pass the check performed
+        // in `isContained(in:)`.
+        // Lower level API like POSIX `fopen` - which is used at a later point during extraction - expands
+        // `/tmp//../secret.txt` to `/secret.txt` though. This would lead to an escape to the parent directory.
+        // To avoid that, we replicate the behavior of `fopen`s path expansion and replace all double delimiters
+        // with single delimiters.
+        // More details: https://github.com/weichsel/ZIPFoundation/issues/281
+        let sanitizedEntryPathURL: URL = {
+            let sanitizedPath = self.path.replacingOccurrences(of: "//", with: "/")
+            return URL(fileURLWithPath: sanitizedPath)
+        }()
+        return sanitizedEntryPathURL.standardized.absoluteString.hasPrefix(parentDirectoryURL.absoluteString)
     }
 }

Tests/ZIPFoundationTests/ZIPFoundationReadingTests.swift

@@ -275,7 +275,15 @@ extension ZIPFoundationTests {
                             throws: Archive.ArchiveError.invalidCRC32)
     }
 
-    func testTraversalAttack() {
+    func testSimpleTraversalAttack() {
+        let fileManager = FileManager()
+        let archive = self.archive(for: #function, mode: .read)
+        let destinationURL = self.createDirectory(for: #function)
+        XCTAssertCocoaError(try fileManager.unzipItem(at: archive.url, to: destinationURL),
+                            throwsErrorWithCode: .fileReadInvalidFileName)
+    }
+
+    func testPathDelimiterTraversalAttack() {
         let fileManager = FileManager()
         let archive = self.archive(for: #function, mode: .read)
         let destinationURL = self.createDirectory(for: #function)

Tests/ZIPFoundationTests/ZIPFoundationTests.swift

@@ -118,16 +118,16 @@ class ZIPFoundationTests: XCTestCase {
 
     func createDirectory(for testFunction: String) -> URL {
         let fileManager = FileManager()
-        var URL = ZIPFoundationTests.tempZipDirectoryURL
-        URL = URL.appendingPathComponent(self.pathComponent(for: testFunction))
+        var url = ZIPFoundationTests.tempZipDirectoryURL
+        url = url.appendingPathComponent(self.pathComponent(for: testFunction), isDirectory: true)
         do {
-            try fileManager.createDirectory(at: URL, withIntermediateDirectories: true, attributes: nil)
+            try fileManager.createDirectory(at: url, withIntermediateDirectories: true, attributes: nil)
         } catch {
             XCTFail("Failed to get create directory for test function:\(testFunction)")
             type(of: self).tearDown()
             preconditionFailure()
         }
-        return URL
+        return url
     }
 
     func runWithUnprivilegedGroup(handler: () throws -> Void) {
@@ -253,7 +253,8 @@ extension ZIPFoundationTests {
             ("testRemoveEntryErrorConditions", testRemoveEntryErrorConditions),
             ("testRemoveUncompressedEntry", testRemoveUncompressedEntry),
             ("testTemporaryReplacementDirectoryURL", testTemporaryReplacementDirectoryURL),
-            ("testTraversalAttack", testTraversalAttack),
+            ("testSimpleTraversalAttack", testSimpleTraversalAttack),
+            ("testPathDelimiterTraversalAttack", testPathDelimiterTraversalAttack),
             ("testUnzipItem", testUnzipItem),
             ("testUnzipItemWithPreferredEncoding", testUnzipItemWithPreferredEncoding),
             ("testUnzipItemErrorConditions", testUnzipItemErrorConditions),