T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/coordinator/coordinator-check.sh:126
- Finding
- Unescaped Workflow Data Allows JSON State Injection<![CDATA[ ## Vulnerability Details **File Location**: `scripts/coordinator/coordinator-check.sh`, source at line 14 and vulnerable output construction at lines 126–136 **Vulnerability Type**: Improper escaping when generating JSON **Risk Level**: Medium ### Vulnerable Code ```bash check_workflow() { if [ -f "$WORKFLOW_DIR/state.json" ]; then python3 -c "import json; d=json.load(open('$WORKFLOW_DIR/state.json')); print(d.get('phase', 'unknown'))" 2>/dev/null || echo "error" else echo "no_workflow" fi } ``` The resulting value is inserted directly into a JSON heredoc: ```bash cat > "$STATE_FILE" << EOF { "workflow": "$workflow", "gates_passed": $passed, "gates_total": $total, "skills_with_check": $with_check, "skills_with_version": $with_version, "skills_total": $skill_total, "pending_tasks": $pending, "score": $score, "checked_at": "$(date -Iseconds)" } EOF ``` ### Technical Analysis The `phase` property is read from `/root/.openclaw/workspace/.workflow/state.json` and printed as raw text. The script subsequently places that text inside a quoted JSON value without applying JSON escaping. A `phase` value containing quotation marks, backslashes, control characters, or newlines can therefore break out of the intended `workflow` string. This can inject additional properties into the generated `.coordinator/state.json` file or make the file invalid. For example, a valid source JSON document could contain a value logically equivalent to: ```json { "phase": "ok\",\n \"forged_state\": true,\n \"padding\": \"" } ``` When interpolated by the heredoc, this introduces attacker-selected structure into the generated status document. This is a data-integrity vulnerability rather than shell command injection: shell metacharacters resulting from variable expansion are not reparsed as shell syntax. ### Attack Path 1. An attacker or compromised component obtains permission to modify `/root/.opencl ...[truncated 1379 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Generate the output document with a JSON-aware serializer instead of interpolating values into a heredoc. For example, pass the collected values as arguments to Python and use `json.dump`: ```bash python3 - "$STATE_FILE" "$workflow" "$passed" "$total" \ "$with_check" "$with_version" "$skill_total" "$pending" "$score" <<'PY' import datetime import json import sys ( output_file, workflow, passed, total, with_check, with_version, skill_total, pending, score, ) = sys.argv[1:] document = { "workflow": workflow, "gates_passed": int(passed), "gates_total": int(total), "skills_with_check": int(with_check), "skills_with_version": int(with_version), "skills_total": int(skill_total), "pending_tasks": int(pending), "score": int(score), "checked_at": datetime.datetime.now( datetime.timezone.utc ).astimezone().isoformat(), } with open(output_file, "w", encoding="utf-8") as stream: json.dump(document, stream, ensure_ascii=False, indent=4) stream.write("\n") PY ``` Additional hardening measures should include: 1. Validate that `phase` is a string and, if the accepted states are known, enforce an allowlist such as `pending`, `running`, `complete`, and `failed`. 2. Reject unexpected control characters and impose a reasonable maximum length. 3. Write to a securely created temporary file in the destination directory and atomically rename it to prevent readers from observing partial output. 4. Apply restrictive permissions to the state directory and files so untrusted users cannot modify workflow input or replace coordinator output. 5. Add tests using phase values containing quotes, backslashes, newlines, Unicode characters, and other JSON boundary cases. ]]>