Swift: Add Unsafe Unpacking Query (CWE-022)

[maikypedia]

This pull request adds a query for Unsafe Unpacking covering Zip and ZIPFoundation.

Looking forward to your suggestions.

[ghsecuritylab]

Hello maikypedia 👋
You have submitted this pull request as a bug bounty report in the github/securitylab repository and therefore this pull request has been put into draft state to give time for the GitHub Security Lab to assess the PR. When GitHub Security Lab has finished assessing your pull request, it will be marked automatically as Ready for review. Until then, please don't change the draft state.

In the meantime, feel free to make changes to the pull request. If you'd like to maximize payout for your this and future submissions, here are a few general guidelines, that we might take into consideration when reviewing a submission.

• the submission models widely-used frameworks/libraries
• the vulnerability modeled in the submission is impactful
• the submission finds new true positive vulnerabilities
• the submission finds very few false positives
• code in the submission is easy to read and will be easy to maintain
• documentation is written clearly, highlighting the impact of the issue it finds and is written without grammatical or other errors. The code samples clearly show the vulnerability
• the submission includes tests, change note etc.

Please note that these are guidelines, not rules. Since we have a lot of different types of submissions, the guidelines might vary for each submission.

Happy hacking!

[geoffw0]

Hi @maikypedia,

Thank you for this contribution. The QL appears to be to a high standard and I'm interested to see what this query can find. After the Security Lab team have taken a look I'll be happy to give a more detailed review from the Swift / QL perspective. Feel free to @mention me if you have any questions before then.

[github-actions]

QHelp previews:

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

Arbitrary file write during a zip extraction from a user controlled source

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.

Recommendation

Consider using a safer module, such as: ZIPArchive

Example

The following examples unpacks a remote zip using `Zip.unzipFile()` which is vulnerable to path traversal.

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()

The following examples unpacks a remote zip using `fileManager.unzipItem()` which is vulnerable to symlink path traversal.

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()

Consider using a safer module, such as: ZIPArchive

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()

References

• Ostorlab: Zip Packages Exploitation.
• Common Weakness Enumeration: CWE-22.

[geoffw0]

Hi, sorry for the long wait. The query looks great, I've suggested a few minor fixes and changes.

[geoffw0]

Thanks for addressing the comments.

CI has noticed that we're missing a change note. This will be a new file in cpp\ql\src\change-notes with contents along the lines of:

---
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.

[geoffw0]

Great, I think this is ready to merge.

Thank you for your contribution! ✨

[geoffw0]

@maikypedia you should be able to hit "Merge pull request" now if you're also satisfied that this is finished.

[maikypedia]

@maikypedia you should be able to hit "Merge pull request" now if you're also satisfied that this is finished.

I can't 😅

[geoffw0]

Ah, sorry, I didn't know that was a thing. Merging now.