Skip to content

Conversation

@antitree
Copy link

@antitree antitree commented Aug 1, 2025

Summary

  • Fixed path traversal vulnerability in pkg/apk/apk/cache.go
  • Replaced unsafe filepath.Clean + strings.HasPrefix validation with Go 1.24's os.Root
  • Added protection against both directory traversal and symlink attacks

Security Issue

The previous validation in cachePathFromURL() could be bypassed:

// Vulnerable code
cacheFile = filepath.Clean(cacheFile)
cleanroot := filepath.Clean(root)
if \!strings.HasPrefix(cacheFile, cleanroot) {
    return "", fmt.Errorf("cache file %s is not within root %s", cacheFile, cleanroot)
}

Attack example: With root="/cache", a malicious path like "/cache-evil/file" would pass the prefix check.

Solution

Implemented Go 1.24's os.Root which provides:

  • Traversal-resistant path validation
  • Protection against symlink attacks
  • Platform-specific security optimizations
  • Proper containment within the specified root directory

Test Plan

  • All existing tests pass
  • Code builds successfully
  • go vet passes
  • Proper code formatting applied

🤖 Generated with Claude Code

Replace unsafe filepath.Clean + strings.HasPrefix validation with
Go 1.24's os.Root for secure path resolution. This prevents path
traversal attacks including symlink-based attacks.

The previous validation could be bypassed with paths like
"/cache-evil/file" which would pass the prefix check for root "/cache".

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@antitree
Copy link
Author

antitree commented Aug 1, 2025

@antitree antitree requested a review from imjasonh August 1, 2025 16:43
Copy link

@tstromberg tstromberg left a comment

Choose a reason for hiding this comment

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

To prevent a future regression, this should get a test.

}

// Return the absolute path for use by other functions
return filepath.Join(root, relativePath), nil

Choose a reason for hiding this comment

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

"root" isn't necessarily an absolute path; if this comment is to be accurate, you should use filepath.Abs.

return "", fmt.Errorf("cache file %s is not within root %s", cacheFile, cleanroot)
relativePath := filepath.Join(repoDir, dir, filename)

// Use os.Root for secure path validation to prevent path traversal

Choose a reason for hiding this comment

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

Look out for other calls to filepath.Join here to see whether or not they are similarly vulnerable - https://github.com/chainguard-dev/apko/pull/1795/files#diff-085f287e1519fcab77329f1fededeae7b771c1e18f0522e4c4f1fdbe3c202d3bR376 looks suspicious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants