Repository navigation
Conversation
cf472cf to
55108cb
Compare
| members.append(member) | ||
| for output_dir, members in members_by_dir.items(): | ||
| subprocess.run(["tar", "-C", output_dir, "-xf", filepath, *members], check=True) | ||
| filter_kwargs: Dict[str, str] = {"filter": "data"} if hasattr(tarfile, "data_filter") else {} |
There was a problem hiding this comment.
Unsafe members are only rejected when tarfile.data_filter exists. On Python 3.7.17 (python:3.7, hasattr(tarfile, "data_filter") → False; 3.7 is what the min_linux CI job uses) a tar with a ../escaped.txt member is extracted outside output_dir with no error. GNU tar 1.35 (previous behaviour) refuses it (exit 2, nothing written outside), the zip path uses _safe_join, and on 3.13 the data filter raises ArchiveError. Suggest calling _safe_join(output_dir, name) for each member before extracting (I tried it: the existing tests pass and ../ and absolute-path members are rejected even without the data filter), plus a tar counterpart of the zip-slip test (escaped.txt)
There was a problem hiding this comment.
Proposed fix: c375f18
- For named members: adds _safe_join(output_dir, member) before tar.extract(...) (utils.py:417).
- For extract-all (falsy member): iterates tar.getmembers() and validates each with _safe_join(output_dir, info.name) before tar.extractall(...) (utils.py:420-421).
- _safe_join (utils.py:333) resolves real paths and raises on escape; the surrounding except Exception block (utils.py:425) converts it to ArchiveError.
- This guards all Python versions, matching the previous GNU-tar behavior and the zip path which already used _safe_join.
- Adds the requested regression test test_tar_slip_rejected (tests/test_utils.py:144) which builds a ../escaped.txt tar, asserts ArchiveError is raised, and asserts the file is not written outside the output dir.
| with tarfile.open(filepath) as tar: | ||
| names = set(tar.getnames()) | ||
| for output_dir, members in members_by_dir.items(): | ||
| os.makedirs(output_dir, exist_ok=True) |
There was a problem hiding this comment.
nit: os.makedirs(output_dir, exist_ok=True) is redundant. _process_archive already creates dest_dir right before calling extract_archive (download_manager.py L345, its only caller), and tarfile creates missing parents itself. Tests pass without it, and extracting into a non-existent dir works for both named members and extract-all
There was a problem hiding this comment.
c375f18 removes the os.makedirs(output_dir, exist_ok=True) line from the loop (utils.py:411 in the old version).
The commit message and the docstring (utils.py:393-394) were also updated to document the new _safe_join validation fallback for older Pythons.
The external GNU tar relies on the openat2 syscall, which snapd's seccomp profile denies inside strictly confined snaps, making every tar.gz/tgz extraction fail with "Cannot open: Operation not permitted". Extracting with the standard library tarfile avoids the syscall and works in any environment. The tarfile "data" filter is used on Python 3.12+ to reject unsafe members; older Pythons keep the previous behaviour. Signed-off-by: Alexandre Esse <alexandre.esse@canonical.com>
The tarfile data filter only guards Python 3.12+, so on older Pythons (e.g. 3.7 used by min_linux CI) members escaping output_dir were extracted silently. Validate each member path with _safe_join before extraction and drop the redundant os.makedirs. Add a tar-slip test. Signed-off-by: Alexandre Esse <alexandre.esse@canonical.com>
55108cb to
c375f18
Compare
The external GNU tar relies on the openat2 syscall, which snapd's seccomp profile denies inside strictly confined snaps, making every tar.gz/tgz extraction fail with "Cannot open: Operation not permitted". Extracting with the standard library tarfile avoids the syscall and works in any environment. The tarfile "data" filter is used on Python 3.12+ to reject unsafe members; older Pythons keep the previous behaviour.