Define script variables ahead of the script body

changes/script-variable-interpreter-escaping

@@ -0,0 +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, 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.

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,75 @@ 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
+		// 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},
+		{"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.NoFileExists(t, marker)
+			if tc.safetyOnly {
+				return
+			}
+			require.NoError(t, err)
+			require.Equal(t, tc.value, string(output))
+		})
+	}
+}
+
+// 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)
+		})
+	}
+}

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) {

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{

server/service/script_variables.go

@@ -2,6 +2,7 @@ package service
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"slices"
 	"strings"
@@ -13,18 +14,23 @@ 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 (
+	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, 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, 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 {
@@ -37,6 +43,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 +66,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 +88,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 +100,61 @@ 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
+	}
+
+	if dialect == variables.DialectPython {
+		// 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
+	}
+
+	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:
+		return variables.DialectPython, ""
+	}
+	return variables.DialectPOSIX, ""
 }