Opened 3 years ago
Closed 2 years ago
#27391 closed enhancement (invalid)
some additional checks about sws files
Reported by: | chapoton | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | packages: standard | Keywords: | |
Cc: | embray, jdemeyer, dcoudert, tscrim, dimpase | Merged in: | |
Authors: | Frédéric Chapoton | Reviewers: | |
Report Upstream: | Reported upstream. Developers acknowledge bug. | Work issues: | |
Branch: | u/chapoton/27391 (Commits, GitHub, GitLab) | Commit: | 01382e402b2d7ba3f98df8c2e5b31f78218319d6 |
Dependencies: | Stopgaps: |
Description (last modified by )
adding some sanity checks for the extraction of sws files.
Upstream bug: https://bugs.python.org/issue21109
Change History (22)
comment:1 Changed 3 years ago by
- Branch set to u/chapoton/27391
- Commit set to e0bdd71f84b5d31d35328dfbac422bf0d17b5c51
- Status changed from new to needs_review
- Summary changed from some additionnal checks about sws files to some additional checks about sws files
comment:2 Changed 3 years ago by
- Cc dcoudert added
Does anybody have a better way to check that no file is extracted outside the directory ?
comment:3 Changed 3 years ago by
You check that paths don't start with \\
. Shouldn't you also check that it's not /
?
An alternative could be to build absolute paths (using os.path.abspath
) and to check that each absolute path starts with the absolute path of extraction directory.
comment:4 Changed 3 years ago by
feel free to take over!
comment:5 Changed 3 years ago by
- Priority changed from major to critical
comment:6 Changed 3 years ago by
- Commit changed from e0bdd71f84b5d31d35328dfbac422bf0d17b5c51 to 01382e402b2d7ba3f98df8c2e5b31f78218319d6
comment:7 Changed 3 years ago by
better like that ?
comment:8 Changed 3 years ago by
I think so, but I prefer to wait for other opinions. I don't know if one can use other tricks to put files in different directory.
comment:9 Changed 3 years ago by
- Priority changed from critical to blocker
Please have a look, Erik and Jeroen
comment:10 Changed 3 years ago by
Which problem is this ticket supposed to fix? There is very little explanation.
comment:11 Changed 3 years ago by
I am going to forward you an email about that.
comment:12 Changed 3 years ago by
- Component changed from refactoring to packages: standard
- Description modified (diff)
- Priority changed from blocker to major
- Report Upstream changed from N/A to Reported upstream. Developers acknowledge bug.
- Status changed from needs_review to needs_work
I think that this issue should be fixed in Python's tarfile
module. Given that it's known since 2014, there is no way that this is a blocker. After reading the upstream issue, it's clear that there are many more potential problems than just paths starting with /
or containing ..
. So this patch doesn't really fix the security hole.
comment:13 Changed 3 years ago by
but it would be better than nothing, no ??
comment:14 Changed 3 years ago by
- Milestone changed from sage-8.7 to sage-8.8
Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)
comment:16 Changed 3 years ago by
- Status changed from needs_work to needs_info
comment:17 Changed 3 years ago by
I might just leave this open until this is fixed upstream.
comment:18 Changed 3 years ago by
- Milestone changed from sage-8.8 to sage-8.9
Tickets still needing working or clarification should be moved to the next release milestone at the soonest (please feel free to revert if you think the ticket is close to being resolved).
comment:19 Changed 3 years ago by
- Milestone changed from sage-8.9 to sage-9.1
Ticket retargeted after milestone closed
comment:20 Changed 2 years ago by
- Milestone changed from sage-9.1 to sage-9.2
Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.
comment:21 Changed 2 years ago by
- Cc dimpase added
- Milestone changed from sage-9.2 to sage-duplicate/invalid/wontfix
- Status changed from needs_info to needs_review
sagenb stuff, should be closed
comment:22 Changed 2 years ago by
- Resolution set to invalid
- Status changed from needs_review to closed
New commits:
some additionnal checks for the contents of sws files