From 70c8e1d866321411cfded6f14bfae4a28fb5653a Mon Sep 17 00:00:00 2001 From: Hugo Date: Thu, 15 Oct 2026 01:49:17 +0200 Subject: [PATCH] feat(code-injection): report import directories chosen by input Part of #240: sys.path and site.addsitedir. getattr follows in its own change. --- docs/usage.md | 2 +- .../models/python_stdlib/python_stdlib.py | 6 + .../security/code_injection/code_injection.py | 30 ++++- src/coretrace_python/taint/models.py | 6 +- tests/test_import_path_sinks.py | 108 ++++++++++++++++++ 5 files changed, 145 insertions(+), 7 deletions(-) create mode 100644 tests/test_import_path_sinks.py diff --git a/docs/usage.md b/docs/usage.md index 9e8a37f..5c49577 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -121,7 +121,7 @@ variable is not treated that way yet. | Rule | Reached sink | |---|---| | `command-injection` | Shell or process execution (`os.system`, `subprocess` with a string, …), judged on what the input controls in the command. A string a shell runs, a string naming the program, the program itself, or an operand the program runs as a command (after the host for `ssh`, the program of `sudo`, `env`, `xargs`, `timeout`…, any argument of a batch file) is `high`. The script an interpreter runs (`bash run.sh`, `python tool.py`, its first operand when no option such as `-c` gives the code), or the module of `python -m`, is `high` and kept for review: execution is possible, and its exploitability depends on the file the input chooses. The arguments after a script or code are the script's own: options of unknown meaning, reported `medium` (a hotspot after an option of the script), refuted behind a constant prefix. Otherwise an element of an argument list run without a shell can only be read as an option: `medium`, or `high` when a documented option of the program runs a command it chooses (`git push --receive-pack=…`, `ssh -o ProxyCommand=…`). A value a documented option takes is judged by what the option does with it, whatever its prefix: run it (`sh -c`, `cmd /c`, `python -c`: `high`), keep it as data (`git commit -m`: refuted). After an option the rule does not know, the finding stays a hotspot to review. A constant prefix that keeps a standalone input from being an option (`f"v{version}"`), or an operand after `--` for a program that honours it, is refuted. Every case keeps the rule id `command-injection`, so suppressions and baselines match whatever the case. When the rule reads the command element by element and establishes what the input is there, the finding's `injection` metadata (in the JSON report, and as a property of its SARIF result) says so: `command` when the input runs or chooses what runs (a shell string, the program, an operand run as a command, the value of an option that runs it); `option` when it can only reach the program as an option or as the value of an option not established to run it, and the message is then titled `Option injection`. When that cannot be established (an option whose effect is to review, an unknown program or element before the input, a command that is not a list or tuple display, a sink such as `os.system` the rule does not read element by element), the metadata is absent and the title stays `Command injection`. | -| `code-injection` | The code `eval` or `exec` runs; the globals and locals passed with it are data. `dangerous-eval` reports every such call, this rule the ones attacker input reaches. Also the module `importlib.import_module`, `__import__` or `runpy.run_module` imports, and the file `runpy.run_path` runs, by position or by keyword (`name`, `mod_name`, `path_name`); the package of a relative name, the globals and the run name are data. An import runs the top-level code of a module the input chooses among those that can be found, a script path the file it chooses: execution is possible, and how far it goes depends on what the input can reach, so these findings are titled `Untrusted module import` and `Untrusted script execution`. A constant package prefix (`"app.plugins." + name`) confines an import to that package: a hotspot. The environment and the command line reach these sinks as they reach `eval`. | +| `code-injection` | The code `eval` or `exec` runs; the globals and locals passed with it are data. `dangerous-eval` reports every such call, this rule the ones attacker input reaches. Also the module `importlib.import_module`, `__import__` or `runpy.run_module` imports, and the file `runpy.run_path` runs, by position or by keyword (`name`, `mod_name`, `path_name`); the package of a relative name, the globals and the run name are data. An import runs the top-level code of a module the input chooses among those that can be found, a script path the file it chooses: execution is possible, and how far it goes depends on what the input can reach, so these findings are titled `Untrusted module import` and `Untrusted script execution`. A constant package prefix (`"app.plugins." + name`) confines an import to that package: a hotspot. The environment and the command line reach these sinks as they reach `eval`. Also the directory `sys.path.insert` (its second argument), `sys.path.append` and `sys.path.extend` add to the import path, and the directory `site.addsitedir` adds (`sitedir`), judged the same way. A directory on the import path runs code only when a later import resolves to a module the attacker placed there: a hotspot, titled `Untrusted import path`. `site.addsitedir` runs the import lines of the directory's `.pth` files during the call: a vulnerability, titled `Untrusted site directory`, exploitable when the directory holds files the attacker controls, as the finding's evidence states. Coverage of the import path is partial: writes to `sys.path` that are not calls, a slice assignment (`sys.path[:0] = [d]`), `sys.path += [d]` or rebinding `sys.path`, are not reported. | | `sql-injection` | A database statement (`cursor.execute`, SQLAlchemy `text`, Django `raw`, …). Parameters of a parameterised query are not statements. | | `nosql-injection` | A NoSQL query, such as a MongoDB filter, given attacker input that may hold a structure: a JSON body giving `{"$ne": null}` matches any value. The input that may hold a structure is JSON, the raw bodies a `json.loads` decodes, files, WebSocket messages and standard input. Text cannot hold an operator: form fields, query parameters, headers, cookies, URL parameters, the command line, the environment, and an entry-point parameter annotated with scalars (`str`, `int`, `Optional[str]`, `Annotated[str, Query()]`, `list[str]`, …), which FastAPI validates. A Django, REST framework, aiohttp or tornado view reads a request object: its text attributes (`GET`, `POST`, `headers`, `query_params`, `query`, `match_info`, `arguments`, …) and the URL parameters after it are text, its body, files and `data` are not. Limits: a helper function the request is passed to reads it as a whole; the keys of a dict are not followed, so a string concatenated into `$where` is not reported. The analyzer ships no NoSQL sink: model plugins declare them. | | `path-traversal` | A file system path (`open`, `send_file`, `os.remove`, …). | diff --git a/src/coretrace_python/bundled/models/python_stdlib/python_stdlib.py b/src/coretrace_python/bundled/models/python_stdlib/python_stdlib.py index 276ea86..e9a3afd 100644 --- a/src/coretrace_python/bundled/models/python_stdlib/python_stdlib.py +++ b/src/coretrace_python/bundled/models/python_stdlib/python_stdlib.py @@ -74,6 +74,12 @@ class PythonStdlibModels(ModelPlugin): ("runpy.run_path", "path_name"), ) ), + # The directory imports are looked up in, and the site directory whose ``.pth`` + # files run; the index of ``insert`` is data (#240). + Sink(_sym("sys.path.insert"), TaintKind.CODE, ((TaintKind.CODE, (1,)),), selects=True), + Sink(_sym("sys.path.append"), TaintKind.CODE, ((TaintKind.CODE, (0,)),), selects=True), + Sink(_sym("sys.path.extend"), TaintKind.CODE, ((TaintKind.CODE, (0,)),), selects=True), + Sink(_sym("site.addsitedir"), TaintKind.CODE, ((TaintKind.CODE, (0,)),), ((TaintKind.CODE, ("sitedir",)),), selects=True), Sink(_sym("builtins.open"), TaintKind.PATH), Sink(_sym("os.remove"), TaintKind.PATH), Sink(_sym("os.unlink"), TaintKind.PATH), diff --git a/src/coretrace_python/bundled/security/code_injection/code_injection.py b/src/coretrace_python/bundled/security/code_injection/code_injection.py index a33ba3c..6a8280a 100644 --- a/src/coretrace_python/bundled/security/code_injection/code_injection.py +++ b/src/coretrace_python/bundled/security/code_injection/code_injection.py @@ -1,13 +1,17 @@ """Code injection: attacker-controlled input reaching a CODE sink, such as the code ``eval`` or ``exec`` runs, the module ``importlib.import_module``, ``__import__`` or -``runpy.run_module`` imports, or the file ``runpy.run_path`` runs. ``dangerous-eval`` -reports every ``eval`` or ``exec``; this rule reports the calls attacker input reaches. +``runpy.run_module`` imports, the file ``runpy.run_path`` runs, or the directory +``sys.path`` and ``site.addsitedir`` load code from. ``dangerous-eval`` reports every +``eval`` or ``exec``; this rule reports the calls attacker input reaches. An import runs the top-level code of a module the input chooses among those that can be found, and a script path runs the file the input chooses: execution is possible, and how far it goes depends on what the input can reach. Their findings are titled for that. A constant package prefix (``"app.plugins." + name``) confines an import to the -modules of that package: a hotspot.""" +modules of that package: a hotspot. A directory added to ``sys.path`` runs code only +when a later import resolves there: a hotspot. ``site.addsitedir`` runs the import lines +of the directory's ``.pth`` files during the call: a vulnerability, exploitable when the +directory holds files the attacker controls, as the finding states.""" from __future__ import annotations @@ -28,6 +32,8 @@ for name in ("importlib.import_module", "builtins.__import__", "importlib.__import__", "runpy.run_module") ) _RUN_PATH = SymbolId("python.runpy.run_path") +_IMPORT_PATHS = frozenset(SymbolId(f"python.sys.path.{name}") for name in ("insert", "append", "extend")) +_SITE_DIRECTORY = SymbolId("python.site.addsitedir") class CodeInjectionPlugin(TaintDetector): @@ -44,6 +50,24 @@ def assess( symbol = flow.sink.symbol if symbol == _RUN_PATH: return Assessment(verdict, self.severity, "Untrusted script execution") + if symbol in _IMPORT_PATHS: + if verdict.status is Status.VULNERABILITY: + verdict = Verdict( + flow, + Status.HOTSPOT, + "the input chooses a directory imports are looked up in: code runs only when a later " + "import resolves to a module the attacker placed there", + ) + return Assessment(verdict, self.severity, "Untrusted import path") + if symbol == _SITE_DIRECTORY: + if verdict.status is Status.VULNERABILITY: + verdict = Verdict( + flow, + Status.VULNERABILITY, + f"{verdict.evidence}; the import lines of the .pth files of the directory run during the " + "call: exploitable when the input chooses a directory holding files the attacker controls", + ) + return Assessment(verdict, self.severity, "Untrusted site directory") if symbol not in _IMPORTS: return Assessment(verdict, self.severity) if verdict.status is Status.VULNERABILITY and flow.through is None: diff --git a/src/coretrace_python/taint/models.py b/src/coretrace_python/taint/models.py index 0cf2c94..f9cdd20 100644 --- a/src/coretrace_python/taint/models.py +++ b/src/coretrace_python/taint/models.py @@ -66,9 +66,9 @@ class Sink: ``keywords`` restrict some kinds to the arguments they list: a SQL statement is the first argument of ``execute``, its parameter tuple is not a statement; the target of ``requests.get`` is its first argument or ``url=``, its body is no destination. - ``selects`` marks a sink whose argument chooses by name the code that runs, a module - or a file: only a check that limits the value to constants proves it, not a check of - its shape (#240).""" + ``selects`` marks a sink whose argument chooses by name the code that runs, a module, + a file or a directory code is loaded from: only a check that limits the value to + constants proves it, not a check of its shape (#240).""" symbol: SymbolId kinds: TaintKind diff --git a/tests/test_import_path_sinks.py b/tests/test_import_path_sinks.py new file mode 100644 index 0000000..600c374 --- /dev/null +++ b/tests/test_import_path_sinks.py @@ -0,0 +1,108 @@ +"""Acceptance tests for issue #240, part 2: the directories imports are looked up in. + +``sys.path.insert``, ``append`` and ``extend`` let the input choose a directory imports +are looked up in: code runs only when a later import resolves to the attacker's module +there, so the finding is a hotspot, ``Untrusted import path``. ``site.addsitedir`` runs +the import lines of the directory's ``.pth`` files during the call: a vulnerability, +``Untrusted site directory``, exploitable when that directory holds files the attacker +controls, which the finding states. As for the import sinks, only a check that limits +the directory to constants proves it, and the environment and the command line reach +these sinks as they reach ``eval``. Writes to ``sys.path`` (slice assignment, ``+=``, +rebinding) are not calls and are not covered. +""" + +from __future__ import annotations + +import pytest + +from coretrace_python import engine +from coretrace_python.findings import Finding +from coretrace_python.source import SourceManager + +HEADER = "import os\nimport site\nimport sys\nfrom sys import path\n\n\ndef run():\n d = input()\n" + + +def injections(body: str) -> list[Finding]: + text = HEADER + "".join(f" {line}\n" for line in body.splitlines()) + findings = engine.check(SourceManager().add_source("app.py", text), [engine.BUNDLED_PLUGINS]) + return [f for f in findings if f.rule_id == "code-injection"] + + +def verdict(finding: Finding) -> tuple[str, str, str]: + return finding.metadata["verdict"], finding.confidence.value, finding.message.split(":", 1)[0] + + +@pytest.mark.parametrize( + "call", + [ + "sys.path.insert(0, d)", + "sys.path.append(d)", + "sys.path.extend([d])", + "path.insert(0, d)", + ], +) +def test_an_import_directory_the_input_chooses_is_a_hotspot(call: str) -> None: + (finding,) = injections(call) + + assert verdict(finding) == ("hotspot", "medium", "Untrusted import path") + assert "later import" in finding.metadata["evidence"] + + +@pytest.mark.parametrize("call", ["site.addsitedir(d)", "site.addsitedir(sitedir=d)"]) +def test_a_site_directory_the_input_chooses_is_a_vulnerability_stating_its_condition(call: str) -> None: + (finding,) = injections(call) + + assert verdict(finding) == ("vulnerability", "high", "Untrusted site directory") + assert ".pth" in finding.metadata["evidence"] + assert "attacker" in finding.metadata["evidence"] + + +@pytest.mark.parametrize( + "call", + [ + # The index is not a directory. + "sys.path.insert(int(d), '/srv/lib')", + "sys.path.insert(len(d), '/srv/lib')", + # Constants and the project's own directories. + "sys.path.insert(0, '/srv/lib')", + "sys.path.append(os.path.join(os.path.dirname(__file__), 'lib'))", + "site.addsitedir('/srv/site')", + ], +) +def test_a_directory_the_input_does_not_choose_is_not_reported(call: str) -> None: + assert injections(call) == [] + + +@pytest.mark.parametrize( + ("call", "expected"), + [ + ("sys.path.insert(0, d)", ("hotspot", "medium", "Untrusted import path")), + ("site.addsitedir(d)", ("hotspot", "medium", "Untrusted site directory")), + ], +) +@pytest.mark.parametrize("guard", ["d.isalnum()", "d.isidentifier()"]) +def test_a_check_of_the_shape_of_the_directory_does_not_prove_it( + call: str, expected: tuple[str, str, str], guard: str +) -> None: + (finding,) = injections(f"if {guard}:\n {call}") + + assert verdict(finding) == expected + + +@pytest.mark.parametrize("call", ["sys.path.insert(0, d)", "site.addsitedir(d)"]) +@pytest.mark.parametrize("guard", ["d in ('/srv/a', '/srv/b')", "d == '/srv/a'"]) +def test_a_check_against_constants_proves_the_directory(call: str, guard: str) -> None: + assert injections(f"if {guard}:\n {call}") == [] + + +@pytest.mark.parametrize( + ("body", "expected"), + [ + ("sys.path.insert(0, os.environ['PLUGIN_DIR'])", ("hotspot", "medium", "Untrusted import path")), + ("site.addsitedir(sys.argv[1])", ("vulnerability", "high", "Untrusted site directory")), + ], +) +def test_the_environment_and_the_command_line_reach_these_sinks(body: str, expected: tuple[str, str, str]) -> None: + (finding,) = injections(body) + + assert verdict(finding) == expected