T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/cron-helper.sh:145
- Finding
- GNU sed Program Injection Through Unvalidated Line Number<![CDATA[ ## Vulnerability Details **File Location**: `scripts/cron-helper.sh:145-180` **Vulnerability Type**: Command injection through dynamically constructed sed program **Risk Level**: High ### Vulnerable Code ```bash remove_job() { check_crontab local mode="$1" local value="$2" local temp_file temp_file=$(mktemp) # Get current crontab local current_crontab current_crontab=$(crontab -l 2>/dev/null) || true if [[ -z "$current_crontab" ]]; then log_error "No cron jobs to remove" rm -f "$temp_file" exit 1 fi if [[ "$mode" == "line" ]]; then # Remove by line number echo "$current_crontab" | sed -n "H;1h;\$!d;x;s/^[0-9]*[ \t]*//;${value}d" > "$temp_file" log_success "Removed job at line $value" elif [[ "$mode" == "pattern" ]]; then # Remove by pattern echo "$current_crontab" | grep -v "$value" > "$temp_file" log_success "Removed jobs matching pattern: $value" else log_error "Invalid mode. Use 'line' or 'pattern'" rm -f "$temp_file" exit 1 fi crontab "$temp_file" rm -f "$temp_file" log_success "Cron job(s) removed" } ``` ### Technical Analysis The `remove` argument is expected to be a positive line number, but the script performs no numeric validation before interpolating it into a sed program: ```bash sed -n "...;${value}d" ``` Shell quoting prevents shell expansion of the value at this stage, but it does not prevent injection into sed's command language. On GNU sed, an attacker can include additional sed commands, including the GNU-specific `e` command, which executes a shell command. For example, a crafted value structured like `1e <shell-command> #` can cause the generated sed program to execute the supplied shell command when processing the first line. The appended `d` can be neutralized as part of a sed comment. This exploitation method is GNU sed-specific, m ...[truncated 1176 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Strictly require a positive decimal line number before invoking any text-processing utility: ```bash if [[ ! "$value" =~ ^[1-9][0-9]*$ ]]; then log_error "Line number must be a positive integer" rm -f "$temp_file" exit 1 fi ``` Avoid constructing an executable sed program from user input. Use a fixed program and pass the value as data, for example: ```bash awk -v target="$value" 'NR != target' <<< "$current_crontab" > "$temp_file" ``` Additional hardening should include: - Verify that the requested line exists before replacing the crontab. - Preview the exact entry to be removed and request confirmation. - Install the modified crontab only after the transformation succeeds. - Use an exit trap to remove temporary files on errors or interruption. - Add regression tests containing sed metacharacters and GNU sed `e` syntax. ]]>