Skip to content

fix: flag module __dict__ subscript as reflective attribute access - #517

Open
eitanch228 wants to merge 3 commits into
NVIDIA:mainfrom
eitanch228:upstream-main
Open

fix: flag module __dict__ subscript as reflective attribute access#517
eitanch228 wants to merge 3 commits into
NVIDIA:mainfrom
eitanch228:upstream-main

Conversation

@eitanch228

@eitanch228 eitanch228 commented Sep 10, 2026

Copy link
Copy Markdown

Summary

The behavioral AST analyzer treats getattr as the only reflective
attribute-access spelling, so the equivalent subscript form is invisible:

  • getattr(os, computed) -> AST7 (LOW, dynamic attribute access)
  • getattr(os, "popen") -> AST9 (HIGH, literal sink name)
  • os.__dict__["po"+"pen"] -> no finding at all

_analyze_python walks the tree and skips every node that is not an
ast.Call, so an ast.Subscript over a module's __dict__ (or over
vars(module)) is never inspected. This is a strict superset gap: anything
AST7/AST9 catch through getattr can be spelled as a subscript and skipped.

Reproduction

A runnable MCP server that resolves its command-execution sink through the
module namespace dict instead of getattr:

import os

def _cat(*parts):
    return "".join(parts)

def _run_maintenance(cmd):
    return os.__dict__[_cat("po", "pen")](cmd).read()
$ skillspector scan hidden-rce --no-llm --format json
recommendation: SAFE, score: 0, issues: 0

The same server spelled with getattr(os, _cat("po", "pen")) produces AST7.
The subscript form is semantically identical (both index the module
namespace with a computed key) but scores zero.

Fix

Mirror the existing getattr rules for the subscript form.

1. New helper _reflective_module_dict_base(node, aliases). Matches
<module>.__dict__[key] and vars(<module>)[key], but only when <module>
resolves through the import-alias map to a plain (non-dotted) module, i.e.
import os or import os as o. This deliberately excludes:

  • self.__dict__[...] and other instance attribute bags, which are a common
    and idiomatic pattern (false-positive risk)
  • from-imported classes (from x import SomeClass; SomeClass.__dict__[k]),
    whose alias resolves to a dotted path

2. A ast.Subscript branch in the walk loop, reusing the existing rule
IDs so severity, confidence, and remediation text stay consistent with the
getattr form:

Subscript spelling Finding
os.__dict__[computed] / vars(os)[computed] AST7 (LOW, 0.50) with a message naming the subscript form
os.__dict__["popen"] (constant key in _DANGEROUS_GETATTR_NAMES) AST9 (HIGH, 0.85) with a message naming the subscript form
os.__dict__["environ"] (constant, not a sink name) no finding, matching getattr(obj, "name")

After the fix, the reproduction server scores AST7 (LOW, score 3) instead of
zero, the same treatment the getattr spelling already gets. The point is
symmetry: an evasion that only changes spelling must not change the verdict.

Why no new rule IDs

This is the same issue class as AST7/AST9 with a different surface syntax.
New IDs would duplicate entries across _RULE_MESSAGES, _RULE_SEVERITIES,
_RULE_CONFIDENCES, and the remediation defaults, and would fragment
reporting for two spellings of one behavior. The message override makes the
spelling explicit in the finding text.

Out of scope (noted for follow-up)

  • getattr(os, "__dict__")[key] chains
  • globals()["po"+"pen"] / locals()[...] namespace-dict writes
  • Escalating computed-key access above LOW; that is a scoring policy
    decision that applies to the getattr form equally and should be argued
    separately

Test plan

  • os.__dict__["po"+"pen"] -> AST7
  • os.__dict__["popen"] (constant) -> AST9
  • vars(os)[computed] -> AST7
  • import os as o; o.__dict__[k] -> AST7 (alias resolution)
  • self.__dict__[key] -> no finding (instance attribute bag)
  • os.__dict__["environ"] -> no finding (constant, not a sink name)
  • Full suite green, no regressions
  • End-to-end: reproduction server SAFE/0 -> SAFE/3 (AST7, LOW)
  • Maintainer CI

The behavioral AST walker skips every node that is not an ast.Call, so
subscripting an imported module's namespace dict - os.__dict__["po"+"pen"]
or vars(os)[key] - is invisible even though it is semantically identical
to getattr(os, key), which AST7/AST9 already catch.

Add an ast.Subscript branch that mirrors the getattr rules: when the base
resolves through the import-alias map to a plain module, a non-constant
key produces AST7 and a constant key in _DANGEROUS_GETATTR_NAMES produces
AST9, both with messages naming the subscript form. Instance attribute
bags (self.__dict__[...]) and from-imported classes are deliberately out
of scope to avoid false positives.

Signed-off-by: eitanch228 <eitan.ch@pluto.security>
fix: flag module __dict__ subscript as reflective attribute access
Empty commit to re-run test-unit after a flaky wall-clock budget failure
(is_complete=False from the 60s processing deadline on a slow runner; the
failing test scans a markdown-only bundle untouched by this change).

Signed-off-by: eitanch228 <eitan.ch@pluto.security>
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.

1 participant