From 3919a7030070ad0156acd6beb1642899d483f460 Mon Sep 17 00:00:00 2001 From: tw93 Date: Mon, 26 Jan 2026 20:27:46 +0800 Subject: [PATCH] fix: enhance uninstall security per audit review - Validate bundle_id format (reverse-DNS) in stop_launch_services() to prevent glob injection attacks - Add common word exclusion list for LaunchAgents name search to avoid false positive matches (Music, Notes, Photos, etc.) - Add security comments explaining symlink handling in remove_file_list() - Improve brew_uninstall_cask() timeout handling: exit code 124 now returns failure immediately - Update SECURITY_AUDIT.md with remediation details --- SECURITY_AUDIT.md | 7 +++++++ lib/core/app_protection.sh | 23 ++++++++++++++++------- lib/uninstall/batch.sh | 15 +++++++++++++++ lib/uninstall/brew.sh | 9 ++++++++- 4 files changed, 46 insertions(+), 8 deletions(-) diff --git a/SECURITY_AUDIT.md b/SECURITY_AUDIT.md index 52551e5..c28fd77 100644 --- a/SECURITY_AUDIT.md +++ b/SECURITY_AUDIT.md @@ -33,6 +33,11 @@ **Recent Remediations:** +- **Uninstall Audit (Jan 2026)**: Enhanced security in uninstall logic per comprehensive security review. + - `stop_launch_services()` now validates bundle_id format (reverse-DNS) before use in find patterns to prevent glob injection attacks. + - `find_app_files()` LaunchAgents search now excludes common words (Music, Notes, etc.) to prevent false positive matches. + - `remove_file_list()` symlink handling documented with detailed security comments explaining the TOCTOU protection bypass rationale. + - `brew_uninstall_cask()` timeout handling improved: exit code 124 (timeout) now returns failure immediately without verification. - Symlink cleanup in `bin/clean.sh` now routes through `safe_remove` for target validation. - Orphaned helper cleanup in `lib/clean/apps.sh` now uses `safe_sudo_remove`. - ByHost preference cleanup in `lib/uninstall/batch.sh` validates bundle IDs and deletes via `safe_remove`. @@ -199,6 +204,8 @@ When users uninstall applications via `mo uninstall`, Mole automatically removes - Unloads services via `launchctl` before deletion (via `stop_launch_services()`) - **Safer than orphan detection:** Only removes plists when the associated app is explicitly being uninstalled - Prevents accumulation of orphaned startup items that persist after app removal +- **Common word exclusion:** LaunchAgent name searches exclude generic terms (Music, Notes, Photos, etc.) to prevent false positives +- **Bundle ID validation:** `stop_launch_services()` validates reverse-DNS format before find patterns **Code:** `lib/core/app_protection.sh:find_app_files()`, `lib/uninstall/batch.sh:stop_launch_services()` diff --git a/lib/core/app_protection.sh b/lib/core/app_protection.sh index 1b754b8..446a478 100755 --- a/lib/core/app_protection.sh +++ b/lib/core/app_protection.sh @@ -768,14 +768,23 @@ find_app_files() { # Note: LaunchDaemons are system-level and handled in find_app_system_files() # Minimum 5-char threshold prevents false positives (e.g., "Time" matching system agents) # Short-name apps (e.g., Zoom, Arc) are still cleaned via bundle_id matching above + # Security: Common words are excluded to prevent matching unrelated plist files if [[ ${#app_name} -ge 5 ]] && [[ -d ~/Library/LaunchAgents ]]; then - while IFS= read -r -d '' plist; do - local plist_name=$(basename "$plist") - if [[ "$plist_name" =~ ^com\.apple\. ]]; then - continue - fi - files_to_clean+=("$plist") - done < <(command find ~/Library/LaunchAgents -maxdepth 1 -name "*$app_name*.plist" -print0 2> /dev/null) + # Skip common words that could match many unrelated LaunchAgents + # These are either generic terms or names that overlap with system/common utilities + local common_words="Music|Notes|Photos|Finder|Safari|Preview|Calendar|Contacts|Messages|Reminders|Clock|Weather|Stocks|Books|News|Podcasts|Voice|Files|Store|System|Helper|Agent|Daemon|Service|Update|Sync|Backup|Cloud|Manager|Monitor|Server|Client|Worker|Runner|Launcher|Driver|Plugin|Extension|Widget|Utility" + if [[ "$app_name" =~ ^($common_words)$ ]]; then + debug_log "Skipping LaunchAgent name search for common word: $app_name" + else + while IFS= read -r -d '' plist; do + local plist_name=$(basename "$plist") + # Skip Apple's LaunchAgents + if [[ "$plist_name" =~ ^com\.apple\. ]]; then + continue + fi + files_to_clean+=("$plist") + done < <(command find ~/Library/LaunchAgents -maxdepth 1 -name "*$app_name*.plist" -print0 2> /dev/null) + fi fi # Handle specialized toolchains and development environments diff --git a/lib/uninstall/batch.sh b/lib/uninstall/batch.sh index 9c405ef..5f11973 100755 --- a/lib/uninstall/batch.sh +++ b/lib/uninstall/batch.sh @@ -72,12 +72,20 @@ decode_file_list() { # Note: find_app_files() and calculate_total_size() are in lib/core/common.sh. # Stop Launch Agents/Daemons for an app. +# Security: bundle_id is validated to be reverse-DNS format before use in find patterns stop_launch_services() { local bundle_id="$1" local has_system_files="${2:-false}" [[ -z "$bundle_id" || "$bundle_id" == "unknown" ]] && return 0 + # Validate bundle_id format: must be reverse-DNS style (e.g., com.example.app) + # This prevents glob injection attacks if bundle_id contains special characters + if [[ ! "$bundle_id" =~ ^[a-zA-Z0-9][-a-zA-Z0-9]*(\.[a-zA-Z0-9][-a-zA-Z0-9]*)+$ ]]; then + debug_log "Invalid bundle_id format for LaunchAgent search: $bundle_id" + return 0 + fi + if [[ -d ~/Library/LaunchAgents ]]; then while IFS= read -r -d '' plist; do launchctl unload "$plist" 2> /dev/null || true @@ -135,6 +143,7 @@ remove_login_item() { } # Remove files (handles symlinks, optional sudo). +# Security: All paths pass validate_path_for_deletion() before any deletion. remove_file_list() { local file_list="$1" local use_sudo="${2:-false}" @@ -147,6 +156,12 @@ remove_file_list() { continue fi + # Symlinks are handled separately using rm (not safe_remove/safe_sudo_remove) + # because safe_sudo_remove() refuses symlinks entirely as a TOCTOU protection. + # This is safe because: + # 1. The path has already passed validate_path_for_deletion() above + # 2. rm on a symlink only removes the link itself, NOT the target + # 3. The symlink deletion is logged via operations.log if [[ -L "$file" ]]; then if [[ "$use_sudo" == "true" ]]; then sudo rm "$file" 2> /dev/null && ((++count)) || true diff --git a/lib/uninstall/brew.sh b/lib/uninstall/brew.sh index 5e2f13f..adba229 100644 --- a/lib/uninstall/brew.sh +++ b/lib/uninstall/brew.sh @@ -196,15 +196,22 @@ brew_uninstall_cask() { fi # Run with timeout to prevent hangs from problematic cask scripts + local brew_exit=0 if HOMEBREW_NO_ENV_HINTS=1 HOMEBREW_NO_AUTO_UPDATE=1 NONINTERACTIVE=1 \ run_with_timeout "$timeout" brew uninstall --cask "$cask_name" 2>&1; then uninstall_ok=true else brew_exit=$? debug_log "brew uninstall timeout or failed with exit code: $brew_exit" + # Exit code 124 indicates timeout from run_with_timeout + # On timeout, fail immediately without verification to avoid inconsistent state + if [[ $brew_exit -eq 124 ]]; then + debug_log "brew uninstall timed out after ${timeout}s, returning failure" + return 1 + fi fi - # Verify removal + # Verify removal (only if not timed out) local cask_gone=true app_gone=true HOMEBREW_NO_ENV_HINTS=1 brew list --cask 2> /dev/null | grep -qxF "$cask_name" && cask_gone=false [[ -n "$app_path" && -e "$app_path" ]] && app_gone=false