feat(detection): add insecure-deserialization rule for Python (pickle, yaml.load, marshal, shelve)

[AdvaitVarhade]

Summary

This pull request implements the Python insecure deserialization detection rule (py.insecure_deserialization, category deserialization, CWE-502, severity HIGH) in signetry_core/pipeline/findings/deterministic.py.

Changes

• Added AST detection for pickle.loads, pickle.load, _pickle.loads, _pickle.load, marshal.loads, marshal.load, and shelve.open when invoked with non-constant inputs.
• Added AST inspection for yaml.load calls without Loader=yaml.SafeLoader or Loader=SafeLoader (or when using unsafe loaders like yaml.Loader / yaml.UnsafeLoader / yaml.FullLoader).
• Preserved zero false positives on safe deserializers such as yaml.safe_load, yaml.load(..., Loader=SafeLoader), hardcoded constant byte payloads to pickle.loads, and standard json.loads.
• Added unit tests in tests/test_findings_engine.py covering positive detection on vulnerable calls and negative tests for safe patterns. All 61 findings engine tests pass cleanly.

Related to issue #87.

Please check and tell if any changes are required.

[bkd-dotcom]

Thanks for going straight onto #87 after the SSRF rule — and the test structure here is good, the negative-case test especially.

Safety: no concerns. No new dependencies, no workflow or packaging changes, no subprocess/network/eval, pure AST inspection. _attr_chain returns "" on unexpected nodes, so nothing here can crash the scanner on valid Python. CLA, CodeQL, self-admission and all three test matrix jobs are green; I ran the full suite and ruff locally too. I'd run this code without hesitation.

