Go: zip-slip FP / missed a zip-slip guard in argoproj/argo-cd

[jsoref]

https://github.com/github/codeql/blob/590e93d8edec4d7216935ed4425a7ab77b3b2f34/go/ql/src/Security/CWE-022/ZipSlip.ql#L22-L23

Here's my fork's report: https://github.com/check-spelling-sandbox/argo-cd/security/code-scanning/4

---

Arbitrary file access during archive extraction ("Zip Slip")

Code snippet util/io/files/tar.go:75

	tr := tar.NewReader(lr)

	for {
		header, err := tr.Next()

Unsanitized archive entry, which may contain '..', is used in a .

---

Here's the accused flow:

Arbitrary file access during archive extraction ("Zip Slip") Step 1 ... := ...[0] Source util/io/files/tar.go:75

	tr := tar.NewReader(lr)

	for {
		header, err := tr.Next()

Unsanitized archive entry, which may contain '..', is used in a . Unsanitized archive entry, which may contain '..', is used in a . Unsanitized archive entry, which may contain '..', is used in a .

		if err != nil {
			if err == io.EOF {
				break

Step 2 selection of Name util/io/files/tar.go:86

			continue
		}

		target := filepath.Join(dstPath, header.Name)

[!NOTE] There is a check for zip-slip right here in the form of Inbound:

		// Sanity check to protect against zip-slip
		if !Inbound(target, dstPath) {
			return fmt.Errorf("illegal filepath in archive: %s", target)

Step 3 call to Join util/io/files/tar.go:86

			continue
		}

		target := filepath.Join(dstPath, header.Name)
		// Sanity check to protect against zip-slip
		if !Inbound(target, dstPath) {
			return fmt.Errorf("illegal filepath in archive: %s", target)

Step 4 target Sink util/io/files/tar.go:98

			if preserveFileMode {
				mode = os.FileMode(header.Mode)
			}
			err := os.MkdirAll(target, mode)
			if err != nil {
				return fmt.Errorf("error creating nested folders: %w", err)
			}

[smowton]

I'm afraid while the Go library does make an effort to 'promote' sanitizers or validation checks out of wrapper methods like Inbound, the method at https://github.com/check-spelling-sandbox/argo-cd/blob/4014cc8b040f55dc698295d658cf0eb780ea7203/util/io/files/util.go#L83 defeats our current implementation in two ways:

1. The promotion of guards of the form return strings.hasPrefix(...) relies on that being the unique return from the function, but we also have the return false for the case where the input is malformed (to consider: should that be a panic, since it indicates a bug?)
2. The guard promotion code relies on a parameter being tested (perhaps through value-preserving steps such as local variable assignments). However here it isn't candidate that gets tested but rather filepath.Join(candidate, ...) -- we can't currently trace backwards through that and treat it as validation of candidate.

We'll take this into account for future improvements to our guard-promotion logic, but can't promise a timescale on that because it's a nontrivial effort. For the time being I'm afraid you'll have to dismiss this as a false alert.

[jsoref]

In the interim, can the qhelp at least be improved to at least draw people's attention to these deficiencies? https://github.com/github/codeql/blob/590e93d8edec4d7216935ed4425a7ab77b3b2f34/go/ql/src/Security/CWE-022/ZipSlip.qhelp#L33-L41

I understand that improved algorithms take time, but if I can get some documentation improved to save me from similar reports in the interim I would definitely take/appreciate that.

[smowton]

I note this isn't zipslip-specific -- there are always a lot of ways to sanitize something, and all our queries do their best to identify when you have made an appropriate check, but will sometimes fail to identify the check and so raise an alert regardless. So I will share the feedback, but probably won't make a zipslip-specific note for this.