T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/script.sh:30
- Finding
- Arbitrary Command Execution Through Configuration-Based sed Injection<![CDATA[ ## Vulnerability Details **File Location**: `scripts/script.sh:30-39` **Vulnerability Type**: Shell command injection through a dynamically constructed GNU `sed` program **Risk Level**: High ### Vulnerable Code ```bash _config_set() { _config_init local key="$1" value="$2" if grep -q "^${key}=" "$CONFIG_FILE" 2>/dev/null; then sed -i "s|^${key}=.*|${key}=${value}|" "$CONFIG_FILE" else echo "${key}=${value}" >> "$CONFIG_FILE" fi echo " ✓ ${key} = ${value}" } ``` ### Technical Analysis The `config` command accepts a configuration key and value from command-line arguments. `_config_set` interpolates both values directly into a GNU `sed` substitution expression: ```bash sed -i "s|^${key}=.*|${key}=${value}|" "$CONFIG_FILE" ``` Shell quoting prevents a direct shell metacharacter injection at the outer Bash parsing layer, but it does not make the dynamically generated `sed` program safe. An attacker can inject the `|` delimiter, a newline, and GNU `sed`'s `e` substitution flag into the value. The GNU `sed` `e` flag sends the resulting pattern space to `/bin/sh` for execution. Because an attacker-controlled key is also copied into the replacement text, the attacker can place shell separators and commands in that key. The application permits arbitrary new keys, making it possible to first plant a malicious key and then update it using an injected `sed` flag. The same unsanitized key is also interpreted as a regular expression by `grep` and `sed`, allowing regex metacharacters to alter which configuration line is selected. ### Attack Path A local caller, or an Agent induced to run attacker-supplied configuration arguments, can exploit the issue as follows: 1. Create a configuration entry whose key contains a shell command and separator: ```bash bash scripts/script.sh config 'x; id >/tmp/doc-summarize-pwned #' 1 ``` Since the key does not initially exist, the application appends a line similar t ...[truncated 1749 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. **Restrict configuration keys to an explicit allowlist.** Only the documented settings should be accepted: ```bash case "$key" in summary_sentences|keyword_count) ;; *) echo "Error: unsupported configuration key: $key" >&2 return 1 ;; esac ``` 2. **Validate values according to their expected type and range.** Both current settings represent bounded positive integers: ```bash if [[ ! "$value" =~ ^[0-9]+$ ]] || (( value < 1 || value > 100 )); then echo "Error: value must be an integer between 1 and 100" >&2 return 1 fi ``` Use tighter setting-specific limits where practical. 3. **Do not interpolate untrusted data into a `sed` program.** Rewrite the configuration through a temporary file while passing values as data rather than source code. With strict key and numeric-value validation, an implementation can use `awk` safely: ```bash local tmp tmp=$(mktemp "$DATA_DIR/config.XXXXXX") trap 'rm -f "$tmp"' RETURN awk -F= -v target="$key" -v replacement="$value" ' BEGIN { found = 0 } $1 == target { print target "=" replacement found = 1 next } { print } END { if (!found) print target "=" replacement } ' "$CONFIG_FILE" > "$tmp" chmod 600 "$tmp" mv -- "$tmp" "$CONFIG_FILE" trap - RETURN ``` 4. **Harden configuration storage.** Create the data directory and configuration file with user-only permissions: ```bash umask 077 mkdir -p -- "$DATA_DIR" ``` 5. **Treat existing configuration files as potentially compromised.** After deploying the fix, inspect or recreate `$HOME/.doc-summarize-pro/config` because an attacker may already have planted malformed keys or values. 6. **Add regression tests.** Test keys and values containing delimiters, newlines, backslashes, regex operators, ...[truncated 127 chars]