T09 · Insecure Skill Coding Practices
Error
- Location
- source/composer.py:67
- Finding
- Arbitrary Code Execution Through Unsafe Workflow Condition Evaluation<![CDATA[ ## Vulnerability Details **File Location**: `source/composer.py:67-80` **Vulnerability Type**: Unsafe evaluation of attacker-controlled Python expressions **Risk Level**: High ### Vulnerable Code ```python try: return eval(condition, {"__builtins__": {}}) except: return True ``` ### Technical Analysis The application reads the `if` property of each workflow step directly from a YAML file and passes it to Python's `eval()` function. Although the evaluation globals replace `__builtins__` with an empty dictionary, this is not a secure sandbox. Python expressions can traverse the object model through attributes such as `__class__`, `__base__`, and `__subclasses__`. Depending on the classes loaded in the Python process, an attacker may locate a class that provides access to operating-system or subprocess functionality and use it to execute commands. The module imports `subprocess`, increasing the likelihood that useful process-related classes are available. Variable interpolation does not make the expression safe because it performs string replacement without parsing or restricting the resulting expression. In addition, the broad exception handler returns `True`, causing malformed or rejected conditions to fail open and execute the associated workflow step. ### Attack Path 1. An attacker creates or modifies a workflow YAML file accepted by the composer. 2. The attacker inserts a malicious Python expression into a step's `if` field. 3. A user invokes `composer.py run` with the attacker-controlled workflow. 4. `Workflow.load()` reads the expression without enforcing a restricted condition grammar. 5. `evaluate_condition()` substitutes available workflow variables into the expression. 6. The resulting expression is passed to `eval()`. 7. The expression traverses Python runtime objects to reach command-execution functionality. 8. The attacker's command executes with the operating-system privileges of the user running the composer. No shell me ...[truncated 732 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Remove `eval()` entirely. Do not attempt to secure it only by removing built-ins. 2. Implement a small, explicit condition grammar supporting only required operations, such as: - Equality and inequality comparisons. - Boolean constants. - References to known step status values. - Boolean `and`, `or`, and `not`, if necessary. 3. Parse conditions into tokens or an abstract syntax tree and reject every node or operator not explicitly allowed. 4. Keep variable values separate from the condition syntax rather than inserting them through raw string replacement. 5. Validate variable names against a strict identifier pattern and permit only known workflow variables. 6. Restrict status comparisons to an allowlist such as `pending`, `success`, `failed`, `timeout`, and `skipped`. 7. Fail closed: if a condition is malformed or cannot be evaluated, report a validation error and stop or skip the step according to a documented policy. Do not return `True`. 8. Validate all conditions before any workflow step executes. 9. Add regression tests containing object traversal, attribute access, function calls, comprehensions, malformed expressions, and attempted imports to verify that they are rejected. ]]>