[PPSC-602] CWE-22: Path traversal in install.ps1#131
Open
Conversation
Test Coverage Reporttotal: (statements) 81.0% Coverage by function |
|
| Severity | Count |
|---|---|
| 🟠 HIGH | 1 |
Total: 1
View all 1 findings
🟠 HIGH (1)
CWE-22_armis-cli_38295677_scripts/install.ps1_262_9_262_73 - Broken Access Control (CWE-22
Location: scripts/install.ps1:262
Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')): The script downloads a ZIP archive from a URL that can be influenced by the -Version parameter supplied by the user. After optional checksum verification, the archive is extracted with Expand-Archive (line 262) directly into a temporary directory without inspecting the archive’s internal file paths. Because no validation of the entries inside the archive is performed, a malicious archive containing relative path components (e.g., "....") could cause files to be written outside the intended temporary folder, leading to arbitrary file writes on the host. The user‑controlled Version argument provides a taint source that reaches the Expand-Archive sink, and the checksum step does not mitigate path‑traversal concerns. This makes the vulnerability exploitable in the local execution context of the installer script.
CWEs: CWE-22: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
3 tasks
…-22) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove Program Files from allowed prefixes to prevent DLL planting - Normalize prefixes with trailing backslash to avoid prefix confusion - Detect non-interactive mode and fail fast instead of hanging on Read-Host - Add cross-reference comments between install.sh and install.ps1 - Fix shellcheck warnings (redundant patterns, declare/assign separation)
yiftach-armis
force-pushed
the
fix/PPSC-602-cwe22-path-traversal-install-ps1
branch
from
e1c6fe2 to
60de804
Compare
…nd injection - CWE-269: Refuse sudo escalation when INSTALL_DIR is set via env var to a non-writable directory. Only the auto-detected fallback may use sudo. - CWE-78: Remove pre-existing binary execution for version detection in both install.sh and install.ps1. An attacker could plant a malicious binary at the install path before the installer runs.
…-install execution - CWE-426: Only add install directory to PATH for allowlisted locations (LOCALAPPDATA, APPDATA, USERPROFILE). Non-standard paths confirmed interactively are not added to PATH automatically, preventing an attacker-influenced directory from polluting the search path. - CWE-78: Only execute the installed binary for version display when integrity verification was performed (checksums validated). Without verification, the binary content cannot be trusted.
There was a problem hiding this comment.
Pull request overview
Hardens the platform install scripts to mitigate the reported CWE-22/path traversal risk (GitHub security alert #578) by tightening install-directory validation on Windows and adding additional defense-in-depth changes around installer safety.
Changes:
install.ps1: allowlist install locations to standard user-profile directories with an interactive confirmation gate for non-standard paths; restrict PATH modifications to allowlisted locations.install.sh: tighten install-dir validation, avoid executing an existing binary during upgrade detection, and refuse sudo escalation whenINSTALL_DIRis user-supplied via env var.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| scripts/install.sh | Adds/adjusts install-dir validation and mitigations around executing existing binaries and sudo escalation. |
| scripts/install.ps1 | Adds user-profile allowlist + confirmation for non-standard install dirs and limits PATH modification to allowlisted locations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [ -f "$TARGET_PATH" ] || fail "Install appeared to succeed, but $TARGET_PATH does not exist" | ||
|
|
||
| # Get installed version | ||
| INSTALLED_VERSION=$("$TARGET_PATH" --version 2>/dev/null | head -n1 || echo "unknown") |
There was a problem hiding this comment.
main still executes the newly installed binary to obtain INSTALLED_VERSION even when integrity verification is explicitly skipped (VERIFY=false). Since this PR is hardening against unsafe binary execution, consider skipping version execution unless verification succeeded (or printing a static/unknown version) to avoid running an unverified download.
Suggested change
| INSTALLED_VERSION=$("$TARGET_PATH" --version 2>/dev/null | head -n1 || echo "unknown") | |
| if [ "$VERIFY" != "false" ]; then | |
| INSTALLED_VERSION=$("$TARGET_PATH" --version 2>/dev/null | head -n1 || echo "unknown") | |
| else | |
| INSTALLED_VERSION="unknown (verification skipped; binary not executed)" | |
| fi |
Comment on lines
+188
to
+189
| $normalizedPrefix = $prefix.TrimEnd('\') + '\' | ||
| if ($InstallDir.StartsWith($normalizedPrefix, [System.StringComparison]::OrdinalIgnoreCase)) { |
There was a problem hiding this comment.
Allowlist check won’t treat an install dir that is exactly equal to an allowed prefix (e.g., -InstallDir $env:LOCALAPPDATA or $env:USERPROFILE) as allowlisted because StartsWith($prefix + '\\') requires a trailing separator. Consider allowing equality as well (compare trimmed paths) so standard root locations don’t incorrectly trigger the confirmation/fail-fast path.
Suggested change
| $normalizedPrefix = $prefix.TrimEnd('\') + '\' | |
| if ($InstallDir.StartsWith($normalizedPrefix, [System.StringComparison]::OrdinalIgnoreCase)) { | |
| $normalizedPrefixRoot = $prefix.TrimEnd('\') | |
| $normalizedPrefix = $normalizedPrefixRoot + '\' | |
| if ( | |
| [string]::Equals( | |
| $InstallDir.TrimEnd('\'), | |
| $normalizedPrefixRoot, | |
| [System.StringComparison]::OrdinalIgnoreCase | |
| ) -or | |
| $InstallDir.StartsWith($normalizedPrefix, [System.StringComparison]::OrdinalIgnoreCase) | |
| ) { |
Comment on lines
+174
to
+179
| # Validate install directory is under standard user-profile locations. | ||
| # Program Files is intentionally excluded — it requires admin privileges and | ||
| # writing there could enable DLL/EXE planting. Non-standard paths are allowed | ||
| # only with explicit interactive confirmation. | ||
| # Note: install.sh uses a different model (character-set + traversal deny-list) | ||
| # because Unix paths don't have canonical env-var-based user directories. |
There was a problem hiding this comment.
The header usage comment still shows an example installing under C:\Program Files\..., but this block now explicitly discourages/excludes Program Files and may require confirmation (or fail in non-interactive mode). Please update the usage comment/help text to reflect the new recommended install locations/behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
go build ./...)Generated with Claude Code