Skip to content

Fix path traversal in download_dir via server-controlled filenames#81

Merged
OscarAkaElvis merged 1 commit into
Hackplayers:devfrom
TristanInSec:fix/download-path-traversal
Jun 1, 2026
Merged

Fix path traversal in download_dir via server-controlled filenames#81
OscarAkaElvis merged 1 commit into
Hackplayers:devfrom
TristanInSec:fix/download-path-traversal

Conversation

@TristanInSec
Copy link
Copy Markdown

Summary

  • download_dir() parses filenames from Get-ChildItem stdout and passes them directly to File.join() to build local file paths
  • A rogue server can return filenames containing ../ sequences (e.g. ../../.ssh/authorized_keys), causing files to be written outside the intended download directory
  • Ruby's File.join does not sanitize traversal sequences: File.join("/home/user/loot", "../../.ssh/authorized_keys") resolves to /home/user/.ssh/authorized_keys

Fix

Use File.basename() on server-supplied filenames for the local path construction only. The remote download path still uses the original filename so the correct file is fetched. Entries that resolve to empty, ., or .. are skipped.

Steps to reproduce

  1. Set up a rogue Windows server that returns crafted filenames via Get-ChildItem
  2. Connect with evil-winrm and run download C:\loot\*
  3. Server returns filename ../../.ssh/authorized_keys
  4. File is written to ~/.ssh/authorized_keys instead of the download directory

download_dir() uses filenames from Get-ChildItem stdout directly in
File.join() to construct local paths. A rogue server can return names
containing ../ sequences, writing files outside the download directory.

Use File.basename() on server-supplied filenames for the local path to
strip directory components while preserving the original name for the
remote download path.
@cybervaca
Copy link
Copy Markdown
Collaborator

Thanks for the PR and for reporting this issue.

Could you please re-submit the PR against the dev branch instead of master? We do not accept pull requests directly into master.

Once it is opened against dev, we will review it there.

Thanks again for the contribution.

@TristanInSec TristanInSec changed the base branch from master to dev May 26, 2026 16:55
@TristanInSec
Copy link
Copy Markdown
Author

Done -- rebased the PR to target dev. Thanks for the heads-up.

@OscarAkaElvis
Copy link
Copy Markdown
Member

I've been doing some tests and everything looks good to me. Merging.

@OscarAkaElvis OscarAkaElvis merged commit 6ecd570 into Hackplayers:dev Jun 1, 2026
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.

3 participants