T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/backup.sh:21
- Finding
- Predictable and Insufficiently Protected Temporary Backup Files<![CDATA[ ## Vulnerability Details **File Location**: `scripts/backup.sh`, lines 21-23 and 81-113 **Vulnerability Type**: Unsafe temporary-file handling and plaintext exposure of sensitive backup data **Risk Level**: Medium ### Vulnerable Code ```bash BACKUP_DIR="$TEMP_DIR/safe-backup-$TS" STATE_DIR="${OPENCLAW_STATE_DIR:-$HOME/.openclaw}" WORKSPACE_DIR="${OPENCLAW_WORKSPACE_DIR:-$HOME/.openclaw/workspace}" ``` ```bash # 1. Create temporary backup directory echo "[1/4] Creating backup directory..." mkdir -p "$BACKUP_DIR" # 2. Copy state directory (exclude sensitive files) echo "[2/4] Copying state directory..." # Build rsync args array RSYNC_ARGS=("-a" "--delete") for pattern in "${EXCLUDE_PATTERNS[@]}"; do RSYNC_ARGS+=("--exclude=$pattern") done rsync "${RSYNC_ARGS[@]}" "$STATE_DIR/" "$BACKUP_DIR/state/" # 3. Copy workspace (if exists) - use rsync with same exclusions echo "[3/4] Copying workspace..." if [ -d "$WORKSPACE_DIR" ]; then # Use rsync with same exclusion patterns as state directory rsync "${RSYNC_ARGS[@]}" "$WORKSPACE_DIR/" "$BACKUP_DIR/workspace/" else echo "Warning: Workspace directory not found: $WORKSPACE_DIR" fi # 4. Package echo "[4/4] Packaging backup..." cd "$TEMP_DIR" tar -czf "safe-backup-$TS.tar.gz" "safe-backup-$TS" # Cleanup temp directory rm -rf "$BACKUP_DIR" ``` ### Technical Analysis The script creates its staging directory and output archive directly under a temporary directory using a timestamp with one-second precision. On Linux, this is normally the shared `/tmp` directory. It uses `mkdir -p` rather than a securely and atomically created directory such as one produced by `mktemp -d`. Because the path is predictable, another local user can pre-create the expected staging path or insert symbolic links into it before the script reaches `mkdir` or `rsync`. The script does not verify that the staging directory and its components are real directories owned by the invoking user. The script also does not set ...[truncated 2704 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Set restrictive permissions before creating any backup content: ```bash umask 077 ``` 2. Create the staging directory atomically with `mktemp`: ```bash BACKUP_DIR="$(mktemp -d "${TEMP_DIR%/}/safe-backup.XXXXXXXX")" ``` 3. Register cleanup immediately so partial staging data is removed on both success and failure: ```bash cleanup() { rm -rf -- "$BACKUP_DIR" } trap cleanup EXIT HUP INT TERM ``` 4. Validate the temporary-directory environment before use: - Require it to exist and be a directory. - Reject symbolic links where appropriate. - Verify it is owned by the invoking user or has secure sticky-directory semantics. - Canonicalize paths before destructive operations. 5. Securely create the output archive and explicitly restrict its permissions: ```bash BACKUP_FILE="$(mktemp "${TEMP_DIR%/}/safe-backup.XXXXXXXX.tar.gz")" chmod 600 "$BACKUP_FILE" tar -czf "$BACKUP_FILE" -C "$(dirname "$BACKUP_DIR")" "$(basename "$BACKUP_DIR")" ``` 6. Prefer authenticated encryption as part of the backup operation rather than leaving an unencrypted archive in shared temporary storage. If encryption is optional, clearly warn the user that the archive remains plaintext until encrypted. 7. Validate that `BACKUP_DIR` is non-empty, canonical, owned by the current user, and located beneath the intended temporary root before passing it to `rm -rf`. 8. Treat exclusion patterns as defense in depth rather than a complete secret-management control. Consider an allowlist of necessary backup paths and add checks for sensitive file permissions or recognized key formats before packaging. ]]>