fix Arbitrary file write extracting an archive containing symbolic links on common()

[odaysec]

elastic-agent/dev-tools/mage/common.go

Line 493 in 2a274c2

if err := os.Symlink(header.Linkname, path); err != nil {

fix the issue need to ensure that the target of the symbolic link (header.Linkname) is resolved and validated before creating the symlink. Specifically:

1. Use filepath.EvalSymlinks to resolve any symbolic links in the target path (header.Linkname).
2. Check that the resolved path is within the destinationDir to prevent directory traversal attacks.
3. Only create the symlink if the resolved path is valid.

The changes will be made in the untar function, specifically in the case handling tar.TypeSymlink.

Extracting symbolic links from a malicious zip archive, without validating that the destination file path is within the destination directory, can cause files outside the destination directory to be overwritten. This can happen if there are previously-extracted symbolic links or directory traversal elements and links (..) in archive paths. This problem is related to the ZipSlip vulnerability which is detected by the go/zipslip query; please see that query's help for more general information about malicious archive file vulnerabilities. This query considers the specific case where symbolic links are extracted from an archive, in which case the extraction code must be aware of existing symbolic links when checking whether it is about to extract a link pointing to a location outside the target extraction directory.

Recommendation

Ensure that output paths constructed from zip archive entries are validated. This includes resolving any previously extracted symbolic links, using path/filepath.EvalSymlinks, to prevent writing files or links to unexpected locations.

POC

links are extracted from an archive using the syntactic filepath.Rel function to check whether the link and its target fall within the destination directory. However, the extraction code doesn't resolve previously-extracted links, so a pair of links like subdir/parent -> .. followed by escape -> subdir/parent/.. -> subdir/../.. leaves a link pointing to the parent of the archive root. The syntactic Rel is ineffective because it equates subdir/parent/.. with subdir/, but this is not the case when subdir/parent is a symbolic link.

package main

import (
	"archive/tar"
	"io"
	"os"
	"path/filepath"
	"strings"
        "elastic/elastic-agent"
)

func isRel(candidate, target string) bool {
	// BAD: purely syntactic means are used to check
	// that `candidate` does not escape from `target`
	if filepath.IsAbs(candidate) {
		return false
	}
	relpath, err := filepath.Rel(target, filepath.Join(target, candidate))
	return err == nil && !strings.HasPrefix(filepath.Clean(relpath), "..")
}

func unzipSymlink(f io.Reader, target string) {
	r := tar.NewReader(f)
	for {
		header, err := r.Next()
		if err != nil {
			break
		}
		if isRel(header.Linkname, target) && isRel(header.Name, target) {
			os.Symlink(header.Linkname, header.Name)
		}
	}
}

To fix this vulnerability, resolve pre-existing symbolic links before checking that the link's target is acceptable:

package main

func isRel(candidate, target string) bool {
	// GOOD: resolves all symbolic links before checking
	// that `candidate` does not escape from `target`
	if filepath.IsAbs(candidate) {
		return false
	}
	realpath, err := filepath.EvalSymlinks(filepath.Join(target, candidate))
	if err != nil {
		return false
	}
	relpath, err := filepath.Rel(target, realpath)
	return err == nil && !strings.HasPrefix(filepath.Clean(relpath), "..")
}

References

Zip Slip Vulnerability
Path Traversal
CWE-22

Checklist

• I have read and understood the pull request guidelines of this project.
• My code follows the style guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have made corresponding change to the default configuration files
• I have added tests that prove my fix is effective or that my feature works
• I have added an entry in ./changelog/fragments using the changelog tool
• I have added an integration test or an E2E test

[mergify]

This pull request does not have a backport label. Could you fix it @odaysec? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-./d./d is the label that automatically backports to the 8./d branch. /d is the digit
• backport-active-all is the label that automatically backports to all active branches.
• backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
• backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

[elasticmachine]

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

[ycombinator]

Hi @odaysec, thank you for this contribution. It seems the additional safety check you added is pretty generic, so it would be good to extract it into it's own function, something like safeSymlink that takes path, destinationDir, and header.LinkName as arguments.

Also, we use mage package to build a tarball artifact for release. When I ran this with this PR, I'm getting a failure with the error message, error resolving symlink target ...:

$ SNAPSHOT=true EXTERNAL=true PLATFORMS="linux/arm64" PACKAGES="targz" mage clean package

--- Package Elastic-Agent
...
--- Package artifact
>> package: Building linux-arm64
>> package: Building elastic-agent type=tar.gz for platform=linux/arm64 fips=false
--- TestPackages, the generated packages (i.e. file modes, owners, groups).
--- TestPackages
>> Testing package contents
package ran for 5m40.999178083s
Error: error running package_test.go: running "go test /Users/shaunak/development/github/elastic-agent/dev-tools/packaging/testing/package_test.go -root-owner -files /Users/shaunak/development/github/elastic-agent/build/distributions/*" failed with exit code 1, stdout: --- FAIL: TestTar (10.47s)
    --- FAIL: TestTar/elastic-agent-9.1.0-SNAPSHOT-linux-arm64.tar.gz (10.47s)
        package_test.go:229:
            	Error Trace:	/Users/shaunak/development/github/elastic-agent/dev-tools/packaging/testing/package_test.go:229
            	            				/Users/shaunak/development/github/elastic-agent/dev-tools/packaging/testing/package_test.go:103
            	Error:      	Received unexpected error:
            	            	error resolving symlink target /var/folders/mc/qr61t2n91z3g63ffq5sy4cm00000gn/T/TestTarelastic-agent-9.1.0-SNAPSHOT-linux-arm64.tar.gz1945348434/001/data/elastic-agent-bfa34f/elastic-agent: lstat /private/var/folders/mc/qr61t2n91z3g63ffq5sy4cm00000gn/T/TestTarelastic-agent-9.1.0-SNAPSHOT-linux-arm64.tar.gz1945348434/001/data: no such file or directory
            	Test:       	TestTar/elastic-agent-9.1.0-SNAPSHOT-linux-arm64.tar.gz
            	Messages:   	error extracting archive "/Users/shaunak/development/github/elastic-agent/build/distributions/elastic-agent-9.1.0-SNAPSHOT-linux-arm64.tar.gz"
FAIL
FAIL	command-line-arguments	10.789s
FAIL

Could you please look into this and make the necessary fixes in this PR? Thanks.