Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add entry url validation for unzip utility. Defends against Zip Slip #491

Merged
merged 5 commits into from
Jan 7, 2021
Merged

Add entry url validation for unzip utility. Defends against Zip Slip #491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
28db284
Add entry url validation for unzip utility. Defends against Zip Slip
cdhoffmann Jan 7, 2021
654feac
Merge branch 'dev' into unzipSecurity
cdhoffmann Jan 7, 2021
bfba46b
run swift format
cdhoffmann Jan 7, 2021
f6ecf5a
Merge branch 'unzipSecurity' of https://github.com/cdhoffmann/aepsdk-…
cdhoffmann Jan 7, 2021
8a23742
Merge branch 'dev' into unzipSecurity
cdhoffmann Jan 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions AEPCore.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@
3FF829692509937100483C74 /* SignalIntegrationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3FF829682509937100483C74 /* SignalIntegrationTests.swift */; };
3FF8296C2509942300483C74 /* rules_signal.zip in Resources */ = {isa = PBXBuildFile; fileRef = 3FF8296A2509942200483C74 /* rules_signal.zip */; };
3FF8296D2509942300483C74 /* rules_signal.json in Resources */ = {isa = PBXBuildFile; fileRef = 3FF8296B2509942300483C74 /* rules_signal.json */; };
787505E025A67BE200E5203E /* TestZipSlip.zip in Resources */ = {isa = PBXBuildFile; fileRef = 787505DF25A67BE200E5203E /* TestZipSlip.zip */; };
78AA4EBA2502DF2200205AE9 /* ZipArchiveTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 78AA4EB92502DF2200205AE9 /* ZipArchiveTest.swift */; };
78AA4EBC2502E42400205AE9 /* TestCorruptFile.zip in Resources */ = {isa = PBXBuildFile; fileRef = 78AA4EBB2502E42400205AE9 /* TestCorruptFile.zip */; };
78AA4EBE2502F4AF00205AE9 /* TestInvalidCompressionMethod.zip in Resources */ = {isa = PBXBuildFile; fileRef = 78AA4EBD2502F4AF00205AE9 /* TestInvalidCompressionMethod.zip */; };
Expand Down Expand Up @@ -835,6 +836,7 @@
3FF829682509937100483C74 /* SignalIntegrationTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SignalIntegrationTests.swift; sourceTree = "<group>"; };
3FF8296A2509942200483C74 /* rules_signal.zip */ = {isa = PBXFileReference; lastKnownFileType = archive.zip; path = rules_signal.zip; sourceTree = "<group>"; };
3FF8296B2509942300483C74 /* rules_signal.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = rules_signal.json; sourceTree = "<group>"; };
787505DF25A67BE200E5203E /* TestZipSlip.zip */ = {isa = PBXFileReference; lastKnownFileType = archive.zip; path = TestZipSlip.zip; sourceTree = "<group>"; };
78AA4EB92502DF2200205AE9 /* ZipArchiveTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ZipArchiveTest.swift; sourceTree = "<group>"; };
78AA4EBB2502E42400205AE9 /* TestCorruptFile.zip */ = {isa = PBXFileReference; lastKnownFileType = archive.zip; path = TestCorruptFile.zip; sourceTree = "<group>"; };
78AA4EBD2502F4AF00205AE9 /* TestInvalidCompressionMethod.zip */ = {isa = PBXFileReference; lastKnownFileType = archive.zip; path = TestInvalidCompressionMethod.zip; sourceTree = "<group>"; };
Expand Down Expand Up @@ -1429,6 +1431,7 @@
78AA4EBB2502E42400205AE9 /* TestCorruptFile.zip */,
78AA4EBD2502F4AF00205AE9 /* TestInvalidCompressionMethod.zip */,
78AA4EC1250AB55800205AE9 /* TestLarge.zip */,
787505DF25A67BE200E5203E /* TestZipSlip.zip */,
);
path = resources;
sourceTree = "<group>";
Expand Down Expand Up @@ -2199,6 +2202,7 @@
3F03983824BE62AA0019F095 /* TestRules.zip in Resources */,
3F03983B24BE62AA0019F095 /* TestConfig.json in Resources */,
3F03983A24BE62AA0019F095 /* ADBMobileConfig.json in Resources */,
787505E025A67BE200E5203E /* TestZipSlip.zip in Resources */,
78AA4EBC2502E42400205AE9 /* TestCorruptFile.zip in Resources */,
78AA4EC2250AB55800205AE9 /* TestLarge.zip in Resources */,
78AA4EBE2502F4AF00205AE9 /* TestInvalidCompressionMethod.zip in Resources */,
Expand Down
25 changes: 22 additions & 3 deletions AEPServices/Sources/utility/unzip/FileUnzipper.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ public class FileUnzipper: Unzipping {
let path = entry.path
entryNames.append(path)
let destinationEntryURL = destinationURL.appendingPathComponent(path)

guard destinationEntryURL.isContained(in: destinationURL) else {
Log.warning(label: LOG_PREFIX, "Unable to extract files from a domain different than the source.")
// Validate path for entry
if !isValidPath(destinationEntryURL, destination: destinationURL) {
Log.error(label: LOG_PREFIX, "The zip file contained an invalid path. Verify that your zip file is formatted correctly and has not been tampered with.")
return []
}

Expand All @@ -58,4 +58,23 @@ public class FileUnzipper: Unzipping {

return entryNames
}
///
/// Validates the entryUrl path
/// - Parameters:
/// - entryUrl: The url for the entry being extracted from the archive
/// - destination: The destination url for the entry being extracted
/// - Returns: True if valid, false if invalid
private func isValidPath(_ entryUrl: URL, destination: URL) -> Bool {
let destinationComponents = canonicalize(destination).pathComponents
let entryComponents = canonicalize(entryUrl).pathComponents
return destinationComponents.count < entryComponents.count && !zip(destinationComponents, entryComponents).contains(where: !=)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, learnt a new method zip

}

///
/// Canonicalizes the Url by standardizing as a file url and resolving symlinks in the path
/// - Parameter url: The url to canonicalize
/// - Returns: The canonicalized url
private func canonicalize(_ url: URL) -> URL {
url.standardizedFileURL.resolvingSymlinksInPath()
}
}
Binary file added AEPServices/Tests/resources/TestZipSlip.zip
View file
Open in desktop
Binary file not shown.
69 changes: 53 additions & 16 deletions AEPServices/Tests/services/UnzipperTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,13 @@
@testable import AEPServices
import XCTest


// TODO: - Add more robust testing. Also make sure to make use of the new return type for the unzip api in the tests
class FileUnzipperTest: XCTestCase {
let unzipper = FileUnzipper()
let testDataFileName = "TestRules"
let testLargeFileName = "TestLarge"
let testCorruptFileName = "TestCorruptFile"
let testInvalidCompressionMethodFileName = "TestInvalidCompressionMethod"
let testZipSlipFileName = "TestZipSlip"

enum TestFileNames: String {
case testDataSubFolderRulesName = "rules"
Expand All @@ -38,19 +37,11 @@ class FileUnzipperTest: XCTestCase {
return tempZipDirectory
}()

override func setUp() {
do {
let fileManager = FileManager()
guard let path = FileUnzipperTest.bundle.url(forResource: testDataFileName, withExtension: "zip")?.deletingLastPathComponent().appendingPathComponent(testDataFileName) else {
return
}
try fileManager.removeItem(at: path)
} catch {
return
}
}

///
/// Test a simple unzip of a sample zip file which holds a rules.txt file
///
func testUnzippingRulesSuccessSimple() {
removeResourceWith(name: testDataFileName)
guard let sourceURL = getResourceURLWith(name: testDataFileName) else {
XCTFail()
return
Expand All @@ -60,7 +51,11 @@ class FileUnzipperTest: XCTestCase {
XCTAssertFalse(unzippedItems.isEmpty)
}

///
/// Test to make sure that when unzipping the archive, the correct entries are found at the destination
///
func testUnzippingRulesSuccessFilesExist() {
removeResourceWith(name: testDataFileName)
let fileManager = FileManager()
guard let sourceURL = getResourceURLWith(name: testDataFileName) else {
XCTFail()
Expand Down Expand Up @@ -108,6 +103,9 @@ class FileUnzipperTest: XCTestCase {
XCTAssert(imageFileExists)
}

///
/// Test to make sure correct behavior when unzipping a file which doesn't exist
///
func testUnzippingRulesDoesntExist() {
let testFileName = "doesntExist"
let testFileExt = ".zip"
Expand All @@ -117,6 +115,9 @@ class FileUnzipperTest: XCTestCase {
XCTAssertTrue(unzippedItems.isEmpty)
}

///
/// Test that correct errors are thrown when extracting a corrupt file
///
func testExtractCorruptFile() {
guard let sourceUrl = getResourceURLWith(name: testCorruptFileName) else {
XCTFail()
Expand Down Expand Up @@ -148,6 +149,9 @@ class FileUnzipperTest: XCTestCase {
}
}

///
/// Test that correct error is thrown when invalid compression method is used for zip file
///
func testExtractInvalidCompressionMethod() {
guard let sourceUrl = getResourceURLWith(name: testInvalidCompressionMethodFileName) else {
XCTFail()
Expand All @@ -172,6 +176,9 @@ class FileUnzipperTest: XCTestCase {
}
}

///
/// Test large unzip file performance
///
func testLargeUnzipPerformance() {
guard let sourceUrl = getResourceURLWith(name: testLargeFileName) else {
XCTFail()
Expand All @@ -194,8 +201,11 @@ class FileUnzipperTest: XCTestCase {
}
}

///
/// Test small unzip file performance
///
func testSmallUnzipPerformance() {

removeResourceWith(name: testDataFileName)
guard let sourceURL = getResourceURLWith(name: testDataFileName) else {
XCTFail()
return
Expand All @@ -207,8 +217,35 @@ class FileUnzipperTest: XCTestCase {
}
}

///
/// Test Zip Slip attempt does not succeed
///
func testZipSlipCatch() {
removeResourceWith(name: testZipSlipFileName)
guard let sourceURL = getResourceURLWith(name: testZipSlipFileName) else {
XCTFail()
return
}

let destinationURL = sourceURL.deletingLastPathComponent().appendingPathComponent(testZipSlipFileName)
let unzippedItems = unzipper.unzipItem(at: sourceURL, to: destinationURL)
XCTAssertTrue(unzippedItems.isEmpty)
}

// MARK: - Helpers
func getResourceURLWith(name: String) -> URL? {
private func getResourceURLWith(name: String) -> URL? {
return FileUnzipperTest.bundle.url(forResource: name, withExtension: "zip")
}

private func removeResourceWith(name: String) {
do {
let fileManager = FileManager()
guard let path = FileUnzipperTest.bundle.url(forResource: name, withExtension: "zip")?.deletingLastPathComponent().appendingPathComponent(name) else {
return
}
try fileManager.removeItem(at: path)
} catch {
return
}
}
}