From 59852430aa06c64a31b64ea43158687807feb753 Mon Sep 17 00:00:00 2001 From: Binay Date: Tue, 18 Aug 2026 16:19:10 -0400 Subject: [PATCH] feat(checks): flag insecure deserialization introduced in a PR diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #2. scan_deserialization flags a deserialization sink an added line introduces, across Python (pickle, marshal, shelve, yaml.load without a safe Loader, yaml.unsafe_load), Java (ObjectInputStream / readObject), PHP (unserialize), Ruby (Marshal.load / YAML.load) and .NET (BinaryFormatter, NetDataContractSerializer, LosFormatter, ObjectStateFormatter). Advisory, never blocking, as the issue asks. Deserialization is an RCE surface, but whether the input is attacker-controlled cannot be decided from a diff hunk — a fixture loading its own artifact is routine. So this adds reviewer context and never decides mergeability, matching how arch.dynamic_exec treats eval/exec. Precision details: * added lines only, so a REMOVED sink (a fix) is not a finding * whole-line comments are skipped in all four comment styles — the mistake ci.pull_request_target made, where documenting a risk tripped the rule meant to catch it * yaml.load(..., Loader=SafeLoader/CSafeLoader/BaseLoader), yaml.safe_load and json.loads are not flagged; yaml.unsafe_load always is * one finding per added line (first matching sink wins) * sinks under tests/fixtures/testdata/examples/benchmarks report MEDIUM instead of HIGH 6 tests: all ten sinks across six languages, advisory/non-blocking, the four safe forms, comments in Python and Java, added-lines-only via a removal-only diff, and the test-path severity split. --- CHANGELOG.md | 14 +++++++- signetry_reviewer/checks.py | 71 +++++++++++++++++++++++++++++++++++++ tests/test_reviewer.py | 68 +++++++++++++++++++++++++++++++++++ 3 files changed, 152 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 726eb75..2bdc48f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,19 @@ Follows [Keep a Changelog](https://keepachangelog.com/) / [SemVer](https://semve ## [Unreleased] -_Nothing yet._ +### Added — insecure-deserialization check (`deser.introduced`) + +- Flags a deserialization sink introduced by the diff, across Python (`pickle`, + `marshal`, `shelve`, `yaml.load` without a safe Loader, `yaml.unsafe_load`), Java + (`ObjectInputStream` / `readObject`), PHP (`unserialize`), Ruby (`Marshal.load` / + `YAML.load`) and .NET (`BinaryFormatter` and friends). +- **Advisory, never blocking.** Whether the input is attacker-controlled cannot be + read off a diff hunk, so this adds reviewer context rather than deciding + mergeability — matching how `arch.dynamic_exec` treats `eval`/`exec`. +- Added lines only, so a *removed* sink (i.e. a fix) is not a finding. +- Whole-line comments are skipped, and `yaml.load(..., Loader=SafeLoader)` / + `yaml.safe_load` / `json.loads` are not flagged. Sinks in test/fixture paths are + reported at MEDIUM rather than HIGH. ## [0.2.0] — 2026-08-18 diff --git a/signetry_reviewer/checks.py b/signetry_reviewer/checks.py index ed3e50e..6ba7d2e 100644 --- a/signetry_reviewer/checks.py +++ b/signetry_reviewer/checks.py @@ -300,6 +300,76 @@ def scan_injection_surfaces(diff: str) -> list[Finding]: return findings +# --- 3b. insecure deserialization introduced in the diff -------------------- + +# (label, pattern). Each entry is a deserialization *sink* that executes attacker +# data during decoding. Ordered per language; the first match on a line wins so one +# added line yields at most one finding. +_DESER_SINKS: tuple[tuple[str, re.Pattern[str]], ...] = ( + ("Python pickle", re.compile(r"\b(?:c?pickle|_pickle)\.loads?\s*\(")), + ("Python marshal", re.compile(r"\bmarshal\.loads?\s*\(")), + ("Python shelve", re.compile(r"\bshelve\.open\s*\(")), + # yaml.load is only unsafe without a safe Loader; yaml.unsafe_load always is. + ("Python yaml.load", re.compile( + r"\byaml\.unsafe_load\s*\(" + r"|\byaml\.load\s*\((?![^)]*(?:Safe|CSafe|Base)Loader)" + )), + ("Java native deserialization", re.compile( + r"new\s+(?:[\w.]*\.)?ObjectInputStream\s*\(|\.readObject\s*\(\s*\)" + )), + ("PHP unserialize", re.compile(r"\bunserialize\s*\(")), + ("Ruby Marshal/YAML load", re.compile(r"\bMarshal\.load\s*\(|\bYAML\.load\s*\(")), + (".NET BinaryFormatter", re.compile( + r"\bnew\s+(?:BinaryFormatter|NetDataContractSerializer|LosFormatter|ObjectStateFormatter)\s*\(" + )), +) + +# Deserializing a value is not automatically a vulnerability — a fixture loading its +# own artifact is routine. These paths are where that is the likely reading. +_DESER_BENIGN_PATH = re.compile(r"(?:^|/)(?:tests?|fixtures?|testdata|examples?|benchmarks?)(?:/|$)") + + +def scan_deserialization(diff: str) -> list[Finding]: + """Flag an insecure-deserialization sink introduced by the diff. + + Advisory only. Deserialization is the shape of an RCE surface, but whether the + input is attacker-controlled cannot be decided from a diff hunk — so this adds + context for a reviewer and never decides mergeability, matching how + ``arch.dynamic_exec`` treats ``eval``/``exec``. + + Added lines only, like every other scanner here: a PR is judged on what it + introduces, not on deserialization that already existed. + """ + findings: list[Finding] = [] + for file, ln, text in added_lines(diff): + t = text.strip() + # Skip comment-only lines across the languages this covers, so documenting a + # sink does not trip the rule (the mistake ci.pull_request_target made). + if t.startswith(("#", "//", "*", "/*")): + continue + for label, pat in _DESER_SINKS: + if not pat.search(t): + continue + in_test_path = bool(_DESER_BENIGN_PATH.search(file.lower())) + findings.append(Finding( + id="deser.introduced", + category=Category.SECURITY, + severity=Severity.MEDIUM if in_test_path else Severity.HIGH, + title=f"Insecure deserialization introduced ({label})" + + (" in a test/fixture path" if in_test_path else ""), + detail=f"An added line deserializes data via {label}. On attacker-controlled " + "input this is remote code execution during decoding, not merely a " + "parsing bug. Confirm the input is trusted, or move to a safe format.", + file=file, line=ln, + remediation="Use a data-only format (JSON) for untrusted input; for YAML use " + "yaml.safe_load / Loader=SafeLoader; if a binary format is " + "required, sign and verify it before decoding.", + blocking=False, + )) + break # one finding per added line + return findings + + # --- 4. architectural smells ------------------------------------------------ @@ -348,5 +418,6 @@ def run_all_deterministic(diff: str, *, protected_globs: tuple[str, ...] = ()) - findings += scan_ci_permissions(diff) findings += scan_dependency_skew(diff) findings += scan_injection_surfaces(diff) + findings += scan_deserialization(diff) findings += scan_architecture(diff, protected_globs=protected_globs) return findings diff --git a/tests/test_reviewer.py b/tests/test_reviewer.py index 705093d..83bb35b 100644 --- a/tests/test_reviewer.py +++ b/tests/test_reviewer.py @@ -285,3 +285,71 @@ def test_review_json_serializable(): r = review_diff(_diff("src/util.py", [" return 1"]), repo="a/b", required_check="success") json.dumps(r.to_public()) assert r.to_public()["advisory"] is True + + +# --- insecure deserialization introduced in the diff (#2) ------------------- + + +def test_deserialization_sinks_flagged_across_languages(): + for path, line in [ + ("app.py", " obj = pickle.loads(raw)"), + ("app.py", " c = marshal.loads(b)"), + ("app.py", " db = shelve.open(name)"), + ("app.py", " cfg = yaml.load(text)"), + ("app.py", " cfg = yaml.unsafe_load(text)"), + ("Dao.java", " ObjectInputStream in = new ObjectInputStream(s);"), + ("Dao.java", " Object o = in.readObject();"), + ("index.php", "$o = unserialize($_POST['d']);"), + ("app.rb", " o = Marshal.load(data)"), + ("Repo.cs", " var f = new BinaryFormatter();"), + ]: + r = review_diff(_diff(path, [line]), repo="a/b", required_check="success") + hits = [f for f in r.findings if f.id == "deser.introduced"] + assert len(hits) == 1, f"expected one finding for {line!r}, got {len(hits)}" + + +def test_deserialization_is_advisory_never_blocking(): + # A deser sink alone must not decide mergeability — whether the input is + # attacker-controlled cannot be read off a diff hunk. + r = review_diff(_diff("app.py", [" obj = pickle.loads(raw)"]), repo="a/b", required_check="success") + f = next(x for x in r.findings if x.id == "deser.introduced") + assert f.blocking is False + assert r.verdict != Verdict.BLOCK + + +def test_deserialization_safe_forms_not_flagged(): + for line in [ + " cfg = yaml.load(text, Loader=yaml.SafeLoader)", + " cfg = yaml.load(text, Loader=SafeLoader)", + " cfg = yaml.safe_load(text)", + " cfg = json.loads(text)", + ]: + r = review_diff(_diff("app.py", [line]), repo="a/b", required_check="success") + assert not [f for f in r.findings if f.id == "deser.introduced"], line + + +def test_deserialization_ignores_comments(): + for path, line in [ + ("app.py", " # never use pickle.loads( on untrusted input"), + ("Dao.java", " // ObjectInputStream is unsafe here"), + ]: + r = review_diff(_diff(path, [line]), repo="a/b", required_check="success") + assert not [f for f in r.findings if f.id == "deser.introduced"], line + + +def test_deserialization_added_lines_only(): + # A REMOVED deser line is a fix, not a finding. + removal = ("--- a/app.py\n+++ b/app.py\n@@ -1,2 +1,1 @@\n unchanged\n" + "- obj = pickle.loads(raw)\n") + r = review_diff(removal, repo="a/b", required_check="success") + assert not [f for f in r.findings if f.id == "deser.introduced"] + + +def test_deserialization_lower_severity_in_test_paths(): + prod = review_diff(_diff("app.py", [" obj = pickle.loads(raw)"]), repo="a/b", required_check="success") + test = review_diff(_diff("tests/test_x.py", [" obj = pickle.loads(raw)"]), repo="a/b", required_check="success") + p = next(f for f in prod.findings if f.id == "deser.introduced") + t = next(f for f in test.findings if f.id == "deser.introduced") + assert p.severity == Severity.HIGH + assert t.severity == Severity.MEDIUM + assert "test/fixture" in t.title