refactor: sanitize shell arguments in WireGuard update script#2197
refactor: sanitize shell arguments in WireGuard update script#2197elibosley wants to merge 3 commits into
Conversation
Updated the update.wireguard.php script to use escapeshellarg for all shell command arguments, enhancing security by preventing command injection vulnerabilities. This change affects various functions including isPort, carrier, thisNet, and others that execute system commands.
|
""" WalkthroughThe update applies Changes
Possibly related PRs
Suggested labels
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (18)
emhttp/plugins/dynamix.docker.manager/include/DockerContainers.php(2 hunks)emhttp/plugins/dynamix.docker.manager/include/Helpers.php(1 hunks)emhttp/plugins/dynamix.plugin.manager/include/UnraidUpdateCancel.php(1 hunks)emhttp/plugins/dynamix.plugin.manager/scripts/language(4 hunks)emhttp/plugins/dynamix.plugin.manager/scripts/multiplugin(1 hunks)emhttp/plugins/dynamix.plugin.manager/scripts/plugin(2 hunks)emhttp/plugins/dynamix/include/FileSystemStatus.php(1 hunks)emhttp/plugins/dynamix/include/Helpers.php(1 hunks)emhttp/plugins/dynamix/include/OpenTerminal.php(2 hunks)emhttp/plugins/dynamix/include/ProcessStatus.php(1 hunks)emhttp/plugins/dynamix/include/SMTPtest.php(1 hunks)emhttp/plugins/dynamix/include/SmartInfo.php(1 hunks)emhttp/plugins/dynamix/include/StartCommand.php(1 hunks)emhttp/plugins/dynamix/include/SysDevs.php(7 hunks)emhttp/plugins/dynamix/include/update.wireguard.php(15 hunks)emhttp/plugins/dynamix/nchan/file_manager(3 hunks)emhttp/plugins/dynamix/scripts/diagnostics(1 hunks)emhttp/plugins/dynamix/scripts/netconfig(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- emhttp/plugins/dynamix/include/Helpers.php
- emhttp/plugins/dynamix/scripts/diagnostics
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix/include/update.wireguard.php
🧰 Additional context used
🧬 Code Graph Analysis (2)
emhttp/plugins/dynamix.docker.manager/include/Helpers.php (1)
emhttp/plugins/dynamix/include/OpenTerminal.php (1)
command(40-43)
emhttp/plugins/dynamix/include/OpenTerminal.php (2)
emhttp/plugins/dynamix/include/Secure.php (1)
unbundle(23-25)emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (1)
docker(1135-1138)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (44)
emhttp/plugins/dynamix/scripts/netconfig (1)
44-49: Security improvement through consistent escaping of shell argumentsThe updates to the
update_wireguardfunction now applyescapeshellarg()to all variables used in shell commands. This properly sanitizes inputs forgrep,sed, andwg-quickcommands, preventing potential command injection vulnerabilities.emhttp/plugins/dynamix/include/SMTPtest.php (2)
33-36: Enhanced security by proper type casting and escapingGood security improvement by explicitly casting
$pidto an integer and then escaping it withescapeshellarg(). This dual-layer approach ensures the value is both numerically valid and properly escaped when used in thepscommand.
39-41: Enhanced security by proper type casting and escapingSimilar to the previous improvement, this code now properly casts the process ID to an integer and escapes it with
escapeshellarg()before using it in thekillcommand, preventing command injection.emhttp/plugins/dynamix.docker.manager/include/Helpers.php (1)
630-631: Security enhancement using escapeshellcmdGood improvement by applying
escapeshellcmd()to the entire command string before passing it topopen(). This protects against shell injection vulnerabilities by escaping potentially dangerous characters in the command.emhttp/plugins/dynamix.plugin.manager/scripts/multiplugin (1)
44-44: Good security improvement: Shell command arguments properly escapedThe added
escapeshellarg()function properly sanitizes both the$methodand$pluginArgvariables before they're passed to the shell viapopen(). This prevents potential command injection vulnerabilities where malicious input could execute arbitrary commands.emhttp/plugins/dynamix.plugin.manager/scripts/language (6)
184-184: Good security improvement: Directory path properly escapedThe
escapeshellarg()function properly sanitizes the$pathvariable before it's passed tomkdir -pviaexec(). This prevents potential directory traversal or command injection attacks through malicious path input.
187-187: Good security improvement: Unzip command arguments properly escapedBoth the destination path (
$path) and archive file ($save) are now properly escaped withescapeshellarg()before being passed to theunzipcommand. This prevents command injection vulnerabilities.
190-190: Good security improvement: rm -rf command path properly escapedThe
escapeshellarg()function sanitizes the$pathvariable before it's passed to the dangerousrm -rfcommand. This is especially critical since improper escaping could lead to deletion of unintended files.
199-199: Good security improvement: rm -rf command path properly escapedSimilar to the previous instance, the
$pathvariable is properly escaped before being passed torm -rf, preventing potential command injection vulnerabilities during language removal.
246-246: Good security improvement: Language name properly escapedThe language name (
$name) is now properly escaped withescapeshellarg()when recursively executing the script. This prevents command injection vulnerabilities when checking language packs.
268-268: Good security improvement: Language name properly escapedThe language name (
$name) is now properly escaped withescapeshellarg()when recursively executing the update command. This prevents command injection vulnerabilities when updating language packs.emhttp/plugins/dynamix/include/StartCommand.php (1)
22-22: Good security improvement: User input properly escapedThe
$_POST['kill']variable (coming directly from user input) is now properly escaped withescapeshellarg()before being passed to thekillcommand. This is critical since user input from web requests is a common attack vector for command injection vulnerabilities.emhttp/plugins/dynamix.docker.manager/include/DockerContainers.php (3)
56-56: Good security improvement: Container name properly escapedThe Docker container name (
$name) is now properly escaped withescapeshellarg()before being passed todocker exec. This prevents command injection vulnerabilities when executing commands in Docker containers.
71-71: Good security improvement: File path and URL properly escapedBoth the output file path (
$file) and URL ($url) are now properly escaped withescapeshellarg()before being passed to thewgetcommand. This prevents command injection vulnerabilities during file downloads.
76-76: Good security improvement: File path and URL properly escapedConsistent with the previous instance, both the output file path (
$file) and URL ($url) are properly escaped withescapeshellarg()when refreshing the downloaded file. This maintains security throughout the code path.emhttp/plugins/dynamix/include/ProcessStatus.php (1)
26-27: Excellent security improvement for device input handlingThe changes properly sanitize the device input by escaping it before use in shell commands. This prevents command injection vulnerabilities where an attacker could potentially insert malicious shell commands through the
$_POST['device']parameter.emhttp/plugins/dynamix/include/SmartInfo.php (2)
247-249: Good security improvement for file path sanitizationThis change properly sanitizes user-supplied filenames by:
- Removing potentially dangerous characters with
preg_replace- Using
basename()to prevent directory traversal attacks- Using
escapeshellarg()to properly escape the filename for shell command usageThis effectively prevents both directory traversal and command injection vulnerabilities.
253-256: Proper path validation for deletion operationThe code now sanitizes the filename before deletion and adds an additional safety check using
realpath()to verify the file is within the expected/var/tmp/directory. This prevents directory traversal attacks that could lead to unauthorized file deletion.emhttp/plugins/dynamix/include/FileSystemStatus.php (2)
24-26: Secure handling of path variables in shell commandsThe code now properly escapes the
$pathvariable in both thepsandzpool statuscommands, preventing potential command injection vulnerabilities.
31-32: Properly escaped path in btrfs commandThe
$pathvariable is now correctly escaped usingescapeshellarg()when passed to the btrfs command, which prevents command injection vulnerabilities.emhttp/plugins/dynamix/nchan/file_manager (4)
141-142: Secure handling of wildcard in find commandThe wildcard
*is now properly escaped usingescapeshellarg(), preventing potential shell injection vulnerabilities in the find command.
200-201: Secure parameter handling in chown commandThe
$targetparameter is now properly escaped withescapeshellarg()before being used in the chown command, preventing command injection vulnerabilities.
207-208: Secure parameter handling in chmod commandThe
$targetparameter is now properly escaped withescapeshellarg()before being used in the chmod command, preventing command injection vulnerabilities.
218-219: Secure PID handling in kill commandThe
$pidvariable is now properly escaped withescapeshellarg()before being used in the kill command, preventing potential command injection vulnerabilities.emhttp/plugins/dynamix.plugin.manager/scripts/plugin (5)
242-242: Improve security by escaping shell arguments in download functionThe addition of
escapeshellarg()to sanitize the$nameand$urlparameters in the wget command properly protects against command injection vulnerabilities.
463-464: Properly escape plugin run command and attributesThe use of
escapeshellcmd()for the Run attribute protects against potential command injection attacks when executing plugin commands.
466-467: Properly escape file name in command executionEscaping the
$nameparameter before passing it to the command prevents potential command injection vulnerabilities.
469-470: Properly escape LOCAL file path in command executionEscaping the
$file->LOCALparameter when running commands ensures that file paths containing special characters don't lead to command injection.
474-475: Properly escape temporary file path in command executionEscaping the
$nameparameter for inline content prevents command injection when executing the generated script.emhttp/plugins/dynamix/include/SysDevs.php (7)
24-28: Secure USB device identification by escaping bus and device valuesThe modification properly escapes USB bus and device numbers before using them in shell commands, preventing command injection vulnerabilities.
81-83: Enhance security for device path handlingProperly escaping the device name before using it in the udevadm command prevents potential command injection attacks.
101-103: Securely handle network interface namesEscaping network interface names before passing them to the readlink command prevents command injection through maliciously crafted interface names.
119-121: Secure IOMMU device path handlingEscaping the PCI device path before using it in the ls command prevents command injection vulnerabilities.
152-154: Securely process PCI device informationProperly escaping PCI addresses before passing them to lspci protects against command injection.
194-196: Enhance security for USB controller enumerationEscaping the PCI address when searching for USB controllers prevents command injection vulnerabilities in the complex shell command.
215-217: Secure storage device enumerationEscaping the PCI address when listing storage devices prevents command injection in the grep command.
emhttp/plugins/dynamix/include/OpenTerminal.php (8)
34-39: Strengthen security with dual protection for command execution scriptsThis change applies two layers of security:
- Input sanitization with regex to limit characters to alphanumeric, dash, underscore, and dot
- Shell argument escaping for the final script path
This effectively prevents both path traversal and command injection vulnerabilities.
42-43: Secure file path handling in tail commandProperly escaping the file path before using it in the tail command prevents command injection vulnerabilities.
51-56: Enhance security for process handlingEscaping the process ID, socket path, and shell path when managing ttyd processes prevents command injection vulnerabilities.
62-63: Secure socket and command executionEscaping the socket path when executing the ttyd command prevents command injection attacks.
68-71: Enhance security for disk log accessProperly escaping file paths, device names, and search patterns in the grep command prevents command injection vulnerabilities.
78-79: Secure log file accessEscaping the socket path and command output ensures safer terminal execution for log files.
86-95: Comprehensive security for Docker container interactionAll Docker-related commands now properly escape container names and command parameters, preventing command injection vulnerabilities when interacting with containers.
101-102: Secure LXC container accessEscaping the container name and command parameters when attaching to LXC containers prevents command injection vulnerabilities.
…el.php Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Updated the update.wireguard.php script to use escapeshellarg for all shell command arguments, enhancing security by preventing command injection vulnerabilities. This change affects various functions including isPort, carrier, thisNet, and others that execute system commands.
Summary by CodeRabbit