T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/add-invoice.sh:92
- Finding
- jq Program Injection Through an Unescaped Invoice Number<![CDATA[ ## Vulnerability Details **File Location**: `scripts/add-invoice.sh`, lines 92 and 148 **Vulnerability Type**: User-controlled jq program injection **Risk Level**: Medium ### Vulnerable Code ```bash # Check if invoice number already exists if jq -e ".invoices[\"$INV_NUMBER\"]" "$INVOICES_FILE" > /dev/null; then echo "Error: Invoice number '$INV_NUMBER' already exists." exit 1 fi # ... # --- SAVE TO FILE --- temp_file=$(mktemp) jq ".invoices[\"$INV_NUMBER\"] = $invoice_json" "$INVOICES_FILE" > "$temp_file" && mv "$temp_file" "$INVOICES_FILE" ``` ### Technical Analysis The `INV_NUMBER` value is obtained from the user-controlled `--number` command-line argument and interpolated directly into two jq programs. Quoting the shell variable prevents shell word splitting, but it does not make the value safe for inclusion in jq source code. An invoice number containing a quote, closing bracket, and jq operators can terminate the intended property lookup and inject additional jq expressions. The first vulnerable invocation is used for duplicate detection, while the second transforms the database and writes the result back to `invoices.json`. For example, an invoice number shaped like: ```text x"] = {} | .invoices["attacker ``` changes the duplicate-check filter into an expression equivalent to: ```jq .invoices["x"] = {} | .invoices["attacker"] ``` and changes the database update into an expression equivalent to: ```jq .invoices["x"] = {} | .invoices["attacker"] = <new invoice object> ``` This can bypass the intended unique-key behavior and modify records other than the literal invoice number supplied by the caller. More generally, arbitrary jq filters can be introduced, allowing an attacker to delete, replace, or restructure data in the invoice database. This is jq-language injection rather than direct shell command injection. The demonstrated code does not provide operating-system command execution, because the injected content is interpreted ...[truncated 1895 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Never construct jq source code by directly interpolating user-controlled values. Pass the invoice number as a jq string argument and the invoice object as a JSON argument: ```bash # Safe duplicate check if jq -e --arg number "$INV_NUMBER" '.invoices[$number]' \ "$INVOICES_FILE" > /dev/null; then echo "Error: Invoice number '$INV_NUMBER' already exists." exit 1 fi # Safe database update temp_file=$(mktemp) if jq \ --arg number "$INV_NUMBER" \ --argjson invoice "$invoice_json" \ '.invoices[$number] = $invoice' \ "$INVOICES_FILE" > "$temp_file"; then mv -- "$temp_file" "$INVOICES_FILE" else rm -f -- "$temp_file" echo "Error: Failed to update invoice database." >&2 exit 1 fi ``` Apply additional defense-in-depth validation to invoice numbers. If business requirements permit, restrict them to a documented allowlist such as letters, digits, periods, underscores, and hyphens: ```bash if [[ ! "$INV_NUMBER" =~ ^[A-Za-z0-9._-]{1,100}$ ]]; then echo "Error: Invalid invoice number format." >&2 exit 1 fi ``` Additional hardening should include: 1. Validate that the existing database has the expected object structure before updating it. 2. Validate all numeric and date inputs before creating the invoice object. 3. Remove the temporary file through a cleanup trap if the script exits before `mv`. 4. Add regression tests using invoice numbers containing quotes, brackets, pipes, backslashes, and jq operators. 5. Consider file locking around the read-check-write sequence to prevent concurrent invocations from losing updates. ]]>