diff --git a/test/test_composer_path_containment.py b/test/test_composer_path_containment.py index c47ca884..f8e1d090 100644 --- a/test/test_composer_path_containment.py +++ b/test/test_composer_path_containment.py @@ -94,10 +94,66 @@ def test_a_sibling_directory_with_a_shared_prefix_is_not_inside(tmp_path, monkey (tmp_path / "plugins-evil").mkdir() monkeypatch.setattr(C.composer_bp, "plugins_dir", str(base), raising=False) import re + # Neutralise the two layers in front so this exercises the containment + # check itself; otherwise secure_filename rejects the payload first and a + # startswith regression would go unnoticed here. monkeypatch.setattr(C, "_PLUGIN_ID_RE", re.compile(r"\A[\w./\\~-]+\Z")) + monkeypatch.setattr(C, "secure_filename", lambda v: v) assert C._plugin_dir("../plugins-evil") is None +def test_containment_still_holds_if_the_sanitiser_is_defeated(plugins_dir, monkeypatch): + """Each layer is tested on its own, not just the stack. + + secure_filename's equality guard rejects every traversal payload before the + containment check sees it, so removing containment does not fail the other + tests -- which would make it look load-bearing when it is not. Neutralise + the regex *and* the sanitiser, and the realpath/commonpath check must still + refuse everything on its own. + """ + import re + monkeypatch.setattr(C, "_PLUGIN_ID_RE", re.compile(r"\A[\w./\\~-]+\Z")) + monkeypatch.setattr(C, "secure_filename", lambda v: v) + import os + base = os.path.realpath(str(plugins_dir)) + escaped = [] + for payload in TRAVERSAL: + resolved = C._plugin_dir(payload) + if resolved is None: + continue + real = os.path.realpath(str(resolved)) + # Inside the base is fine -- "...." and "~" are ordinary directory + # names on Linux, so they are not escapes. What must never happen is + # landing outside the base, or on the base itself: install() rmtrees + # its target, so the plugins root resolving to a "plugin" would wipe + # every installed plugin. + if real == base or os.path.commonpath([base, real]) != base: + escaped.append((payload, real)) + assert not escaped, f"containment alone let these through: {escaped}" + + +def test_secure_filename_never_rewrites_an_accepted_id(plugins_dir): + """The sanitiser must be a no-op on everything the regex accepts. + + If secure_filename ever altered an accepted id, _plugin_dir would resolve + to a *different* plugin's directory than the caller asked for -- a silent + redirect, which is worse than a refusal. The guard turns that into a + refusal; this proves the guard never has to fire in practice. + """ + import random + from werkzeug.utils import secure_filename + random.seed(1) + alphabet = "abcdefghijklmnopqrstuvwxyz0123456789-" + altered = [] + for _ in range(2000): + n = random.randint(1, 63) + cand = random.choice("abcdefghijklmnopqrstuvwxyz") + "".join( + random.choice(alphabet) for _ in range(n - 1)) + if C._PLUGIN_ID_RE.match(cand) and secure_filename(cand) != cand: + altered.append((cand, secure_filename(cand))) + assert not altered, f"secure_filename rewrote accepted ids: {altered[:5]}" + + def test_a_trailing_newline_is_not_a_valid_id(): r"""Python's `$` also matches before a trailing newline, so the original `^...$` accepted "myplugin\n" and would have created a directory whose diff --git a/web_interface/blueprints/composer.py b/web_interface/blueprints/composer.py index eac307d1..0fd360c5 100644 --- a/web_interface/blueprints/composer.py +++ b/web_interface/blueprints/composer.py @@ -22,6 +22,7 @@ from pathlib import Path import jinja2 import jsonschema from flask import Blueprint, jsonify, render_template, request, send_file +from werkzeug.utils import secure_filename logger = logging.getLogger(__name__) @@ -618,12 +619,24 @@ def _plugin_dir(plugin_id: str) -> Optional[Path]: """ if not _PLUGIN_ID_RE.match(plugin_id or ''): return None + # secure_filename strips path separators and traversal. Every id the regex + # above accepts passes through it byte-for-byte -- verified across the whole + # accepted alphabet -- so this cannot rewrite a caller's id into a + # different plugin's directory; if it changes anything, the id was not one + # we accept and we refuse rather than silently redirect. + safe_id = secure_filename(plugin_id) + if safe_id != plugin_id: + return None base = os.path.realpath(str(composer_bp.plugins_dir)) - candidate = os.path.realpath(os.path.join(base, plugin_id)) + candidate = os.path.realpath(os.path.join(base, safe_id)) + # A plugin directory must be a *child* of the base, never the base itself: + # install() calls shutil.rmtree(target) when force is set, so resolving to + # the plugins root would delete every installed plugin. + # # commonpath, not startswith: "/plugins-evil" starts with "/plugins" but is # a different directory. This is also the form static analysis recognises # as a containment check. - if candidate != base and os.path.commonpath([base, candidate]) != base: + if candidate == base or os.path.commonpath([base, candidate]) != base: return None return Path(candidate)