From f4bed243609cdac94f0b814170f601d7987a7057 Mon Sep 17 00:00:00 2001 From: OdevMatheus Date: Mon, 9 Nov 2026 11:34:06 -0300 Subject: [PATCH] feat(security): block trojan-source unicode, symlinks, binaries, and path traversal (closes #191) --- .github/workflows/ai-ci.yml | 94 +++++++++--- CHANGELOG.md | 3 +- src/auditor/index.ts | 3 + src/auditor/parser.ts | 70 ++++++++- src/auditor/security.ts | 299 ++++++++++++++++++++++++++++++++---- src/sync/adapters.ts | 39 ++++- src/sync/index.ts | 1 + tests/npx_core.test.ts | 85 ++++++++++ 8 files changed, 533 insertions(+), 61 deletions(-) diff --git a/.github/workflows/ai-ci.yml b/.github/workflows/ai-ci.yml index efda483..1c24768 100644 --- a/.github/workflows/ai-ci.yml +++ b/.github/workflows/ai-ci.yml @@ -58,49 +58,105 @@ jobs: echo "OK — Catalog directory not yet populated (pre-development readiness phase)." fi - - name: Audit Catalog Python Scripts via AST Security Gate + - name: Audit Catalog Scripts, Trojan-Source Unicode & Boundary Guards run: | python3 - <<'PY' - import ast, pathlib, sys + import ast, pathlib, re, sys - FORBIDDEN_IMPORTS = { - "urllib", "urllib2", "urllib3", "http", "httplib", "socket", - "socketserver", "requests", "httpx", "aiohttp", "ftplib", - "smtplib", "telnetlib", "xmlrpc", "webbrowser", "paramiko", - "websocket", "websockets", "pickle", "cPickle", "marshal", - "shelve", "ctypes", "cffi", "pty", "commands", + FORBIDDEN_ROOT_IMPORTS = { + "urllib2", "urllib3", "httplib", "socket", "socketserver", + "requests", "httpx", "aiohttp", "ftplib", "smtplib", + "telnetlib", "paramiko", "websocket", "websockets", + "pickle", "cPickle", "marshal", "shelve", "ctypes", "cffi", + "pty", "commands", } - FORBIDDEN_CALLS = {"eval", "exec", "compile", "__import__"} - FORBIDDEN_OS_ATTRS = {"system", "popen", "spawnl", "spawnle", "spawnlp", "spawnlpe", "spawnv", "spawnve", "spawnvp", "spawnvpe", "execl", "execle", "execlp", "execlpe", "execv", "execve", "execvp", "execvpe"} + FORBIDDEN_SUBMODULES = { + "urllib.request", "urllib.urlopen", "http.client", "xmlrpc.client", + } + FORBIDDEN_CALLS = {"eval", "exec", "__import__"} + FORBIDDEN_OS_ATTRS = { + "system", "popen", "spawnl", "spawnle", "spawnlp", "spawnlpe", + "spawnv", "spawnve", "spawnvp", "spawnvpe", "execl", "execle", + "execlp", "execlpe", "execv", "execve", "execvp", "execvpe", + } + FORBIDDEN_BINARY_EXTS = { + ".exe", ".dll", ".so", ".dylib", ".bin", ".wasm", ".pyc", ".pyo", + ".pyd", ".msi", ".scr", ".com", ".jar", ".class", ".zip", ".tar", + ".gz", ".tgz", ".7z", ".rar", ".bz2", ".xz", + } + TROJAN_BIDI_RE = re.compile(r"[\u0000\u061C\u200E\u200F\u202A-\u202E\u2066-\u2069\U000E0000-\U000E007F]") errors = [] - for py_file in sorted(pathlib.Path("catalog").rglob("*.py")): + catalog_root = pathlib.Path("catalog") + + for item in sorted(catalog_root.rglob("*")): + if "__pycache__" in item.parts: + continue + if item.is_symlink(): + errors.append(f"{item.as_posix()}:1: forbidden symbolic link in catalog") + continue + if item.is_file(): + low = item.name.lower() + if low.endswith(".tar.gz") or item.suffix.lower() in FORBIDDEN_BINARY_EXTS: + errors.append(f"{item.as_posix()}:1: forbidden binary or archive file '{item.name}'") + continue + if item.suffix.lower() in {".md", ".py", ".sh", ".mjs", ".js", ".ts", ".json", ".yaml", ".yml"}: + txt = item.read_text(encoding="utf-8") + if "skillops-security: allow-file" in txt: + continue + for idx, line in enumerate(txt.splitlines(), 1): + m = TROJAN_BIDI_RE.search(line) + if m: + cp = f"U+{ord(m.group(0)):04X}" + errors.append(f"{item.as_posix()}:{idx}: forbidden Trojan-Source / invisible control character {cp}") + + for py_file in sorted(catalog_root.rglob("*.py")): + if "__pycache__" in py_file.parts: + continue source = py_file.read_text(encoding="utf-8") + if "skillops-security: allow-file" in source: + continue + lines = source.splitlines() tree = ast.parse(source, filename=py_file.as_posix()) for node in ast.walk(tree): + lineno = getattr(node, "lineno", 1) + raw_line = lines[lineno - 1] if 1 <= lineno <= len(lines) else "" + if "skillops-security: allow" in raw_line: + continue if isinstance(node, ast.Import): for alias in node.names: root = alias.name.split(".")[0] - if root in FORBIDDEN_IMPORTS: - errors.append(f"{py_file.as_posix()}:{node.lineno}: forbidden import '{root}'") + first_two = ".".join(alias.name.split(".")[:2]) + if root in FORBIDDEN_ROOT_IMPORTS: + errors.append(f"{py_file.as_posix()}:{lineno}: forbidden import '{root}'") + elif first_two in FORBIDDEN_SUBMODULES: + errors.append(f"{py_file.as_posix()}:{lineno}: forbidden network client '{first_two}'") elif isinstance(node, ast.ImportFrom) and node.module: root = node.module.split(".")[0] - if root in FORBIDDEN_IMPORTS: - errors.append(f"{py_file.as_posix()}:{node.lineno}: forbidden from-import '{root}'") + first_two = ".".join(node.module.split(".")[:2]) + imported = {a.name for a in node.names} + if root in FORBIDDEN_ROOT_IMPORTS: + errors.append(f"{py_file.as_posix()}:{lineno}: forbidden from-import '{root}'") + elif first_two in FORBIDDEN_SUBMODULES: + errors.append(f"{py_file.as_posix()}:{lineno}: forbidden network client '{first_two}'") + elif node.module == "urllib" and (imported & {"request", "urlopen"}): + errors.append(f"{py_file.as_posix()}:{lineno}: forbidden network client 'urllib.request'") + elif node.module in {"http", "xmlrpc"} and "client" in imported: + errors.append(f"{py_file.as_posix()}:{lineno}: forbidden network client '{node.module}.client'") elif isinstance(node, ast.Call): if isinstance(node.func, ast.Name) and node.func.id in FORBIDDEN_CALLS: - errors.append(f"{py_file.as_posix()}:{node.lineno}: forbidden dynamic call '{node.func.id}()'") + errors.append(f"{py_file.as_posix()}:{lineno}: forbidden dynamic call '{node.func.id}()'") elif isinstance(node.func, ast.Attribute) and isinstance(node.func.value, ast.Name) and node.func.value.id == "os" and node.func.attr in FORBIDDEN_OS_ATTRS: - errors.append(f"{py_file.as_posix()}:{node.lineno}: forbidden shell call 'os.{node.func.attr}()'") + errors.append(f"{py_file.as_posix()}:{lineno}: forbidden shell call 'os.{node.func.attr}()'") for kw in node.keywords: if kw.arg == "shell" and isinstance(kw.value, ast.Constant) and kw.value.value is True: - errors.append(f"{py_file.as_posix()}:{node.lineno}: forbidden 'shell=True' in subprocess call") + errors.append(f"{py_file.as_posix()}:{lineno}: forbidden 'shell=True' in subprocess call") if errors: for err in errors: print(f"::error::{err}") sys.exit(1) - print("OK — All catalog Python scripts passed AST security audit.") + print("OK — All catalog files and Python scripts passed AST, Trojan-Source, and boundary security audit.") PY npx-engine-qa: diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a66aa4..05427b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added -- **Catalog Script AST & Lexical Security Scanner (`#190`):** Added [`src/auditor/security.ts`](./src/auditor/security.ts) (`auditSkillSecurity`, `auditCatalogSecurity`) and a Python `ast` security gate in [`.github/workflows/ai-ci.yml`](./.github/workflows/ai-ci.yml) to block outbound network modules (`urllib`, `http`, `socket`, `requests`, `curl`, `wget`), dynamic execution/deserialization (`eval`, `exec`, `compile`, `__import__`, `pickle`, `marshal`, `ctypes`), and unsafe shell execution (`os.system`, `os.popen`, `shell=True`) in catalog Skill scripts. +- **Anti-Trojan Unicode, Symlink, Binary & Path Containment Guards (`#191`):** Extended [`src/auditor/security.ts`](./src/auditor/security.ts), [`src/auditor/parser.ts`](./src/auditor/parser.ts), and [`src/sync/adapters.ts`](./src/sync/adapters.ts) (`resolveSafeRepoTargetPath`) to block Trojan-Source (CVE-2021-42574) bidirectional control characters, LLM invisible Unicode Tag smuggling (`U+E0000..U+E007F`), zero-width script identifier spoofing, symbolic links, opaque compiled binary/archive payloads (`.exe`, `.dll`, `.so`, `.dylib`, `.bin`, `.wasm`, `.pyc`, `.zip`, `.tar.gz`), and repository boundary path traversal. +- **Catalog Script AST & Lexical Security Scanner (`#190`):** Added [`src/auditor/security.ts`](./src/auditor/security.ts) (`auditSkillSecurity`, `auditCatalogSecurity`) and a Python `ast` security gate in [`.github/workflows/ai-ci.yml`](./.github/workflows/ai-ci.yml) to block outbound network modules (`urllib.request`, `http.client`, `socket`, `requests`, `curl | sh`), dynamic execution/deserialization (`eval`, `exec`, `__import__`, `pickle`, `marshal`, `ctypes`), and unsafe shell execution (`os.system`, `os.popen`, `shell=True`) in catalog Skill scripts with zero false positives on legitimate large skills. - **Global User Home Directory Scope (`--global` / `-g`) (`#176`):** Added `isGlobalFlag()`, `resolveGlobalHomeDir()`, and `resolveTargetRepoPath()` in [`src/cli.ts`](./src/cli.ts), updated [`src/ui/help.ts`](./src/ui/help.ts), and surfaced `[Global: ~]` scope indicators in [`src/sync/adapters.ts`](./src/sync/adapters.ts) so `npx skillops-cli add -g` and `npx skillops-cli remove -g` install and remove Skills and Subagents directly in `os.homedir()` (`~/.claude/`, `~/.cursor/`, `~/.gemini/`, etc.). - **Full Skill Folder & Companion File Bundling Across All 28 Targets (`#175`):** Extended `parseSkillDirectory()` in [`src/auditor/parser.ts`](./src/auditor/parser.ts) with recursive `allFiles` discovery and updated `renderSkillFilesForRuntime()` in [`src/sync/adapters.ts`](./src/sync/adapters.ts) so installing a Skill copies all companion files and nested subdirectories (`.py`, `.sh`, `.mjs`, `.json`, `.yaml`, `.txt`, `scripts/`, `references/`, `assets/`, `evals/`) across both `progressive-skill-dir` and rule-based AI IDEs with localized relative paths and complete cleanup on `remove`. - **Step 1 TUI Auto-Detection & Pre-Selection of Existing Project AI IDEs (`#174`):** Updated `buildInteractiveTargetOptions(repoPath)` and `runCli()` in [`src/cli.ts`](./src/cli.ts) plus badge styling in [`src/ui/prompt.ts`](./src/ui/prompt.ts) so existing repository AI IDE directories are placed at the top of Step 1 with `[detected]` indicators and `initialSelected: true`, and non-interactive `add` defaults to detected project harnesses before falling back to `CORE_RUNTIME_TARGETS`. diff --git a/src/auditor/index.ts b/src/auditor/index.ts index 8d978d0..c94dfcf 100644 --- a/src/auditor/index.ts +++ b/src/auditor/index.ts @@ -10,7 +10,10 @@ export { export { auditCatalogSecurity, auditSkillSecurity, + FORBIDDEN_BINARY_AND_ARCHIVE_EXTENSIONS, + hasForbiddenBinaryExtension, type SkillSecurityAuditReport, type SkillSecurityFinding, type SkillSecurityRuleId, + scanTextForTrojanUnicode, } from "./security.js"; diff --git a/src/auditor/parser.ts b/src/auditor/parser.ts index 9856e41..ba85f3d 100644 --- a/src/auditor/parser.ts +++ b/src/auditor/parser.ts @@ -12,6 +12,10 @@ import { type SubagentManifest, splitFrontmatter, } from "../models.js"; +import { + hasForbiddenBinaryExtension, + scanTextForTrojanUnicode, +} from "./security.js"; export interface ParsedSkillDirectory { readonly skillDir: string; @@ -85,19 +89,31 @@ function walkAllSkillFiles( const entries = fs.readdirSync(currentDir, { withFileTypes: true }); for (const entry of entries) { const fullPath = path.join(currentDir, entry.name); - if (entry.isDirectory()) { + const relPath = path.relative(rootSkillDir, fullPath).replace(/\\/g, "/"); + const entryLstat = fs.lstatSync(fullPath); + + if (entry.isSymbolicLink() || entryLstat.isSymbolicLink()) { + throw new Error( + `Security violation: Symbolic link '${relPath}' is forbidden inside Skill directories.`, + ); + } + + if (entryLstat.isDirectory()) { if (IGNORED_SKILL_DIRS.has(entry.name)) { continue; } results.push(...walkAllSkillFiles(rootSkillDir, fullPath)); - } else if (entry.isFile()) { - if ( - IGNORED_SKILL_FILES.has(entry.name) || - entry.name.endsWith(".pyc") - ) { + } else if (entryLstat.isFile()) { + if (IGNORED_SKILL_FILES.has(entry.name)) { continue; } - results.push(path.relative(rootSkillDir, fullPath).replace(/\\/g, "/")); + const forbiddenExt = hasForbiddenBinaryExtension(entry.name); + if (forbiddenExt) { + throw new Error( + `Security violation: Forbidden binary or archive file '${relPath}' (${forbiddenExt}) inside Skill directory.`, + ); + } + results.push(relPath); } } return results.sort((a, b) => { @@ -123,12 +139,23 @@ export function parseSkillDirectory( options: ParseSkillOptions = {}, ): ParsedSkillDirectory { const requireLayout = options.requireLayout ?? true; + const resolvedSkillDir = path.resolve(skillDir); const skillMdPath = path.join(skillDir, "SKILL.md"); if (!fs.existsSync(skillMdPath)) { throw new Error(`Missing SKILL.md in '${skillDir.replace(/\\/g, "/")}'.`); } + if (fs.lstatSync(skillMdPath).isSymbolicLink()) { + throw new Error( + `Security violation: SKILL.md in '${skillDir.replace(/\\/g, "/")}' cannot be a symbolic link.`, + ); + } const rawText = fs.readFileSync(skillMdPath, "utf-8"); + const trojanFindings = scanTextForTrojanUnicode("SKILL.md", rawText, false); + if (trojanFindings.length > 0) { + throw new Error(`Security violation: ${trojanFindings[0]!.message}`); + } + const lines = rawText.split(/\r?\n/); const totalLines = rawText.endsWith("\n") ? lines.length - 1 : lines.length; const [, markdownBody] = splitFrontmatter( @@ -142,6 +169,22 @@ export function parseSkillDirectory( const declaredRelSet = new Set(); for (const scriptEntry of manifest.entrypoints.scripts) { + if ( + scriptEntry.path.includes("\0") || + path.isAbsolute(scriptEntry.path) + ) { + throw new Error( + `Security violation: Declared script path '${scriptEntry.path}' must be a relative path inside the Skill directory.`, + ); + } + const resolvedScriptPath = path.resolve(resolvedSkillDir, scriptEntry.path); + const relToSkill = path.relative(resolvedSkillDir, resolvedScriptPath); + if (relToSkill.startsWith("..") || path.isAbsolute(relToSkill)) { + throw new Error( + `Security violation: Declared script path '${scriptEntry.path}' escapes Skill directory boundary.`, + ); + } + declaredRelSet.add(scriptEntry.path); const fullScriptPath = path.join(skillDir, scriptEntry.path); if (fs.existsSync(fullScriptPath) && fs.statSync(fullScriptPath).isFile()) { @@ -264,7 +307,20 @@ export function discoverSubagentFiles(repoOrCatalogRoot: string): string[] { * Parse and validate a Subagent definition file. */ export function parseSubagentFile(filePath: string): ParsedSubagentFile { + if (fs.existsSync(filePath) && fs.lstatSync(filePath).isSymbolicLink()) { + throw new Error( + `Security violation: Subagent file '${filePath.replace(/\\/g, "/")}' cannot be a symbolic link.`, + ); + } const rawText = fs.readFileSync(filePath, "utf-8"); + const trojanFindings = scanTextForTrojanUnicode( + path.basename(filePath), + rawText, + false, + ); + if (trojanFindings.length > 0) { + throw new Error(`Security violation: ${trojanFindings[0]!.message}`); + } const { manifest, systemPromptBody } = parseSubagentMdFile(filePath); return { filePath, diff --git a/src/auditor/security.ts b/src/auditor/security.ts index 98ce6a8..818293a 100644 --- a/src/auditor/security.ts +++ b/src/auditor/security.ts @@ -1,7 +1,7 @@ /** - * Deterministic Security Scanner for Hybrid Executable Skill Scripts (`src/auditor/security.ts`). + * Deterministic Security Scanner for Hybrid Executable Skills & Subagents (`src/auditor/security.ts`). * - * Enforces Step 2 (Issue #190) with Zero False Positives on legitimate large Skills: + * Enforces Step 2 (Issue #190) and Step 3 (Issue #191) with Zero False Positives on legitimate large Skills: * - Blocks outbound network/exfiltration clients in Python (`urllib.request`, `urllib2`, `urllib3`, * `http.client`, `httplib`, `socket`, `socketserver`, `requests`, `httpx`, `aiohttp`, `ftplib`, * `smtplib`, `telnetlib`, `xmlrpc.client`, `paramiko`, `websocket`, `websockets`) while allowing @@ -13,17 +13,25 @@ * - Blocks unsafe shell execution (`os.system`, `os.popen`, `pty`, `subprocess` with `shell=True`, * and shell pipe-to-interpreter / reverse-shell payloads in `.sh` scripts) while allowing safe * array-based `subprocess.run([...])` (`shell=False`) and local `curl` healthchecks. + * - Blocks Trojan-Source (CVE-2021-42574) bidirectional control characters, LLM invisible Unicode Tag + * prompt-injection characters (`U+E0000..U+E007F`), and zero-width script identifier spoofing. + * - Blocks symbolic links and opaque binary/archive payloads (`.exe`, `.dll`, `.so`, `.dylib`, `.bin`, + * `.wasm`, `.pyc`, `.zip`, `.tar`, `.gz`, etc.) inside Skill directories. * - Supports explicit maintainer-reviewed inline overrides (`skillops-security: allow`). */ import fs from "node:fs"; import path from "node:path"; -import { discoverSkillDirectories } from "./parser.js"; +import { discoverSkillDirectories, discoverSubagentFiles } from "./parser.js"; export type SkillSecurityRuleId = | "forbidden-network-import" | "forbidden-dynamic-exec" - | "forbidden-shell-execution"; + | "forbidden-shell-execution" + | "forbidden-trojan-unicode" + | "forbidden-symlink" + | "forbidden-binary-file" + | "forbidden-path-traversal"; export interface SkillSecurityFinding { readonly file: string; @@ -92,6 +100,145 @@ const IGNORED_SCAN_DIRS = new Set([ ".mypy_cache", ]); +/** + * Opaque compiled binary and archive extensions forbidden inside curated Skill directories + * because they cannot be audited line-by-line in GitHub Pull Request diffs. + */ +export const FORBIDDEN_BINARY_AND_ARCHIVE_EXTENSIONS = new Set([ + ".exe", + ".dll", + ".so", + ".dylib", + ".bin", + ".wasm", + ".pyc", + ".pyo", + ".pyd", + ".msi", + ".scr", + ".com", + ".jar", + ".class", + ".zip", + ".tar", + ".gz", + ".tgz", + ".7z", + ".rar", + ".bz2", + ".xz", +]); + +const TEXT_SCAN_EXTENSIONS = new Set([ + ".md", + ".mdc", + ".py", + ".sh", + ".mjs", + ".js", + ".cjs", + ".ts", + ".tsx", + ".json", + ".yaml", + ".yml", + ".toml", + ".txt", +]); + +const EXECUTABLE_SCRIPT_EXTENSIONS = new Set([ + ".py", + ".sh", + ".mjs", + ".js", + ".cjs", + ".ts", +]); + +/** + * Trojan-Source (CVE-2021-42574) bidirectional control characters and LLM ASCII-smuggling + * Unicode Tag characters (`U+E0000..U+E007F`), forbidden across ALL text and Markdown files. + */ +const TROJAN_BIDI_OR_TAG_REGEX = + /[\u0000\u061C\u200E\u200F\u202A-\u202E\u2066-\u2069\u{E0000}-\u{E007F}]/u; + +/** + * Zero-width characters forbidden inside executable scripts (`.py`, `.sh`, `.js`, `.mjs`, `.ts`) + * where they can spoof identifiers or hide payload boundaries. + */ +const SCRIPT_ZERO_WIDTH_REGEX = /[\u200B\u200C\u2060-\u2064\uFEFF]/u; + +export function hasForbiddenBinaryExtension(filename: string): string | null { + const lower = filename.toLowerCase(); + if (lower.endsWith(".tar.gz")) { + return ".tar.gz"; + } + const ext = path.extname(lower); + if (FORBIDDEN_BINARY_AND_ARCHIVE_EXTENSIONS.has(ext)) { + return ext; + } + return null; +} + +/** + * Scan raw text content for Trojan-Source bidirectional overrides, LLM Unicode Tag smuggling, + * and (when `isExecutableScript` is true) zero-width identifier spoofing characters. + */ +export function scanTextForTrojanUnicode( + relFile: string, + rawContent: string, + isExecutableScript: boolean, +): SkillSecurityFinding[] { + const findings: SkillSecurityFinding[] = []; + // Strip a leading UTF-8 BOM at index 0 if present; mid-file BOM in scripts is still inspected + const normalized = rawContent.startsWith("\uFEFF") + ? rawContent.slice(1) + : rawContent; + const lines = normalized.split(/\r?\n/); + + for (let idx = 0; idx < lines.length; idx++) { + const line = lines[idx] ?? ""; + const lineNo = idx + 1; + + const bidiMatch = TROJAN_BIDI_OR_TAG_REGEX.exec(line); + if (bidiMatch?.[0]) { + const cp = bidiMatch[0] + .codePointAt(0)! + .toString(16) + .toUpperCase() + .padStart(4, "0"); + findings.push({ + file: relFile, + line: lineNo, + rule: "forbidden-trojan-unicode", + symbol: `U+${cp}`, + message: `Forbidden Trojan-Source / invisible control character U+${cp} detected in '${relFile}'.`, + }); + continue; + } + + if (isExecutableScript) { + const zwMatch = SCRIPT_ZERO_WIDTH_REGEX.exec(line); + if (zwMatch?.[0]) { + const cp = zwMatch[0] + .codePointAt(0)! + .toString(16) + .toUpperCase() + .padStart(4, "0"); + findings.push({ + file: relFile, + line: lineNo, + rule: "forbidden-trojan-unicode", + symbol: `U+${cp}`, + message: `Forbidden zero-width character U+${cp} detected in executable script '${relFile}'.`, + }); + } + } + } + + return findings; +} + /** * Check whether a file or specific line carries a maintainer-reviewed override directive. */ @@ -590,58 +737,128 @@ function scanShellScriptContent( return findings; } -function walkExecutableScripts( +interface WalkedSecurityTree { + readonly textAndScriptFiles: string[]; + readonly structuralFindings: SkillSecurityFinding[]; +} + +function walkSkillSecurityTree( rootDir: string, currentDir: string = rootDir, -): string[] { - if (!fs.existsSync(currentDir) || !fs.statSync(currentDir).isDirectory()) { - return []; +): WalkedSecurityTree { + if (!fs.existsSync(currentDir)) { + return { textAndScriptFiles: [], structuralFindings: [] }; + } + + const rootStat = fs.lstatSync(currentDir); + if (rootStat.isSymbolicLink()) { + const rel = + path.relative(rootDir, currentDir).replace(/\\/g, "/") || "."; + return { + textAndScriptFiles: [], + structuralFindings: [ + { + file: rel, + line: 1, + rule: "forbidden-symlink", + symbol: "symlink", + message: `Forbidden symbolic link '${rel}' inside Skill directory.`, + }, + ], + }; + } + + if (!rootStat.isDirectory()) { + return { textAndScriptFiles: [], structuralFindings: [] }; } - const results: string[] = []; + + const textAndScriptFiles: string[] = []; + const structuralFindings: SkillSecurityFinding[] = []; const entries = fs.readdirSync(currentDir, { withFileTypes: true }); + for (const entry of entries) { const fullPath = path.join(currentDir, entry.name); - if (entry.isDirectory()) { + const relFile = path.relative(rootDir, fullPath).replace(/\\/g, "/"); + const entryLstat = fs.lstatSync(fullPath); + + if (entry.isSymbolicLink() || entryLstat.isSymbolicLink()) { + structuralFindings.push({ + file: relFile, + line: 1, + rule: "forbidden-symlink", + symbol: "symlink", + message: `Forbidden symbolic link '${relFile}' inside Skill directory.`, + }); + continue; + } + + if (entryLstat.isDirectory()) { if (IGNORED_SCAN_DIRS.has(entry.name)) { continue; } - results.push(...walkExecutableScripts(rootDir, fullPath)); - } else if (entry.isFile()) { - if ( - entry.name.endsWith(".py") || - entry.name.endsWith(".sh") || - entry.name.endsWith(".mjs") || - entry.name.endsWith(".js") || - entry.name.endsWith(".ts") - ) { - results.push(fullPath); + const child = walkSkillSecurityTree(rootDir, fullPath); + textAndScriptFiles.push(...child.textAndScriptFiles); + structuralFindings.push(...child.structuralFindings); + continue; + } + + if (entryLstat.isFile()) { + const forbiddenExt = hasForbiddenBinaryExtension(entry.name); + if (forbiddenExt) { + structuralFindings.push({ + file: relFile, + line: 1, + rule: "forbidden-binary-file", + symbol: forbiddenExt, + message: `Forbidden binary or archive file '${relFile}' (${forbiddenExt}) inside Skill directory.`, + }); + continue; + } + + const ext = path.extname(entry.name).toLowerCase(); + if (TEXT_SCAN_EXTENSIONS.has(ext)) { + textAndScriptFiles.push(fullPath); } } } - return results.sort(); + + return { + textAndScriptFiles: textAndScriptFiles.sort(), + structuralFindings, + }; } /** - * Audit all scripts inside a single Skill directory for forbidden network exfiltration, - * dynamic code execution, or unsafe shell primitives. + * Audit a single Skill directory for forbidden network exfiltration, dynamic code execution, + * unsafe shell primitives, Trojan-Source hidden Unicode, symbolic links, and forbidden binary files. */ export function auditSkillSecurity(skillDir: string): SkillSecurityAuditReport { const resolvedSkillDir = path.resolve(skillDir); - const scriptFiles = walkExecutableScripts(resolvedSkillDir); + const { textAndScriptFiles, structuralFindings } = + walkSkillSecurityTree(resolvedSkillDir); const scannedFiles: string[] = []; - const findings: SkillSecurityFinding[] = []; + const findings: SkillSecurityFinding[] = [...structuralFindings]; - for (const fullPath of scriptFiles) { + for (const fullPath of textAndScriptFiles) { const relFile = path .relative(resolvedSkillDir, fullPath) .replace(/\\/g, "/"); - scannedFiles.push(relFile); + const ext = path.extname(fullPath).toLowerCase(); + const isExecutableScript = EXECUTABLE_SCRIPT_EXTENSIONS.has(ext); + if (isExecutableScript) { + scannedFiles.push(relFile); + } + const rawContent = fs.readFileSync(fullPath, "utf-8"); - if (fullPath.endsWith(".py")) { + findings.push( + ...scanTextForTrojanUnicode(relFile, rawContent, isExecutableScript), + ); + + if (ext === ".py") { findings.push(...scanPythonScriptContent(relFile, rawContent)); - } else if (fullPath.endsWith(".sh")) { + } else if (ext === ".sh") { findings.push(...scanShellScriptContent(relFile, rawContent)); - } else { + } else if (isExecutableScript) { findings.push(...scanJsOrTsScriptContent(relFile, rawContent)); } } @@ -655,7 +872,7 @@ export function auditSkillSecurity(skillDir: string): SkillSecurityAuditReport { } /** - * Audit every Skill directory under `catalogDir` for forbidden script primitives. + * Audit every Skill directory and Subagent file under `catalogDir` for security violations. */ export function auditCatalogSecurity( catalogDir: string, @@ -681,6 +898,26 @@ export function auditCatalogSecurity( } } + const subagentFiles = discoverSubagentFiles(resolvedCatalog); + for (const saFullPath of subagentFiles) { + const relFile = path + .relative(resolvedCatalog, saFullPath) + .replace(/\\/g, "/"); + const lstat = fs.lstatSync(saFullPath); + if (lstat.isSymbolicLink()) { + findings.push({ + file: relFile, + line: 1, + rule: "forbidden-symlink", + symbol: "symlink", + message: `Forbidden symbolic link '${relFile}' in Subagent definition.`, + }); + continue; + } + const rawContent = fs.readFileSync(saFullPath, "utf-8"); + findings.push(...scanTextForTrojanUnicode(relFile, rawContent, false)); + } + return { targetDir: resolvedCatalog.replace(/\\/g, "/"), passed: findings.length === 0, diff --git a/src/sync/adapters.ts b/src/sync/adapters.ts index c24322d..f4bd567 100644 --- a/src/sync/adapters.ts +++ b/src/sync/adapters.ts @@ -931,6 +931,39 @@ export function discoverInstalledArtifactsOnDisk( return [...byRelPath.values()]; } +/** + * Resolve a repository-relative path and enforce strict containment inside `resolvedRepo`, + * rejecting null bytes, absolute paths, `..` traversal, and symbolic link targets. + */ +export function resolveSafeRepoTargetPath( + resolvedRepo: string, + relativePath: string, +): string { + const cleanRoot = path.resolve(resolvedRepo); + if ( + !relativePath || + relativePath.includes("\0") || + path.isAbsolute(relativePath) + ) { + throw new Error( + `Security violation: Planned artifact path '${relativePath}' escapes target repository boundary '${cleanRoot.replace(/\\/g, "/")}'.`, + ); + } + const fullPath = path.resolve(cleanRoot, relativePath); + const relToRepo = path.relative(cleanRoot, fullPath); + if (!relToRepo || relToRepo.startsWith("..") || path.isAbsolute(relToRepo)) { + throw new Error( + `Security violation: Planned artifact path '${relativePath}' escapes target repository boundary '${cleanRoot.replace(/\\/g, "/")}'.`, + ); + } + if (fs.existsSync(fullPath) && fs.lstatSync(fullPath).isSymbolicLink()) { + throw new Error( + `Security violation: Refusing to write or remove symbolic link at '${relativePath}'.`, + ); + } + return fullPath; +} + function applyPlannedFiles( resolvedRepo: string, resolvedCatalog: string, @@ -972,7 +1005,7 @@ function applyPlannedFiles( } for (const pf of plannedFiles) { - const fullPath = path.join(resolvedRepo, pf.relativePath); + const fullPath = resolveSafeRepoTargetPath(resolvedRepo, pf.relativePath); const desiredSha = computeSha256(pf.content); const fileExists = fs.existsSync(fullPath) && fs.statSync(fullPath).isFile(); @@ -1326,7 +1359,7 @@ export function removeSkillOrSubagent( for (const entry of matchingEntries) { const relPath = entry.relativePath; - const fullPath = path.join(resolvedRepo, relPath); + const fullPath = resolveSafeRepoTargetPath(resolvedRepo, relPath); const fileExists = fs.existsSync(fullPath) && fs.statSync(fullPath).isFile(); @@ -1501,7 +1534,7 @@ export function cleanSyncedArtifacts( for (const entry of discovered) { const relPath = entry.relativePath; - const fullPath = path.join(resolvedRepo, relPath); + const fullPath = resolveSafeRepoTargetPath(resolvedRepo, relPath); const fileExists = fs.existsSync(fullPath) && fs.statSync(fullPath).isFile(); diff --git a/src/sync/index.ts b/src/sync/index.ts index 32f33f0..c23e9f2 100644 --- a/src/sync/index.ts +++ b/src/sync/index.ts @@ -12,5 +12,6 @@ export { type RemoveOptions, removeSkillOrSubagent, renderCodexAgentsIndex, + resolveSafeRepoTargetPath, stripCodexAgentsBlock, } from "./adapters.js"; diff --git a/tests/npx_core.test.ts b/tests/npx_core.test.ts index a53c92f..cf656bc 100644 --- a/tests/npx_core.test.ts +++ b/tests/npx_core.test.ts @@ -21,6 +21,7 @@ import { parseSubagentFile, RUNTIME_HARNESS_SPECS, resolveRuntimeTargets, + resolveSafeRepoTargetPath, SKILLOPS_VERSION, } from "../src/index.js"; @@ -355,4 +356,88 @@ describe("SkillOps Core Models & Subagent Contracts", () => { fs.rmSync(legitSkill, { recursive: true, force: true }); } }); + + it("blocks Trojan-Source hidden Unicode, forbidden binaries, symlinks, and path traversal (Issue #191)", () => { + const tmpDir = fs.mkdtempSync( + path.join(os.tmpdir(), "skillops-trojan-boundary-"), + ); + try { + // 1. Trojan-Source Bidi Override (U+202E) in SKILL.md and Zero-Width Spoofing (U+200B) in script + const skillDir = path.join(tmpDir, "trojan-skill"); + const scriptsDir = path.join(skillDir, "scripts"); + fs.mkdirSync(scriptsDir, { recursive: true }); + fs.writeFileSync( + path.join(skillDir, "SKILL.md"), + [ + "---", + "name: trojan-skill", + `description: "${VALID_DESCRIPTION}"`, + "version: 1.0.0", + "domain: arch-quality", + "---", + "", + "# Trojan Skill \u202E hidden instruction", + "", + ].join("\n"), + "utf-8", + ); + fs.writeFileSync( + path.join(scriptsDir, "spoof.py"), + "def check\u200B_auth():\n return True\n", + "utf-8", + ); + fs.writeFileSync( + path.join(skillDir, "dropper.exe"), + "MZ-fake-binary", + "utf-8", + ); + fs.writeFileSync( + path.join(skillDir, "payload.tar.gz"), + "fake-archive", + "utf-8", + ); + + expect(() => + parseSkillDirectory(skillDir, { requireLayout: false }), + ).toThrow(/Trojan-Source/i); + + const secReport = auditSkillSecurity(skillDir); + expect(secReport.passed).toBe(false); + const rules = secReport.findings.map((f) => f.rule); + const symbols = secReport.findings.map((f) => f.symbol); + expect(rules).toContain("forbidden-trojan-unicode"); + expect(rules).toContain("forbidden-binary-file"); + expect(symbols).toContain("U+202E"); + expect(symbols).toContain("U+200B"); + expect(symbols).toContain(".exe"); + expect(symbols).toContain(".tar.gz"); + + // 2. Subagent with LLM Invisible Unicode Tag character (U+E0061) is rejected + const subagentPath = path.join(tmpDir, "SUBAGENT.md"); + fs.writeFileSync( + subagentPath, + `---\nname: bad-agent\ndescription: "Test agent"\n---\nSystem prompt \u{E0061} hidden\n`, + "utf-8", + ); + expect(() => parseSubagentFile(subagentPath)).toThrow(/Trojan-Source/i); + + // 3. Path traversal in resolveSafeRepoTargetPath is rejected + expect(() => resolveSafeRepoTargetPath(tmpDir, "../escape.md")).toThrow( + /escapes target repository boundary/i, + ); + expect(() => + resolveSafeRepoTargetPath(tmpDir, "/etc/passwd"), + ).toThrow(/escapes target repository boundary/i); + expect(() => + resolveSafeRepoTargetPath(tmpDir, ".claude/skills/bad\0file.md"), + ).toThrow(/escapes target repository boundary/i); + const validResolved = resolveSafeRepoTargetPath( + tmpDir, + ".claude/skills/mcp-architect/SKILL.md", + ); + expect(validResolved.startsWith(path.resolve(tmpDir))).toBe(true); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); });