(The failing review check is my infrastructure bug, not yours — fork PRs get a read-only GITHUB_TOKEN, so the reviewer workflow gets a 403 trying to post its comment. Ignore it, I'm fixing it separately.)

But I can't merge it as-is, and the reason isn't visible from the diff alone, so let me show the evidence.

Blocker: this rule already exists, so the PR doubles every finding

main already has an unsafe-deserialisation block about 30 lines above where you added yours — deterministic.py:226-238:

# --- Unsafe deserialisation ---
if target in ("pickle.loads", "pickle.load", "cPickle.loads", "_pickle.loads"):
    self._add(node, "py.insecure_deserialization", "insecure_deserialization", ...)
if target in ("yaml.load",) and not any(kw.arg == "Loader" for kw in node.keywords):
    self._add(node, "py.yaml_load", "insecure_deserialization", ...)

Findings are de-duplicated on Finding.key(), which is (file, line, category) — deliberately keyed on category so that "the same class of issue at the same location is the same finding, regardless of which layer found it" (model.py:60).

Your block emits category deserialization; the existing one emits insecure_deserialization. Different keys, so they no longer collapse — both survive.

On this 8-line file:

import pickle, yaml, flask

def load_session():
    raw = flask.request.data
    return pickle.loads(raw)

def load_config(text):
    return yaml.load(text)

main (2 findings)                                this PR (4 findings)
L5 py.insecure_deserialization                   L5 py.insecure_deserialization [insecure_deserialization]
      [insecure_deserialization]                 L5 py.insecure_deserialization [deserialization]     <-- duplicate
L8 py.yaml_load                                  L8 py.yaml_load                [insecure_deserialization]
      [insecure_deserialization]                 L8 py.insecure_deserialization [deserialization]     <-- duplicate

Every pickle.loads and yaml.load in every scanned repo would report twice.

Two knock-on problems:

1. The same rule_id (py.insecure_deserialization) now emits two different categories, breaking the one-rule-one-category invariant the rest of the engine assumes.
2. deserialization becomes a fifth, orphan spelling of a class the codebase already names insecure_deserialization in multilang.py (Java/Ruby/PHP) and crossfile.py.

Why test_no_false_positive_on_safe_deserialization passes, but not for the reason it looks like

It asserts "deserialization" not in _cats(...) — your new label. The pre-existing rule still fires insecure_deserialization on two of those same lines:

• yaml.load(user_input, yaml.SafeLoader) — positional loader, so main's any(kw.arg == "Loader") check misses it
• pickle.loads(b'hardcoded_constant_bytes') — main's pickle branch has no constant guard

So "zero false positives" holds for the new category but not for the report a user actually sees. Those are real pre-existing FPs in main, not something you introduced — but it does mean the test doesn't prove what it claims.

The good news: three of your four additions are real gaps

I checked each against main:

case	main	verdict
marshal.loads(x)	miss	real gap
shelve.open(x)	miss	real gap
yaml.load(x, Loader=yaml.Loader)	miss	real gap
pickle.loads(x)	caught	already covered
yaml.load(x)	caught	already covered

The third is the best catch in this PR: main treats any Loader= kwarg as safe, so an explicit Loader=yaml.Loader sails straight through.

Suggested fix — extend the existing block instead of adding a second one

I wrote and tested this. 252 tests pass, ruff clean, all three gaps closed, no duplicates:

        # --- Unsafe deserialisation ---
        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,
                      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", "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")

Measured against your version:

• marshal, shelve, Loader=yaml.Loader — caught (your three gaps) ✅
• yaml.unsafe_load(x) — also caught; explicit unsafe entry point, missed today
• yaml.load(x, yaml.SafeLoader) — now correctly silent, fixing the pre-existing positional-loader FP
• resolves Loader by last path segment, so both yaml.SafeLoader and a bare SafeLoader import work
• every case reports exactly once; one rule_id → one category

Two notes on your version, for the rewrite

1. not isinstance(args[0], ast.Constant) reads as a taint check but isn't one — it only excludes literals. pickle.load(f) on a local file handle is flagged identically to a request body. That's defensible (bandit flags the API, not the flow, and the existing rule does the same), so in my version I kept the unconditional flag and used _is_tainted to modulate confidence (0.88 vs 0.8) the way the surrounding rules do, rather than to gate the finding.
2. yaml.load with a constant first arg (yaml.load(SOME_LITERAL)) is still flagged — you guarded pickle against constants but not yaml. I left it flagged for consistency with the existing rule; push back if you disagree.

What I'd like

Push the rewrite and I'll re-review same day. Or, if you'd rather I apply the patch above myself, say so and I'll land it with you credited in CONTRIBUTORS.md — you found the gaps, which is the hard part, and I don't want the structural note to cost you the contribution.

One housekeeping item either way: the PR body describes category deserialization; after the rewrite it becomes insecure_deserialization, so worth updating the description so the changelog reads correctly.

[bkd-dotcom]

Closing this — #87 is implemented and merged in #103, and your three findings are what it's built on. Credited in CONTRIBUTORS.md and the changelog.

The review offered to apply the patch with credit if you'd rather not rewrite; the owner asked me to land it, so that's what happened. Not a reflection on the work.

What you found that main genuinely missed, all now caught:

gap	status
marshal.loads / marshal.load	✅
shelve.open	✅
yaml.load(x, Loader=yaml.Loader)	✅

That third one was the best catch. The old condition was:

if target in ("yaml.load",) and not any(kw.arg == "Loader" for kw in node.keywords):

Any Loader= counted as safe — so an explicitly unsafe loader passed silently. Finding that required actually reading the condition rather than the rule's name.

It also led to two things beyond the issue:

• yaml.unsafe_load was missing too, so it's now covered.
• Chasing the Loader logic exposed a pre-existing false positive: yaml.load(x, yaml.SafeLoader) — a positional safe loader — was being flagged, because the old check only inspected keywords. Fixed.

Why it couldn't merge as written, for the record: findings dedup on (file, line, category), and the new block used category deserialization while the existing rule uses insecure_deserialization. Different key → no collapse → every pickle/yaml finding reported twice. On the 8-line fixture from the review that was 2 findings → 4. #103 extends the existing block instead, and there's now a test asserting exactly 2 findings with a single category, so that mistake can't come back.

You've landed two rules in this engine now (#73, #89) and directly caused a third plus a false-positive fix. That's a real contribution record. The board is at Signetry/signetry#10 if you want the next one — though fair warning, you've cleared most of it.