Skip to content

Security hardening: 11 vulnerability fixes#1

Open
khalidelmerrah wants to merge 1 commit into
ilukezippo:mainfrom
khalidelmerrah:security-fixes
Open

Security hardening: 11 vulnerability fixes#1
khalidelmerrah wants to merge 1 commit into
ilukezippo:mainfrom
khalidelmerrah:security-fixes

Conversation

@khalidelmerrah

Copy link
Copy Markdown

Summary

Security audit found several vulnerabilities in the current codebase. This PR fixes all of them without changing any features, UI, or adding dependencies.

Critical fixes

  • Batch script injection - Self-update embeds paths in a .bat file via f-string. Paths containing ", %, ^, &, | could inject commands. Added _sanitize_batch_path() to strip dangerous characters
  • Symlink attack in temp cleanup - onerr callback in clear_temp_async calls os.chmod() which follows symlinks. An attacker could plant a symlink in %TEMP% to modify system files. Added os.path.islink() check before chmod
  • Download URL validation - Self-update accepts any URL from GitHub API response. Added HTTPS + github.com domain validation. Asset filenames sanitized with os.path.basename() to prevent path traversal

High fixes

  • DLL hijacking - _check_vc_runtime() loads DLLs by bare name, which searches CWD first. Changed to absolute %WINDIR%\System32 paths
  • UAC parameter injection - relaunch_as_admin() joins argv with manual quoting that can be bypassed. Replaced with subprocess.list2cmdline() for proper Windows escaping

Medium fixes

  • Dead code - Removed unused _download_file_verified static method
  • Log memory cap - Log widget now capped at 5,000 lines (was unbounded)
  • Symlink-safe snapshots - _snapshot_temp() now uses os.lstat() instead of os.stat()
  • Thread safety - Added threading.Lock() for shared state between UI and worker threads

UI fix

  • Window maximize - Removed maxsize constraint so the window can be maximized
  • Fixed SyntaxWarning for unescaped \W in docstring

Test plan

  • App launches without errors
  • Check for Updates works
  • Update Selected works
  • Clear Temp works
  • Self-update check works
  • Window can be maximized
  • Run as Admin works

No new dependencies. No UI changes beyond allowing maximize. Drop-in safe.

Generated with Claude Code

Critical:
- Sanitize paths in self-update batch script to prevent command injection
- Add symlink check in temp cleanup to prevent chmod on symlink targets
- Validate download URLs are HTTPS from GitHub only, sanitize asset filenames

High:
- Use absolute System32 path in _check_vc_runtime to prevent DLL hijacking
- Use subprocess.list2cmdline in relaunch_as_admin to prevent parameter injection

Medium:
- Remove dead _download_file_verified code
- Cap log widget at 5000 lines to prevent memory exhaustion
- Use os.lstat instead of os.stat in _snapshot_temp to avoid symlink following
- Add threading.Lock for shared state between UI and worker threads

UI:
- Remove maxsize constraint to allow window maximizing
- Fix SyntaxWarning in docstring

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant