From 681125a055653c6a39fde31b06a5f6087c09fc42 Mon Sep 17 00:00:00 2001 From: twangboy Date: Thu, 7 May 2026 15:15:48 -0600 Subject: [PATCH 1/5] Add input parameter validation for Source and MinionVersion Windows: Validate the Source and MinionVersion parameters early in the script (after the test-mode guard, before Main) so malformed or potentially malicious values are rejected with exit code 126 before any network or filesystem activity occurs. - Test-SourceParameter: rejects whitespace, control characters, and shell-injection characters; validates URL structure via System.Uri for HTTP/HTTPS/FTP; enforces UNC and local path format - Test-MinionVersionParameter: validates against the known Salt CalVer format (latest, YYYY, YYYY.N, YYYY.N.N, YYYY.NrcN) - _parse_config: skips any key or value containing a control character to prevent YAML injection via ConfigOptions - Functional tests cover valid and invalid cases for both validators and the control-character config guard; integration tests verify exit code 126 end-to-end via subprocess invocation --- .../functional/test_input_validation.ps1 | 62 ++++++++++ .../windows/functional/test_parse_config.ps1 | 14 +++ .../integration/test_input_validation.ps1 | 35 ++++++ windows/svtminion.ps1 | 109 ++++++++++++++++++ 4 files changed, 220 insertions(+) create mode 100644 tests/windows/functional/test_input_validation.ps1 create mode 100644 tests/windows/integration/test_input_validation.ps1 diff --git a/tests/windows/functional/test_input_validation.ps1 b/tests/windows/functional/test_input_validation.ps1 new file mode 100644 index 0000000..5425339 --- /dev/null +++ b/tests/windows/functional/test_input_validation.ps1 @@ -0,0 +1,62 @@ +function test_Test-SourceParameter { + $cases = @( + @{ Source = "https://packages.broadcom.com/artifactory/salt/onedir"; Expected = $true } + @{ Source = "http://my.server.com/salt"; Expected = $true } + @{ Source = "ftp://ftp.example.com/salt"; Expected = $true } + @{ Source = "\\fileserver\salt"; Expected = $true } + @{ Source = "\\file-server\salt packages"; Expected = $true } + @{ Source = "C:\salt\repo"; Expected = $true } + @{ Source = "C:\My Salt Repo"; Expected = $true } + @{ Source = "https://evil.com/path with spaces"; Expected = $false } + @{ Source = 'https://evil.com/path`bad'; Expected = $false } + @{ Source = "https://evil.com/path;bad"; Expected = $false } + @{ Source = "https://evil.com/path&bad"; Expected = $false } + @{ Source = "https://evil.com/path|bad"; Expected = $false } + @{ Source = "https://evil.com/path $null + return $LASTEXITCODE +} + +function test_invalid_source_spaces_exits_126 { + $exit_code = invoke_script @("-Install", "-Source", "https://bad url.com") + if ($exit_code -ne 126) { return 1 } + return 0 +} + +function test_invalid_source_semicolon_exits_126 { + $exit_code = invoke_script @("-Install", "-Source", "https://evil.com/path;bad") + if ($exit_code -ne 126) { return 1 } + return 0 +} + +function test_invalid_source_bad_scheme_exits_126 { + $exit_code = invoke_script @("-Install", "-Source", "notascheme://bad") + if ($exit_code -ne 126) { return 1 } + return 0 +} + +function test_invalid_minionversion_exits_126 { + $exit_code = invoke_script @("-Install", "-MinionVersion", "bad version") + if ($exit_code -ne 126) { return 1 } + return 0 +} + +function test_invalid_minionversion_injection_exits_126 { + $exit_code = invoke_script @("-Install", "-MinionVersion", "3006;evil") + if ($exit_code -ne 126) { return 1 } + return 0 +} diff --git a/windows/svtminion.ps1 b/windows/svtminion.ps1 index 756a0d7..62f34fc 100644 --- a/windows/svtminion.ps1 +++ b/windows/svtminion.ps1 @@ -1131,6 +1131,10 @@ function _parse_config { if ($key_value -like "*=*") { Write-Log "Found config: $key_value" -Level debug $key, $value = $key_value -split "=" + if ($key -match '[\x00-\x1f\x7f]' -or $value -match '[\x00-\x1f\x7f]') { + Write-Log "Config option with control characters ignored: $key_value" -Level warning + continue + } if ($value) { $config_options[$key] = $value } else { @@ -1665,6 +1669,104 @@ function Get-RandomizedMinionId { } +function Test-SourceParameter { + # Validates the Source parameter. Returns $true if valid, $false otherwise. + [CmdletBinding()] + param( + [Parameter(Mandatory=$true)] + [AllowEmptyString()] + [String] $Source + ) + + if ([String]::IsNullOrWhiteSpace($Source)) { + $msg = "Invalid Source: must not be empty" + Write-Log $msg -Level error + Write-Host $msg -ForegroundColor Red + return $false + } + + if ($Source -match '[\x00-\x1f\x7f]') { + $msg = "Invalid Source: contains control characters" + Write-Log $msg -Level error + Write-Host $msg -ForegroundColor Red + return $false + } + + if ($Source -match '[`;&|<>]') { + $msg = "Invalid Source: contains disallowed characters" + Write-Log $msg -Level error + Write-Host $msg -ForegroundColor Red + return $false + } + + if ($Source -match '^(https?|ftp)://') { + if ($Source -match '\s') { + $msg = "Invalid Source: URL must not contain whitespace" + Write-Log $msg -Level error + Write-Host $msg -ForegroundColor Red + return $false + } + $uri = $null + if (-not [System.Uri]::TryCreate( + $Source, [System.UriKind]::Absolute, [ref]$uri)) { + $msg = "Invalid Source: not a valid URL" + Write-Log $msg -Level error + Write-Host $msg -ForegroundColor Red + return $false + } + if ([String]::IsNullOrEmpty($uri.Host)) { + $msg = "Invalid Source: URL has no host" + Write-Log $msg -Level error + Write-Host $msg -ForegroundColor Red + return $false + } + } elseif ($Source -match '^\\\\') { + if ($Source -notmatch '^\\\\[^\\/:*?"<>|]+\\[^\\/:*?"<>|]+') { + $msg = "Invalid Source: invalid UNC path" + Write-Log $msg -Level error + Write-Host $msg -ForegroundColor Red + return $false + } + } elseif ($Source -match '^[A-Za-z]:\\') { + # Local path - format already confirmed by prefix match + } else { + $msg = "Invalid Source: must start with http://, https://, " + + "ftp://, \\ (UNC), or a drive letter" + Write-Log $msg -Level error + Write-Host $msg -ForegroundColor Red + return $false + } + + return $true +} + + +function Test-MinionVersionParameter { + # Validates the MinionVersion parameter. Returns $true if valid, $false + # otherwise. + [CmdletBinding()] + param( + [Parameter(Mandatory=$true)] + [AllowEmptyString()] + [String] $MinionVersion + ) + + # Valid: "latest", 4-digit major (3006), major.minor (3006.2), + # multi-part (3006.24), rc suffix (3008.0rc1) + if ($MinionVersion -notmatch '^(latest|\d{4}(\.\d+(\.\d+)*(rc\d+)?)?)$') { + $msg = "Invalid MinionVersion: $MinionVersion" + Write-Log $msg -Level error + Write-Host $msg -ForegroundColor Red + $msg = "Must be 'latest', a major version (e.g. 3006), " + + "or a full version (e.g. 3006.2, 3008.0rc1)" + Write-Host $msg -ForegroundColor Yellow + return $false + } + + return $true +} + + ############################### MAIN FUNCTIONS ################################# @@ -2644,6 +2746,13 @@ if (($Action) -and ($Action.ToLower() -eq "test")) { exit $STATUS_CODES["scriptSuccess"] } +if (!(Test-SourceParameter -Source $Source)) { + exit $STATUS_CODES["scriptFailed"] +} +if (!(Test-MinionVersionParameter -MinionVersion $MinionVersion)) { + exit $STATUS_CODES["scriptFailed"] +} + try { $exit_code = Main exit $exit_code From 6e222249664c8f26beeb84f235f15552662fbd85 Mon Sep 17 00:00:00 2001 From: twangboy Date: Thu, 7 May 2026 15:33:04 -0600 Subject: [PATCH 2/5] Add input parameter validation for --source and --minionversion on Linux Adds _validate_source_param and _validate_minion_version_param helper functions to linux/svtminion.sh, mirroring the Windows validation added in 681125a. Both functions exit 126 via _error_log on invalid input, consistent with all other error handling in the script. Adds 5 integration test cases to tests/linux/test-linux.sh covering URL spaces, unknown schemes, injection characters, and malformed version strings. --- linux/svtminion.sh | 84 +++++++++++++++++++++++++++++++++++++++ tests/linux/test-linux.sh | 17 ++++++++ 2 files changed, 101 insertions(+) diff --git a/linux/svtminion.sh b/linux/svtminion.sh index 48d8755..00b2524 100755 --- a/linux/svtminion.sh +++ b/linux/svtminion.sh @@ -1933,6 +1933,88 @@ _install_fn () { } +# +# _validate_source_param +# +# Validates the --source parameter value. Exits with scriptFailed (126) +# if the value is empty, contains control characters, contains shell +# injection characters, or does not match a recognised protocol prefix. +# +# Input: +# $1 raw SOURCE_PARAMS string (first word is extracted, matching _source_fn) +# +# Results: +# Returns 0 if valid; exits 126 via _error_log otherwise +# + +_validate_source_param() { + + local source_val="" + source_val=$(echo "$1" | cut -d ' ' -f 1) + + if [[ -z "${source_val}" ]]; then + _error_log "$0:${FUNCNAME[0]} Invalid --source: must not be empty" + fi + + if [[ "${source_val}" =~ [[:cntrl:]] ]]; then + _error_log "$0:${FUNCNAME[0]} Invalid --source: contains control characters" + fi + + if echo "${source_val}" | grep -qE '[`;|&<>]'; then + _error_log "$0:${FUNCNAME[0]} Invalid --source: contains disallowed characters" + fi + + if echo "${source_val}" | grep -qE '^(https?|ftp)://'; then + if echo "${source_val}" | grep -q ' '; then + _error_log "$0:${FUNCNAME[0]} Invalid --source: URL must not contain whitespace" + fi + if ! echo "${source_val}" | grep -qE '^(https?|ftp)://[^/]+'; then + _error_log "$0:${FUNCNAME[0]} Invalid --source: URL has no host" + fi + elif echo "${source_val}" | grep -qE '^/|^file://'; then + # absolute local path or file URI + : + else + _error_log "$0:${FUNCNAME[0]} Invalid --source: '${source_val}' "\ + "must start with http://, https://, ftp://, file://, or /" + fi + + return 0 +} + + +# +# _validate_minion_version_param +# +# Validates the --minionversion parameter value. Exits with scriptFailed +# (126) if the value does not match the expected Salt CalVer format. +# +# Valid: latest, YYYY, YYYY.N, YYYY.N.N, YYYY.NrcN (e.g. 3006, 3006.2, +# 3008.0, 3008.0rc1) +# +# Input: +# $1 raw MINION_VERSION_PARAMS string (first word is extracted) +# +# Results: +# Returns 0 if valid; exits 126 via _error_log otherwise +# + +_validate_minion_version_param() { + + local version_val="" + version_val=$(echo "$1" | cut -d ' ' -f 1) + + if ! echo "${version_val}" | \ + grep -qE '^(latest|[0-9]{4}(\.[0-9]+(\.[0-9]+)*(rc[0-9]+)?)?)$'; then + _error_log "$0:${FUNCNAME[0]} Invalid --minionversion: '${version_val}'. "\ + "Must be 'latest', a major version (e.g. 3006), "\ + "or a full version (e.g. 3006.2, 3008.0rc1)" + fi + + return 0 +} + + # # _source_fn # @@ -2497,12 +2579,14 @@ if [[ ${SOURCE_FLAG} -eq 1 ]]; then CLI_ACTION=1 LOG_ACTION="install" # ensure this is processed before install + _validate_source_param "${SOURCE_PARAMS}" _source_fn "${SOURCE_PARAMS}" retn=$? fi if [[ ${MINION_VERSION_FLAG} -eq 1 ]]; then CLI_ACTION=1 # ensure this is processed before install + _validate_minion_version_param "${MINION_VERSION_PARAMS}" _set_install_minion_version_fn "${MINION_VERSION_PARAMS}" retn=$? fi diff --git a/tests/linux/test-linux.sh b/tests/linux/test-linux.sh index 0a79c80..707f788 100755 --- a/tests/linux/test-linux.sh +++ b/tests/linux/test-linux.sh @@ -19,6 +19,23 @@ yum -y install open-vm-tools yum -y install --allowerasing curl yum -y install wget yum -y install procps-ng +# Validate --source parameter +./svtminion.sh --source "https://bad url.com" --install || \ + { _retn=$?; if [[ ${_retn} -eq 126 ]]; then echo "test correct"; \ + else echo "test failed, bad source should exit 126, got '${_retn}'"; exit 1; fi; } +./svtminion.sh --source "notascheme://bad" --install || \ + { _retn=$?; if [[ ${_retn} -eq 126 ]]; then echo "test correct"; \ + else echo "test failed, bad source scheme should exit 126, got '${_retn}'"; exit 1; fi; } +./svtminion.sh --source "https://evil.com/path;bad" --install || \ + { _retn=$?; if [[ ${_retn} -eq 126 ]]; then echo "test correct"; \ + else echo "test failed, source with injection char should exit 126, got '${_retn}'"; exit 1; fi; } +# Validate --minionversion parameter +./svtminion.sh --minionversion "bad version" --install || \ + { _retn=$?; if [[ ${_retn} -eq 126 ]]; then echo "test correct"; \ + else echo "test failed, bad minionversion should exit 126, got '${_retn}'"; exit 1; fi; } +./svtminion.sh --minionversion "3006;evil" --install || \ + { _retn=$?; if [[ ${_retn} -eq 126 ]]; then echo "test correct"; \ + else echo "test failed, minionversion injection should exit 126, got '${_retn}'"; exit 1; fi; } ./svtminion.sh --depend --loglevel info || { _retn=$?; echo "test failed, there should be no missing dependencies, returned '${_retn}'"; } ls -l /var/log/vmware-svtminion.sh-depend-* | wc -l if [[ 2 -eq $(ls -l /var/log/vmware-svtminion.sh-depend-* | wc -l) ]]; then echo "test correct"; else "test failed, should be 2 depend log files"; exit 1; fi From 4fb88a46a827b83ebc07f6b787a9803190553235 Mon Sep 17 00:00:00 2001 From: twangboy Date: Thu, 7 May 2026 15:37:08 -0600 Subject: [PATCH 3/5] Update docs --- linux/svtminion.sh | 2 ++ windows/svtminion.ps1 | 5 +++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/linux/svtminion.sh b/linux/svtminion.sh index 00b2524..5b7105d 100755 --- a/linux/svtminion.sh +++ b/linux/svtminion.sh @@ -298,6 +298,7 @@ esac echo " //my_path/my_salt_onedir" echo " if specific version of Salt Minion specified, -m" echo " then its appended to source, default[latest]" + echo " invalid value exits with code 126" echo " -l, --loglevel set log level for logging," echo " silent error warning debug info" echo " default loglevel is warning" @@ -305,6 +306,7 @@ esac echo " 'latest' and four-digit major (e.g. 3006) pick" echo " newest GA onedir only; prerelease dirs need" echo " the exact directory name (e.g. 3008.0rc1)" + echo " invalid value exits with code 126" echo " -n, --reconfig salt-minion restarts after reading updated config" echo " -q, --stop stop salt-minion" echo " -p, --start start salt-minion (restarts salt-minion)" diff --git a/windows/svtminion.ps1 b/windows/svtminion.ps1 index 62f34fc..87593f8 100644 --- a/windows/svtminion.ps1 +++ b/windows/svtminion.ps1 @@ -112,7 +112,7 @@ param( # prerelease build, pass the exact directory name shown on Source (for example # "3008.0rc1"). Alternatively, specify a major version number to install the # latest GA build in that series (for example pass "3006" for the newest - # 3006.x release). + # 3006.x release). An invalid value causes the script to exit with code 126. [String] $MinionVersion="latest", [Parameter(Mandatory=$false, ParameterSetName="Install")] @@ -128,7 +128,8 @@ param( # https://packages.broadcom.com/artifactory/saltproject-generic/onedir # # The Source parameter supports common protocols such as HTTP, HTTPS, FTP, - # UNC paths, and local file paths. + # UNC paths, and local file paths. An invalid value causes the script to + # exit with code 126. [String] $Source=( "https://packages.broadcom.com/artifactory/saltproject-generic/onedir" ), From 0d05c3fa1cb4cc4c89696e2ec5d59a11dde030f7 Mon Sep 17 00:00:00 2001 From: twangboy Date: Thu, 7 May 2026 16:07:46 -0600 Subject: [PATCH 4/5] Fix --source and --minionversion to capture single arg, not $* Changing SOURCE_PARAMS and MINION_VERSION_PARAMS from "$*" to "$1" in the argument parser ensures the validators receive only the user-supplied value, not the full remaining argument list. This allows _validate_source_param to correctly detect spaces in HTTP/S/FTP URLs rather than silently truncating to the first word. Removes the cut workaround from both validator functions since the value is now unambiguous. --- linux/svtminion.sh | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/linux/svtminion.sh b/linux/svtminion.sh index 5b7105d..36382ba 100755 --- a/linux/svtminion.sh +++ b/linux/svtminion.sh @@ -1951,8 +1951,7 @@ _install_fn () { _validate_source_param() { - local source_val="" - source_val=$(echo "$1" | cut -d ' ' -f 1) + local source_val="$1" if [[ -z "${source_val}" ]]; then _error_log "$0:${FUNCNAME[0]} Invalid --source: must not be empty" @@ -2003,8 +2002,7 @@ _validate_source_param() { _validate_minion_version_param() { - local version_val="" - version_val=$(echo "$1" | cut -d ' ' -f 1) + local version_val="$1" if ! echo "${version_val}" | \ grep -qE '^(latest|[0-9]{4}(\.[0-9]+(\.[0-9]+)*(rc[0-9]+)?)?)$'; then @@ -2496,7 +2494,7 @@ while true; do -j | --source ) SOURCE_FLAG=1; shift; - SOURCE_PARAMS="$*"; + SOURCE_PARAMS="$1"; ;; -l | --loglevel ) LOG_LEVEL_FLAG=1; @@ -2506,7 +2504,7 @@ while true; do -m | --minionversion ) MINION_VERSION_FLAG=1; shift; - MINION_VERSION_PARAMS="$*"; + MINION_VERSION_PARAMS="$1"; ;; -n | --reconfig ) RECONFIG_FLAG=1; From 9232df5dad28e3c8fb7473af3e5ecb5f385f3266 Mon Sep 17 00:00:00 2001 From: twangboy Date: Thu, 7 May 2026 16:21:44 -0600 Subject: [PATCH 5/5] Fix minionversion regex to allow Linux build suffix (e.g. 3004.2-1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-3005 Linux Salt packages used a -N build suffix in their version strings. The CalVer regex only allowed rc suffixes, causing _validate_minion_version_param to reject 3004.2-1 and 3003.3-1 with exit 126 and breaking existing CI tests that rely on those versions defaulting to latest. Adds (-[0-9]+) as an alternative to (rc[0-9]+) in the Linux validator only — Windows packages were not released with this suffix format. --- linux/svtminion.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/linux/svtminion.sh b/linux/svtminion.sh index 36382ba..d6ef2cf 100755 --- a/linux/svtminion.sh +++ b/linux/svtminion.sh @@ -1990,8 +1990,8 @@ _validate_source_param() { # Validates the --minionversion parameter value. Exits with scriptFailed # (126) if the value does not match the expected Salt CalVer format. # -# Valid: latest, YYYY, YYYY.N, YYYY.N.N, YYYY.NrcN (e.g. 3006, 3006.2, -# 3008.0, 3008.0rc1) +# Valid: latest, YYYY, YYYY.N, YYYY.N.N, YYYY.NrcN, YYYY.N.N-N +# (e.g. 3006, 3006.2, 3008.0, 3008.0rc1, 3004.2-1) # # Input: # $1 raw MINION_VERSION_PARAMS string (first word is extracted) @@ -2005,7 +2005,7 @@ _validate_minion_version_param() { local version_val="$1" if ! echo "${version_val}" | \ - grep -qE '^(latest|[0-9]{4}(\.[0-9]+(\.[0-9]+)*(rc[0-9]+)?)?)$'; then + grep -qE '^(latest|[0-9]{4}(\.[0-9]+(\.[0-9]+)*(rc[0-9]+|-[0-9]+)?)?)$'; then _error_log "$0:${FUNCNAME[0]} Invalid --minionversion: '${version_val}'. "\ "Must be 'latest', a major version (e.g. 3006), "\ "or a full version (e.g. 3006.2, 3008.0rc1)"