T09 · Insecure Skill Coding Practices
Warning
- Location
scripts/ledger.sh:80- Finding
Unvalidated line selector permits sed program injection
- Content
View full analysis
}" if [[ ! -f "$LEDGER_FILE" ]]; then echo "No entries."; exit 1; fi sed -i '' "${LINE}s/\"pending\"/\"paid\"/" "$LEDGER_FILE" echo "💚 Marked line $LINE as paid." ;; ``` ### Technical Analysis The `pay` command expects `LINE` to be a positive ledger line number, but it does not validate the value before embedding it into a sed program. Shell quoting prevents ordinary shell metacharacter expansion, but it does not prevent sed-language injection because the entire attacker-controlled value is interpreted as part of the sed script. An attacker can supply sed commands, command separators, ranges, or embedded newline characters instead of a numeric line selector. This allows operations beyond changing the requested entry from `pending` to `paid`. For example, the following argument introduces an additional substitution: ```bash bash scripts/ledger.sh pay \ '1s/"amount":[0-9.]*/"amount":999999/;2' ``` The resulting sed program is effectively: ```sed 1s/"amount":[0-9.]*/"amount":999999/;2s/"pending"/"paid"/ ``` This changes the amount in the first record and marks the second record as paid. Implementations supporting sed's `w` command can also be made to copy ledger content to an attacker-selected local file. Embedded newlines can terminate the injected command and preserve a syntactically valid final substitution. The exact available commands and syntax depend on the installed sed implementation. The use of `sed -i ''` is also specific to BSD-style sed and is not portable to common GNU sed invocations, but that compatibility problem does not mitigate the injection weakness. ### Attack Path 1. An attacker or untrusted automation component gains the ability to choose the argument passed to `ledger.sh pay`. 2. In ...[truncated 1114 chars]- Remediation
View remediation
}" if [[ ! "$LINE" =~ ^[1-9][0-9]*$ ]]; then echo "Error: line number must be a positive integer." >&2 exit 1 fi TOTAL_LINES=$(wc -l < "$LEDGER_FILE") if (( LINE > TOTAL_LINES )); then echo "Error: ledger line does not exist." >&2 exit 1 fi ``` Prefer replacing the sed-based update with structured JSON processing using `jq`. Write the result to a securely created temporary file in the same directory, check that processing succeeded, and then atomically rename it over the ledger: ```bash tmp=$(mktemp "$LEDGER_DIR/ledger.jsonl.XXXXXX") trap 'rm -f "$tmp"' EXIT jq -c --argjson target "$LINE" ' if input_line_number == $target and .status == "pending" then .status = "paid" else . end ' "$LEDGER_FILE" > "$tmp" mv -- "$tmp" "$LEDGER_FILE" trap - EXIT ``` The exact jq filter should be tested against JSON Lines input. Also reject attempts to pay an entry whose current status is not `pending`, and report whether a record was actually updated. ]]>