Skip to content
This repository has been archived by the owner on Jan 5, 2024. It is now read-only.

Check extract path and ensure minimal perms #67

Merged
merged 5 commits into from
Mar 17, 2021
Merged

Conversation

sssoleileraaa
Copy link
Contributor

@sssoleileraaa sssoleileraaa commented Mar 16, 2021

Summary

When a file is received from sd-app, ensure that directories are created using minimal permissions and extracted only to allowed paths.

Test Plan for Export

Sanity check

  1. see if you can attach files with malicious filenames from the Source Interface (ones with .. and /)
  • confirm that currently a leading / gets stripped and you can't attach a file that leads with a ..

Ensure safe extraction

  1. checkout first commit in this PR (no fix yet)
  2. run make semgrep
    • see community rules pass
    • see rule:semgrep.makedirs-insecure-permissions fail
    • see rule:semgrep.tarfile-extractall-traversal fail
  3. run make test
    • see test_extract_tarball fail because files have insecure 644 permissions instead of 600
    • see test_extract_tarball_raises_if_doing_path_traversal fail because it does not raise (also see that /tmp/traversed exists)
    • see test_extract_tarball_raises_if_doing_path_traversal_with_symlink fail because it does not raise
  4. rm /tmp/traversed and checkout second commit in this PR (with the fix)
  5. run make semgrep
    • confirm all checks pass
  6. run make test
    • confirm all tests pass
      • Note: You can also create the tarfiles directly that you see in test_extract_tarball_raises_if_doing_path_traversal and test_extract_tarball_raises_if_doing_path_traversal_with_symlink, and qvm-open-in-vm from sd-app to sd-devices the way you see in the audit to perform this check another way.

Ensure no regressions

  • confirm you can successfully export files from the client

Allie Crevier added 3 commits March 16, 2021 14:49
@conorsch
Copy link
Contributor

We're seeing a CI failure related to the py dependency: https://app.circleci.com/pipelines/github/freedomofpress/securedrop-export/200/workflows/aa192726-304c-404c-ba7a-9e62277fc31e/jobs/532 That's test-only, so I'm continue to proceed with review and file a follow-up issue to address the safety check.

@conorsch conorsch force-pushed the check-extract-path branch 2 times, most recently from e1a51c6 to 8347114 Compare March 16, 2021 22:32
Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Approving these changes based on prior functional review.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants