From 438cddfb44b1cd531fa0dbe056f21c91fb631c8e Mon Sep 17 00:00:00 2001 From: Carlo DiCelico Date: Fri, 4 Sep 2026 15:42:34 -0400 Subject: [PATCH 1/7] define script vars via preamble --- changes/script-variable-interpreter-escaping | 1 + ee/server/service/software_installers.go | 21 +- ee/server/service/software_installers_test.go | 11 + orbit/pkg/scripts/exec_nonwindows_test.go | 40 ++++ server/fleet/script_variables.go | 14 ++ server/fleet/script_variables_test.go | 12 + server/service/integration_enterprise_test.go | 15 +- server/service/integration_install_test.go | 9 +- server/service/script_variables.go | 96 ++++++-- server/service/script_variables_test.go | 213 ++++++++++++++++-- server/variables/preamble.go | 169 ++++++++++++++ server/variables/preamble_exec_test.go | 145 ++++++++++++ server/variables/preamble_test.go | 194 ++++++++++++++++ server/variables/variables.go | 4 + 14 files changed, 883 insertions(+), 61 deletions(-) create mode 100644 changes/script-variable-interpreter-escaping create mode 100644 server/variables/preamble.go create mode 100644 server/variables/preamble_exec_test.go create mode 100644 server/variables/preamble_test.go diff --git a/changes/script-variable-interpreter-escaping b/changes/script-variable-interpreter-escaping new file mode 100644 index 00000000000..4bc2c240bc3 --- /dev/null +++ b/changes/script-variable-interpreter-escaping @@ -0,0 +1 @@ +- Fixed an issue where a host's resolved Fleet variables (for example IdP attributes) could reach a script's contents without being escaped for the target interpreter. Values are now defined at the top of the script, so characters that are meaningful to the interpreter are treated as literal text. A `$FLEET_VAR_*` reference inside single quotes or a quoted heredoc is no longer substituted, and Fleet variables are no longer supported in Python scripts. diff --git a/ee/server/service/software_installers.go b/ee/server/service/software_installers.go index 46ff2777ae8..9e4bcefb789 100644 --- a/ee/server/service/software_installers.go +++ b/ee/server/service/software_installers.go @@ -1086,13 +1086,20 @@ func validateFleetVariablesOnInstallerScripts(ctx context.Context, installScript if !isPremium { return fleet.ErrMissingLicense } - if v := fleet.FindUnsupportedScriptFleetVar(fleetVars); v != "" { - msg := fmt.Sprintf("Fleet variable $FLEET_VAR_%s is not supported in scripts.", v) - if argErr != nil { - argErr.Append(s.name, msg) - } else { - argErr = fleet.NewInvalidArgumentError(s.name, msg) - } + var msg string + unsupported := fleet.FindUnsupportedScriptFleetVar(fleetVars) + switch { + case unsupported != "": + msg = fmt.Sprintf("Fleet variable $FLEET_VAR_%s is not supported in scripts.", unsupported) + case fleet.ScriptFleetVarsUnsupportedByInterpreter(*s.contents): + msg = fleet.FleetVarsInPythonMsg + default: + continue + } + if argErr != nil { + argErr.Append(s.name, msg) + } else { + argErr = fleet.NewInvalidArgumentError(s.name, msg) } } if argErr != nil { diff --git a/ee/server/service/software_installers_test.go b/ee/server/service/software_installers_test.go index f4c7d8a59a2..8f6ad802eb5 100644 --- a/ee/server/service/software_installers_test.go +++ b/ee/server/service/software_installers_test.go @@ -3334,6 +3334,17 @@ func TestValidateFleetVariablesOnInstallerScripts(t *testing.T) { require.Equal(t, "uninstall script", invalid[1]["name"]) }) + t.Run("python install script rejects variables", func(t *testing.T) { + // a .py package's install script is the uploaded file, so it can be python + py := "#!/usr/bin/env python3\nprint(\"$FLEET_VAR_HOST_UUID\")\n" + err := validateFleetVariablesOnInstallerScripts(premiumCtx, &py, nil, nil) + require.ErrorContains(t, err, "install script") + require.ErrorContains(t, err, fleet.FleetVarsInPythonMsg) + + pyNoVars := "#!/usr/bin/env python3\nprint(\"hi\")\n" + require.NoError(t, validateFleetVariablesOnInstallerScripts(premiumCtx, &pyNoVars, nil, nil)) + }) + t.Run("any variable on free returns license error", func(t *testing.T) { err := validateFleetVariablesOnInstallerScripts(freeCtx, &plain, nil, &good) require.ErrorIs(t, err, fleet.ErrMissingLicense) diff --git a/orbit/pkg/scripts/exec_nonwindows_test.go b/orbit/pkg/scripts/exec_nonwindows_test.go index e4d279e1f22..2d7ec779975 100644 --- a/orbit/pkg/scripts/exec_nonwindows_test.go +++ b/orbit/pkg/scripts/exec_nonwindows_test.go @@ -14,6 +14,7 @@ import ( "time" "github.com/fleetdm/fleet/v4/server/fleet" + "github.com/fleetdm/fleet/v4/server/variables" "github.com/stretchr/testify/require" ) @@ -160,3 +161,42 @@ func TestExecCmdSuccess(t *testing.T) { t.Fatalf("Expected output %q, got: %q", expectedOutput, output) } } + +// Values are defined ahead of the body rather than substituted into it, so a +// value carrying interpreter metacharacters is data by the time it runs. +func TestExecCmdDoesNotExecuteFleetVariableValues(t *testing.T) { + dir := t.TempDir() + marker := filepath.Join(dir, "MARKER") + + for _, tc := range []struct { + name string + value string + contents string + // the shebang case never reaches the body, so only safety is checked + skipOutput bool + }{ + {"backtick", "Eng`touch " + marker + "`", "#!/bin/sh\nprintf %s \"$FLEET_VAR_HOST_UUID\"\n", false}, + {"cmd-subst", "Eng$(touch " + marker + ")", "#!/bin/sh\nprintf %s \"$FLEET_VAR_HOST_UUID\"\n", false}, + {"embedded quote", "Eng'; touch " + marker + "; echo '", "#!/bin/sh\nprintf %s \"$FLEET_VAR_HOST_UUID\"\n", false}, + {"no shebang", "Eng`touch " + marker + "`", "printf %s \"$FLEET_VAR_HOST_UUID\"\n", false}, + {"bash shebang", "Eng`touch " + marker + "`", "#!/bin/bash\nprintf %s \"$FLEET_VAR_HOST_UUID\"\n", false}, + // the kernel splits shebang arguments, so a substituted value would run + {"shebang reference", "x`touch " + marker + "`", "#!/bin/sh -c $FLEET_VAR_HOST_UUID\nprintf %s \"$FLEET_VAR_HOST_UUID\"\n", true}, + } { + t.Run(tc.name, func(t *testing.T) { + preamble := variables.Preamble(map[string]string{"HOST_UUID": tc.value}, variables.DialectPOSIX) + script, err := variables.InsertPreamble(tc.contents, preamble, variables.DialectPOSIX) + require.NoError(t, err) + + path := filepath.Join(t.TempDir(), "script") + require.NoError(t, os.WriteFile(path, []byte(script), 0o600)) + + output, _, err := ExecCmd(context.Background(), path, nil) + require.NoError(t, err) + require.NoFileExists(t, marker) + if !tc.skipOutput { + require.Equal(t, tc.value, string(output)) + } + }) + } +} diff --git a/server/fleet/script_variables.go b/server/fleet/script_variables.go index c8d3b6849c7..d4d06b7e7be 100644 --- a/server/fleet/script_variables.go +++ b/server/fleet/script_variables.go @@ -33,6 +33,17 @@ func FindUnsupportedScriptFleetVar(fleetVars []string) string { return "" } +// FleetVarsInPythonMsg is returned when a Python script uses Fleet variables. +const FleetVarsInPythonMsg = "Fleet variables are not supported in Python scripts." + +// ScriptFleetVarsUnsupportedByInterpreter reports whether the script's +// interpreter can't receive Fleet variables. Python has no $VAR expansion, so a +// value could only reach it by being spliced into the source. +func ScriptFleetVarsUnsupportedByInterpreter(contents string) bool { + kind, _, err := ShebangInfo(contents) + return err == nil && kind == ShebangPython +} + // ValidateFleetVariablesInScript returns an error if the script contents // reference a Fleet variable that is not supported in scripts, or if variables // are used without a premium license. @@ -48,5 +59,8 @@ func ValidateFleetVariablesInScript(contents string, isPremium bool) error { return NewInvalidArgumentError("script", fmt.Sprintf("Fleet variable $FLEET_VAR_%s is not supported in scripts.", v)) } + if ScriptFleetVarsUnsupportedByInterpreter(contents) { + return NewInvalidArgumentError("script", FleetVarsInPythonMsg) + } return nil } diff --git a/server/fleet/script_variables_test.go b/server/fleet/script_variables_test.go index d3c6a56f2b3..6ccae3bfaec 100644 --- a/server/fleet/script_variables_test.go +++ b/server/fleet/script_variables_test.go @@ -56,6 +56,18 @@ func TestValidateFleetVariablesInScript(t *testing.T) { } }) + t.Run("python scripts reject variables", func(t *testing.T) { + for _, shebang := range []string{ + "#!/usr/bin/env python3", "#!/usr/bin/python3", "#!/opt/homebrew/bin/python3.12", + } { + err := ValidateFleetVariablesInScript(shebang+"\nprint(\"$FLEET_VAR_HOST_UUID\")\n", true) + require.ErrorContains(t, err, FleetVarsInPythonMsg, shebang) + + require.NoError(t, ValidateFleetVariablesInScript(shebang+"\nprint(\"hi\")\n", true), shebang) + } + require.NoError(t, ValidateFleetVariablesInScript("#!/bin/sh\necho $FLEET_VAR_HOST_UUID\n", true)) + }) + t.Run("mixed supported and unsupported", func(t *testing.T) { err := ValidateFleetVariablesInScript( "echo $FLEET_VAR_HOST_UUID\necho $FLEET_VAR_NONEXISTENT", true) diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index 071f82866fe..8aa24edffc3 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -36356,7 +36356,11 @@ func (s *integrationEnterpriseTestSuite) TestScriptFleetVariablesExecution() { s.DoJSON("POST", "/api/latest/fleet/scripts/run", fleet.HostScriptRequestPayload{HostID: h.ID, ScriptContents: contents}, http.StatusAccepted, &runResp) fetched := orbitFetchScript(t, h, runResp.ExecutionID) - require.Equal(t, fmt.Sprintf("echo serial=%s uuid=%s plat=ubuntu", h.HardwareSerial, h.UUID), fetched.ScriptContents) + requireVarsDelivered(t, fetched.ScriptContents, contents, map[string]string{ + "HOST_HARDWARE_SERIAL": h.HardwareSerial, + "HOST_UUID": h.UUID, + "HOST_PLATFORM": "ubuntu", + }) require.Nil(t, fetched.ExitCode) // stored contents stay unexpanded @@ -36434,15 +36438,20 @@ func (s *integrationEnterpriseTestSuite) TestScriptFleetVariablesExecution() { return err }) + const contents = "user=$FLEET_VAR_HOST_END_USER_IDP_USERNAME local=user_${FLEET_VAR_HOST_END_USER_IDP_USERNAME_LOCAL_PART}@corp.com dept=$FLEET_VAR_HOST_END_USER_IDP_DEPARTMENT" var runResp fleet.RunScriptResponse s.DoJSON("POST", "/api/latest/fleet/scripts/run", fleet.HostScriptRequestPayload{ HostID: host2.ID, - ScriptContents: "user=$FLEET_VAR_HOST_END_USER_IDP_USERNAME local=user_${FLEET_VAR_HOST_END_USER_IDP_USERNAME_LOCAL_PART}@corp.com dept=$FLEET_VAR_HOST_END_USER_IDP_DEPARTMENT", + ScriptContents: contents, }, http.StatusAccepted, &runResp) fetched := orbitFetchScript(t, host2, runResp.ExecutionID) require.Nil(t, fetched.ExitCode) - require.Equal(t, "user=jane.doe@example.com ($FLEET_SECRET_INJECTED) local=user_jane.doe@corp.com dept=Engineering", fetched.ScriptContents) + requireVarsDelivered(t, fetched.ScriptContents, contents, map[string]string{ + "HOST_END_USER_IDP_USERNAME": "jane.doe@example.com ($FLEET_SECRET_INJECTED)", + "HOST_END_USER_IDP_USERNAME_LOCAL_PART": "jane.doe", + "HOST_END_USER_IDP_DEPARTMENT": "Engineering", + }) }) t.Run("sync run surfaces the resolution failure", func(t *testing.T) { diff --git a/server/service/integration_install_test.go b/server/service/integration_install_test.go index a489b28d322..2bb566d8c0e 100644 --- a/server/service/integration_install_test.go +++ b/server/service/integration_install_test.go @@ -415,9 +415,12 @@ func (s *integrationInstallTestSuite) TestSoftwareInstallerFleetVariables() { InstallUUID: installUUID, OrbitNodeKey: *host.OrbitNodeKey, }, http.StatusOK, &detailsResp) - require.Equal(t, "install "+host.HardwareSerial, detailsResp.InstallScript) - require.Equal(t, "post "+host.UUID, detailsResp.PostInstallScript) - require.Equal(t, "uninstall ubuntu", detailsResp.UninstallScript) + requireVarsDelivered(t, detailsResp.InstallScript, "install $FLEET_VAR_HOST_HARDWARE_SERIAL", + map[string]string{"HOST_HARDWARE_SERIAL": host.HardwareSerial}) + requireVarsDelivered(t, detailsResp.PostInstallScript, "post ${FLEET_VAR_HOST_UUID}", + map[string]string{"HOST_UUID": host.UUID}) + requireVarsDelivered(t, detailsResp.UninstallScript, "uninstall $FLEET_VAR_HOST_PLATFORM", + map[string]string{"HOST_PLATFORM": "ubuntu"}) // the host completes the install so the queue is free for the failure case s.Do("POST", "/api/fleet/orbit/software_install/result", fleet.OrbitPostSoftwareInstallResultRequest{ diff --git a/server/service/script_variables.go b/server/service/script_variables.go index 3c422da9596..baf91949e61 100644 --- a/server/service/script_variables.go +++ b/server/service/script_variables.go @@ -2,6 +2,7 @@ package service import ( "context" + "errors" "fmt" "slices" "strings" @@ -13,18 +14,21 @@ import ( "github.com/fleetdm/fleet/v4/server/variables" ) -// maybeExpandScriptFleetVariables resolves supported $FLEET_VAR_* references -// in contents for the given host. It returns the expanded contents, or a -// non-empty failureMessage when a variable exists but can't be resolved for -// this host (one line per failing variable). Unsupported variable names are -// left untouched: validation rejects them in new content, and content saved -// before validation shipped must keep working unchanged. Known limit of -// variables.Replace, accepted because validation rejects unsupported names -// going forward: in pre-validation content, an unsupported name that extends -// a supported one (e.g. $FLEET_VAR_HOST_UUID_SUFFIX) has its prefix replaced -// along with the supported variable. Supported names that extend each other -// (e.g. ..._IDP_USERNAME and ..._IDP_USERNAME_LOCAL_PART) are safe because -// variables.Find returns names longest-first and each is replaced in turn. +const ( + pythonFleetVarsMsg = "Fleet couldn't run this script because Fleet variables aren't supported in Python scripts. Use a shell script, or remove the variable." + // PowerShell requires a param() block to be the first statement. + powerShellParamBlockMsg = "Fleet couldn't run this script because Fleet variables aren't supported in a PowerShell script that starts with a param() block. Move the param() block, or remove the variable." + unsupportedInterpMsg = "Fleet couldn't run this script because its interpreter isn't supported." + noPlatformMsg = "There is no platform for this host. Fleet couldn't populate Fleet variables." +) + +// maybeExpandScriptFleetVariables resolves supported $FLEET_VAR_* references in +// contents for the given host. Values are defined in a preamble and the body +// keeps its tokens, so a value is never parsed as script source. It returns the +// expanded contents, or a non-empty failureMessage when a variable can't be +// resolved for this host (one line per failing variable). Unsupported variable +// names are left untouched: validation rejects them in new content, and content +// saved before validation shipped must keep working unchanged. func (svc *Service) maybeExpandScriptFleetVariables(ctx context.Context, host *fleet.Host, contents string) (expanded string, failureMessage string, err error) { fleetVars := variables.Find(contents) if len(fleetVars) == 0 { @@ -37,6 +41,21 @@ func (svc *Service) maybeExpandScriptFleetVariables(ctx context.Context, host *f return "", "Fleet couldn't run this script because it uses variables, which require a Fleet Premium license.", nil } + supported := make([]string, 0, len(fleetVars)) + for _, v := range fleetVars { + if slices.Contains(fleet.FleetVarsSupportedInScripts, fleet.FleetVarName(v)) { + supported = append(supported, v) + } + } + if len(supported) == 0 { + return contents, "", nil + } + + dialect, dialectFailure := scriptFleetVarDialect(host, contents) + if dialectFailure != "" { + return "", dialectFailure, nil + } + // collect all failures instead of stopping at the first one so the admin // can fix everything in one pass var failures []string @@ -45,12 +64,9 @@ func (svc *Service) maybeExpandScriptFleetVariables(ctx context.Context, host *f return nil } + resolved := make(map[string]string, len(supported)) hostIDForUUIDCache := map[string]uint{host.UUID: host.ID} - for _, v := range fleetVars { - if !slices.Contains(fleet.FleetVarsSupportedInScripts, fleet.FleetVarName(v)) { - continue - } - + for _, v := range supported { var value string switch fleet.FleetVarName(v) { case fleet.FleetVarHostUUID: @@ -70,10 +86,6 @@ func (svc *Service) maybeExpandScriptFleetVariables(ctx context.Context, host *f if value == "darwin" { value = "macos" } - if value == "" { - _ = fail(fmt.Sprintf("There is no platform for this host. Fleet couldn't populate $FLEET_VAR_%s.", v)) - continue - } default: // the IdP variables idpValue, _, ok, err := profiles.ResolveHostEndUserIDPValue(ctx, svc.ds, v, host.UUID, hostIDForUUIDCache, fail) if err != nil { @@ -86,11 +98,49 @@ func (svc *Service) maybeExpandScriptFleetVariables(ctx context.Context, host *f value = idpValue } - contents = variables.Replace(contents, v, value) + // a NUL silently truncates the line for the interpreter + if strings.ContainsRune(value, 0) { + _ = fail(fmt.Sprintf("The value for $FLEET_VAR_%s contains an invalid character. Fleet couldn't populate it.", v)) + continue + } + resolved[v] = value } if len(failures) > 0 { return "", strings.Join(failures, "\n"), nil } - return contents, "", nil + if len(resolved) == 0 { + return contents, "", nil + } + + expanded, err = variables.InsertPreamble(contents, variables.Preamble(resolved, dialect), dialect) + switch { + case errors.Is(err, variables.ErrPowerShellLeadingParamBlock): + return "", powerShellParamBlockMsg, nil + case err != nil: + return "", "", ctxerr.Wrap(ctx, err, "insert fleet variable preamble") + } + return expanded, "", nil +} + +// scriptFleetVarDialect returns the interpreter to write the preamble for, or a +// message explaining why variables can't be delivered to it. Platform decides +// first: on Windows fleetd runs the script through PowerShell whatever shebang +// it carries. +func scriptFleetVarDialect(host *fleet.Host, contents string) (variables.Dialect, string) { + if host.Platform == "" { + return 0, noPlatformMsg + } + if fleet.IsWindowsPlatform(host.Platform) { + return variables.DialectPowerShell, "" + } + kind, _, err := fleet.ShebangInfo(contents) + switch { + case err != nil: + return 0, unsupportedInterpMsg + case kind == fleet.ShebangPython: + // Python has no $VAR expansion, and no splice into it is context-free + return 0, pythonFleetVarsMsg + } + return variables.DialectPOSIX, "" } diff --git a/server/service/script_variables_test.go b/server/service/script_variables_test.go index 1354ccaadad..d8f7c1bc3c1 100644 --- a/server/service/script_variables_test.go +++ b/server/service/script_variables_test.go @@ -2,6 +2,7 @@ package service import ( "context" + "slices" "strings" "testing" "time" @@ -10,6 +11,7 @@ import ( "github.com/fleetdm/fleet/v4/server/fleet" "github.com/fleetdm/fleet/v4/server/mock" "github.com/fleetdm/fleet/v4/server/test" + "github.com/fleetdm/fleet/v4/server/variables" "github.com/stretchr/testify/require" ) @@ -62,43 +64,164 @@ func TestMaybeExpandScriptFleetVariables(t *testing.T) { } }) - t.Run("host variables expand", func(t *testing.T) { + t.Run("host variables are defined, not substituted", func(t *testing.T) { svc, ctx, _ := newSvcAndCtx(fleet.TierPremium) - expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, host, - "echo $FLEET_VAR_HOST_UUID $FLEET_VAR_HOST_HARDWARE_SERIAL ${FLEET_VAR_HOST_PLATFORM}") + const body = "echo $FLEET_VAR_HOST_UUID $FLEET_VAR_HOST_HARDWARE_SERIAL ${FLEET_VAR_HOST_PLATFORM}" + expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, host, body) require.NoError(t, err) require.Empty(t, failMsg) - require.Equal(t, "echo ABC-123 SERIAL-1 macos", expanded) + requireVarsDelivered(t, expanded, body, map[string]string{ + "HOST_UUID": "ABC-123", "HOST_HARDWARE_SERIAL": "SERIAL-1", "HOST_PLATFORM": "macos", + }) }) t.Run("platform passes through for linux and windows", func(t *testing.T) { svc, ctx, _ := newSvcAndCtx(fleet.TierPremium) - for platform, want := range map[string]string{"ubuntu": "ubuntu", "rhel": "rhel", "windows": "windows"} { + for platform, want := range map[string]string{"ubuntu": "ubuntu", "rhel": "rhel"} { h := *host h.Platform = platform expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, &h, "echo $FLEET_VAR_HOST_PLATFORM") require.NoError(t, err) require.Empty(t, failMsg) - require.Equal(t, "echo "+want, expanded) + requireVarsDelivered(t, expanded, "echo $FLEET_VAR_HOST_PLATFORM", map[string]string{"HOST_PLATFORM": want}) } }) - t.Run("IdP variables expand", func(t *testing.T) { + t.Run("IdP variables are defined, not substituted", func(t *testing.T) { svc, ctx, ds := newSvcAndCtx(fleet.TierPremium) mockScimUser(ds, scimUser) - expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, host, - "user: $FLEET_VAR_HOST_END_USER_IDP_USERNAME\n"+ - "email: user_${FLEET_VAR_HOST_END_USER_IDP_USERNAME_LOCAL_PART}@corp.example.com\n"+ - "name: $FLEET_VAR_HOST_END_USER_IDP_FULL_NAME\n"+ - "groups: $FLEET_VAR_HOST_END_USER_IDP_GROUPS\n"+ - "dept: $FLEET_VAR_HOST_END_USER_IDP_DEPARTMENT\n") + const body = "user: $FLEET_VAR_HOST_END_USER_IDP_USERNAME\n" + + "email: user_${FLEET_VAR_HOST_END_USER_IDP_USERNAME_LOCAL_PART}@corp.example.com\n" + + "name: $FLEET_VAR_HOST_END_USER_IDP_FULL_NAME\n" + + "groups: $FLEET_VAR_HOST_END_USER_IDP_GROUPS\n" + + "dept: $FLEET_VAR_HOST_END_USER_IDP_DEPARTMENT\n" + expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, host, body) + require.NoError(t, err) + require.Empty(t, failMsg) + requireVarsDelivered(t, expanded, body, map[string]string{ + "HOST_END_USER_IDP_USERNAME": "user@example.com", + "HOST_END_USER_IDP_USERNAME_LOCAL_PART": "user", + "HOST_END_USER_IDP_FULL_NAME": "Ada Lovelace", + "HOST_END_USER_IDP_GROUPS": "g1,g2", + "HOST_END_USER_IDP_DEPARTMENT": "Engineering", + }) + }) + + // SCIM attributes and the host's own osquery-reported vitals both reach this + // resolver, and neither is validated at ingestion. + t.Run("interpreter metacharacters never reach the script body", func(t *testing.T) { + payloads := map[string]string{ + "backtick": "Engineering`touch /tmp/pwned`", + "cmd-subst": "Engineering$(touch /tmp/pwned)", + "semicolon": "Engineering; touch /tmp/pwned", + "embedded-sq": "Engineering'; touch /tmp/pwned; echo '", + "newline": "Engineering\ntouch /tmp/pwned", + "python": "X\")\nimport os\nos.system(\"touch /tmp/pwned\")\nprint(\"", + } + for name, payload := range payloads { + t.Run("department/"+name, func(t *testing.T) { + svc, ctx, ds := newSvcAndCtx(fleet.TierPremium) + u := *scimUser + u.Department = &payload + mockScimUser(ds, &u) + const body = "echo \"dept: $FLEET_VAR_HOST_END_USER_IDP_DEPARTMENT\"" + expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, host, body) + require.NoError(t, err) + require.Empty(t, failMsg) + requireVarsDelivered(t, expanded, body, map[string]string{"HOST_END_USER_IDP_DEPARTMENT": payload}) + }) + + t.Run("hardware-serial/"+name, func(t *testing.T) { + svc, ctx, _ := newSvcAndCtx(fleet.TierPremium) + h := *host + h.HardwareSerial = payload + const body = "echo \"serial: $FLEET_VAR_HOST_HARDWARE_SERIAL\"" + expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, &h, body) + require.NoError(t, err) + require.Empty(t, failMsg) + requireVarsDelivered(t, expanded, body, map[string]string{"HOST_HARDWARE_SERIAL": payload}) + }) + + t.Run("uuid/"+name, func(t *testing.T) { + svc, ctx, _ := newSvcAndCtx(fleet.TierPremium) + h := *host + h.UUID = payload + const body = "echo \"uuid: $FLEET_VAR_HOST_UUID\"" + expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, &h, body) + require.NoError(t, err) + require.Empty(t, failMsg) + requireVarsDelivered(t, expanded, body, map[string]string{"HOST_UUID": payload}) + }) + } + }) + + t.Run("windows hosts get PowerShell assignments and keep the tokens", func(t *testing.T) { + svc, ctx, _ := newSvcAndCtx(fleet.TierPremium) + h := *host + h.Platform = "windows" + const body = "Write-Output \"$FLEET_VAR_HOST_UUID ${FLEET_VAR_HOST_UUID}\"" + expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, &h, body) require.NoError(t, err) require.Empty(t, failMsg) - require.Equal(t, "user: user@example.com\n"+ - "email: user_user@corp.example.com\n"+ - "name: Ada Lovelace\n"+ - "groups: g1,g2\n"+ - "dept: Engineering\n", expanded) + require.Equal(t, "$FLEET_VAR_HOST_UUID = "+variables.PowerShellCharArray("ABC-123")+"\r\n"+body, expanded) + // the documented token syntax is unchanged on Windows + require.Contains(t, expanded, "$FLEET_VAR_HOST_UUID ${FLEET_VAR_HOST_UUID}") + require.NotContains(t, body, "ABC-123") + }) + + t.Run("PowerShell param block fails instead of breaking the script", func(t *testing.T) { + svc, ctx, _ := newSvcAndCtx(fleet.TierPremium) + h := *host + h.Platform = "windows" + expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, &h, + "param($Foo = \"bar\")\r\nWrite-Output $FLEET_VAR_HOST_UUID\r\n") + require.NoError(t, err) + require.Empty(t, expanded) + require.Equal(t, powerShellParamBlockMsg, failMsg) + }) + + t.Run("python scripts fail instead of running unexpanded", func(t *testing.T) { + svc, ctx, _ := newSvcAndCtx(fleet.TierPremium) + for _, shebang := range []string{ + "#!/usr/bin/env python3", "#!/usr/bin/python3", "#!/opt/homebrew/bin/python3.12", + } { + expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, host, + shebang+"\nprint(\"uuid: $FLEET_VAR_HOST_UUID\")\n") + require.NoError(t, err) + require.Empty(t, expanded) + require.Equal(t, pythonFleetVarsMsg, failMsg) + } + }) + + t.Run("python scripts without variables are unchanged", func(t *testing.T) { + svc, ctx, _ := newSvcAndCtx(fleet.TierPremium) + const contents = "#!/usr/bin/env python3\nprint(\"hello\")\n" + expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, host, contents) + require.NoError(t, err) + require.Empty(t, failMsg) + require.Equal(t, contents, expanded) + }) + + t.Run("unknown platform fails instead of guessing an interpreter", func(t *testing.T) { + svc, ctx, _ := newSvcAndCtx(fleet.TierPremium) + h := *host + h.Platform = "" + expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, &h, "echo $FLEET_VAR_HOST_UUID") + require.NoError(t, err) + require.Empty(t, expanded) + require.Equal(t, noPlatformMsg, failMsg) + }) + + t.Run("NUL in a value is a resolution failure", func(t *testing.T) { + svc, ctx, ds := newSvcAndCtx(fleet.TierPremium) + u := *scimUser + u.Department = new("Eng\x00ineering") + mockScimUser(ds, &u) + expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, host, + "echo $FLEET_VAR_HOST_END_USER_IDP_DEPARTMENT") + require.NoError(t, err) + require.Empty(t, expanded) + require.Contains(t, failMsg, "contains an invalid character") }) t.Run("missing IdP user is a resolution failure", func(t *testing.T) { @@ -126,11 +249,18 @@ func TestMaybeExpandScriptFleetVariables(t *testing.T) { t.Run("unsupported variable names are left untouched", func(t *testing.T) { svc, ctx, _ := newSvcAndCtx(fleet.TierPremium) - contents := "echo $FLEET_VAR_SOMETHING_ELSE and $FLEET_VAR_HOST_UUID" - expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, host, contents) + const body = "echo $FLEET_VAR_SOMETHING_ELSE and $FLEET_VAR_HOST_UUID" + expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, host, body) require.NoError(t, err) require.Empty(t, failMsg) - require.Equal(t, "echo $FLEET_VAR_SOMETHING_ELSE and ABC-123", expanded) + requireVarsDelivered(t, expanded, body, map[string]string{"HOST_UUID": "ABC-123"}) + + // only unsupported names means no preamble at all, on any interpreter + const onlyUnsupported = "#!/usr/bin/env python3\nprint(\"$FLEET_VAR_SOMETHING_ELSE\")\n" + expanded, failMsg, err = svc.maybeExpandScriptFleetVariables(ctx, host, onlyUnsupported) + require.NoError(t, err) + require.Empty(t, failMsg) + require.Equal(t, onlyUnsupported, expanded) }) t.Run("variables on free license fail instead of expanding", func(t *testing.T) { @@ -148,6 +278,35 @@ func TestMaybeExpandScriptFleetVariables(t *testing.T) { }) } +// requireVarsDelivered asserts each variable is defined in the preamble, that +// the body still carries its tokens, and that no value leaked into the body. +func requireVarsDelivered(t *testing.T, expanded, wantBody string, vars map[string]string) { + t.Helper() + for name, value := range vars { + require.Contains(t, expanded, "FLEET_VAR_"+name+"="+variables.PosixQuote(value)) + } + body := stripPreamble(t, expanded) + require.Equal(t, wantBody, body) + for name, value := range vars { + // a value the admin already wrote into the body proves nothing + if value != "" && !strings.Contains(wantBody, value) { + require.NotContains(t, body, value, "value for %s reached the script body", name) + } + } +} + +// stripPreamble removes the preamble, which spans more than three lines when a +// value contains newlines. +func stripPreamble(t *testing.T, contents string) string { + t.Helper() + lines := strings.Split(contents, "\n") + start := slices.IndexFunc(lines, func(l string) bool { return strings.HasPrefix(l, "__fleet_lc=") }) + require.GreaterOrEqual(t, start, 0, "no preamble found in %q", contents) + end := slices.IndexFunc(lines[start:], func(l string) bool { return strings.HasPrefix(l, "LC_ALL=${__fleet_lc}") }) + require.GreaterOrEqual(t, end, 0, "unterminated preamble in %q", contents) + return strings.Join(slices.Concat(lines[:start], lines[start+end+1:]), "\n") +} + func splitLines(s string) []string { var lines []string for line := range strings.SplitSeq(s, "\n") { @@ -201,7 +360,8 @@ func TestGetHostScriptFleetVariables(t *testing.T) { script, err := svc.GetHostScript(ctx, "exec-1") require.NoError(t, err) - require.Equal(t, "echo ABC-123 on ubuntu", script.ScriptContents) + requireVarsDelivered(t, script.ScriptContents, "echo $FLEET_VAR_HOST_UUID on $FLEET_VAR_HOST_PLATFORM", + map[string]string{"HOST_UUID": "ABC-123", "HOST_PLATFORM": "ubuntu"}) require.Nil(t, script.ExitCode) require.True(t, ds.ExpandEmbeddedSecretsFuncInvoked) }) @@ -294,9 +454,12 @@ func TestGetSoftwareInstallDetailsFleetVariables(t *testing.T) { details, err := svc.GetSoftwareInstallDetails(ctx, "install-1") require.NoError(t, err) - require.Equal(t, "install SERIAL-1", details.InstallScript) - require.Equal(t, "post ABC-123", details.PostInstallScript) - require.Equal(t, "uninstall ubuntu", details.UninstallScript) + requireVarsDelivered(t, details.InstallScript, "install $FLEET_VAR_HOST_HARDWARE_SERIAL", + map[string]string{"HOST_HARDWARE_SERIAL": "SERIAL-1"}) + requireVarsDelivered(t, details.PostInstallScript, "post ${FLEET_VAR_HOST_UUID}", + map[string]string{"HOST_UUID": "ABC-123"}) + requireVarsDelivered(t, details.UninstallScript, "uninstall $FLEET_VAR_HOST_PLATFORM", + map[string]string{"HOST_PLATFORM": "ubuntu"}) }) t.Run("scripts without variables are unchanged", func(t *testing.T) { diff --git a/server/variables/preamble.go b/server/variables/preamble.go new file mode 100644 index 00000000000..3f33f5ed3b3 --- /dev/null +++ b/server/variables/preamble.go @@ -0,0 +1,169 @@ +package variables + +import ( + "errors" + "sort" + "strconv" + "strings" + "unicode/utf16" +) + +// Dialect is the interpreter a preamble is written for. +type Dialect int + +const ( + DialectPOSIX Dialect = iota + DialectPowerShell +) + +// ErrPowerShellLeadingParamBlock reports that the script opens with a param() +// block, which PowerShell requires to be the first statement. +var ErrPowerShellLeadingParamBlock = errors.New("PowerShell script starts with a param() block") + +// PosixQuote returns value as a single-quoted word. It is only safe inside the +// LC_ALL=C region Preamble builds: some libc decoders accept 0x27 as a +// multi-byte trail byte, letting a value ending on a lead byte consume its own +// closing quote. +func PosixQuote(value string) string { + return "'" + strings.ReplaceAll(value, "'", `'\''`) + "'" +} + +// PowerShellCharArray returns an expression evaluating to value, built from its +// UTF-16 code units. Nothing from value reaches the script as source text, so no +// quoting rule applies to it; PowerShell treats four Unicode code points besides +// ' as single-quote delimiters. It avoids method calls so it still evaluates +// under ConstrainedLanguage mode. +func PowerShellCharArray(value string) string { + if value == "" { + return "''" + } + units := utf16.Encode([]rune(value)) + var b strings.Builder + b.WriteString("([char[]](") + for i, u := range units { + if i > 0 { + b.WriteByte(',') + } + b.WriteString(strconv.FormatUint(uint64(u), 10)) + } + b.WriteString(") -join '')") + return b.String() +} + +// Preamble builds the assignments defining vars, keyed by name without the +// FLEET_VAR_ prefix. Output is ordered by name. +func Preamble(vars map[string]string, dialect Dialect) string { + if len(vars) == 0 { + return "" + } + names := make([]string, 0, len(vars)) + for name := range vars { + names = append(names, name) + } + sort.Strings(names) + + var b strings.Builder + if dialect == DialectPowerShell { + for _, name := range names { + b.WriteString("$FLEET_VAR_") + b.WriteString(name) + b.WriteString(" = ") + b.WriteString(PowerShellCharArray(vars[name])) + b.WriteString("\r\n") + } + return b.String() + } + + // see PosixQuote: the assignments must parse in a single-byte locale + b.WriteString("__fleet_lc=${LC_ALL-}; __fleet_lg=${LANG-}; LC_ALL=C; LANG=C\n") + b.WriteString("export") + for _, name := range names { + b.WriteString(" FLEET_VAR_") + b.WriteString(name) + b.WriteByte('=') + b.WriteString(PosixQuote(vars[name])) + } + b.WriteString("\nLC_ALL=${__fleet_lc}; LANG=${__fleet_lg}; unset __fleet_lc __fleet_lg\n") + return b.String() +} + +// InsertPreamble places preamble ahead of the body, keeping any shebang on line +// 1 so the interpreter is still chosen the same way. +func InsertPreamble(contents, preamble string, dialect Dialect) (string, error) { + if preamble == "" { + return contents, nil + } + if dialect == DialectPowerShell { + if hasLeadingParamBlock(contents) { + return "", ErrPowerShellLeadingParamBlock + } + return preamble + contents, nil + } + if !strings.HasPrefix(contents, "#!") { + return preamble + contents, nil + } + if i := strings.IndexByte(contents, '\n'); i >= 0 { + return contents[:i+1] + preamble + contents[i+1:], nil + } + return contents + "\n" + preamble, nil +} + +// hasLeadingParamBlock reports whether the first statement is a param() block. +// Only comments, #Requires and attributes may precede one. A block comment +// counts as a param block: finding its end needs a real tokenizer. +func hasLeadingParamBlock(contents string) bool { + rest := contents + for { + rest = strings.TrimLeft(rest, " \t\r\n") + switch { + case rest == "": + return false + case strings.HasPrefix(rest, "<#"): + return true + case strings.HasPrefix(rest, "#"): + i := strings.IndexByte(rest, '\n') + if i < 0 { + return false + } + rest = rest[i+1:] + case strings.HasPrefix(rest, "["): + // [CmdletBinding()] may precede param(); [Type]::Member is ordinary code + after, ok := skipBracketed(rest) + if !ok { + return true + } + rest = after + default: + return isParamKeyword(rest) + } + } +} + +func skipBracketed(s string) (string, bool) { + depth := 0 + for i, c := range s { + switch c { + case '[': + depth++ + case ']': + depth-- + if depth == 0 { + return s[i+1:], true + } + } + } + return "", false +} + +func isParamKeyword(s string) bool { + const kw = "param" + if len(s) < len(kw) || !strings.EqualFold(s[:len(kw)], kw) { + return false + } + after := s[len(kw):] + if after == "" { + return true + } + c := after[0] + return !(c == '_' || c == '-' || (c >= '0' && c <= '9') || (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z')) +} diff --git a/server/variables/preamble_exec_test.go b/server/variables/preamble_exec_test.go new file mode 100644 index 00000000000..ce80d14e9fd --- /dev/null +++ b/server/variables/preamble_exec_test.go @@ -0,0 +1,145 @@ +//go:build !windows + +package variables + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// Every case defines two variables, the corpus value first (names are emitted in +// sorted order) and a live payload second. A single-variable case can't detect +// the multi-byte break: it needs a following quote to swallow. +const execPayloadVar = "HOST_UUID" + +const valueVar = "HOST_HARDWARE_SERIAL" + +type execShape struct { + name string + // body must contain OUT, replaced with the round-trip target path + body string + // want transforms the input value into what the body should write + want func(value string) string +} + +func execShapes() []execShape { + self := func(v string) string { return v } + return []execShape{ + {"double-quoted", "#!/bin/sh\nprintf %s \"$FLEET_VAR_HOST_HARDWARE_SERIAL\" > OUT\n", self}, + {"braced", "#!/bin/sh\nprintf %s \"${FLEET_VAR_HOST_HARDWARE_SERIAL}\" > OUT\n", self}, + {"no-shebang", "printf %s \"$FLEET_VAR_HOST_HARDWARE_SERIAL\" > OUT\n", self}, + {"child-shell", "#!/bin/sh\nsh -c 'printf %s \"$FLEET_VAR_HOST_HARDWARE_SERIAL\"' > OUT\n", self}, + {"path-concat", "#!/bin/sh\nprintf %s \"/var/tmp/c/$FLEET_VAR_HOST_HARDWARE_SERIAL/x\" > OUT\n", + func(v string) string { return "/var/tmp/c/" + v + "/x" }}, + } +} + +// byteFaithfulLocale reports whether the shell reproduces a UTF-8 value +// byte-for-byte in this locale. In an EUC locale it does not, for any variable +// holding UTF-8, preamble or environment alike; that is the admin's locale +// acting on the admin's script, so only safety is asserted there. +func byteFaithfulLocale(locale string) bool { + return locale == "C" || strings.HasSuffix(locale, ".UTF-8") +} + +func availableLocales() []string { + locales := []string{"C"} + out, err := exec.Command("locale", "-a").Output() + if err != nil { + return locales + } + installed := make(map[string]struct{}) + for line := range strings.SplitSeq(string(out), "\n") { + installed[strings.TrimSpace(line)] = struct{}{} + } + // libc decoders for these accept 0x27 as a trail byte + for _, l := range []string{"en_US.UTF-8", "ja_JP.eucJP", "ko_KR.eucKR", "zh_CN.eucCN"} { + if _, ok := installed[l]; ok { + locales = append(locales, l) + } + } + return locales +} + +func availableShells() []string { + shells := []string{"/bin/sh"} + for _, s := range []string{"/bin/bash", "/bin/zsh"} { + if _, err := os.Stat(s); err == nil { + shells = append(shells, s) + } + } + return shells +} + +func runScript(t *testing.T, shell, script, locale string) { + t.Helper() + dir := t.TempDir() + path := filepath.Join(dir, "script") + require.NoError(t, os.WriteFile(path, []byte(script), 0o600)) + cmd := exec.Command(shell, path) + cmd.Env = append(os.Environ(), "LC_ALL="+locale, "LANG="+locale) + _ = cmd.Run() // a payload that fails to parse is still a pass +} + +func TestPreambleNeverExecutesValues(t *testing.T) { + shells := availableShells() + locales := availableLocales() + + for name, value := range payloadCorpus() { + t.Run(name, func(t *testing.T) { + for _, shape := range execShapes() { + for _, shell := range shells { + for _, locale := range locales { + dir := t.TempDir() + marker := filepath.Join(dir, "MARKER") + out := filepath.Join(dir, "out") + + vars := map[string]string{ + valueVar: value, + execPayloadVar: "x$(touch " + marker + ");#", + } + script, err := InsertPreamble( + strings.ReplaceAll(shape.body, "OUT", out), + Preamble(vars, DialectPOSIX), DialectPOSIX) + require.NoError(t, err) + + runScript(t, shell, script, locale) + + require.NoFileExists(t, marker, + "value executed: shape=%s shell=%s locale=%s", shape.name, shell, locale) + + if !byteFaithfulLocale(locale) { + continue + } + got, err := os.ReadFile(out) + require.NoError(t, err, "shape=%s shell=%s locale=%s", shape.name, shell, locale) + require.Equal(t, shape.want(value), string(got), + "value did not round-trip: shape=%s shell=%s locale=%s", shape.name, shell, locale) + } + } + } + }) + } +} + +// Keeps the test above honest: a harness that never detects execution would pass +// for the wrong reason. +func TestSubstitutingValuesIntoTheBodyExecutesThem(t *testing.T) { + dir := t.TempDir() + marker := filepath.Join(dir, "MARKER") + out := filepath.Join(dir, "out") + + body := strings.ReplaceAll(execShapes()[0].body, "OUT", out) + script := Replace(body, valueVar, "Eng`touch "+marker+"`") + + runScript(t, "/bin/sh", script, "C") + + require.FileExists(t, marker) + got, _ := os.ReadFile(out) + require.Equal(t, "Eng", string(got)) +} diff --git a/server/variables/preamble_test.go b/server/variables/preamble_test.go new file mode 100644 index 00000000000..0c42a078dd0 --- /dev/null +++ b/server/variables/preamble_test.go @@ -0,0 +1,194 @@ +package variables + +import ( + "strconv" + "strings" + "testing" + "unicode/utf16" + + "github.com/stretchr/testify/require" +) + +// payloadCorpus is shared with the execution tests. Every entry either executed +// under plain substitution or exercises a quoting edge. +func payloadCorpus() map[string]string { + return map[string]string{ //nolint:gosec // G101: injection payloads, not credentials + "plain": "Engineering", + "backtick": "Eng`id`", + "cmd-subst": "Eng$(id)", + "semicolon": "Eng; id", + "pipe": "Eng | id", + "embedded-sq": "Eng'; id; echo '", + "lone-sq": "'", + "only-sq": "''''", + "backslash": `Eng\'; id; #`, + "trailing-bs": `Eng\`, + "quote-soup": "Eng'\"`id`\"'", + "newline": "Eng\nid", + "crlf": "Eng\r\nid", + "tab-space": " Eng\tOps ", + "dollar-var": "$HOME and ${PATH}", + "fleet-secret": "$FLEET_SECRET_FOO", + "nested-var": "$FLEET_VAR_HOST_UUID", + "glob": "*", + "ifs": "a${IFS}b", + "non-ascii": "Ops — Zürich 🙂", + "smart-quotes": "Ann’s ‘Team’", + "cjk-lead-byte": "情報システム部", + "hangul": "정보시스템부", + "empty": "", + "long": strings.Repeat("A", 4096), + } +} + +func TestPosixQuote(t *testing.T) { + require.Equal(t, `'abc'`, PosixQuote("abc")) + require.Equal(t, `''`, PosixQuote("")) + require.Equal(t, `''\'''`, PosixQuote("'")) + require.Equal(t, `'Ann'\''s'`, PosixQuote("Ann's")) + + for name, value := range payloadCorpus() { + t.Run(name, func(t *testing.T) { + got := PosixQuote(value) + require.True(t, strings.HasPrefix(got, "'")) + require.True(t, strings.HasSuffix(got, "'")) + // the outer pair, plus three quotes per '\'' escape sequence + require.Equal(t, 2+3*strings.Count(value, "'"), strings.Count(got, "'")) + }) + } +} + +func TestPowerShellCharArray(t *testing.T) { + require.Equal(t, "''", PowerShellCharArray("")) + require.Equal(t, "([char[]](65,66) -join '')", PowerShellCharArray("AB")) + + for name, value := range payloadCorpus() { + t.Run(name, func(t *testing.T) { + got := PowerShellCharArray(value) + + // no byte of the value reaches the script as source text + for _, r := range got { + require.Contains(t, "0123456789,()[] -join'char", string(r), + "unexpected character %q in %q", r, got) + } + + if value == "" { + return + } + inner := strings.TrimSuffix(strings.TrimPrefix(got, "([char[]]("), ") -join '')") + var units []uint16 + for f := range strings.SplitSeq(inner, ",") { + u, err := strconv.ParseUint(f, 10, 16) + require.NoError(t, err) + units = append(units, uint16(u)) + } + require.Equal(t, value, string(utf16.Decode(units))) + }) + } +} + +func TestPreamble(t *testing.T) { + t.Run("empty", func(t *testing.T) { + require.Empty(t, Preamble(nil, DialectPOSIX)) + require.Empty(t, Preamble(map[string]string{}, DialectPowerShell)) + }) + + t.Run("posix is sorted and locale-guarded", func(t *testing.T) { + got := Preamble(map[string]string{"HOST_UUID": "u", "HOST_HARDWARE_SERIAL": "s"}, DialectPOSIX) + require.Equal(t, + "__fleet_lc=${LC_ALL-}; __fleet_lg=${LANG-}; LC_ALL=C; LANG=C\n"+ + "export FLEET_VAR_HOST_HARDWARE_SERIAL='s' FLEET_VAR_HOST_UUID='u'\n"+ + "LC_ALL=${__fleet_lc}; LANG=${__fleet_lg}; unset __fleet_lc __fleet_lg\n", got) + }) + + // a missing guard is invisible on a UTF-8-only machine, so assert on the + // emitted text as well as executing it + t.Run("posix always emits the locale guard", func(t *testing.T) { + for name, value := range payloadCorpus() { + got := Preamble(map[string]string{"HOST_UUID": value}, DialectPOSIX) + require.Contains(t, got, "LC_ALL=C; LANG=C", name) + require.Contains(t, got, "LC_ALL=${__fleet_lc}", name) + } + }) + + t.Run("powershell is one CRLF line per variable", func(t *testing.T) { + got := Preamble(map[string]string{"HOST_UUID": "AB", "HOST_PLATFORM": "w"}, DialectPowerShell) + require.Equal(t, + "$FLEET_VAR_HOST_PLATFORM = ([char[]](119) -join '')\r\n"+ + "$FLEET_VAR_HOST_UUID = ([char[]](65,66) -join '')\r\n", got) + }) +} + +func TestInsertPreamble(t *testing.T) { + const pre = "PRE\n" + + for _, tc := range []struct { + name string + contents string + want string + }{ + {"no shebang", "echo hi\n", "PRE\necho hi\n"}, + {"sh shebang", "#!/bin/sh\necho hi\n", "#!/bin/sh\nPRE\necho hi\n"}, + {"env bash shebang", "#!/usr/bin/env bash\necho hi\n", "#!/usr/bin/env bash\nPRE\necho hi\n"}, + {"shebang with args", "#!/bin/sh -e\necho hi\n", "#!/bin/sh -e\nPRE\necho hi\n"}, + {"crlf", "#!/bin/sh\r\necho hi\r\n", "#!/bin/sh\r\nPRE\necho hi\r\n"}, + {"shebang only, no newline", "#!/bin/sh", "#!/bin/sh\nPRE\n"}, + {"comment first line", "# not a shebang\necho hi\n", "PRE\n# not a shebang\necho hi\n"}, + {"empty", "", "PRE\n"}, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := InsertPreamble(tc.contents, pre, DialectPOSIX) + require.NoError(t, err) + require.Equal(t, tc.want, got) + }) + } + + t.Run("empty preamble is a no-op", func(t *testing.T) { + got, err := InsertPreamble("#!/bin/sh\necho hi\n", "", DialectPOSIX) + require.NoError(t, err) + require.Equal(t, "#!/bin/sh\necho hi\n", got) + }) + + t.Run("powershell prepends", func(t *testing.T) { + got, err := InsertPreamble("Write-Output hi\r\n", pre, DialectPowerShell) + require.NoError(t, err) + require.Equal(t, "PRE\nWrite-Output hi\r\n", got) + }) +} + +// A preamble above a param() block is a parse error, not a working script. +func TestInsertPreamblePowerShellParamBlock(t *testing.T) { + for _, tc := range []struct { + name string + contents string + refused bool + }{ + {"leading param", "param($Foo = \"bar\")\r\nWrite-Output hi\r\n", true}, + {"leading param, no space", "param($Foo)\r\n", true}, + {"uppercase param", "PARAM($Foo)\r\n", true}, + {"attribute then param", "[CmdletBinding()]\r\nparam($Foo)\r\n", true}, + {"comments above param", "# c\r\n#Requires -Version 5\r\nparam($Foo)\r\n", true}, + {"blank lines above param", "\r\n\r\nparam($Foo)\r\n", true}, + {"block comment", "<#\r\ndoc\r\n#>\r\nWrite-Output hi\r\n", true}, + {"multiple attributes then param", "[CmdletBinding()]\r\n[OutputType([int])]\r\nparam($Foo)\r\n", true}, + {"unterminated attribute", "[CmdletBinding(\r\n", true}, + {"param inside function", "function F { param($a) $a }\r\nWrite-Output hi\r\n", false}, + {"type accelerator statement", "[Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12\r\nWrite-Output hi\r\n", false}, + {"array literal", "[int[]]$a = 1,2\r\nWrite-Output hi\r\n", false}, + {"parameter-like name", "paramX($Foo)\r\n", false}, + {"requires only", "#Requires -Version 5\r\nWrite-Output hi\r\n", false}, + {"ordinary script", "Write-Output hi\r\n", false}, + {"comment only", "# nothing here\r\n", false}, + {"empty", "", false}, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := InsertPreamble(tc.contents, "PRE\r\n", DialectPowerShell) + if tc.refused { + require.ErrorIs(t, err, ErrPowerShellLeadingParamBlock) + return + } + require.NoError(t, err) + require.Equal(t, "PRE\r\n"+tc.contents, got) + }) + } +} diff --git a/server/variables/variables.go b/server/variables/variables.go index c4e1a3a24ad..d4c15cf0c27 100644 --- a/server/variables/variables.go +++ b/server/variables/variables.go @@ -93,6 +93,10 @@ func Dedupe(varsWithDupes []string) []string { // // For example, Replace(content, "HOST_UUID", "123-456") will replace both // $FLEET_VAR_HOST_UUID and ${FLEET_VAR_HOST_UUID} with "123-456". +// +// It performs no escaping, and must never be used on content an interpreter will +// execute: the quoting context at the substitution point is unknown, so no +// escaping is correct there. Use Preamble instead. func Replace(contents string, variableName string, value string) string { // Replace both braced and non-braced versions result := strings.ReplaceAll(contents, "$FLEET_VAR_"+variableName, value) From 352df54a754f597da7b24d64eaab1f2cc2deaa15 Mon Sep 17 00:00:00 2001 From: Carlo DiCelico Date: Fri, 4 Sep 2026 15:57:10 -0400 Subject: [PATCH 2/7] fix powershell preamble --- server/variables/preamble.go | 95 ++++++++++++++++++++----------- server/variables/preamble_test.go | 76 ++++++++++++++++--------- 2 files changed, 113 insertions(+), 58 deletions(-) diff --git a/server/variables/preamble.go b/server/variables/preamble.go index 3f33f5ed3b3..4e5c9fad436 100644 --- a/server/variables/preamble.go +++ b/server/variables/preamble.go @@ -20,19 +20,19 @@ const ( // block, which PowerShell requires to be the first statement. var ErrPowerShellLeadingParamBlock = errors.New("PowerShell script starts with a param() block") +const utf8BOM = "\ufeff" + // PosixQuote returns value as a single-quoted word. It is only safe inside the -// LC_ALL=C region Preamble builds: some libc decoders accept 0x27 as a -// multi-byte trail byte, letting a value ending on a lead byte consume its own -// closing quote. +// LC_ALL=C region Preamble builds: some libc decoders accept 0x27 as a multi-byte +// trail byte, letting a value ending on a lead byte consume its closing quote. func PosixQuote(value string) string { return "'" + strings.ReplaceAll(value, "'", `'\''`) + "'" } // PowerShellCharArray returns an expression evaluating to value, built from its -// UTF-16 code units. Nothing from value reaches the script as source text, so no -// quoting rule applies to it; PowerShell treats four Unicode code points besides -// ' as single-quote delimiters. It avoids method calls so it still evaluates -// under ConstrainedLanguage mode. +// UTF-16 code units. Nothing from value reaches the script as source text, so the +// four Unicode code points PowerShell also treats as single quotes can't break +// out. It avoids method calls so it still evaluates under ConstrainedLanguage. func PowerShellCharArray(value string) string { if value == "" { return "''" @@ -94,10 +94,14 @@ func InsertPreamble(contents, preamble string, dialect Dialect) (string, error) return contents, nil } if dialect == DialectPowerShell { - if hasLeadingParamBlock(contents) { - return "", ErrPowerShellLeadingParamBlock + pos, err := powerShellPreamblePos(contents) + if err != nil { + return "", err } - return preamble + contents, nil + if before := strings.TrimPrefix(contents[:pos], utf8BOM); before != "" && !strings.HasSuffix(before, "\n") { + preamble = "\r\n" + preamble + } + return contents[:pos] + preamble + contents[pos:], nil } if !strings.HasPrefix(contents, "#!") { return preamble + contents, nil @@ -108,33 +112,60 @@ func InsertPreamble(contents, preamble string, dialect Dialect) (string, error) return contents + "\n" + preamble, nil } -// hasLeadingParamBlock reports whether the first statement is a param() block. -// Only comments, #Requires and attributes may precede one. A block comment -// counts as a param block: finding its end needs a real tokenizer. -func hasLeadingParamBlock(contents string) bool { - rest := contents +// powerShellPreamblePos returns the offset to insert a preamble at: after a byte +// order mark, which breaks the script unless it stays on line 1, and after any +// using statements, which must precede every other statement. It fails on a +// leading param() block, which must come first and so leaves nowhere to insert. +func powerShellPreamblePos(contents string) (int, error) { + pos := 0 + if strings.HasPrefix(contents, utf8BOM) { + pos = len(utf8BOM) + } + insert := pos + for { - rest = strings.TrimLeft(rest, " \t\r\n") + trimmed := strings.TrimLeft(contents[pos:], " \t\r\n") + pos = len(contents) - len(trimmed) switch { - case rest == "": - return false - case strings.HasPrefix(rest, "<#"): - return true - case strings.HasPrefix(rest, "#"): - i := strings.IndexByte(rest, '\n') - if i < 0 { - return false + case trimmed == "": + return insert, nil + + case strings.HasPrefix(trimmed, "<#"): + // block comments do not nest, and PowerShell rejects an unterminated one + end := strings.Index(trimmed, "#>") + if end < 0 { + return insert, nil } - rest = rest[i+1:] - case strings.HasPrefix(rest, "["): + pos += end + len("#>") + + case strings.HasPrefix(trimmed, "#"): + nl := strings.IndexByte(trimmed, '\n') + if nl < 0 { + return insert, nil + } + pos += nl + 1 + + case hasKeyword(trimmed, "using"): + nl := strings.IndexByte(trimmed, '\n') + if nl < 0 { + return len(contents), nil + } + pos += nl + 1 + insert = pos + + case strings.HasPrefix(trimmed, "["): // [CmdletBinding()] may precede param(); [Type]::Member is ordinary code - after, ok := skipBracketed(rest) + after, ok := skipBracketed(trimmed) if !ok { - return true + return insert, nil } - rest = after + pos = len(contents) - len(after) + default: - return isParamKeyword(rest) + if hasKeyword(trimmed, "param") { + return 0, ErrPowerShellLeadingParamBlock + } + return insert, nil } } } @@ -155,8 +186,8 @@ func skipBracketed(s string) (string, bool) { return "", false } -func isParamKeyword(s string) bool { - const kw = "param" +// hasKeyword reports whether s opens with kw as a complete word. +func hasKeyword(s, kw string) bool { if len(s) < len(kw) || !strings.EqualFold(s[:len(kw)], kw) { return false } diff --git a/server/variables/preamble_test.go b/server/variables/preamble_test.go index 0c42a078dd0..2bb32aaad65 100644 --- a/server/variables/preamble_test.go +++ b/server/variables/preamble_test.go @@ -9,7 +9,7 @@ import ( "github.com/stretchr/testify/require" ) -// payloadCorpus is shared with the execution tests. Every entry either executed +// payloadCorpus is shared with the execution tests: each entry either executed // under plain substitution or exercises a quoting edge. func payloadCorpus() map[string]string { return map[string]string{ //nolint:gosec // G101: injection payloads, not credentials @@ -101,8 +101,7 @@ func TestPreamble(t *testing.T) { "LC_ALL=${__fleet_lc}; LANG=${__fleet_lg}; unset __fleet_lc __fleet_lg\n", got) }) - // a missing guard is invisible on a UTF-8-only machine, so assert on the - // emitted text as well as executing it + // a missing guard is invisible on a UTF-8-only machine t.Run("posix always emits the locale guard", func(t *testing.T) { for name, value := range payloadCorpus() { got := Preamble(map[string]string{"HOST_UUID": value}, DialectPOSIX) @@ -156,39 +155,64 @@ func TestInsertPreamble(t *testing.T) { }) } -// A preamble above a param() block is a parse error, not a working script. -func TestInsertPreamblePowerShellParamBlock(t *testing.T) { +// A preamble above a param() block, a BOM, or a using statement is a parse error. +func TestInsertPreamblePowerShellPlacement(t *testing.T) { + const pre = "PRE\r\n" + const bom = "\ufeff" + for _, tc := range []struct { name string contents string - refused bool + want string // "" means the insert must be refused }{ - {"leading param", "param($Foo = \"bar\")\r\nWrite-Output hi\r\n", true}, - {"leading param, no space", "param($Foo)\r\n", true}, - {"uppercase param", "PARAM($Foo)\r\n", true}, - {"attribute then param", "[CmdletBinding()]\r\nparam($Foo)\r\n", true}, - {"comments above param", "# c\r\n#Requires -Version 5\r\nparam($Foo)\r\n", true}, - {"blank lines above param", "\r\n\r\nparam($Foo)\r\n", true}, - {"block comment", "<#\r\ndoc\r\n#>\r\nWrite-Output hi\r\n", true}, - {"multiple attributes then param", "[CmdletBinding()]\r\n[OutputType([int])]\r\nparam($Foo)\r\n", true}, - {"unterminated attribute", "[CmdletBinding(\r\n", true}, - {"param inside function", "function F { param($a) $a }\r\nWrite-Output hi\r\n", false}, - {"type accelerator statement", "[Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12\r\nWrite-Output hi\r\n", false}, - {"array literal", "[int[]]$a = 1,2\r\nWrite-Output hi\r\n", false}, - {"parameter-like name", "paramX($Foo)\r\n", false}, - {"requires only", "#Requires -Version 5\r\nWrite-Output hi\r\n", false}, - {"ordinary script", "Write-Output hi\r\n", false}, - {"comment only", "# nothing here\r\n", false}, - {"empty", "", false}, + {"leading param", "param($Foo)\r\n", ""}, + {"uppercase param", "PARAM($Foo)\r\n", ""}, + {"attribute then param", "[CmdletBinding()]\r\nparam($Foo)\r\n", ""}, + {"two attributes then param", "[CmdletBinding()]\r\n[OutputType([int])]\r\nparam($Foo)\r\n", ""}, + {"comments above param", "# c\r\n#Requires -Version 5\r\nparam($Foo)\r\n", ""}, + {"help block above param", "<#\r\n.SYNOPSIS\r\n#>\r\nparam($Foo)\r\n", ""}, + {"using above param", "using namespace System.Text\r\nparam($Foo)\r\n", ""}, + {"bom above param", bom + "param($Foo)\r\n", ""}, + + {"ordinary script", "Write-Output hi\r\n", pre + "Write-Output hi\r\n"}, + {"empty", "", pre}, + {"comment only", "# nothing\r\n", pre + "# nothing\r\n"}, + {"requires only", "#Requires -Version 5\r\nWrite-Output hi\r\n", + pre + "#Requires -Version 5\r\nWrite-Output hi\r\n"}, + {"help block, no param", "<#\r\n.SYNOPSIS\r\n#>\r\nWrite-Output hi\r\n", + pre + "<#\r\n.SYNOPSIS\r\n#>\r\nWrite-Output hi\r\n"}, + {"unterminated help block", "<#\r\nWrite-Output hi\r\n", pre + "<#\r\nWrite-Output hi\r\n"}, + {"param inside function", "function F { param($a) $a }\r\n", pre + "function F { param($a) $a }\r\n"}, + {"parameter-like name", "paramX($Foo)\r\n", pre + "paramX($Foo)\r\n"}, + {"type accelerator", "[Net.ServicePointManager]::SecurityProtocol = 1\r\n", + pre + "[Net.ServicePointManager]::SecurityProtocol = 1\r\n"}, + {"array literal", "[int[]]$a = 1,2\r\n", pre + "[int[]]$a = 1,2\r\n"}, + + // moving the BOM off line 1 breaks the script + {"bom", bom + "Write-Output hi\r\n", bom + pre + "Write-Output hi\r\n"}, + {"bom and comment", bom + "# c\r\nWrite-Output hi\r\n", bom + pre + "# c\r\nWrite-Output hi\r\n"}, + + // using statements must precede every other statement + {"using", "using namespace System.Text\r\nWrite-Output hi\r\n", + "using namespace System.Text\r\n" + pre + "Write-Output hi\r\n"}, + {"two using", "using namespace System.Text\r\nusing namespace System.IO\r\nWrite-Output hi\r\n", + "using namespace System.Text\r\nusing namespace System.IO\r\n" + pre + "Write-Output hi\r\n"}, + {"comment then using", "# c\r\nusing namespace System.Text\r\nWrite-Output hi\r\n", + "# c\r\nusing namespace System.Text\r\n" + pre + "Write-Output hi\r\n"}, + {"bom and using", bom + "using namespace System.Text\r\nWrite-Output hi\r\n", + bom + "using namespace System.Text\r\n" + pre + "Write-Output hi\r\n"}, + {"using without trailing newline", "using namespace System.Text", + "using namespace System.Text\r\n" + pre}, + {"using-like name", "usingX 1\r\n", pre + "usingX 1\r\n"}, } { t.Run(tc.name, func(t *testing.T) { - got, err := InsertPreamble(tc.contents, "PRE\r\n", DialectPowerShell) - if tc.refused { + got, err := InsertPreamble(tc.contents, pre, DialectPowerShell) + if tc.want == "" { require.ErrorIs(t, err, ErrPowerShellLeadingParamBlock) return } require.NoError(t, err) - require.Equal(t, "PRE\r\n"+tc.contents, got) + require.Equal(t, tc.want, got) }) } } From ffd526bbe6c777217192dbb5175c1b8e909e1c55 Mon Sep 17 00:00:00 2001 From: Carlo DiCelico Date: Fri, 4 Sep 2026 16:05:49 -0400 Subject: [PATCH 3/7] update changes file --- changes/script-variable-interpreter-escaping | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/changes/script-variable-interpreter-escaping b/changes/script-variable-interpreter-escaping index 4bc2c240bc3..32575c14061 100644 --- a/changes/script-variable-interpreter-escaping +++ b/changes/script-variable-interpreter-escaping @@ -1 +1,3 @@ -- Fixed an issue where a host's resolved Fleet variables (for example IdP attributes) could reach a script's contents without being escaped for the target interpreter. Values are now defined at the top of the script, so characters that are meaningful to the interpreter are treated as literal text. A `$FLEET_VAR_*` reference inside single quotes or a quoted heredoc is no longer substituted, and Fleet variables are no longer supported in Python scripts. +- Fixed an issue where characters in a Fleet variable's value could change what a script does. Values are now defined at the top of the script and read as literal text. +- Fleet variables in scripts are no longer substituted inside single quotes or a quoted heredoc. +- Fleet variables are no longer supported in Python scripts, or in PowerShell scripts that start with a `param()` block. From 786bb88a80b733142652e9cabd3f3949a951df13 Mon Sep 17 00:00:00 2001 From: Carlo DiCelico Date: Fri, 4 Sep 2026 16:40:25 -0400 Subject: [PATCH 4/7] place powershell preamble after param block --- changes/script-variable-interpreter-escaping | 2 +- orbit/pkg/scripts/exec_nonwindows_test.go | 12 ++- server/service/script_variables.go | 5 +- server/service/script_variables_test.go | 17 ++- server/variables/preamble.go | 106 +++++++++++++++++-- server/variables/preamble_test.go | 34 ++++-- 6 files changed, 149 insertions(+), 27 deletions(-) diff --git a/changes/script-variable-interpreter-escaping b/changes/script-variable-interpreter-escaping index 32575c14061..6d4aff658ae 100644 --- a/changes/script-variable-interpreter-escaping +++ b/changes/script-variable-interpreter-escaping @@ -1,3 +1,3 @@ - Fixed an issue where characters in a Fleet variable's value could change what a script does. Values are now defined at the top of the script and read as literal text. - Fleet variables in scripts are no longer substituted inside single quotes or a quoted heredoc. -- Fleet variables are no longer supported in Python scripts, or in PowerShell scripts that start with a `param()` block. +- Fleet variables are no longer supported in Python scripts. diff --git a/orbit/pkg/scripts/exec_nonwindows_test.go b/orbit/pkg/scripts/exec_nonwindows_test.go index 2d7ec779975..47468734043 100644 --- a/orbit/pkg/scripts/exec_nonwindows_test.go +++ b/orbit/pkg/scripts/exec_nonwindows_test.go @@ -172,8 +172,9 @@ func TestExecCmdDoesNotExecuteFleetVariableValues(t *testing.T) { name string value string contents string - // the shebang case never reaches the body, so only safety is checked - skipOutput bool + // macOS splits shebang arguments and Linux doesn't, so the shebang case + // only gets the safety assertion + safetyOnly bool }{ {"backtick", "Eng`touch " + marker + "`", "#!/bin/sh\nprintf %s \"$FLEET_VAR_HOST_UUID\"\n", false}, {"cmd-subst", "Eng$(touch " + marker + ")", "#!/bin/sh\nprintf %s \"$FLEET_VAR_HOST_UUID\"\n", false}, @@ -192,11 +193,12 @@ func TestExecCmdDoesNotExecuteFleetVariableValues(t *testing.T) { require.NoError(t, os.WriteFile(path, []byte(script), 0o600)) output, _, err := ExecCmd(context.Background(), path, nil) - require.NoError(t, err) require.NoFileExists(t, marker) - if !tc.skipOutput { - require.Equal(t, tc.value, string(output)) + if tc.safetyOnly { + return } + require.NoError(t, err) + require.Equal(t, tc.value, string(output)) }) } } diff --git a/server/service/script_variables.go b/server/service/script_variables.go index baf91949e61..f8d232ab122 100644 --- a/server/service/script_variables.go +++ b/server/service/script_variables.go @@ -15,9 +15,8 @@ import ( ) const ( - pythonFleetVarsMsg = "Fleet couldn't run this script because Fleet variables aren't supported in Python scripts. Use a shell script, or remove the variable." - // PowerShell requires a param() block to be the first statement. - powerShellParamBlockMsg = "Fleet couldn't run this script because Fleet variables aren't supported in a PowerShell script that starts with a param() block. Move the param() block, or remove the variable." + pythonFleetVarsMsg = "Fleet couldn't run this script because Fleet variables aren't supported in Python scripts. Use a shell script, or remove the variable." + powerShellParamBlockMsg = "Fleet couldn't run this script because Fleet variables aren't supported inside a PowerShell param() block. Use the variable in the script body instead." unsupportedInterpMsg = "Fleet couldn't run this script because its interpreter isn't supported." noPlatformMsg = "There is no platform for this host. Fleet couldn't populate Fleet variables." ) diff --git a/server/service/script_variables_test.go b/server/service/script_variables_test.go index d8f7c1bc3c1..8b1678d1a50 100644 --- a/server/service/script_variables_test.go +++ b/server/service/script_variables_test.go @@ -169,12 +169,25 @@ func TestMaybeExpandScriptFleetVariables(t *testing.T) { require.NotContains(t, body, "ABC-123") }) - t.Run("PowerShell param block fails instead of breaking the script", func(t *testing.T) { + t.Run("PowerShell param block keeps its place", func(t *testing.T) { + svc, ctx, _ := newSvcAndCtx(fleet.TierPremium) + h := *host + h.Platform = "windows" + const body = "param($Foo = \"bar\")\r\nWrite-Output $FLEET_VAR_HOST_UUID\r\n" + expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, &h, body) + require.NoError(t, err) + require.Empty(t, failMsg) + require.Equal(t, "param($Foo = \"bar\")\r\n"+ + "$FLEET_VAR_HOST_UUID = "+variables.PowerShellCharArray("ABC-123")+"\r\n"+ + "Write-Output $FLEET_VAR_HOST_UUID\r\n", expanded) + }) + + t.Run("a variable in a param default fails", func(t *testing.T) { svc, ctx, _ := newSvcAndCtx(fleet.TierPremium) h := *host h.Platform = "windows" expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, &h, - "param($Foo = \"bar\")\r\nWrite-Output $FLEET_VAR_HOST_UUID\r\n") + "param($Foo = $FLEET_VAR_HOST_UUID)\r\nWrite-Output $Foo\r\n") require.NoError(t, err) require.Empty(t, expanded) require.Equal(t, powerShellParamBlockMsg, failMsg) diff --git a/server/variables/preamble.go b/server/variables/preamble.go index 4e5c9fad436..8bbf5761168 100644 --- a/server/variables/preamble.go +++ b/server/variables/preamble.go @@ -16,8 +16,8 @@ const ( DialectPowerShell ) -// ErrPowerShellLeadingParamBlock reports that the script opens with a param() -// block, which PowerShell requires to be the first statement. +// ErrPowerShellLeadingParamBlock reports that a preamble can't be placed around +// the script's leading param() block. var ErrPowerShellLeadingParamBlock = errors.New("PowerShell script starts with a param() block") const utf8BOM = "\ufeff" @@ -112,10 +112,9 @@ func InsertPreamble(contents, preamble string, dialect Dialect) (string, error) return contents + "\n" + preamble, nil } -// powerShellPreamblePos returns the offset to insert a preamble at: after a byte -// order mark, which breaks the script unless it stays on line 1, and after any -// using statements, which must precede every other statement. It fails on a -// leading param() block, which must come first and so leaves nowhere to insert. +// powerShellPreamblePos returns the offset to insert a preamble at. PowerShell +// wants a byte order mark, using statements, and a param() block each to come +// first, so the preamble goes after all of them. func powerShellPreamblePos(contents string) (int, error) { pos := 0 if strings.HasPrefix(contents, utf8BOM) { @@ -163,13 +162,106 @@ func powerShellPreamblePos(contents string) (int, error) { default: if hasKeyword(trimmed, "param") { - return 0, ErrPowerShellLeadingParamBlock + n, ok := skipParamBlock(trimmed) + // a default can't use a variable the preamble defines below it + if !ok || strings.Contains(trimmed[:n], "$FLEET_VAR_") { + return 0, ErrPowerShellLeadingParamBlock + } + return startOfNextLine(contents, pos+n), nil } return insert, nil } } } +// startOfNextLine advances past the rest of the line at i. +func startOfNextLine(s string, i int) int { + for i < len(s) && (s[i] == ' ' || s[i] == '\t' || s[i] == '\r') { + i++ + } + if i < len(s) && s[i] == '\n' { + i++ + } + return i +} + +// skipParamBlock returns the offset past a param(...) block, reporting false +// when it never closes. +func skipParamBlock(s string) (int, bool) { + i := strings.IndexByte(s, '(') + if i < 0 { + return 0, false + } + depth := 0 + for i < len(s) { + switch s[i] { + case '(': + depth++ + case ')': + depth-- + if depth == 0 { + return i + 1, true + } + case '\'': + i = skipSingleQuoted(s, i) + continue + case '"': + i = skipDoubleQuoted(s, i) + continue + case '<': + if i+1 < len(s) && s[i+1] == '#' { + end := strings.Index(s[i:], "#>") + if end < 0 { + return 0, false + } + i += end + len("#>") + continue + } + case '#': + nl := strings.IndexByte(s[i:], '\n') + if nl < 0 { + return 0, false + } + i += nl + 1 + continue + } + i++ + } + return 0, false +} + +// skipSingleQuoted returns the offset past a '...' string; '' is a quote. +func skipSingleQuoted(s string, i int) int { + for i++; i < len(s); i++ { + if s[i] != '\'' { + continue + } + if i+1 < len(s) && s[i+1] == '\'' { + i++ + continue + } + return i + 1 + } + return len(s) +} + +// skipDoubleQuoted returns the offset past a "..." string; a backtick escapes. +func skipDoubleQuoted(s string, i int) int { + for i++; i < len(s); i++ { + switch s[i] { + case '`': + i++ + case '"': + if i+1 < len(s) && s[i+1] == '"' { + i++ + continue + } + return i + 1 + } + } + return len(s) +} + func skipBracketed(s string) (string, bool) { depth := 0 for i, c := range s { diff --git a/server/variables/preamble_test.go b/server/variables/preamble_test.go index 2bb32aaad65..8fcb2a93b6c 100644 --- a/server/variables/preamble_test.go +++ b/server/variables/preamble_test.go @@ -155,7 +155,7 @@ func TestInsertPreamble(t *testing.T) { }) } -// A preamble above a param() block, a BOM, or a using statement is a parse error. +// A preamble above a BOM, a using statement, or a param() block is a parse error. func TestInsertPreamblePowerShellPlacement(t *testing.T) { const pre = "PRE\r\n" const bom = "\ufeff" @@ -165,14 +165,30 @@ func TestInsertPreamblePowerShellPlacement(t *testing.T) { contents string want string // "" means the insert must be refused }{ - {"leading param", "param($Foo)\r\n", ""}, - {"uppercase param", "PARAM($Foo)\r\n", ""}, - {"attribute then param", "[CmdletBinding()]\r\nparam($Foo)\r\n", ""}, - {"two attributes then param", "[CmdletBinding()]\r\n[OutputType([int])]\r\nparam($Foo)\r\n", ""}, - {"comments above param", "# c\r\n#Requires -Version 5\r\nparam($Foo)\r\n", ""}, - {"help block above param", "<#\r\n.SYNOPSIS\r\n#>\r\nparam($Foo)\r\n", ""}, - {"using above param", "using namespace System.Text\r\nparam($Foo)\r\n", ""}, - {"bom above param", bom + "param($Foo)\r\n", ""}, + // a param() block has to stay first, so the preamble goes after it + {"leading param", "param($Foo)\r\nWrite-Output hi\r\n", + "param($Foo)\r\n" + pre + "Write-Output hi\r\n"}, + {"uppercase param", "PARAM($Foo)\r\n", "PARAM($Foo)\r\n" + pre}, + {"attribute then param", "[CmdletBinding()]\r\nparam($Foo)\r\n", + "[CmdletBinding()]\r\nparam($Foo)\r\n" + pre}, + {"two attributes then param", "[CmdletBinding()]\r\n[OutputType([int])]\r\nparam($Foo)\r\n", + "[CmdletBinding()]\r\n[OutputType([int])]\r\nparam($Foo)\r\n" + pre}, + {"comments above param", "# c\r\n#Requires -Version 5\r\nparam($Foo)\r\n", + "# c\r\n#Requires -Version 5\r\nparam($Foo)\r\n" + pre}, + {"help block above param", "<#\r\n.SYNOPSIS\r\n#>\r\nparam($Foo)\r\n", + "<#\r\n.SYNOPSIS\r\n#>\r\nparam($Foo)\r\n" + pre}, + {"using above param", "using namespace System.Text\r\nparam($Foo)\r\n", + "using namespace System.Text\r\nparam($Foo)\r\n" + pre}, + {"bom above param", bom + "param($Foo)\r\n", bom + "param($Foo)\r\n" + pre}, + {"multiline param", "param(\r\n [string]$Foo = \"a)b\",\r\n [int]$N = 2\r\n)\r\nWrite-Output hi\r\n", + "param(\r\n [string]$Foo = \"a)b\",\r\n [int]$N = 2\r\n)\r\n" + pre + "Write-Output hi\r\n"}, + {"param with quoted paren", "param($Foo = 'a)b')\r\n", "param($Foo = 'a)b')\r\n" + pre}, + {"param with comment", "param(\r\n # note\r\n $Foo\r\n)\r\n", "param(\r\n # note\r\n $Foo\r\n)\r\n" + pre}, + {"param with block comment", "param(<# note #>$Foo)\r\n", "param(<# note #>$Foo)\r\n" + pre}, + + // a default can't reference a variable the preamble defines below it + {"param default uses a variable", "param($Foo = $FLEET_VAR_HOST_UUID)\r\n", ""}, + {"unterminated param", "param($Foo\r\nWrite-Output hi\r\n", ""}, {"ordinary script", "Write-Output hi\r\n", pre + "Write-Output hi\r\n"}, {"empty", "", pre}, From 162397104ecea900107e1bef7c0dd52becc01684 Mon Sep 17 00:00:00 2001 From: Carlo DiCelico Date: Fri, 4 Sep 2026 16:55:46 -0400 Subject: [PATCH 5/7] pin interpreter choice, fix comment formatting --- orbit/pkg/scripts/exec_nonwindows_test.go | 31 +++++++++++++++++++++++ server/variables/preamble.go | 3 ++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/orbit/pkg/scripts/exec_nonwindows_test.go b/orbit/pkg/scripts/exec_nonwindows_test.go index 47468734043..a4190a6a64d 100644 --- a/orbit/pkg/scripts/exec_nonwindows_test.go +++ b/orbit/pkg/scripts/exec_nonwindows_test.go @@ -202,3 +202,34 @@ func TestExecCmdDoesNotExecuteFleetVariableValues(t *testing.T) { }) } } + +// An older agent decides how to run a script from its first line, so the +// preamble has to leave that decision unchanged. +func TestPreambleDoesNotChangeInterpreterChoice(t *testing.T) { + pre := variables.Preamble(map[string]string{"HOST_UUID": "ABC"}, variables.DialectPOSIX) + + for _, contents := range []string{ + "echo hi\n", + "#!/bin/sh\necho hi\n", + "#!/bin/sh -e\necho hi\n", + "#!/bin/bash\necho hi\n", + "#!/bin/zsh\necho hi\n", + "#!/usr/bin/env bash\necho hi\n", + "#!/bin/sh\r\necho hi\r\n", + "# not a shebang\necho hi\n", + } { + t.Run(contents, func(t *testing.T) { + withPreamble, err := variables.InsertPreamble(contents, pre, variables.DialectPOSIX) + require.NoError(t, err) + + wantDirect, wantErr := fleet.ValidateShebang(contents) + gotDirect, gotErr := fleet.ValidateShebang(withPreamble) + require.Equal(t, wantErr, gotErr) + require.Equal(t, wantDirect, gotDirect) + + wantKind, _, _ := fleet.ShebangInfo(contents) + gotKind, _, _ := fleet.ShebangInfo(withPreamble) + require.Equal(t, wantKind, gotKind) + }) + } +} diff --git a/server/variables/preamble.go b/server/variables/preamble.go index 8bbf5761168..2b4fd4d1252 100644 --- a/server/variables/preamble.go +++ b/server/variables/preamble.go @@ -230,7 +230,8 @@ func skipParamBlock(s string) (int, bool) { return 0, false } -// skipSingleQuoted returns the offset past a '...' string; '' is a quote. +// skipSingleQuoted returns the offset past a '...' string, where a doubled +// quote is a literal one. func skipSingleQuoted(s string, i int) int { for i++; i < len(s); i++ { if s[i] != '\'' { From 3f23dc7131a73ea11e5dafb36d22d6fce0358039 Mon Sep 17 00:00:00 2001 From: Carlo DiCelico Date: Fri, 4 Sep 2026 18:51:22 -0400 Subject: [PATCH 6/7] escape fleet variables for python --- changes/script-variable-interpreter-escaping | 6 +- ee/server/service/software_installers.go | 21 ++--- ee/server/service/software_installers_test.go | 11 --- server/fleet/script_variables.go | 14 ---- server/fleet/script_variables_test.go | 12 --- server/service/script_variables.go | 23 +++--- server/service/script_variables_test.go | 21 ++++- server/variables/preamble.go | 2 + server/variables/preamble_exec_test.go | 80 +++++++++++++++++++ server/variables/variables.go | 20 +++++ server/variables/variables_test.go | 31 +++++++ 11 files changed, 175 insertions(+), 66 deletions(-) diff --git a/changes/script-variable-interpreter-escaping b/changes/script-variable-interpreter-escaping index 6d4aff658ae..aa8af63dc4d 100644 --- a/changes/script-variable-interpreter-escaping +++ b/changes/script-variable-interpreter-escaping @@ -1,3 +1,3 @@ -- Fixed an issue where characters in a Fleet variable's value could change what a script does. Values are now defined at the top of the script and read as literal text. -- Fleet variables in scripts are no longer substituted inside single quotes or a quoted heredoc. -- Fleet variables are no longer supported in Python scripts. +- Fixed an issue where characters in a Fleet variable's value could change what a script does. Values are now defined at the top of the script, or escaped for Python, and read as literal text. +- Fleet variables in shell scripts are no longer substituted inside single quotes or a quoted heredoc. +- In Python scripts, Fleet variables work inside string literals. They aren't supported inside raw (`r"..."`) or bytes (`b"..."`) literals. diff --git a/ee/server/service/software_installers.go b/ee/server/service/software_installers.go index 9e4bcefb789..46ff2777ae8 100644 --- a/ee/server/service/software_installers.go +++ b/ee/server/service/software_installers.go @@ -1086,20 +1086,13 @@ func validateFleetVariablesOnInstallerScripts(ctx context.Context, installScript if !isPremium { return fleet.ErrMissingLicense } - var msg string - unsupported := fleet.FindUnsupportedScriptFleetVar(fleetVars) - switch { - case unsupported != "": - msg = fmt.Sprintf("Fleet variable $FLEET_VAR_%s is not supported in scripts.", unsupported) - case fleet.ScriptFleetVarsUnsupportedByInterpreter(*s.contents): - msg = fleet.FleetVarsInPythonMsg - default: - continue - } - if argErr != nil { - argErr.Append(s.name, msg) - } else { - argErr = fleet.NewInvalidArgumentError(s.name, msg) + if v := fleet.FindUnsupportedScriptFleetVar(fleetVars); v != "" { + msg := fmt.Sprintf("Fleet variable $FLEET_VAR_%s is not supported in scripts.", v) + if argErr != nil { + argErr.Append(s.name, msg) + } else { + argErr = fleet.NewInvalidArgumentError(s.name, msg) + } } } if argErr != nil { diff --git a/ee/server/service/software_installers_test.go b/ee/server/service/software_installers_test.go index 8f6ad802eb5..f4c7d8a59a2 100644 --- a/ee/server/service/software_installers_test.go +++ b/ee/server/service/software_installers_test.go @@ -3334,17 +3334,6 @@ func TestValidateFleetVariablesOnInstallerScripts(t *testing.T) { require.Equal(t, "uninstall script", invalid[1]["name"]) }) - t.Run("python install script rejects variables", func(t *testing.T) { - // a .py package's install script is the uploaded file, so it can be python - py := "#!/usr/bin/env python3\nprint(\"$FLEET_VAR_HOST_UUID\")\n" - err := validateFleetVariablesOnInstallerScripts(premiumCtx, &py, nil, nil) - require.ErrorContains(t, err, "install script") - require.ErrorContains(t, err, fleet.FleetVarsInPythonMsg) - - pyNoVars := "#!/usr/bin/env python3\nprint(\"hi\")\n" - require.NoError(t, validateFleetVariablesOnInstallerScripts(premiumCtx, &pyNoVars, nil, nil)) - }) - t.Run("any variable on free returns license error", func(t *testing.T) { err := validateFleetVariablesOnInstallerScripts(freeCtx, &plain, nil, &good) require.ErrorIs(t, err, fleet.ErrMissingLicense) diff --git a/server/fleet/script_variables.go b/server/fleet/script_variables.go index d4d06b7e7be..c8d3b6849c7 100644 --- a/server/fleet/script_variables.go +++ b/server/fleet/script_variables.go @@ -33,17 +33,6 @@ func FindUnsupportedScriptFleetVar(fleetVars []string) string { return "" } -// FleetVarsInPythonMsg is returned when a Python script uses Fleet variables. -const FleetVarsInPythonMsg = "Fleet variables are not supported in Python scripts." - -// ScriptFleetVarsUnsupportedByInterpreter reports whether the script's -// interpreter can't receive Fleet variables. Python has no $VAR expansion, so a -// value could only reach it by being spliced into the source. -func ScriptFleetVarsUnsupportedByInterpreter(contents string) bool { - kind, _, err := ShebangInfo(contents) - return err == nil && kind == ShebangPython -} - // ValidateFleetVariablesInScript returns an error if the script contents // reference a Fleet variable that is not supported in scripts, or if variables // are used without a premium license. @@ -59,8 +48,5 @@ func ValidateFleetVariablesInScript(contents string, isPremium bool) error { return NewInvalidArgumentError("script", fmt.Sprintf("Fleet variable $FLEET_VAR_%s is not supported in scripts.", v)) } - if ScriptFleetVarsUnsupportedByInterpreter(contents) { - return NewInvalidArgumentError("script", FleetVarsInPythonMsg) - } return nil } diff --git a/server/fleet/script_variables_test.go b/server/fleet/script_variables_test.go index 6ccae3bfaec..d3c6a56f2b3 100644 --- a/server/fleet/script_variables_test.go +++ b/server/fleet/script_variables_test.go @@ -56,18 +56,6 @@ func TestValidateFleetVariablesInScript(t *testing.T) { } }) - t.Run("python scripts reject variables", func(t *testing.T) { - for _, shebang := range []string{ - "#!/usr/bin/env python3", "#!/usr/bin/python3", "#!/opt/homebrew/bin/python3.12", - } { - err := ValidateFleetVariablesInScript(shebang+"\nprint(\"$FLEET_VAR_HOST_UUID\")\n", true) - require.ErrorContains(t, err, FleetVarsInPythonMsg, shebang) - - require.NoError(t, ValidateFleetVariablesInScript(shebang+"\nprint(\"hi\")\n", true), shebang) - } - require.NoError(t, ValidateFleetVariablesInScript("#!/bin/sh\necho $FLEET_VAR_HOST_UUID\n", true)) - }) - t.Run("mixed supported and unsupported", func(t *testing.T) { err := ValidateFleetVariablesInScript( "echo $FLEET_VAR_HOST_UUID\necho $FLEET_VAR_NONEXISTENT", true) diff --git a/server/service/script_variables.go b/server/service/script_variables.go index f8d232ab122..25b2ea6f486 100644 --- a/server/service/script_variables.go +++ b/server/service/script_variables.go @@ -15,19 +15,18 @@ import ( ) const ( - pythonFleetVarsMsg = "Fleet couldn't run this script because Fleet variables aren't supported in Python scripts. Use a shell script, or remove the variable." powerShellParamBlockMsg = "Fleet couldn't run this script because Fleet variables aren't supported inside a PowerShell param() block. Use the variable in the script body instead." unsupportedInterpMsg = "Fleet couldn't run this script because its interpreter isn't supported." noPlatformMsg = "There is no platform for this host. Fleet couldn't populate Fleet variables." ) // maybeExpandScriptFleetVariables resolves supported $FLEET_VAR_* references in -// contents for the given host. Values are defined in a preamble and the body -// keeps its tokens, so a value is never parsed as script source. It returns the -// expanded contents, or a non-empty failureMessage when a variable can't be -// resolved for this host (one line per failing variable). Unsupported variable -// names are left untouched: validation rejects them in new content, and content -// saved before validation shipped must keep working unchanged. +// contents for the given host. Values are defined in a preamble, or escaped and +// substituted for Python, so a value is never parsed as script source. It +// returns the expanded contents, or a non-empty failureMessage when a variable +// can't be resolved for this host (one line per failing variable). Unsupported +// variable names are left untouched: validation rejects them in new content, and +// content saved before validation shipped must keep working unchanged. func (svc *Service) maybeExpandScriptFleetVariables(ctx context.Context, host *fleet.Host, contents string) (expanded string, failureMessage string, err error) { fleetVars := variables.Find(contents) if len(fleetVars) == 0 { @@ -112,6 +111,13 @@ func (svc *Service) maybeExpandScriptFleetVariables(ctx context.Context, host *f return contents, "", nil } + if dialect == variables.DialectPython { + for name, value := range resolved { + contents = variables.Replace(contents, name, variables.PythonEscape(value)) + } + return contents, "", nil + } + expanded, err = variables.InsertPreamble(contents, variables.Preamble(resolved, dialect), dialect) switch { case errors.Is(err, variables.ErrPowerShellLeadingParamBlock): @@ -138,8 +144,7 @@ func scriptFleetVarDialect(host *fleet.Host, contents string) (variables.Dialect case err != nil: return 0, unsupportedInterpMsg case kind == fleet.ShebangPython: - // Python has no $VAR expansion, and no splice into it is context-free - return 0, pythonFleetVarsMsg + return variables.DialectPython, "" } return variables.DialectPOSIX, "" } diff --git a/server/service/script_variables_test.go b/server/service/script_variables_test.go index 8b1678d1a50..f7ecef14da9 100644 --- a/server/service/script_variables_test.go +++ b/server/service/script_variables_test.go @@ -193,7 +193,7 @@ func TestMaybeExpandScriptFleetVariables(t *testing.T) { require.Equal(t, powerShellParamBlockMsg, failMsg) }) - t.Run("python scripts fail instead of running unexpanded", func(t *testing.T) { + t.Run("python scripts get escaped values", func(t *testing.T) { svc, ctx, _ := newSvcAndCtx(fleet.TierPremium) for _, shebang := range []string{ "#!/usr/bin/env python3", "#!/usr/bin/python3", "#!/opt/homebrew/bin/python3.12", @@ -201,11 +201,26 @@ func TestMaybeExpandScriptFleetVariables(t *testing.T) { expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, host, shebang+"\nprint(\"uuid: $FLEET_VAR_HOST_UUID\")\n") require.NoError(t, err) - require.Empty(t, expanded) - require.Equal(t, pythonFleetVarsMsg, failMsg) + require.Empty(t, failMsg) + require.Equal(t, shebang+"\nprint(\"uuid: "+variables.PythonEscape("ABC-123")+"\")\n", expanded) + require.NotContains(t, expanded, "ABC-123") } }) + t.Run("python values carrying source stay literal", func(t *testing.T) { + svc, ctx, ds := newSvcAndCtx(fleet.TierPremium) + payload := "X\")\nimport os\nos.system(\"touch /tmp/pwned\")\nprint(\"" + u := *scimUser + u.Department = &payload + mockScimUser(ds, &u) + expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, host, + "#!/usr/bin/env python3\nprint(\"$FLEET_VAR_HOST_END_USER_IDP_DEPARTMENT\")\n") + require.NoError(t, err) + require.Empty(t, failMsg) + require.NotContains(t, expanded, "os.system") + require.Contains(t, expanded, variables.PythonEscape(payload)) + }) + t.Run("python scripts without variables are unchanged", func(t *testing.T) { svc, ctx, _ := newSvcAndCtx(fleet.TierPremium) const contents = "#!/usr/bin/env python3\nprint(\"hello\")\n" diff --git a/server/variables/preamble.go b/server/variables/preamble.go index 2b4fd4d1252..20b916eeb18 100644 --- a/server/variables/preamble.go +++ b/server/variables/preamble.go @@ -14,6 +14,8 @@ type Dialect int const ( DialectPOSIX Dialect = iota DialectPowerShell + // DialectPython takes no preamble; see PythonEscape. + DialectPython ) // ErrPowerShellLeadingParamBlock reports that a preamble can't be placed around diff --git a/server/variables/preamble_exec_test.go b/server/variables/preamble_exec_test.go index ce80d14e9fd..4d8ff519243 100644 --- a/server/variables/preamble_exec_test.go +++ b/server/variables/preamble_exec_test.go @@ -143,3 +143,83 @@ func TestSubstitutingValuesIntoTheBodyExecutesThem(t *testing.T) { got, _ := os.ReadFile(out) require.Equal(t, "Eng", string(got)) } + +// Nothing but a backslash, "U" and hex digits reaches the source, so a value can +// neither close a string literal nor, outside one, parse at all. +func TestPythonEscapeNeverExecutesValues(t *testing.T) { + python, err := exec.LookPath("python3") + if err != nil { + t.Skip("python3 not installed") + } + + // where a $FLEET_VAR_* token can sit; OUT is the round-trip target + inString := map[string]string{ + "double-quoted": `open("OUT","w").write("TOKEN")`, + "single-quoted": `open("OUT","w").write('TOKEN')`, + "triple-quoted": `open("OUT","w").write("""TOKEN""")`, + "f-string": `open("OUT","w").write(f"TOKEN")`, + "adjacent": `open("OUT","w").write("TOKEN"[0:0] + "TOKEN")`, + } + + // the shared corpus carries shell payloads; these are executable Python + values := payloadCorpus() + values["py-break-double"] = "X\")\nimport os\nos.system(\"touch \")\nprint(\"" + values["py-break-single"] = "X')\nimport os\nos.system('touch ')\nprint('" + values["py-break-triple"] = "X\"\"\")\nimport os\nos.system(\"touch \")\nprint(\"\"\"" + values["py-fstring-expr"] = "{__import__('os').system('touch ')}" + + for name, value := range values { + t.Run(name, func(t *testing.T) { + for shape, body := range inString { + dir := t.TempDir() + marker := filepath.Join(dir, "MARKER") + out := filepath.Join(dir, "out") + value := strings.ReplaceAll(value, "", marker) + + src := "#!/usr/bin/env python3\n" + strings.NewReplacer( + "OUT", out, + "TOKEN", PythonEscape(value), + ).Replace(body) + "\n" + + script := filepath.Join(dir, "s.py") + require.NoError(t, os.WriteFile(script, []byte(src), 0o600)) + _ = exec.Command(python, script).Run() + + require.NoFileExists(t, marker, "value executed: shape=%s", shape) + got, err := os.ReadFile(out) + require.NoError(t, err, "shape=%s", shape) + require.Equal(t, value, string(got), "value did not round-trip: shape=%s", shape) + } + }) + } + + t.Run("bare code fails to parse", func(t *testing.T) { + dir := t.TempDir() + marker := filepath.Join(dir, "MARKER") + script := filepath.Join(dir, "s.py") + payload := `__import__(chr(111)+chr(115)).system("touch ` + marker + `")` + src := "#!/usr/bin/env python3\nx = " + PythonEscape(payload) + "\n" + require.NoError(t, os.WriteFile(script, []byte(src), 0o600)) + + require.Error(t, exec.Command(python, script).Run()) + require.NoFileExists(t, marker) + }) +} + +// Keeps the test above honest: substituting a value into Python source executes it. +func TestSubstitutingIntoPythonExecutesValues(t *testing.T) { + python, err := exec.LookPath("python3") + if err != nil { + t.Skip("python3 not installed") + } + dir := t.TempDir() + marker := filepath.Join(dir, "MARKER") + script := filepath.Join(dir, "s.py") + + payload := "X\")\nimport os\nos.system(\"touch " + marker + "\")\nprint(\"" + src := "#!/usr/bin/env python3\n" + Replace(`print("v: $FLEET_VAR_HOST_UUID")`, "HOST_UUID", payload) + "\n" + require.NoError(t, os.WriteFile(script, []byte(src), 0o600)) + _ = exec.Command(python, script).Run() + + require.FileExists(t, marker) +} diff --git a/server/variables/variables.go b/server/variables/variables.go index d4c15cf0c27..5d1c31a76fc 100644 --- a/server/variables/variables.go +++ b/server/variables/variables.go @@ -104,6 +104,26 @@ func Replace(contents string, variableName string, value string) string { return result } +// PythonEscape returns value as Python \U escapes, one per code point, for +// substitution into Python source, which has no $VAR expansion of its own. Only +// a backslash, "U" and hex digits reach the script, so the value can't close a +// string literal, and outside one the leading backslash is a line continuation +// so the file won't parse. Escaping every character rather than just the +// dangerous ones is what buys that second property: executable Python needs no +// quotes at all. +func PythonEscape(value string) string { + const hexDigits = "0123456789abcdef" + var b strings.Builder + b.Grow(len(value) * 10) + for _, r := range value { + b.WriteString(`\U`) + for shift := 28; shift >= 0; shift -= 4 { + b.WriteByte(hexDigits[(r>>shift)&0xf]) + } + } + return b.String() +} + // Contains checks if the given content contains any Fleet variables. func Contains(contents string) bool { return fleetVariableRegex.MatchString(contents) diff --git a/server/variables/variables_test.go b/server/variables/variables_test.go index 45aa162107c..04e83d356fd 100644 --- a/server/variables/variables_test.go +++ b/server/variables/variables_test.go @@ -1,9 +1,12 @@ package variables import ( + "strconv" + "strings" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestFind(t *testing.T) { @@ -175,3 +178,31 @@ func TestReplace(t *testing.T) { }) } } + +func TestPythonEscape(t *testing.T) { + require.Equal(t, `\U00000041\U00000042`, PythonEscape("AB")) + require.Empty(t, PythonEscape("")) + + for name, value := range payloadCorpus() { + t.Run(name, func(t *testing.T) { + got := PythonEscape(value) + + // nothing is left that could close a literal or start an expression + for _, r := range got { + require.Contains(t, `\U0123456789abcdef`, string(r), + "unexpected character %q in %q", r, got) + } + + var decoded strings.Builder + for field := range strings.SplitSeq(got, `\U`) { + if field == "" { + continue + } + cp, err := strconv.ParseInt(field, 16, 32) + require.NoError(t, err) + decoded.WriteRune(rune(cp)) + } + require.Equal(t, value, decoded.String()) + }) + } +} From 3b5bee68037865b4e960937f6c835c2675e516af Mon Sep 17 00:00:00 2001 From: Carlo DiCelico Date: Tue, 8 Sep 2026 13:49:16 -0400 Subject: [PATCH 7/7] pin python substitution order --- server/service/script_variables.go | 18 ++++++++++++++---- server/service/script_variables_test.go | 21 +++++++++++++++++++++ server/variables/preamble.go | 2 +- server/variables/variables.go | 7 ++++--- 4 files changed, 40 insertions(+), 8 deletions(-) diff --git a/server/service/script_variables.go b/server/service/script_variables.go index 25b2ea6f486..7c0dd97b81a 100644 --- a/server/service/script_variables.go +++ b/server/service/script_variables.go @@ -24,9 +24,13 @@ const ( // contents for the given host. Values are defined in a preamble, or escaped and // substituted for Python, so a value is never parsed as script source. It // returns the expanded contents, or a non-empty failureMessage when a variable -// can't be resolved for this host (one line per failing variable). Unsupported -// variable names are left untouched: validation rejects them in new content, and -// content saved before validation shipped must keep working unchanged. +// can't be resolved for this host (one line per failing variable). +// +// Unsupported variable names are left untouched, since validation rejects them +// in new content and content saved before validation shipped must keep working. +// On the Python path that holds except for an unsupported name that extends a +// supported one (e.g. $FLEET_VAR_HOST_UUID_LEGACY), whose prefix +// variables.Replace rewrites along with the supported variable. func (svc *Service) maybeExpandScriptFleetVariables(ctx context.Context, host *fleet.Host, contents string) (expanded string, failureMessage string, err error) { fleetVars := variables.Find(contents) if len(fleetVars) == 0 { @@ -112,7 +116,13 @@ func (svc *Service) maybeExpandScriptFleetVariables(ctx context.Context, host *f } if dialect == variables.DialectPython { - for name, value := range resolved { + // supported is longest-first from variables.Find, which keeps a shorter + // name from matching inside a longer token that contains it + for _, name := range supported { + value, ok := resolved[name] + if !ok { + continue + } contents = variables.Replace(contents, name, variables.PythonEscape(value)) } return contents, "", nil diff --git a/server/service/script_variables_test.go b/server/service/script_variables_test.go index f7ecef14da9..2d0c09fd957 100644 --- a/server/service/script_variables_test.go +++ b/server/service/script_variables_test.go @@ -207,6 +207,27 @@ func TestMaybeExpandScriptFleetVariables(t *testing.T) { } }) + // HOST_END_USER_IDP_USERNAME is a prefix of HOST_END_USER_IDP_USERNAME_LOCAL_PART, + // so replacing the shorter name first corrupts the longer token + t.Run("python substitution order does not corrupt overlapping names", func(t *testing.T) { + svc, ctx, ds := newSvcAndCtx(fleet.TierPremium) + mockScimUser(ds, scimUser) + const body = "#!/usr/bin/env python3\n" + + "print(\"u: $FLEET_VAR_HOST_END_USER_IDP_USERNAME\")\n" + + "print(\"l: $FLEET_VAR_HOST_END_USER_IDP_USERNAME_LOCAL_PART\")\n" + want := "#!/usr/bin/env python3\n" + + "print(\"u: " + variables.PythonEscape("user@example.com") + "\")\n" + + "print(\"l: " + variables.PythonEscape("user") + "\")\n" + + // map iteration order is unspecified, so a single pass can pass by luck + for range 100 { + expanded, failMsg, err := svc.maybeExpandScriptFleetVariables(ctx, host, body) + require.NoError(t, err) + require.Empty(t, failMsg) + require.Equal(t, want, expanded) + } + }) + t.Run("python values carrying source stay literal", func(t *testing.T) { svc, ctx, ds := newSvcAndCtx(fleet.TierPremium) payload := "X\")\nimport os\nos.system(\"touch /tmp/pwned\")\nprint(\"" diff --git a/server/variables/preamble.go b/server/variables/preamble.go index 20b916eeb18..8bd31a216f1 100644 --- a/server/variables/preamble.go +++ b/server/variables/preamble.go @@ -55,7 +55,7 @@ func PowerShellCharArray(value string) string { // Preamble builds the assignments defining vars, keyed by name without the // FLEET_VAR_ prefix. Output is ordered by name. func Preamble(vars map[string]string, dialect Dialect) string { - if len(vars) == 0 { + if len(vars) == 0 || dialect == DialectPython { return "" } names := make([]string, 0, len(vars)) diff --git a/server/variables/variables.go b/server/variables/variables.go index 5d1c31a76fc..78ea8e7e0d4 100644 --- a/server/variables/variables.go +++ b/server/variables/variables.go @@ -94,9 +94,10 @@ func Dedupe(varsWithDupes []string) []string { // For example, Replace(content, "HOST_UUID", "123-456") will replace both // $FLEET_VAR_HOST_UUID and ${FLEET_VAR_HOST_UUID} with "123-456". // -// It performs no escaping, and must never be used on content an interpreter will -// execute: the quoting context at the substitution point is unknown, so no -// escaping is correct there. Use Preamble instead. +// It performs no escaping. For content an interpreter will execute, never pass a +// raw value: the quoting context at the substitution point is unknown. Use +// Preamble, or an encoding that leaves the value inert at any position, such as +// PythonEscape. func Replace(contents string, variableName string, value string) string { // Replace both braced and non-braced versions result := strings.ReplaceAll(contents, "$FLEET_VAR_"+variableName, value)