Add input parameter validation for --source and --minionversion on Windows and Linux

linux/svtminion.sh

@@ -298,13 +298,15 @@ 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"
      echo "  -m, --minionversion install salt-minion version, default[latest]"
      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)"
@@ -1933,6 +1935,86 @@ _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="$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, 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)
+#
+# Results:
+#   Returns 0 if valid; exits 126 via _error_log otherwise
+#
+
+_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]+|-[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
 #
@@ -2412,7 +2494,7 @@ while true; do
         -j | --source )
             SOURCE_FLAG=1;
             shift;
-            SOURCE_PARAMS="$*";
+            SOURCE_PARAMS="$1";
             ;;
         -l | --loglevel )
             LOG_LEVEL_FLAG=1;
@@ -2422,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;
@@ -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

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

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<bad"; Expected = $false }
+        @{ Source = "https://evil.com/path>bad"; Expected = $false }
+        @{ Source = "notascheme://bad"; Expected = $false }
+        @{ Source = ""; Expected = $false }
+        @{ Source = "   "; Expected = $false }
+        @{ Source = "\\missingshare"; Expected = $false }
+    )
+    $failed = 0
+    foreach ($c in $cases) {
+        $got = Test-SourceParameter -Source $c.Source
+        if ($got -ne $c.Expected) {
+            Write-Host ""
+            Write-Host "Source: '$($c.Source)' Expected: $($c.Expected) Got: $got"
+            $failed = 1
+        }
+    }
+    return $failed
+}
+
+
+function test_Test-MinionVersionParameter {
+    $cases = @(
+        @{ MinionVersion = "latest"; Expected = $true }
+        @{ MinionVersion = "3006"; Expected = $true }
+        @{ MinionVersion = "3006.2"; Expected = $true }
+        @{ MinionVersion = "3006.24"; Expected = $true }
+        @{ MinionVersion = "3008.0"; Expected = $true }
+        @{ MinionVersion = "3008.0rc1"; Expected = $true }
+        @{ MinionVersion = "3009.0rc2"; Expected = $true }
+        @{ MinionVersion = "3009.1.2"; Expected = $true }
+        @{ MinionVersion = "bad version"; Expected = $false }
+        @{ MinionVersion = "3006/evil"; Expected = $false }
+        @{ MinionVersion = "abc"; Expected = $false }
+        @{ MinionVersion = '3006;rm -rf /'; Expected = $false }
+        @{ MinionVersion = ""; Expected = $false }
+        @{ MinionVersion = "3006 .2"; Expected = $false }
+    )
+    $failed = 0
+    foreach ($c in $cases) {
+        $got = Test-MinionVersionParameter -MinionVersion $c.MinionVersion
+        if ($got -ne $c.Expected) {
+            Write-Host ""
+            Write-Host "MinionVersion: '$($c.MinionVersion)' Expected: $($c.Expected) Got: $got"
+            $failed = 1
+        }
+    }
+    return $failed
+}

tests/windows/functional/test_parse_config.ps1

@@ -18,3 +18,17 @@ function test__parse_config_preserve_case{
     if ($result["encryption_algorithm"] -cne "OAEP-SHA224") { return 1 }
     return 0
 }
+
+function test__parse_config_control_chars {
+    # Entries containing control characters should be silently skipped; valid
+    # entries in the same input must still be parsed correctly.
+    $null_byte = [char]0x00
+    $bad_key = "bad${null_byte}key=value"
+    $bad_val = "key=bad${null_byte}value"
+    $result = _parse_config -KeyValues "master=test $bad_key $bad_val id=minion"
+    if ($result["master"] -ne "test") { return 1 }
+    if ($result["id"] -ne "minion") { return 1 }
+    if ($result.ContainsKey("bad${null_byte}key")) { return 1 }
+    if ($result.ContainsKey("key")) { return 1 }
+    return 0
+}

tests/windows/integration/test_input_validation.ps1

@@ -0,0 +1,35 @@
+function invoke_script {
+    param([String[]] $Arguments)
+    & powershell.exe -File .\windows\svtminion.ps1 @Arguments *> $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
+}