Skip to content

feat(analyzer): detect insecure deserialization (AST10, TT6, DS1–DS4) - #246

Merged
rng1995 merged 4 commits into
NVIDIA:mainfrom
AbhiramDwivedi:feat/insecure-deserialization-detection
Aug 14, 2026
Merged

feat(analyzer): detect insecure deserialization (AST10, TT6, DS1–DS4)#246
rng1995 merged 4 commits into
NVIDIA:mainfrom
AbhiramDwivedi:feat/insecure-deserialization-detection

Conversation

@AbhiramDwivedi

Copy link
Copy Markdown
Contributor

What & why

Closes #245. Adds insecure-deserialization detection (CWE-502; OWASP ASI05 – Unexpected Code Execution). Before this change an RCE-class skill — e.g. PHP unserialize($_GET…) — scanned SAFE / 0.

Changes

  • behavioral_ast (AST10)pickle/cPickle/_pickle/marshal/dill/jsonpickle/joblib/pandas.read_pickle, plus argument-aware yaml.load, torch.load, numpy.load so hardened forms (SafeLoader, weights_only=True, default allow_pickle=False) are not flagged. MEDIUM / 0.70.
  • behavioral_taint_tracking (TT6) — external or file input → deserialization sink; HIGH / 0.85 (the deserialization analogue of TT5). File-read sources are deliberately included: loading a bundled/downloaded blob is the classic skill vector.
  • static_patterns_deserialization (DS1–DS4) — new language-gated regex module for the non-Python scripts a skill may bundle: PHP unserialize (DS1), Ruby Marshal/restore (DS2), Ruby YAML/Psych/Oj (DS3), JS node-serialize/serialize-to-js/funcster (DS4). Registered in the analyzer registry; Python is intentionally excluded here (covered with AST/taint precision above).
  • pattern_defaults.py — new Insecure Deserialization category plus explanation / remediation / display-name / category metadata for every new rule.

Scope

Only the languages SkillSpector already supports — Python (deep), JS/TS, Ruby, PHP (breadth). Java/.NET are intentionally out of scope.

Tests / validation

  • New unit tests: test_behavioral_ast.py, test_behavioral_taint_tracking.py, test_static_patterns_deserialization.py; test_registry.py updated for the new node.
  • make lint clean; ruff format --check clean; full unit suite: 1294 passed, 15 skipped, 6 xfailed.
  • E2E through the graph: a multi-language fixture → AST10 + TT6 + DS1 + DS2 + DS4, verdict DO_NOT_INSTALL / 90. Hardened Python forms (yaml.safe_load, weights_only=True) confirmed not false-positived.

All commits are DCO signed-off.

Closes the insecure-deserialization gap (OWASP ASI05 - Unexpected Code
Execution) across the analyzer stack:

- behavioral_ast (AST10): flags pickle / marshal / dill / jsonpickle /
  joblib / pandas.read_pickle, plus argument-aware yaml.load, torch.load,
  and numpy.load so the hardened forms (SafeLoader, weights_only=True,
  default allow_pickle=False) are not false-positived.
- behavioral_taint_tracking (TT6): external or file input -> deserialization
  sink, the RCE-class flow analogue of TT5.
- static_patterns_deserialization (DS1-DS4): language-gated regex breadth
  for the non-Python scripts a skill may bundle (PHP unserialize, Ruby
  Marshal/YAML/Oj, JS node-serialize/funcster).

Registers the new analyzer node, adds rule metadata (explanations,
remediations, category, pattern names), and ships unit tests for all rules
including hardened-form and language-gating negative cases.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Ram Dwivedi <abhiram.dwivedi@yahoo.com>
@AbhiramDwivedi
AbhiramDwivedi force-pushed the feat/insecure-deserialization-detection branch from 393942f to 12dff95 Compare July 6, 2026 12:02

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Automated SkillSpector Review]

Requesting changes. The AST/taint/static deserialization coverage is generally well structured, but the advertised argument-aware NumPy detector has a positional-argument bypass. Handle the third positional allow_pickle argument and add a regression before approval.

