feat(detection): close the Python insecure-deserialisation gaps (#87)

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

CONTRIBUTORS.md

@@ -19,4 +19,4 @@ owner.
 <!-- Contributors who have signed the CLA are added here (alphabetical by GitHub
      handle). To be credited, contribute a PR and sign the CLA. -->
 
-- **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))

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"):

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"