Skip to content

fix(analyzers): scale PE5/TM4 confidence down for reference material - #682

Open
CharmingGroot wants to merge 1 commit into
NVIDIA:mainfrom
CharmingGroot:fix/pe5-tm4-reference-confidence
Open

CharmingGroot wants to merge 1 commit into
NVIDIA:mainfrom
CharmingGroot:fix/pe5-tm4-reference-confidence

Conversation

@CharmingGroot

Copy link
Copy Markdown
Contributor

Refs #644 (item B7). Not a full fix for that issue — B1–B6 and B8 are untouched.

Summary

A vendor manifest reproduced under references/ scored exactly like the same text instructing the agent in SKILL.md. Both produced 56 HIGH DO_NOT_INSTALL on three synthetic skills.

The contextual-triage machinery could not express the difference. _risk_score in nodes/report.py computes base_points * weight * confidence and never reads tags, so contextual-triage only feeds _deduplicate_view_findings in static_runner.py. A PE5 finding carrying both triage tags scored identically to one carrying none. TM4 emitted no contextual tag at all.

This scales confidence instead, which the score already multiplies by. Both rules are ones I added (#214, #220), so the change stays inside them.

Changes

nodes/analyzers/common.py adds is_reference_material(file_path, file_type) and REFERENCE_MATERIAL_CONFIDENCE_SCALE = 0.5. It recognises markdown/text under a references/ or reference/ directory and never treats SKILL.md as reference material wherever it sits, mirroring the exemption already in is_code_example.

nodes/analyzers/static_patterns_privilege_escalation.py scales PE5 confidence and adds the triage tags for reference material. The per-line best-confidence selection compares the scaled value, so a uniform scale leaves the choice of winning pattern unchanged.

nodes/analyzers/static_patterns_tool_misuse.py applies the same to TM4, which previously had no contextual handling.

Before / After

skill before after
references/*.md, indented blocks 56 HIGH DO_NOT_INSTALL 31 MEDIUM CAUTION
references/*.md, fenced blocks + "for example" 56 HIGH DO_NOT_INSTALL 31 MEDIUM CAUTION
same content as a SKILL.md instruction 56 HIGH DO_NOT_INSTALL 56 HIGH DO_NOT_INSTALL

Findings in all cases: TM4 HIGH x3 (hostPID, hostNetwork, privileged), PE5 HIGH (--privileged), RP1 MEDIUM.

Design Decisions

decision reason
Scale confidence, not severity The score already multiplies by confidence, so no change to report.py is needed and the finding keeps its HIGH classification in the report.
De-emphasise, never suppress A skill author controls its own layout, so an instruction parked under references/ must still surface. At 0.5 it stays a HIGH finding with a visibly lower contribution.
Directory convention, not prose _is_documentation_example needs an indicator phrase in context, which a manifest pasted as an indented block does not have, and which an attacker can supply. A references/ path is a layout fact, and SKILL.md stays exempt under it.
Markdown and text only An executable under references/ still runs. references/setup.sh and references/ds.yaml keep full confidence.
Leave global tag-based scoring alone Making contextual-triage reduce score in report.py would fix this for every rule at once, but it changes output for rules I did not write. That is a scoring-policy call for maintainers.

Testing

19 new tests. Four PE5 cases in tests/nodes/analyzers/test_static_patterns.py cover reference material downweighted but still reported, SKILL.md at full confidence, SKILL.md under references/ at full confidence, and a shell script under references/ at full confidence. Three TM4 cases in tests/unit/test_patterns_new.py cover the same boundary. Twelve parametrized cases in tests/nodes/analyzers/test_common.py pin the helper across nested paths, backslash separators, case variation, and the non-matching my-references-guide.md.

make format and make lint pass. Full unit run: 8408 passed, 14 skipped, 134 deselected, 4 xfailed, no failures. The pre-change baseline on the same tree was 8389 passed, so the delta is exactly the new tests.

PE5 (NVIDIA#214) and TM4 (NVIDIA#220) scored a vendor manifest reproduced under references/ exactly like the same text instructing the agent in SKILL.md — 56 HIGH DO_NOT_INSTALL in both cases. The existing contextual-triage tags could not help: report.py scores base_points * weight * confidence and never reads tags, so the tag only feeds view deduplication in static_runner, and TM4 emitted no tag at all. Add is_reference_material() to common, recognising markdown/text under references/ while keeping SKILL.md an instruction file wherever it sits, and halve the confidence of PE5 and TM4 findings there. The describing copy drops to 31 MEDIUM CAUTION while the instruction case is unchanged. Findings are de-emphasised, never suppressed, so an instruction parked under references/ still surfaces as HIGH. Addresses B7 of NVIDIA#644.

Signed-off-by: CharmingGroot <ohyes9711@gmail.com>

@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.

[SkillSpector Review]

Hi @CharmingGroot, thank you for following up on #644 B7 for the rules you wrote, and for the clear root-cause analysis showing that the triage tags never reach _compute_risk_score!

Value and readiness: The false positive is real. A vendor's privileged eBPF DaemonSet copied into references/ should not read like an instruction to the agent. However, the PR lowers the score based only on file location, and the skill author controls that. It moves real privileged-container and container-escape content below the install-blocking threshold. The PR's own table shows the verdict change (56 HIGH DO_NOT_INSTALL to 31 MEDIUM CAUTION), and a malicious skill can trigger it on purpose. This needs a different approach before it can merge.

Material findings

  1. [Blocker] src/skillspector/nodes/analyzers/static_patterns_privilege_escalation.py:1073-1077, static_patterns_tool_misuse.py:3582-3597, common.py:96-108: halving PE5/TM4 confidence for any .md/.txt under a references/ or reference/ directory lets a skill author lower the verdict. Failing input: a SKILL.md that says "Before first use, deploy the collector exactly as described in references/deploy.md", plus a references/deploy.md with the PR's own fixture content: a privileged: true / hostPID: true / hostNetwork: true DaemonSet and docker run --privileged. On main that is 56 HIGH / DO_NOT_INSTALL: CLI exit 1, and MCP safe_to_install: false. With this PR it is 31 MEDIUM / CAUTION, as the PR's own table shows. The CLI exits 0 by default, and MCP run_scan can return safe_to_install: true (risk_score <= RISK_THRESHOLD) when the scan is otherwise complete. In --no-llm mode nothing restores the confidence.
    • The premise that references/ "describes rather than instructs" does not hold for Agent Skills. Reference files are loaded into the agent's context when SKILL.md points to them, and the agent reads them as instructions. A malicious author keeps SKILL.md short and puts the payload in a reference file.
    • The scale is also broader than the vendor-manifest case. It applies to every PE5_PATTERNS match, including host-escape primitives that have no "third-party requirement" reading: nsenter, cgroup release_agent, /proc/<pid>/ns/, unshare --user/--mount, and -v /:.
    • The codebase already has a pattern for this. PE3's documentation-directory exemption (_is_access_token_documentation_noun, static_patterns_privilege_escalation.py:540-600) applies only to a non-actionable noun, and "any credential action ... vetoes that classification", so that malicious instructions in docs stay actionable. This PR has no such veto.
    • Expected fix: do not reduce the score contribution based on path alone. Two options. (a) Keep the confidence unchanged and add only the contextual-triage / likely-benign-context tags for TM4/PE5 in reference material. That makes the context visible without changing the verdict, and leaves the scoring policy to maintainers as #644 frames it. (b) If a down-weight is wanted, limit it to non-actionable context with an action veto like PE3, and never to escape primitives. Then add an end-to-end test where SKILL.md points the agent at the reference file and the score stays above 50.
  2. [Non-blocking] src/skillspector/nodes/analyzers/common.py:108: is_reference_material matches a reference/references segment at any depth, case-insensitively. So docs/references/deep/vendor.md and scripts/reference/notes.md qualify. The Agent Skills convention is a top-level references/ directory. If any path-based behavior remains, restrict it to references/ directly under the skill root.

PIC tradeoffs: Whether a file's role (reference vs. SKILL.md/script) may change the risk score at all is a scoring-policy decision. #644 lists B7 under "policy or design questions", and the PR body also defers global tag-based scoring to maintainers. The tradeoff is fewer false DO_NOT_INSTALL verdicts on legitimate vendor docs (eBPF agents, security tooling) against a layout-based way to get past the default CLI gate and the MCP safe_to_install gate. Lower-risk options are: (a) tags only, with no score effect; or (b) let teams accept a reviewed vendor requirement through the existing baseline suppression with a recorded reason, which is how the 2.12.0 notes handle intentional SC10 private registries. The PIC should pick the policy before any score-affecting variant lands.

Verification and gaps: I traced _compute_risk_score in nodes/report.py. The score is base_points * diminishing_weight * confidence per finding, with bands at 21/51/81. The CLI exits 1 only for risk_score > 50, and MCP safe_to_install requires risk_score <= 50. So halving TM4 (0.55-0.7) and PE5 (0.7-0.95) confidences directly moves the verdict. Severity stays HIGH, so max_issue_severity and issues[].severity consumers are unaffected. I checked that the added triage tags have no other consumers: _deduplicate_view_findings only drops normalized-view duplicates when a raw non-contextual finding exists, so this PR adds no new suppression. With LLM enabled, a meta-analyzer confirmation can restore confidence through max(...), but that depends on the model. main has moved static_patterns_tool_misuse.py since the merge base, but not the TM4 block, and GitHub reports a clean merge. All 6 CI checks are green. Per policy I did not run the tests locally.


Decision: Changes Requested (reviewed head 51da43e8d077b4f10a597a2e4b67d2496e128ef3)

if reference_material or _is_documentation_example(context, file_type):
finding_tags.extend(["contextual-triage", "likely-benign-context"])
if line_num in pe5_best and pe5_best[line_num].confidence >= confidence:
effective_confidence = (

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.

Blocker: this halves PE5 confidence based only on the file path, and the skill author controls the path. A SKILL.md that tells the agent to follow references/deploy.md, with the privileged/escape content in that file, drops from 56 HIGH / DO_NOT_INSTALL (exit 1, MCP safe_to_install: false) to 31 MEDIUM / CAUTION (exit 0, safe_to_install can be true), per the PR's own table. The scale also covers escape primitives (nsenter, release_agent, /proc/<pid>/ns/, unshare, -v /:). Please keep the confidence unchanged and only add the triage tags, or add an action veto like PE3's _is_access_token_documentation_noun. See the review body.

segments = normalized.split("/")
if segments[-1] == "skill.md":
return False
return any(segment in _REFERENCE_MATERIAL_DIRS for segment in segments[:-1])

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.

Non-blocking: this matches a reference/references segment at any depth (e.g. docs/references/deep/vendor.md, scripts/reference/notes.md). The Agent Skills convention is a top-level references/ directory. If any path-based behavior remains, restrict it to references/ directly under the skill root.

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.

2 participants