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.

Sign up for GitHub

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

Jump to bottom

Fix code scanning alert no. 4: Arbitrary file write during tarfile extraction #495

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Fix code scanning alert no. 4: Arbitrary file write during tarfile extraction #495

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions dpdispatcher/contexts/ssh_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,6 +972,9 @@
self.ssh_session.get(from_f, to_f)
# extract
with tarfile.open(to_f, mode=tarfile_mode) as tar:
for member in tar.getmembers():
if os.path.isabs(member.name) or ".." in member.name:
raise ValueError(f"Illegal tar archive entry: {member.name}")

Check warning on line 977 in dpdispatcher/contexts/ssh_context.py

View check run for this annotation

Codecov / codecov/patch

dpdispatcher/contexts/ssh_context.py#L975-L977 

Added lines #L975 - L977 were not covered by tests
Comment on lines +975 to +977
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add unit tests for the new security validation

Lines 975-977 introduce critical validation to prevent security vulnerabilities. Currently, these lines are not covered by tests. Adding unit tests will ensure that the validation works as intended and remains effective against potential directory traversal attacks.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 975-977: dpdispatcher/contexts/ssh_context.py#L975-L977
Added lines #L975 - L977 were not covered by tests

⚠️ Potential issue

Enhance directory traversal protection in tar extraction

The current validation in the _get_files method checks if member.name is absolute or contains ".." to prevent directory traversal attacks. However, this approach may not be sufficient, as attackers can craft filenames like "../" or use multiple consecutive dots to bypass the check.

Consider normalizing the path using os.path.normpath() and validating that the resulting path does not escape the intended extraction directory.

Apply this diff to improve the security validation:

 for member in tar.getmembers():
+    member_path = os.path.normpath(member.name)
-    if os.path.isabs(member.name) or ".." in member.name:
+    if os.path.isabs(member_path) or member_path.startswith(".."):
         raise ValueError(f"Illegal tar archive entry: {member.name}")
+    member.name = member_path
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for member in tar.getmembers():
if os.path.isabs(member.name) or ".." in member.name:
raise ValueError(f"Illegal tar archive entry: {member.name}")
for member in tar.getmembers():
member_path = os.path.normpath(member.name)
if os.path.isabs(member_path) or member_path.startswith(".."):
raise ValueError(f"Illegal tar archive entry: {member.name}")
member.name = member_path
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 975-977: dpdispatcher/contexts/ssh_context.py#L975-L977
Added lines #L975 - L977 were not covered by tests

tar.extractall(path=self.local_root)
# cleanup
os.remove(to_f)
Expand Down
Loading