Skip to content

Conversation

opsysdebug
Copy link

@opsysdebug opsysdebug commented Aug 31, 2025

To properly fix this issue:

  • When extracting symlinks, you must ensure the symlink path created will, when resolved, not point outside the intended extraction directory.
  • This requires, before creating the symlink at path pointing to header.Linkname, that you:
    1. Ensure that (a) path is in destinationDir, and (b) the link target (when resolved as if the symlink would exist) would still be within destinationDir.
    2. This involves simulating (or after creation, confirming) that resolving the symlink as placed would not escape.
    3. Additionally, all writes (not just symlink creation) should, before writing, resolve their final location with filepath.EvalSymlinks, starting from destinationDir and the archive entry, to confirm it's "in jail".

For minimal safe fix (without refactoring to check every write):

  • For the symlink case, do not create the symlink if:
    • Its path is not in destinationDir
    • Its resolved destination (as would be followed by a process in destinationDir) does not remain under destinationDir.
  • This requires a helper: a function that, for a desired symlink at path pointing to header.Linkname, simulates following the resulting symlink and checks if it is inside destinationDir.

Changes to implement:

  • Add a helper function, e.g., isSafeSymlink(linkPath, linkTarget, destinationDir string) (bool, error) that:
    • Joins the linkPath's parent and linkTarget to get the symlink destination as seen from the filesystem.
    • Resolves the result using filepath.EvalSymlinks.
    • Checks if the resulting path is still under destinationDir.
  • In the tar.TypeSymlink case, conditionally create the symlink only if this check passes, else error.

What is needed:

  • A new helper function.
  • Possibly import "path/filepath" if not already present (already imported here).
  • Minimal code changes inside the case tar.TypeSymlink block of untar.

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

@opsysdebug opsysdebug requested a review from a team as a code owner August 31, 2025 03:42
Copy link

❌ Author of the following commits did not sign a Contributor Agreement:
0de99a0

Please, read and sign the above mentioned agreement if you want to contribute to this project

Copy link
Contributor

mergify bot commented Aug 31, 2025

This pull request does not have a backport label. Could you fix it @opsysdebug? 🙏
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.

@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Sep 2, 2025
@elasticmachine
Copy link
Collaborator

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

@pierrehilbert
Copy link
Contributor

Hello @opsysdebug
Thanks for contributing to our repository.
Could you please sign our CLA?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants