T09 · Insecure Skill Coding Practices
Error
- Location
- lib/config.js:43
- Finding
- Configuration Read or Parse Failures Cause Destructive Overwrite<![CDATA[ ## Vulnerability Details **File Location**: `lib/config.js:43-52`, `lib/config.js:183-191` **Vulnerability Type**: Fail-open configuration parsing and destructive overwrite **Risk Level**: High ### Vulnerable Code ```javascript function readConfig(path) { try { return JSON.parse(fs.readFileSync(path, "utf8")); } catch (_) { return {}; } } function writeConfig(path, cfg) { fs.writeFileSync(path, JSON.stringify(cfg, null, 2)); } ``` ```javascript if (cmd === "apply-enforce") { const configPath = process.argv[3]; const model = process.argv[4] || ""; const base = process.argv[5] || ""; const apiKey = process.argv[6] || ""; if (!configPath || !model || !base || !apiKey) throw new Error("missing required args"); const cfg = readConfig(configPath); const plan = planEnforce(cfg, model, base, apiKey); writeConfig(configPath, plan.cfg); process.exit(0); } ``` ### Technical Analysis `readConfig()` catches every filesystem and JSON parsing error and returns an empty object. This makes materially different conditions indistinguishable: - The configuration file does not exist. - The configuration contains malformed JSON. - The process lacks permission to read it. - A transient filesystem error occurred. - The file changed during the read. - The path points to an unexpected filesystem object. When `apply-enforce` receives the empty object, it generates a new configuration and writes it directly over the selected path. Existing unrelated OpenClaw settings can consequently be removed. The write is also performed directly with `fs.writeFileSync()` rather than an atomic same-directory temporary file followed by `rename()`. A crash, interruption, or storage failure during the write can leave the configuration truncated or partially written. Although the shell enforcer normally creates a backup before applying changes, that backup does not make the live overwrite safe and may preserve only the already malformed state. ### Atta ...[truncated 1259 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Fail closed on read and parsing failures: - Treat `ENOENT` separately if creating a new configuration is intended. - Reject malformed JSON, permission failures, and other I/O errors. - Display an actionable error and leave the existing file untouched. 2. Validate the parsed root value: - Require a non-null plain object. - Reject arrays, scalar JSON values, and structurally invalid configuration. 3. Implement atomic writes: - Create a temporary file in the same directory. - Set restrictive permissions. - Write and flush the complete JSON document. - Atomically rename the temporary file over the destination. - Clean up the temporary file on failure. 4. Re-read or verify file identity after acquiring the lock to reduce time-of-check/time-of-use races. 5. Preserve permissions and ownership when replacing an existing configuration. 6. Validate the completed temporary configuration before replacing the live file. Example approach: ```javascript function readConfigStrict(configPath) { const raw = fs.readFileSync(configPath, "utf8"); const cfg = JSON.parse(raw); if (!cfg || typeof cfg !== "object" || Array.isArray(cfg)) { throw new Error("configuration root must be a JSON object"); } return cfg; } ``` ]]>