Skip to content

Commit e552907

Browse files
feanilclaude
andcommitted
fix: replace string-prefix check in safe_extractall with commonpath
`startswith` is the wrong primitive for "is target inside directory base": once a trailing separator drops anywhere along the way, sibling directories whose names extend base match. We could spot-fix by re-appending the separator before the check, but `commonpath` makes the directory-boundary intent explicit and removes the failure mode entirely. Fixes GHSA-6cmm-8875-5pcw. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 5eae6e5 commit e552907

2 files changed

Lines changed: 105 additions & 2 deletions

File tree

openedx/core/lib/extract_archive.py

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
"""
88

99
import logging
10-
from os.path import abspath, dirname, realpath
10+
from os.path import abspath, commonpath, dirname, realpath
1111
from os.path import join as joinpath
1212
from tarfile import TarFile, TarInfo
1313
from typing import List, Union # noqa: UP035
@@ -29,8 +29,18 @@ def resolved(rpath):
2929
def _is_bad_path(path, base):
3030
"""
3131
Is (the canonical absolute path of) `path` outside `base`?
32+
33+
Uses ``os.path.commonpath`` for a segment-aware containment check so that
34+
sibling directories whose names happen to extend ``base`` as a string
35+
prefix (e.g. ``<base>evil/...``) are correctly classified as outside.
3236
"""
33-
return not resolved(joinpath(base, path)).startswith(base)
37+
target = resolved(joinpath(base, path))
38+
try:
39+
return commonpath([target, base]) != base
40+
except ValueError:
41+
# commonpath raises when paths are incomparable (e.g. mixed absolute
42+
# and relative, or different drives on Windows). Treat as bad.
43+
return True
3444

3545

3646
def _is_bad_link(info, base):
Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
"""
2+
Tests for openedx.core.lib.extract_archive.
3+
4+
The path-containment helpers must treat ``base`` as a directory boundary, not
5+
a raw string prefix: a sibling whose name extends ``base`` as a string prefix
6+
(e.g. ``<parent>/foo_evil/x`` next to ``<parent>/foo``) is *outside* ``base``.
7+
These tests pin that boundary down at the helper level and end-to-end through
8+
``safe_extractall``.
9+
"""
10+
11+
import io
12+
import os
13+
import tarfile
14+
import tempfile
15+
16+
import pytest
17+
from django.core.exceptions import SuspiciousOperation
18+
from django.test import override_settings
19+
20+
from openedx.core.lib.extract_archive import _is_bad_path, safe_extractall
21+
22+
# Direct tests of the path-containment helper. No Django settings needed.
23+
24+
def test_is_bad_path_prefix_bypass_is_rejected():
25+
"""
26+
A sibling path whose name extends the base's name as a raw string prefix
27+
(e.g. ``<parent>/Y29evil/file`` vs base ``<parent>/Y29``) is outside
28+
``base`` and must be flagged as bad.
29+
"""
30+
with tempfile.TemporaryDirectory() as parent:
31+
base = os.path.join(parent, "Y29")
32+
os.mkdir(base)
33+
assert _is_bad_path("../Y29evil/file", base) is True
34+
35+
36+
def test_is_bad_path_inside_base_is_accepted():
37+
with tempfile.TemporaryDirectory() as parent:
38+
base = os.path.join(parent, "Y29")
39+
os.mkdir(base)
40+
assert _is_bad_path("nested/file.txt", base) is False
41+
42+
43+
def test_is_bad_path_traversal_outside_base_is_rejected():
44+
with tempfile.TemporaryDirectory() as parent:
45+
base = os.path.join(parent, "Y29")
46+
os.mkdir(base)
47+
assert _is_bad_path("../../etc/passwd", base) is True
48+
49+
50+
# End-to-end tests of safe_extractall against crafted .tar.gz archives.
51+
52+
def _add_file(tar, name, content=b""):
53+
info = tarfile.TarInfo(name=name)
54+
info.size = len(content)
55+
tar.addfile(info, io.BytesIO(content))
56+
57+
58+
def _add_symlink(tar, name, linkname):
59+
info = tarfile.TarInfo(name=name)
60+
info.type = tarfile.SYMTYPE
61+
info.linkname = linkname
62+
tar.addfile(info)
63+
64+
65+
def test_safe_extractall_blocks_file_entry_with_prefix_bypass(tmp_path):
66+
root = str(tmp_path)
67+
extract_dir = os.path.join(root, "Y29")
68+
os.mkdir(extract_dir)
69+
archive = os.path.join(root, "malicious.tar.gz")
70+
with tarfile.open(archive, "w:gz") as tar:
71+
_add_file(tar, "../Y29evil/sentinel.txt", b"owned")
72+
73+
with override_settings(GITHUB_REPO_ROOT=root):
74+
with pytest.raises(SuspiciousOperation):
75+
safe_extractall(archive, extract_dir)
76+
77+
escape_target = os.path.join(root, "Y29evil", "sentinel.txt")
78+
assert not os.path.exists(escape_target)
79+
80+
81+
def test_safe_extractall_blocks_symlink_target_with_prefix_bypass(tmp_path):
82+
root = str(tmp_path)
83+
extract_dir = os.path.join(root, "Y29")
84+
os.mkdir(extract_dir)
85+
archive = os.path.join(root, "malicious.tar.gz")
86+
# symlink inside extract_dir pointing at a sibling whose name extends
87+
# extract_dir's basename as a string prefix.
88+
with tarfile.open(archive, "w:gz") as tar:
89+
_add_symlink(tar, name="link", linkname="../Y29evil/secret")
90+
91+
with override_settings(GITHUB_REPO_ROOT=root):
92+
with pytest.raises(SuspiciousOperation):
93+
safe_extractall(archive, extract_dir)

0 commit comments

Comments
 (0)