Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEVPROD-7289 Resolve symlinks and links correctly when extracting tarballs #8574

Merged
merged 11 commits into from
Jan 2, 2025

Conversation

ZackarySantana
Copy link
Contributor

@ZackarySantana ZackarySantana commented Dec 19, 2024

DEVPROD-7289

Description

This switches over to using the absolute path when extracting files

Testing

Old task has a tarball with the symlink file, the symlinked file (original) and tries to extract it. It fails with permission denied.

New tasks has two, one with it incorrectly set up with only the symlinked file (original) in the tarball, and one with it correctly with the symlink file and the symlinked file (original) which passes.

@ZackarySantana ZackarySantana self-assigned this Dec 19, 2024
agent/command/archive_util.go Fixed Show resolved Hide resolved
agent/command/archive_util.go Fixed Show resolved Hide resolved
@ZackarySantana ZackarySantana requested review from a team and removed request for a team December 19, 2024 17:17
@ZackarySantana ZackarySantana requested a review from a team December 19, 2024 18:33
agent/command/archive_util.go Fixed Show fixed Hide fixed
agent/command/archive_util.go Fixed Show fixed Hide fixed
@bynn
Copy link
Contributor

bynn commented Dec 19, 2024

it looks like there's a security risk involved with using absolute paths because it could potentially be abused by symlinking something they shouldn't have access to (that's what I'm assuming) should we still do this then?

@ZackarySantana
Copy link
Contributor Author

Hm, if it tried symlinking something they shouldn't have access to it would get permission denied. The agent app has the same permission as the user / vice versa. The issue that was lately was what you described but unintentional (it was symlinking to /<arfiact_file> rather than /<task_dir>/<artifact_file>

@ZackarySantana
Copy link
Contributor Author

Let me try out the security suggestion the code quality says (I think it is a non-problem but it can't hurt!)

Copy link
Contributor

@bynn bynn left a comment

Choose a reason for hiding this comment

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

I think the new security alert can be dealt with just by validating hdr.Name isn't something weird but it looks good ty for fixing!!!

lgtm with the tests fixed!

@ZackarySantana ZackarySantana merged commit 4f6dda2 into evergreen-ci:main Jan 2, 2025
8 of 10 checks passed
@ZackarySantana ZackarySantana deleted the DEVPROD-7289 branch January 2, 2025 18:11
@ZackarySantana ZackarySantana changed the title DEVPROD-7289 Use absolute path for syslink files when extracting tar.gz files DEVPROD-7289 Resolve symlinks and links correctly when extracting tarballs Jan 2, 2025
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