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

Fix path traversal vulnerability #100

Merged
merged 4 commits into from Oct 18, 2022

Conversation

aidy1991
Copy link
Contributor

@aidy1991 aidy1991 commented Oct 17, 2022

See https://nvd.nist.gov/vuln/detail/CVE-2007-4559

extractall() of tarfile has path traversal vulnerability. We need to check file path before using extractall() to avoid it.
If we need to add a unit test for this, please let me know. I added a unit test.

How to reproduce

  1. Create tar file by below codes
  2. Extract the tar file by right click menu of jupyter-archive
  3. /tmp/test will be created (After this PR, the request of extracting will be failed as 400 bad request)
import tarfile
open("test", 'a').close()
tf = tarfile.open("test.tar.gz", "w:gz")
tf.add("test", "../../../../../../../../../../tmp/test")
tf.close()

Unrelated to this PR, is it the intended behavior that the user is not notified of an error when an extract request fails?

@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch aidy1991/jupyter-archive/fix-path-traversal

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @aidy1991 I have a suggestion.

Regarding reporting an error - this is a very good suggestion. Feel free to open a PR for it (I can provide some pointers if needed).

jupyter_archive/handlers.py Outdated Show resolved Hide resolved
archive_path = jp_root_dir / "test.tar.gz"
open(unsafe_file_path, 'a').close()
with tarfile.open(archive_path, "w:gz") as tf:
tf.add(unsafe_file_path, "../../../../../../../../../../tmp/test")
Copy link
Member

Choose a reason for hiding this comment

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

We could parametrize this test to add also "../test".

Copy link
Contributor Author

@aidy1991 aidy1991 Oct 18, 2022

Choose a reason for hiding this comment

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

I see, it looks good. I'll fix to use parametrize and add ../test

aidy1991 and others added 2 commits October 18, 2022 10:21
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
@aidy1991
Copy link
Contributor Author

@fcollonval
Could you take another look?

Regarding reporting an error - this is a very good suggestion. Feel free to open a PR for it (I can provide some pointers if needed).

Okay, I will try to work on the issue.

Comment on lines +215 to +216
("../../../../../../../../../../tmp/test"),
("../test"),
Copy link
Member

Choose a reason for hiding this comment

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

🚀

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @aidy1991

@fcollonval fcollonval merged commit 0311cf0 into jupyterlab-contrib:master Oct 18, 2022
@aidy1991 aidy1991 deleted the fix-path-traversal branch October 18, 2022 13:40
@aidy1991
Copy link
Contributor Author

Thank you for your review 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants