diff --git a/openedx/core/lib/extract_archive.py b/openedx/core/lib/extract_archive.py index 4706e0caffb7..ce59574b6ffe 100644 --- a/openedx/core/lib/extract_archive.py +++ b/openedx/core/lib/extract_archive.py @@ -7,7 +7,7 @@ """ import logging -from os.path import abspath, dirname +from os.path import abspath, commonpath, dirname from os.path import join as joinpath from os.path import realpath from typing import List, Union @@ -30,8 +30,18 @@ def resolved(rpath): def _is_bad_path(path, base): """ Is (the canonical absolute path of) `path` outside `base`? + + Uses ``os.path.commonpath`` for a segment-aware containment check so that + sibling directories whose names happen to extend ``base`` as a string + prefix (e.g. ``evil/...``) are correctly classified as outside. """ - return not resolved(joinpath(base, path)).startswith(base) + target = resolved(joinpath(base, path)) + try: + return commonpath([target, base]) != base + except ValueError: + # commonpath raises when paths are incomparable (e.g. mixed absolute + # and relative, or different drives on Windows). Treat as bad. + return True def _is_bad_link(info, base): diff --git a/openedx/core/lib/tests/test_extract_archive.py b/openedx/core/lib/tests/test_extract_archive.py new file mode 100644 index 000000000000..6b999586eae3 --- /dev/null +++ b/openedx/core/lib/tests/test_extract_archive.py @@ -0,0 +1,93 @@ +""" +Tests for openedx.core.lib.extract_archive. + +The path-containment helpers must treat ``base`` as a directory boundary, not +a raw string prefix: a sibling whose name extends ``base`` as a string prefix +(e.g. ``/foo_evil/x`` next to ``/foo``) is *outside* ``base``. +These tests pin that boundary down at the helper level and end-to-end through +``safe_extractall``. +""" + +import io +import os +import tarfile +import tempfile + +import pytest +from django.core.exceptions import SuspiciousOperation +from django.test import override_settings + +from openedx.core.lib.extract_archive import _is_bad_path, safe_extractall + +# Direct tests of the path-containment helper. No Django settings needed. + +def test_is_bad_path_prefix_bypass_is_rejected(): + """ + A sibling path whose name extends the base's name as a raw string prefix + (e.g. ``/Y29evil/file`` vs base ``/Y29``) is outside + ``base`` and must be flagged as bad. + """ + with tempfile.TemporaryDirectory() as parent: + base = os.path.join(parent, "Y29") + os.mkdir(base) + assert _is_bad_path("../Y29evil/file", base) is True + + +def test_is_bad_path_inside_base_is_accepted(): + with tempfile.TemporaryDirectory() as parent: + base = os.path.join(parent, "Y29") + os.mkdir(base) + assert _is_bad_path("nested/file.txt", base) is False + + +def test_is_bad_path_traversal_outside_base_is_rejected(): + with tempfile.TemporaryDirectory() as parent: + base = os.path.join(parent, "Y29") + os.mkdir(base) + assert _is_bad_path("../../etc/passwd", base) is True + + +# End-to-end tests of safe_extractall against crafted .tar.gz archives. + +def _add_file(tar, name, content=b""): + info = tarfile.TarInfo(name=name) + info.size = len(content) + tar.addfile(info, io.BytesIO(content)) + + +def _add_symlink(tar, name, linkname): + info = tarfile.TarInfo(name=name) + info.type = tarfile.SYMTYPE + info.linkname = linkname + tar.addfile(info) + + +def test_safe_extractall_blocks_file_entry_with_prefix_bypass(tmp_path): + root = str(tmp_path) + extract_dir = os.path.join(root, "Y29") + os.mkdir(extract_dir) + archive = os.path.join(root, "malicious.tar.gz") + with tarfile.open(archive, "w:gz") as tar: + _add_file(tar, "../Y29evil/sentinel.txt", b"owned") + + with override_settings(GITHUB_REPO_ROOT=root): + with pytest.raises(SuspiciousOperation): + safe_extractall(archive, extract_dir) + + escape_target = os.path.join(root, "Y29evil", "sentinel.txt") + assert not os.path.exists(escape_target) + + +def test_safe_extractall_blocks_symlink_target_with_prefix_bypass(tmp_path): + root = str(tmp_path) + extract_dir = os.path.join(root, "Y29") + os.mkdir(extract_dir) + archive = os.path.join(root, "malicious.tar.gz") + # symlink inside extract_dir pointing at a sibling whose name extends + # extract_dir's basename as a string prefix. + with tarfile.open(archive, "w:gz") as tar: + _add_symlink(tar, name="link", linkname="../Y29evil/secret") + + with override_settings(GITHUB_REPO_ROOT=root): + with pytest.raises(SuspiciousOperation): + safe_extractall(archive, extract_dir)