T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/scan_and_add_skill.sh:95
- Finding
- Scanner Errors Fail Open and Allow Unscanned Skills to Be Installed<![CDATA[ ## Vulnerability Details **File Location**: `scripts/scan_and_add_skill.sh:95-148` **Vulnerability Type**: Fail-open security validation **Risk Level**: High ### Vulnerable Code ```bash SCAN_OUT="$OUT_DIR/${DEST_NAME}_$TS.txt" set +e "$UV_BIN" run skill-scanner scan "$SRC_DIR" --format markdown --detailed --output "$REPORT" >"$SCAN_OUT" 2>&1 SCAN_CODE=$? set -e # Decide install policy: # - BLOCK only if High or Critical findings exist (unless --force) # - ALLOW Medium/Low/Info, but warn. # # Reports are markdown; we scrape the "Findings by Severity" summary. get_count() { local label="$1" # Matches lines like: "- **High:** 3" or "- **Critical:** 0" local n n=$(grep -E "\*\*${label}:\*\*" "$REPORT" 2>/dev/null | head -n 1 | sed -E 's/.*\*\*[^:]+:\*\* *([0-9]+).*/\1/') || true if [[ -z "${n:-}" || ! "$n" =~ ^[0-9]+$ ]]; then echo 0 else echo "$n" fi } CRITICAL_COUNT=$(get_count "Critical") HIGH_COUNT=$(get_count "High") MEDIUM_COUNT=$(get_count "Medium") LOW_COUNT=$(get_count "Low") INFO_COUNT=$(get_count "Info") DEST_BASE="$STATE_DIR/skills" DEST_DIR="$DEST_BASE/$DEST_NAME" BLOCKED=0 if [[ "$CRITICAL_COUNT" -gt 0 || "$HIGH_COUNT" -gt 0 ]]; then BLOCKED=1 fi if [[ $BLOCKED -eq 0 ]]; then mkdir -p "$DEST_BASE" if [[ -e "$DEST_DIR" ]]; then echo "ERROR: Destination already exists: $DEST_DIR" >&2 echo "Remove/rename it, or choose a different --name." >&2 exit 3 fi if [[ "$MEDIUM_COUNT" -gt 0 || "$LOW_COUNT" -gt 0 || "$INFO_COUNT" -gt 0 ]]; then echo "Scan result: ALLOWED WITH WARNINGS (no High/Critical)" echo " Critical: $CRITICAL_COUNT High: $HIGH_COUNT Medium: $MEDIUM_COUNT Low: $LOW_COUNT Info: $INFO_COUNT" else echo "Scan result: CLEAN (no findings)" fi # Copy the directory in a simple, predictable way. cp -a -- "$SRC_DIR" "$DEST_DIR" ``` ### Technical Analysis The script temporarily disables immediate error handling, invokes the scanner, and captures its exit status in `S ...[truncated 2080 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Make scan validation fail closed: 1. Reject any nonzero scanner exit status before parsing the report. 2. Require the report to exist, be a regular file, and be nonempty. 3. Require every expected severity field to be present exactly once and contain a valid nonnegative integer. 4. Reject malformed, incomplete, or unexpected report formats instead of converting parsing failures to zero. 5. Prefer a stable machine-readable format such as JSON over scraping Markdown. 6. Record scanner failures separately from positive security findings so operators can distinguish operational errors from detected threats. 7. Add regression tests covering scanner crashes, absent reports, empty reports, changed report formats, and malformed candidate Skills. Example hardening pattern: ```bash set +e "$UV_BIN" run skill-scanner scan "$SRC_DIR" \ --format markdown --detailed --output "$REPORT" \ >"$SCAN_OUT" 2>&1 SCAN_CODE=$? set -e if [[ $SCAN_CODE -ne 0 ]]; then echo "ERROR: Skill scan failed with status $SCAN_CODE; installation denied." >&2 echo "Scanner output: $SCAN_OUT" >&2 exit 4 fi if [[ ! -f "$REPORT" || ! -s "$REPORT" ]]; then echo "ERROR: Scanner did not produce a valid report; installation denied." >&2 exit 4 fi ``` The severity parser should also return an error rather than zero when a field cannot be found or parsed. ]]>