diff --git a/CHANGELOG.md b/CHANGELOG.md index 000a6e1..a334876 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,24 @@ All notable changes to **signetry-core** are documented here. The format follows [Semantic Versioning](https://semver.org/). Until `1.0.0` the public API may change between minor versions. +## [Unreleased] + +### Added — Python insecure-deserialisation coverage + +- `marshal.load(s)` and `shelve.open` now flagged (CWE-502) — both execute arbitrary + code during decoding, and neither was detected. +- `yaml.unsafe_load` flagged, and the Loader is now **resolved** rather than merely + counted: the previous check treated *any* `Loader=` kwarg as safe, so an explicitly + unsafe `yaml.load(x, Loader=yaml.Loader)` passed silently. +- Gaps identified by @AdvaitVarhade in #87/#91. + +### Fixed — a positional safe Loader was a false positive + +- `yaml.load(x, yaml.SafeLoader)` was flagged, because the old check only inspected + keyword arguments. The Loader is now read from the keyword *or* the second + positional argument, and matched on its last path segment so both `yaml.SafeLoader` + and a bare imported `SafeLoader` are recognised. + ## [0.7.0] — 2026-08-18 ### Added — detection breadth diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 3893daf..7594090 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -19,4 +19,4 @@ owner. -- **Advait Varhade** ([@AdvaitVarhade](https://github.com/AdvaitVarhade)) — SSRF (CWE-918) detection rule for JavaScript/Node ([#73](https://github.com/Signetry/core/pull/73)); SSRF URL-argument resolution for keyword + positional calls, plus httpx/aiohttp coverage ([#89](https://github.com/Signetry/core/pull/89)) +- **Advait Varhade** ([@AdvaitVarhade](https://github.com/AdvaitVarhade)) — SSRF (CWE-918) detection rule for JavaScript/Node ([#73](https://github.com/Signetry/core/pull/73)); SSRF URL-argument resolution for keyword + positional calls, plus httpx/aiohttp coverage ([#89](https://github.com/Signetry/core/pull/89)); identified the Python insecure-deserialisation gaps — `marshal`, `shelve`, and an explicitly unsafe `yaml` Loader ([#87](https://github.com/Signetry/core/issues/87)) diff --git a/signetry_core/pipeline/findings/deterministic.py b/signetry_core/pipeline/findings/deterministic.py index 011964e..74e66c8 100644 --- a/signetry_core/pipeline/findings/deterministic.py +++ b/signetry_core/pipeline/findings/deterministic.py @@ -225,18 +225,34 @@ def visit_Call(self, node: ast.Call) -> None: # noqa: C901 - dispatch table by 0.92, "CWE-78") # --- Unsafe deserialisation --- - if target in ("pickle.loads", "pickle.load", "cPickle.loads", "_pickle.loads"): + if target in ("pickle.loads", "pickle.load", "cPickle.loads", "cPickle.load", + "_pickle.loads", "_pickle.load", "marshal.loads", "marshal.load", + "shelve.open"): self._add(node, "py.insecure_deserialization", "insecure_deserialization", Severity.HIGH, - "Unpickling untrusted data", - "pickle.loads/load can execute arbitrary code during deserialisation.", - "Never unpickle untrusted input; use JSON or a signed, verified format.", + f"Insecure deserialisation via {target}()", + f"{target}() can execute arbitrary code during deserialisation.", + "Never deserialise untrusted input with pickle/marshal/shelve; " + "use JSON or a signed, verified format.", 0.88 if self._is_tainted(node) else 0.8, "CWE-502") - if target in ("yaml.load",) and not any(kw.arg == "Loader" for kw in node.keywords): - self._add(node, "py.yaml_load", "insecure_deserialization", Severity.HIGH, - "yaml.load without SafeLoader", - "yaml.load without a safe Loader can instantiate arbitrary Python objects.", - "Use yaml.safe_load or pass Loader=yaml.SafeLoader.", - 0.82, "CWE-502") + # yaml.load is safe ONLY with a safe Loader. Resolving the Loader (kwarg or + # 2nd positional) by its last path segment is what makes an explicitly + # unsafe loader — Loader=yaml.Loader — a finding rather than a pass: the + # previous `any(kw.arg == "Loader")` check treated *any* Loader as safe. + if target in ("yaml.load", "yaml.unsafe_load"): + loader = next((kw.value for kw in node.keywords if kw.arg == "Loader"), None) + if loader is None and len(args) >= 2: + loader = args[1] + safe_loader = ( + target == "yaml.load" + and loader is not None + and _attr_chain(loader).rsplit(".", 1)[-1] in ("SafeLoader", "CSafeLoader", "BaseLoader") + ) + if not safe_loader: + self._add(node, "py.yaml_load", "insecure_deserialization", Severity.HIGH, + f"{target}() without a safe Loader", + f"{target}() can instantiate arbitrary Python objects.", + "Use yaml.safe_load or pass Loader=yaml.SafeLoader.", + 0.82, "CWE-502") # --- eval / exec code injection --- if target in ("eval", "exec"): diff --git a/tests/test_findings_engine.py b/tests/test_findings_engine.py index afbd268..edf1a1e 100644 --- a/tests/test_findings_engine.py +++ b/tests/test_findings_engine.py @@ -345,6 +345,50 @@ def test_no_false_positive_on_constant_url_with_tainted_kwargs(): assert "ssrf" not in _cats(src_clean_kwargs) +def test_detects_deserialization_marshal_shelve_and_unsafe_loader(): + # The gaps @AdvaitVarhade identified in #91: marshal, shelve, and an explicitly + # unsafe yaml Loader. The last one is the important catch — the previous check + # treated ANY Loader= kwarg as safe, so Loader=yaml.Loader passed silently. + for src in ( + "import marshal\ndef r(b):\n return marshal.loads(b)\n", + "import shelve\ndef o(f):\n return shelve.open(f)\n", + "import yaml\ndef p(r):\n return yaml.load(r, Loader=yaml.Loader)\n", + "import yaml\ndef p(r):\n return yaml.unsafe_load(r)\n", + ): + assert "insecure_deserialization" in _cats(src), src + + +def test_deserialization_safe_loader_forms_not_flagged(): + # Positional SafeLoader was a pre-existing FALSE POSITIVE: the old + # any(kw.arg == "Loader") check only looked at keywords. + for src in ( + "import yaml\ndef p(r):\n return yaml.load(r, Loader=yaml.SafeLoader)\n", + "import yaml\nfrom yaml import SafeLoader\ndef p(r):\n return yaml.load(r, Loader=SafeLoader)\n", + "import yaml\ndef p(r):\n return yaml.load(r, yaml.SafeLoader)\n", + "import yaml\ndef p(r):\n return yaml.load(r, Loader=yaml.CSafeLoader)\n", + "import yaml\ndef p(r):\n return yaml.safe_load(r)\n", + ): + assert "insecure_deserialization" not in _cats(src), src + + +def test_deserialization_reported_once_per_line(tmp_path): + # One rule_id per class, one category. Adding a parallel rule with a different + # category name would defeat the (file, line, category) dedup and double every + # finding — the reason #91 could not be merged as written. + _write(tmp_path, {"app.py": ( + "import pickle, yaml, flask\n" + "def load_session():\n" + " raw = flask.request.data\n" + " return pickle.loads(raw)\n" + "def load_config(text):\n" + " return yaml.load(text)\n" + )}) + report = scan_repository(tmp_path) + deser = [f for f in report.findings if "deserial" in f.category] + assert len(deser) == 2, [(f.rule_id, f.category, f.line) for f in deser] + assert {f.category for f in deser} == {"insecure_deserialization"} + + def test_detects_ssti(): src = ( "import flask\n"