Opened 10 months ago

Last modified 6 months ago

#27391 needs_info enhancement

some additional checks about sws files

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.9
Component: packages: standard Keywords:
Cc: embray, jdemeyer, dcoudert, tscrim Merged in:
Authors: Frédéric Chapoton Reviewers:
Report Upstream: Reported upstream. Developers acknowledge bug. Work issues:
Branch: u/chapoton/27391 (Commits) Commit: 01382e402b2d7ba3f98df8c2e5b31f78218319d6
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

adding some sanity checks for the extraction of sws files.

Upstream bug: https://bugs.python.org/issue21109

Change History (18)

comment:1 Changed 10 months ago by chapoton

  • 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

New commits:

e0bdd71some additionnal checks for the contents of sws files

comment:2 Changed 10 months ago by chapoton

  • Cc dcoudert added

Does anybody have a better way to check that no file is extracted outside the directory ?

comment:3 Changed 10 months ago by dcoudert

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 10 months ago by chapoton

feel free to take over!

comment:5 Changed 10 months ago by chapoton

  • Priority changed from major to critical

comment:6 Changed 10 months ago by git

  • Commit changed from e0bdd71f84b5d31d35328dfbac422bf0d17b5c51 to 01382e402b2d7ba3f98df8c2e5b31f78218319d6

Branch pushed to git repo; I updated commit sha1. New commits:

98fb33bMerge branch 'u/chapoton/27391' in 8.7.b6
01382e4trac 27391 better fix

comment:7 Changed 10 months ago by chapoton

better like that ?

comment:8 Changed 10 months ago by dcoudert

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 9 months ago by chapoton

  • Priority changed from critical to blocker

Please have a look, Erik and Jeroen

comment:10 Changed 9 months ago by jdemeyer

Which problem is this ticket supposed to fix? There is very little explanation.

comment:11 Changed 9 months ago by chapoton

I am going to forward you an email about that.

comment:12 Changed 9 months ago by jdemeyer

  • 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 9 months ago by chapoton

but it would be better than nothing, no ??

comment:14 Changed 9 months ago by embray

  • 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:15 Changed 8 months ago by chapoton

  • Cc tscrim added

so should we just close that one, or what ???

comment:16 Changed 8 months ago by chapoton

  • Status changed from needs_work to needs_info

comment:17 Changed 8 months ago by tscrim

I might just leave this open until this is fixed upstream.

comment:18 Changed 6 months ago by embray

  • 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).

Note: See TracTickets for help on using tickets.