T09 · Insecure Skill Coding Practices
Warning
- Location
references/tool-internals.md:35- Finding
Fail-Open Permission Defaults in Recommended Tool Implementations
- Content
View full analysis
true, isConcurrencySafe: (_input) => false, // Default: disallow concurrency isReadOnly: (_input) => false, // Default: may modify state isDestructive: (_input) => false, // Default: not destructive checkPermissions: (input, ctx) => ({ behavior: 'allow', updatedInput: input }), toAutoClassifierInput: (_input) => '', // Default: excluded from security classifier userFacingName: (_input) => '', // Default to name } ``` The recommended minimal Tool implementation repeats the unconditional authorization pattern: ```typescript checkPermissions: async (input, context) => { return { behavior: 'allow', updatedInput: input } }, ``` The Skill also instructs developers that permission behavior defaults to approval: ```text 5. checkPermissions() — Permission logic, default allow 6. toAutoClassifierInput() — Security classification, default empty string ``` ### Technical Analysis The Skill describes the Tool defaults as “Fail-Closed,” but the documented authorization behavior is fail-open: - `checkPermissions()` unconditionally returns `behavior: 'allow'`. - `isDestructive()` defaults to `false`, even when a Tool may perform destructive operations. - `toAutoClassifierInput()` defaults to an empty string, preventing useful input from reaching the security classifier. - The example implementation encourages developers to copy unconditional authorization into new Tools. `isConcurrencySafe: false` only serializes execution. It does not deny unauthorized actions, request user confirmation, restrict resources, or classif ...[truncated 1691 chars]- Remediation
View remediation
({ behavior: 'prompt', message: 'This Tool may modify state. Approve this operation?' }) ``` 2. Require every state-changing Tool to implement explicit, operation-specific authorization rather than inheriting a generic approval default. 3. Validate and constrain all security-sensitive arguments, including file paths, commands, resource identifiers, destinations, and operation types. 4. Implement `isDestructive()` accurately for deletion, overwrite, publication, privilege, and other difficult-to-reverse operations. 5. Supply meaningful, normalized content through `toAutoClassifierInput()` so the security classifier can assess the actual operation and target. 6. Make documentation examples genuinely read-only, or include scoped permission checks that demonstrate least privilege and user confirmation. 7. Replace the inaccurate “Fail-Closed” description. Clearly distinguish concurrency serialization from authorization enforcement. 8. Add tests confirming that: - Unrecognized operations are denied. - Destructive operations require explicit approval. - Attacker-controlled paths and commands cannot escape their permitted scope. - Empty classifier input is rejected for security-sensitive Tools. - Permission checks cannot be bypassed through modified or internally derived input. ]]>