T09 · Insecure Skill Coding Practices
Error
- Location
- setup.sh:49
- Finding
- Legacy configuration is executed as shell code during migration<![CDATA[ ## Vulnerability Details **File Location**: `setup.sh:49-52` **Vulnerability Type**: Shell command injection through unsafe configuration evaluation **Risk Level**: High ### Vulnerable Code ```bash if [ "$MIGRATE" = true ]; then # Source the legacy .env to get variables set -a source "$LEGACY_CONFIG_FILE" 2>/dev/null set +a ``` ### Technical Analysis The migration process treats `~/.config/imap-smtp-email/.env` as executable Bash code by loading it with `source`. An environment file should be parsed strictly as data, but `source` evaluates all shell syntax in the file. Consequently, command substitutions, function invocations, redirections, expansions, and arbitrary shell commands embedded in the legacy configuration are executed with the privileges of the user running `setup.sh`. Restricting the file to `KEY=value` by convention does not provide any security because Bash does not enforce that convention. The vulnerable behavior is only necessary for extracting configuration values, not for the Skill's declared IMAP/SMTP functionality. It therefore exceeds the minimum behavior required for migration. ### Attack Path 1. An attacker gains the ability to create or modify `~/.config/imap-smtp-email/.env`, such as through insecure permissions, another vulnerable local application, a malicious backup, or a manipulated configuration package. 2. The attacker adds executable shell syntax, for example: ```bash IMAP_HOST=imap.example.com IMAP_USER=user@example.com IMAP_PASS="$(malicious-command)" ``` 3. The user runs `bash setup.sh`. 4. The script detects the legacy configuration and offers migration. 5. The user selects the migration option. 6. `source "$LEGACY_CONFIG_FILE"` evaluates the attacker's shell syntax. 7. The malicious command runs with the setup user's privileges before migration continues. ### Impact Assessment Successful exploitation provides arbitrary command execution as the local user running the setup script ...[truncated 311 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions - Never load `.env` files with `source`, `.`, or another shell evaluation mechanism. - Parse the file with a non-evaluating parser, such as `dotenv.parse()` in Node.js. - Permit only explicitly supported keys and reject malformed names, duplicate keys, NUL bytes, shell syntax, and unexpected multiline values. - Transfer parsed values through a safe serialization format rather than interpolating them into shell commands. - Perform the entire migration in Node.js where configuration values can be handled as strings without a second shell parser. - Verify that the legacy file is a regular file owned by the current user and is not writable by group or others before processing it. ]]>