Comment thread src/skillspector/nodes/analyzers/behavioral_ast.py
@rng1995

rng1995 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

@AbhiramDwivedi Please address review comments and resolve merge conflicts.

numpy.load(file, mmap_mode, allow_pickle) accepts allow_pickle as its
third positional argument; AST10 only checked the allow_pickle= keyword,
so numpy.load(f, None, True) bypassed the deserialization finding.

Signed-off-by: Ram Dwivedi <abhiram.dwivedi@yahoo.com>
…ialization-detection

Signed-off-by: Ram Dwivedi <abhiram.dwivedi@yahoo.com>

# Conflicts:
#	src/skillspector/nodes/analyzers/behavioral_ast.py
@AbhiramDwivedi

Copy link
Copy Markdown
Contributor Author

Fixed. `numpy.load()` passes `allow_pickle` as its third positional parameter (`file, mmap_mode, allow_pickle, ...`), so `numpy.load(f, None, True)` bypassed AST10 since it only checked the keyword form. AST10 now also inspects `node.args[2]` when present.

Added regression tests: positional `allow_pickle=True` (flagged), positional `mmap_mode` alone (not flagged), and positional `allow_pickle=False` (not flagged) — alongside the existing keyword-form tests.

Also re-merged `upstream/main` to clear the conflicts (all in the shared-AST-cache refactor to `behavioral_ast.py`; resolved by keeping the new `ParsedPythonFile`-based signature while preserving the AST10 deserialization logic). Full unit suite passes (2093 passed; the 16 failures on this branch are pre-existing on `upstream/main` too — Windows-only symlink/`os.open`-mocking behavior, unrelated to this PR, verified via worktree against a clean `upstream/main` checkout).

… node

The DS1-DS4 node was written before main's inspection-ledger refactor and
still called run_static_patterns, so it was the only static analyzer that
reported findings without accounting for the files it inspected: it never
appeared in analysis_completeness or the analyzer-status table.

Switch to run_static_patterns_with_ledger, matching the other 14 static
analyzers, and cover the completed/skipped work items with tests.

Signed-off-by: Ram Dwivedi <abhiram.dwivedi@yahoo.com>
@AbhiramDwivedi

Copy link
Copy Markdown
Contributor Author

Follow-up on 7212134: while re-reviewing this branch against the merged main, I found that static_patterns_deserialization was still calling static_runner.run_static_patterns — it was written before the inspection-ledger refactor landed, so it was the only one of the 15 static analyzers that returned findings without emitting inspection_ledger / analyzer_status_events. Effect: its per-file work never showed up in analysis_completeness or the analyzer-status table, so a scan could under-report what was actually inspected.

Switched it to run_static_patterns_with_ledger to match the other 14 static analyzers, and added tests covering both the completed work items (one event per component, correct analyzer_id) and the skipped path for oversized files.

Current state of the branch:

  • d87437d — positional allow_pickle fix for AST10 (numpy.load(f, None, True)), with regression tests for the positional-True, positional-False, and mmap_mode-only forms.
  • 47be7fd — merge of main; the only real conflict was behavioral_ast.py, where the AST10 helpers were reapplied on top of the new ParsedPythonFile signature.
  • 7212134 — the ledger fix above.

ruff check and ruff format --check are clean; pytest -m "not integration" gives 2095 passed. The 16 failures I see locally (test_build_context.py symlink cases, test_create_github_release.py", test_input_handler.py::test_resolve_file_open_failure_does_not_create_temp_dir) reproduce identically on a clean checkout of main` in a separate worktree — they're Windows-only environment artifacts, not from this branch.

PR is MERGEABLE. @rng1995 ready for another look when you have time.

@rng1995
rng1995 merged commit a1fca7c into NVIDIA:main Aug 14, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Security] Insecure deserialization (pickle / PHP unserialize / Ruby Marshal / JS node-serialize) not detected — an RCE-class skill scores 0/SAFE

2 participants