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

[jsoref]

codeql/go/ql/src/Security/CWE-022/ZipSlip.ql

Lines 22 to 23 in 590e93d

	"Unsanitized archive entry, which may contain '..', is used in a $@.", sink.getNode(),
	"file system operation"

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?

codeql/go/ql/src/Security/CWE-022/ZipSlip.qhelp

Lines 33 to 41 in 590e93d

	<p>
	Ensure that output paths constructed from zip archive entries are validated
	to prevent writing files to unexpected locations.
	</p>
	<p>
	The recommended way of writing an output file from a zip archive entry is to check that
	"<code>..</code>" does not occur in the path.
	</p>

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.