diff --git a/docs/plugins.md b/docs/plugins.md index b120460..16872fe 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -85,7 +85,10 @@ class SensitiveLoggingPlugin(SymbolCallDetector): message_template: ClassVar[str] = "call to {symbol} may log sensitive data" ``` -The shipped `dangerous-eval` and `weak-crypto` plugins are of this kind. +The shipped `dangerous-eval` and `weak-crypto` plugins are of this kind. `accepts(call, symbol, defs)` +keeps only some calls; `describe(ctx, call, symbol, defs)` returns the `CallReport` (message and +confidence) of one call, the rule's `message_template` and `confidence` by default, for a rule that +tells its calls apart, as `unsafe-xml` does. ### A detector of taint flows: `TaintDetector` diff --git a/docs/usage.md b/docs/usage.md index e6cb6cb..61f0ebf 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -181,7 +181,7 @@ reads no template. | `debug-enabled` | `app.run(debug=True)` on a Flask application. | | `missing-timeout` | A Requests or httpx call without a `timeout`. | | `insecure-tls` | `verify=False` on a Requests or httpx call, or `ssl._create_unverified_context()`. | -| `unsafe-xml` | A standard-library or lxml XML parser, which expands entities; `defusedxml` is the safe replacement. | +| `unsafe-xml` | An XML parser of the standard library or of lxml, reported for what it risks; `defusedxml` is the safe replacement. The standard library's parsers run on Expat, which reads no local file but expands the entities a document declares: a crafted document can exhaust memory with Expat older than 2.7.2, which depends on the Python build at run time (`pyexpat.EXPAT_VERSION`). lxml resolves external entities by default before 5.0, and in `iterparse` and `ETCompatXMLParser` before 6.1 (CVE-2026-41066), which lets a crafted document read local files; `resolve_entities=True` does so whatever the version, and `'internal'` keeps external entities out only from lxml 5.0 on, an earlier lxml reading the string as true; a parser given `resolve_entities=False` is not reported. XML content the project wrote itself, a string constant or a module-level string bound once passed to a function that parses its argument as the document (`fromstring`, `parseString`, `XML`), is reported at low confidence; a constant path keeps its confidence, since whoever can write the file chooses the document. | | `unsafe-archive-extraction` | `tarfile` or `zipfile` extraction, or `shutil.unpack_archive`, without a member `filter`. | | `insecure-temp-file` | `tempfile.mktemp`, which names a file without creating it. | | `weak-random` | A `random` function whose result is assigned to a credential-like name, or produced by a function named like one. Medium confidence. | diff --git a/src/coretrace_python/bundled/syntax/unsafe_xml/plugin.toml b/src/coretrace_python/bundled/syntax/unsafe_xml/plugin.toml index 311b5ee..b8241e1 100644 --- a/src/coretrace_python/bundled/syntax/unsafe_xml/plugin.toml +++ b/src/coretrace_python/bundled/syntax/unsafe_xml/plugin.toml @@ -1,7 +1,7 @@ name = "unsafe-xml" version = "1.0.0" plugin_api = ">=1,<2" -requires = ["interprocedural.callgraph", "ir.ssa"] +requires = ["interprocedural.callgraph", "ir.ssa", "abstract.module_strings"] provides = ["vulnerability.unsafe-xml"] [entrypoint] diff --git a/src/coretrace_python/bundled/syntax/unsafe_xml/unsafe_xml.py b/src/coretrace_python/bundled/syntax/unsafe_xml/unsafe_xml.py index 87adfff..c682d04 100644 --- a/src/coretrace_python/bundled/syntax/unsafe_xml/unsafe_xml.py +++ b/src/coretrace_python/bundled/syntax/unsafe_xml/unsafe_xml.py @@ -1,35 +1,115 @@ -"""XML parsers of the standard library and lxml expand entities: a crafted document reads -local files or exhausts memory. ``defusedxml`` offers the same functions safely.""" +"""XML parsers of the standard library and of lxml, each reported for what it risks. + +The standard library's parsers run on Expat, which never reads a local file or opens a +connection, but expands the entities a document declares: a crafted document can exhaust +memory with Expat older than 2.7.2, a fact of run time (``pyexpat.EXPAT_VERSION``). lxml +resolves external entities by default before 5.0, and in ``iterparse`` and +``ETCompatXMLParser`` before 6.1 (CVE-2026-41066), which lets a crafted document read +local files; ``resolve_entities=True`` does so whatever the version, ``False`` never. +``'internal'`` keeps external entities out only from lxml 5.0 on: an earlier lxml reads +the string as true. ``defusedxml`` offers the same functions safely. XML content the +project wrote itself, a string constant or a module-level string bound once given to a +function that parses its argument as the document, is no attacker's: its finding is kept +at low confidence. A path is not the document: whoever can write the file chooses it.""" from __future__ import annotations +from collections.abc import Mapping from typing import ClassVar +from coretrace_python.abstract.strings import ModuleStringsAnalysis from coretrace_python.analysis import AnyAnalysis -from coretrace_python.findings import Severity +from coretrace_python.findings import Confidence, Severity from coretrace_python.interprocedural import CallGraphAnalysis +from coretrace_python.ir.model import Call, Constant, Global, Instruction, Value from coretrace_python.ir.ssa import SSAAnalysis -from coretrace_python.plugins import SymbolCallDetector +from coretrace_python.plugins import CallReport, PluginContext, SymbolCallDetector from coretrace_python.semantic.symbols import SymbolId -_PARSERS = ( + +def _symbols(*paths: str) -> frozenset[SymbolId]: + return frozenset(SymbolId(f"python.{path}") for path in paths) + + +_STANDARD_LIBRARY = _symbols( "xml.etree.ElementTree.parse", "xml.etree.ElementTree.fromstring", "xml.etree.ElementTree.iterparse", "xml.etree.ElementTree.XMLParser", "xml.etree.ElementTree.fromstringlist", "xml.dom.minidom.parse", "xml.dom.minidom.parseString", "xml.dom.pulldom.parse", "xml.dom.pulldom.parseString", "xml.sax.parse", "xml.sax.parseString", "xml.sax.make_parser", "xml.dom.expatbuilder.parse", "xml.dom.expatbuilder.parseString", - "lxml.etree.parse", "lxml.etree.fromstring", "lxml.etree.XMLParser", "lxml.etree.iterparse", - "lxml.etree.XML", ) +_LXML = _symbols("lxml.etree.parse", "lxml.etree.fromstring", "lxml.etree.XMLParser", "lxml.etree.XML") +# Their default kept ``resolve_entities=True`` until lxml 6.1 (CVE-2026-41066). +_LXML_UNTIL_6_1 = _symbols("lxml.etree.iterparse", "lxml.etree.ETCompatXMLParser") +# The functions whose first argument is the XML itself, not a file, a URL or a stream. +_CONTENT = _symbols( + "xml.etree.ElementTree.fromstring", "xml.etree.ElementTree.fromstringlist", + "xml.dom.minidom.parseString", "xml.dom.pulldom.parseString", "xml.sax.parseString", + "xml.dom.expatbuilder.parseString", "lxml.etree.fromstring", "lxml.etree.XML", +) +# A keyword that is not a constant: its value is unknown. +_UNKNOWN = object() + + +def _resolve_entities(call: Call, defs: Mapping[Value, Instruction]) -> object: + """The constant ``resolve_entities=`` of ``call``, ``None`` when it is not passed.""" + + for name, value in call.keywords: + if name == "resolve_entities": + made = defs.get(value) + return made.value if isinstance(made, Constant) else _UNKNOWN + return None class UnsafeXmlPlugin(SymbolCallDetector): name: ClassVar[str] = "unsafe-xml" rule_id: ClassVar[str] = "unsafe-xml" - requires: ClassVar[frozenset[AnyAnalysis]] = frozenset({SSAAnalysis, CallGraphAnalysis}) - symbols: ClassVar[frozenset[SymbolId]] = frozenset(SymbolId(f"python.{p}") for p in _PARSERS) + requires: ClassVar[frozenset[AnyAnalysis]] = frozenset({SSAAnalysis, CallGraphAnalysis, ModuleStringsAnalysis}) + symbols: ClassVar[frozenset[SymbolId]] = _STANDARD_LIBRARY | _LXML | _LXML_UNTIL_6_1 severity: ClassVar[Severity] = Severity.MEDIUM - message_template: ClassVar[str] = ( - "call to {symbol} parses XML with entity expansion enabled; use defusedxml" - ) + message_template: ClassVar[str] = "call to {symbol} parses XML unsafely; use defusedxml" + + def accepts(self, call: Call, symbol: SymbolId, defs: Mapping[Value, Instruction]) -> bool: + # ``resolve_entities`` is an lxml option; the standard library has none. + return _resolve_entities(call, defs) is not False + + def describe( + self, ctx: PluginContext, call: Call, symbol: SymbolId, defs: Mapping[Value, Instruction] + ) -> CallReport: + if symbol in _STANDARD_LIBRARY: + risk = "expands the entities a document declares, which can exhaust memory with Expat older than 2.7.2" + advice = "use defusedxml" + elif _resolve_entities(call, defs) is True: + risk = "resolves external entities (resolve_entities=True), which lets a crafted document read local files" + advice = "use defusedxml" + elif _resolve_entities(call, defs) == "internal": + risk = ( + "sets resolve_entities='internal', which keeps external entities out only from lxml 5.0 on: " + "before lxml 5.0 the string reads as true and external entities are resolved, which lets a " + "crafted document read local files" + ) + advice = "use defusedxml or resolve_entities=False" + elif symbol in _LXML_UNTIL_6_1: + risk = ( + "resolves external entities by default before lxml 6.1 (CVE-2026-41066), " + "which lets a crafted document read local files" + ) + advice = "use defusedxml or resolve_entities=False" + else: + risk = "resolves external entities by default before lxml 5.0, which lets a crafted document read local files" + advice = "use defusedxml" + written = symbol in _CONTENT and self.written_by_the_project(ctx, call, defs) + confidence = Confidence.LOW if written else self.confidence + return CallReport(f"call to {symbol} {risk}; {advice}", confidence) + + def written_by_the_project(self, ctx: PluginContext, call: Call, defs: Mapping[Value, Instruction]) -> bool: + """Whether the first argument of ``call``, which a ``_CONTENT`` function parses as + the document, is a string constant or a module-level name bound once to one.""" + + if not call.arguments: + return False + made = defs.get(call.arguments[0]) + if isinstance(made, Constant): + return isinstance(made.value, str | bytes) + return isinstance(made, Global) and made.name in ctx.get(ModuleStringsAnalysis) diff --git a/src/coretrace_python/plugins/__init__.py b/src/coretrace_python/plugins/__init__.py index 648f9e0..8c1738c 100644 --- a/src/coretrace_python/plugins/__init__.py +++ b/src/coretrace_python/plugins/__init__.py @@ -11,7 +11,12 @@ apply_refinement, run_plugins, ) -from coretrace_python.plugins.detectors import Assessment, SymbolCallDetector, TaintDetector +from coretrace_python.plugins.detectors import ( + Assessment, + CallReport, + SymbolCallDetector, + TaintDetector, +) from coretrace_python.plugins.loader import ( IncompatiblePluginError, LoadedPlugin, @@ -36,6 +41,7 @@ __all__ = [ "PLUGIN_API_VERSION", "Assessment", + "CallReport", "Entrypoint", "IncompatiblePluginError", "LoadedPlugin", diff --git a/src/coretrace_python/plugins/detectors.py b/src/coretrace_python/plugins/detectors.py index 557c592..476bddb 100644 --- a/src/coretrace_python/plugins/detectors.py +++ b/src/coretrace_python/plugins/detectors.py @@ -4,7 +4,8 @@ detector is a few class attributes on top of it, and a rule that knows more about its sinks refines the verdict (``judge``) or the severity too (``assess``). ``SymbolCallDetector`` reports calls whose callee resolves to a canonical symbol, through imports and local aliases alike, -by reading the SSA form. +by reading the SSA form; a rule that tells its calls apart words each finding itself +(``describe``). """ from __future__ import annotations @@ -37,6 +38,14 @@ class Assessment: metadata: Mapping[str, str] = field(default_factory=dict) +@dataclass(frozen=True) +class CallReport: + """What a ``SymbolCallDetector`` reports of one call: its message and confidence.""" + + message: str + confidence: Confidence + + class TaintDetector(Plugin): """Report every taint flow carrying ``kind`` into a sink.""" @@ -128,6 +137,14 @@ class SymbolCallDetector(Plugin): def accepts(self, call: Call, symbol: SymbolId, defs: Mapping[Value, Instruction]) -> bool: return True + def describe( + self, ctx: PluginContext, call: Call, symbol: SymbolId, defs: Mapping[Value, Instruction] + ) -> CallReport: + """The message and confidence of the finding for ``call``: the rule's own, unless + the rule tells its calls apart.""" + + return CallReport(self.message_template.format(symbol=symbol), self.confidence) + def analyze(self, ctx: PluginContext) -> Sequence[Finding]: findings: list[Finding] = [] graph = ctx.get(CallGraphAnalysis) if CallGraphAnalysis in self.requires else None @@ -150,12 +167,13 @@ def analyze(self, ctx: PluginContext) -> Sequence[Finding]: continue if not self.accepts(instruction, symbol, defs): continue + described = self.describe(ctx, instruction, symbol, defs) findings.append( Finding( rule_id=self.rule_id, - message=self.message_template.format(symbol=symbol), + message=described.message, severity=self.severity, - confidence=self.confidence, + confidence=described.confidence, span=instruction.location, function=function.name, metadata={"symbol": str(symbol)}, diff --git a/tests/test_unsafe_xml_risk.py b/tests/test_unsafe_xml_risk.py new file mode 100644 index 0000000..03c5b59 --- /dev/null +++ b/tests/test_unsafe_xml_risk.py @@ -0,0 +1,120 @@ +"""Acceptance tests for issue #242: ``unsafe-xml`` says what each parser family risks. + +The standard library's parsers run on Expat, which never reads a local file; they expand +the entities a document declares, which can exhaust memory with Expat older than 2.7.2. +lxml resolves external entities by default before 5.0, and for ``iterparse`` and +``ETCompatXMLParser`` before 6.1 (CVE-2026-41066); ``resolve_entities=True`` resolves them +whatever the version, ``False`` never. ``'internal'`` keeps them out only from lxml 5.0 on: +an earlier lxml reads the string as true. XML content the project wrote itself is no +attacker's: it is reported at low confidence; a constant path is not the document it +names, which anyone able to write the file chooses. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from coretrace_python import engine +from coretrace_python.findings import Confidence, Finding, Severity +from coretrace_python.source import SourceManager + +PLUGINS = Path(__file__).resolve().parent.parent / "src" / "coretrace_python" / "bundled" + + +def check(text: str) -> tuple[Finding, ...]: + findings = engine.check(SourceManager().add_source("m.py", text), [PLUGINS]) + return tuple(f for f in findings if f.rule_id == "unsafe-xml") + + +def only(text: str) -> Finding: + (finding,) = check(text) + assert finding.severity is Severity.MEDIUM + return finding + + +@pytest.mark.parametrize( + "call", + [ + "xml.etree.ElementTree.fromstring(data)", + "xml.dom.minidom.parseString(data)", + "xml.sax.parseString(data, None)", + ], +) +def test_a_standard_library_parser_is_reported_for_entity_amplification(call: str) -> None: + finding = only(f"import xml.etree.ElementTree\nimport xml.dom.minidom\nimport xml.sax\n\ndef f(data):\n {call}\n") + + assert "Expat older than 2.7.2" in finding.message + assert "local file" not in finding.message + assert finding.confidence is Confidence.HIGH + + +@pytest.mark.parametrize("function", ["fromstring", "parse", "XML", "XMLParser"]) +def test_an_lxml_parser_with_its_defaults_is_reported_for_versions_before_5(function: str) -> None: + finding = only(f"import lxml.etree\n\ndef f(data):\n lxml.etree.{function}(data)\n") + + assert "before lxml 5.0" in finding.message + assert "local files" in finding.message + + +@pytest.mark.parametrize("function", ["iterparse", "ETCompatXMLParser"]) +def test_iterparse_and_the_compatibility_parser_are_reported_for_versions_before_6_1(function: str) -> None: + finding = only(f"import lxml.etree\n\ndef f(data):\n lxml.etree.{function}(data)\n") + + assert "before lxml 6.1" in finding.message + assert "CVE-2026-41066" in finding.message + + +def test_resolving_entities_explicitly_is_reported_whatever_the_version() -> None: + finding = only("import lxml.etree\n\ndef f(data):\n lxml.etree.XMLParser(resolve_entities=True)\n") + + assert "resolve_entities=True" in finding.message + assert "before lxml" not in finding.message + + +def test_a_parser_that_resolves_no_external_entity_is_not_reported() -> None: + assert check("import lxml.etree\n\ndef f(data):\n lxml.etree.XMLParser(resolve_entities=False)\n") == () + assert check("import lxml.etree\n\ndef f(data):\n lxml.etree.iterparse(data, resolve_entities=False)\n") == () + + +@pytest.mark.parametrize("function", ["XMLParser", "iterparse"]) +def test_internal_entities_only_are_reported_for_versions_before_5(function: str) -> None: + finding = only(f"import lxml.etree\n\ndef f(data):\n lxml.etree.{function}(data, resolve_entities='internal')\n") + + assert "resolve_entities='internal'" in finding.message + assert "before lxml 5.0" in finding.message + + +@pytest.mark.parametrize( + "text", + [ + "from xml.dom import minidom\n\ndef f():\n minidom.parseString('')\n", + "from xml.dom import minidom\nEXPECTED = ''\n\ndef f():\n minidom.parseString(EXPECTED)\n", + "import lxml.etree\n\ndef f():\n lxml.etree.fromstring('')\n", + ], +) +def test_a_document_the_project_wrote_is_reported_at_low_confidence(text: str) -> None: + assert only(text).confidence is Confidence.LOW + + +@pytest.mark.parametrize( + "call", + [ + "lxml.etree.parse('/tmp/upload.xml')", + "lxml.etree.iterparse('/tmp/upload.xml')", + "xml.etree.ElementTree.parse('/tmp/upload.xml')", + "xml.dom.minidom.parse('/tmp/upload.xml')", + "xml.sax.parse('/tmp/upload.xml', None)", + ], +) +def test_a_constant_path_keeps_high_confidence(call: str) -> None: + text = f"import lxml.etree\nimport xml.dom.minidom\nimport xml.etree.ElementTree\nimport xml.sax\n\ndef f():\n {call}\n" + + assert only(text).confidence is Confidence.HIGH + + +def test_a_document_of_unknown_origin_keeps_high_confidence() -> None: + text = "from xml.dom import minidom\nEXPECTED = ''\nEXPECTED = load()\n\ndef f():\n minidom.parseString(EXPECTED)\n" + + assert only(text).confidence is Confidence.HIGH