From 4f8f31444d63f034ecf4a56c37c72ac7b24b02b0 Mon Sep 17 00:00:00 2001 From: Tw93 Date: Fri, 14 Nov 2025 11:38:25 +0800 Subject: [PATCH] More secure deletion and cannot delete path --- bin/clean.sh | 10 ++++-- bin/uninstall.sh | 18 ++++++++-- lib/common.sh | 71 ++++++++++++++++++++++++++++++++++++++++ lib/uninstall_batch.sh | 45 +++++++++++++++++++++++-- lib/whitelist_manager.sh | 5 ++- 5 files changed, 139 insertions(+), 10 deletions(-) diff --git a/bin/clean.sh b/bin/clean.sh index 3f6991a..4783288 100755 --- a/bin/clean.sh +++ b/bin/clean.sh @@ -450,10 +450,17 @@ start_cleanup() { SYSTEM_CLEAN=true echo -e "${GREEN}${ICON_SUCCESS}${NC} Admin access granted" echo "" - # Start sudo keepalive with error handling + # Start sudo keepalive with robust parent checking + # Store parent PID to ensure keepalive exits if parent dies + parent_pid=$$ ( local retry_count=0 while true; do + # Check if parent process still exists first + if ! kill -0 "$parent_pid" 2> /dev/null; then + exit 0 + fi + if ! sudo -n true 2> /dev/null; then ((retry_count++)) if [[ $retry_count -ge 3 ]]; then @@ -464,7 +471,6 @@ start_cleanup() { fi retry_count=0 sleep 30 - kill -0 "$$" 2> /dev/null || exit done ) 2> /dev/null & SUDO_KEEPALIVE_PID=$! diff --git a/bin/uninstall.sh b/bin/uninstall.sh index 47e91bd..3364b9a 100755 --- a/bin/uninstall.sh +++ b/bin/uninstall.sh @@ -452,8 +452,22 @@ uninstall_applications() { read -p " Force quit $app_name? (y/N): " -n 1 -r echo if [[ $REPLY =~ ^[Yy]$ ]]; then - pkill -f "$app_path" 2> /dev/null || true - sleep 2 + # Retry kill operation with verification to avoid TOCTOU + local retry=0 + while [[ $retry -lt 3 ]]; do + pkill -f "$app_path" 2> /dev/null || true + sleep 1 + # Verify app was killed + if ! pgrep -f "$app_path" > /dev/null 2>&1; then + break + fi + ((retry++)) + done + + # Final check + if pgrep -f "$app_path" > /dev/null 2>&1; then + log_warning "Failed to quit $app_name after $retry attempts" + fi else echo -e " ${BLUE}${ICON_EMPTY}${NC} Skipped $app_name" continue diff --git a/lib/common.sh b/lib/common.sh index 89c087d..3593dbf 100755 --- a/lib/common.sh +++ b/lib/common.sh @@ -42,6 +42,77 @@ mo_spinner_chars() { printf "%s" "$chars" } +# Security and Path Validation Functions + +# Validates a path for safe deletion +# Returns 0 if path is safe to delete, 1 otherwise +validate_path_for_deletion() { + local path="$1" + + # Check path is not empty + if [[ -z "$path" ]]; then + log_error "Path validation failed: empty path" + return 1 + fi + + # Check path is absolute + if [[ "$path" != /* ]]; then + log_error "Path validation failed: path must be absolute: $path" + return 1 + fi + + # Check path doesn't contain dangerous characters + if [[ "$path" =~ [[:cntrl:]] ]] || [[ "$path" =~ $'\n' ]]; then + log_error "Path validation failed: contains control characters: $path" + return 1 + fi + + # Check path isn't critical system directory + case "$path" in + / | /bin | /sbin | /usr | /usr/bin | /usr/sbin | /etc | /var | /System | /Library/Extensions) + log_error "Path validation failed: critical system directory: $path" + return 1 + ;; + esac + + # Path is safe + return 0 +} + +# Safe wrapper around rm -rf with validation and logging +# Usage: safe_remove "/path/to/file" +# Returns 0 on success, 1 on failure +safe_remove() { + local path="$1" + local silent="${2:-false}" + + # Validate path + if ! validate_path_for_deletion "$path"; then + return 1 + fi + + # Check if path exists + if [[ ! -e "$path" ]]; then + [[ "$silent" != "true" ]] && log_warning "Path does not exist, skipping: $path" + return 0 + fi + + # Log what we're about to delete + if [[ -d "$path" ]]; then + log_info "Removing directory: $path" + else + log_info "Removing file: $path" + fi + + # Perform the deletion + if rm -rf "$path" 2> /dev/null; then + return 0 + else + log_error "Failed to remove: $path" + return 1 + fi +} + # Logging configuration readonly LOG_FILE="${HOME}/.config/mole/mole.log" readonly LOG_MAX_SIZE_DEFAULT=1048576 # 1MB diff --git a/lib/uninstall_batch.sh b/lib/uninstall_batch.sh index 817d49a..f3e62e5 100755 --- a/lib/uninstall_batch.sh +++ b/lib/uninstall_batch.sh @@ -8,6 +8,40 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" # Batch uninstall functionality with minimal confirmations # Replaces the overly verbose individual confirmation approach + +# Decode and validate base64 encoded file list +# Returns decoded string if valid, empty string otherwise +decode_file_list() { + local encoded="$1" + local app_name="$2" + local decoded + + # Decode base64 data + if ! decoded=$(printf '%s' "$encoded" | base64 -d 2>/dev/null); then + log_error "Failed to decode file list for $app_name" + echo "" + return 1 + fi + + # Validate decoded data doesn't contain null bytes + if [[ "$decoded" =~ $'\0' ]]; then + log_warning "File list for $app_name contains null bytes, rejecting" + echo "" + return 1 + fi + + # Validate paths look reasonable (each line should be a path or empty) + while IFS= read -r line; do + if [[ -n "$line" && ! "$line" =~ ^/ ]]; then + log_warning "Invalid path in file list for $app_name: $line" + echo "" + return 1 + fi + done <<< "$decoded" + + echo "$decoded" + return 0 +} # Note: find_app_files() and calculate_total_size() functions now in lib/common.sh # Batch uninstall with single confirmation @@ -64,7 +98,7 @@ batch_uninstall_applications() { echo "" for detail in "${app_details[@]}"; do IFS='|' read -r app_name app_path bundle_id total_kb encoded_files <<< "$detail" - local related_files=$(printf '%s' "$encoded_files" | base64 -d) + local related_files=$(decode_file_list "$encoded_files" "$app_name") local app_size_display=$(bytes_to_human "$((total_kb * 1024))") echo -e "${BLUE}${ICON_CONFIRM}${NC} ${app_name} ${GRAY}(${app_size_display})${NC}" @@ -129,10 +163,15 @@ batch_uninstall_applications() { return 1 fi fi + # Start sudo keepalive with robust parent checking + parent_pid=$$ (while true; do + # Check if parent process still exists first + if ! kill -0 "$parent_pid" 2> /dev/null; then + exit 0 + fi sudo -n true sleep 60 - kill -0 "$$" || exit done 2> /dev/null) & sudo_keepalive_pid=$! fi @@ -149,7 +188,7 @@ batch_uninstall_applications() { local -a success_items=() for detail in "${app_details[@]}"; do IFS='|' read -r app_name app_path bundle_id total_kb encoded_files <<< "$detail" - local related_files=$(printf '%s' "$encoded_files" | base64 -d) + local related_files=$(decode_file_list "$encoded_files" "$app_name") local reason="" local needs_sudo=false [[ ! -w "$(dirname "$app_path")" || "$(stat -f%Su "$app_path" 2> /dev/null)" == "root" ]] && needs_sudo=true diff --git a/lib/whitelist_manager.sh b/lib/whitelist_manager.sh index 15652b0..d8227b7 100755 --- a/lib/whitelist_manager.sh +++ b/lib/whitelist_manager.sh @@ -184,9 +184,8 @@ is_whitelisted() { for existing in "${CURRENT_WHITELIST_PATTERNS[@]}"; do local existing_expanded="${existing/#\~/$HOME}" - # Support both exact match and glob pattern match - # shellcheck disable=SC2053 - if [[ "$check_pattern" == "$existing_expanded" ]] || [[ $check_pattern == $existing_expanded ]]; then + # Only use exact string match to prevent glob expansion security issues + if [[ "$check_pattern" == "$existing_expanded" ]]; then return 0 fi done