From d9426402b25e64bed3c9c311e19180cee1dbf472 Mon Sep 17 00:00:00 2001 From: Sharon Katz <121527325+sharon-fdm@users.noreply.github.com> Date: Tue, 21 Jul 2026 17:20:53 -0400 Subject: [PATCH] Normalize LocURI values before validation in Windows profiles (#49708) **Related issue:** Resolves fleetdm/confidential#16883 # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters. - [x] Timeouts are implemented and retries are limited to avoid infinite loops ## Summary Normalized LocURI target values before validation checks in Windows MDM profile handling. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually - [x] Confirmed that the fix is not expected to adversely impact load test results ### Reproduction Wrote test cases that construct Windows SCEP profile XML with trailing whitespace appended to LocURI paths (e.g., `/Install/SubjectName ` with a trailing space). Before the fix, these profiles passed validation without the required renewal-id marker because `strings.HasSuffix` did not match the whitespace-suffixed path. The same bypass applied to Challenge and ServerURL LocURIs. ### Unit tests added 7 new test cases across two test functions: **`TestAdditionalNDESValidationForWindowsProfiles`** (3 new cases): - SubjectName LocURI with trailing whitespace is still validated for renewal id - Challenge LocURI with trailing whitespace still validates correctly - ServerURL LocURI with trailing whitespace still validates correctly **`TestAdditionalCustomSCEPValidationForWindowsProfiles`** (new function, 4 cases): - Valid custom SCEP profile passes - SubjectName missing renewal id is rejected - SubjectName with trailing whitespace in LocURI is still validated for renewal id - SubjectName with internal whitespace (not trailing) is rejected ### Local verification 1. Wrote failing tests first, confirmed the whitespace bypass existed (tests failed as expected before the fix) 2. Applied the fix (`strings.TrimSpace` on target before `HasSuffix` checks) 3. Confirmed all new tests pass after the fix 4. Ran full test suite: `go test ./server/service/ -run "TestAdditionalNDESValidation|TestAdditionalCustomSCEPValidation" -v` with all 14 tests passing 5. Ran `make lint-go-incremental` with 0 issues --- changes/normalize-locuri-validation | 1 + server/service/windows_mdm_profiles.go | 16 ++-- server/service/windows_mdm_profiles_test.go | 83 +++++++++++++++++++++ 3 files changed, 94 insertions(+), 6 deletions(-) create mode 100644 changes/normalize-locuri-validation diff --git a/changes/normalize-locuri-validation b/changes/normalize-locuri-validation new file mode 100644 index 0000000000..87cd7c363a --- /dev/null +++ b/changes/normalize-locuri-validation @@ -0,0 +1 @@ +Normalized LocURI values before validation in Windows profile handling. diff --git a/server/service/windows_mdm_profiles.go b/server/service/windows_mdm_profiles.go index 78f07c5733..77aaf4a0a0 100644 --- a/server/service/windows_mdm_profiles.go +++ b/server/service/windows_mdm_profiles.go @@ -411,14 +411,16 @@ func additionalNDESValidationForWindowsProfiles(contents string, ndesVars *NDESV continue } + target := strings.TrimSpace(*cmd.Target) + dataContent := "" if cmd.Data != nil { dataContent = cmd.Data.Content } - isChallenge := strings.HasSuffix(*cmd.Target, "/Install/Challenge") - isServerURL := strings.HasSuffix(*cmd.Target, "/Install/ServerURL") - isSubjectName := strings.HasSuffix(*cmd.Target, "/Install/SubjectName") + isChallenge := strings.HasSuffix(target, "/Install/Challenge") + isServerURL := strings.HasSuffix(target, "/Install/ServerURL") + isSubjectName := strings.HasSuffix(target, "/Install/SubjectName") // Verify that each NDES variable appears ONLY in its expected field. // This prevents the one-time challenge or proxy URL from being placed in an unexpected field @@ -437,8 +439,8 @@ func additionalNDESValidationForWindowsProfiles(contents string, ndesVars *NDESV } // Variables must not appear in LocURI target paths. - if containsFleetVar(*cmd.Target, fleet.FleetVarNDESSCEPChallenge) || - containsFleetVar(*cmd.Target, fleet.FleetVarNDESSCEPProxyURL) { + if containsFleetVar(target, fleet.FleetVarNDESSCEPChallenge) || + containsFleetVar(target, fleet.FleetVarNDESSCEPProxyURL) { return &fleet.BadRequestError{ Message: "NDES Fleet variables must not appear in LocURI target paths.", } @@ -491,7 +493,9 @@ func additionalCustomSCEPValidationForWindowsProfiles(contents string, customSCE continue } - if strings.HasSuffix(*cmd.Target, "/Install/SubjectName") { + target := strings.TrimSpace(*cmd.Target) + + if strings.HasSuffix(target, "/Install/SubjectName") { // SubjectName item found, check that it contains the expected renewal ID variable if cmd.Data == nil { return errors.New("SubjectName item is missing data") diff --git a/server/service/windows_mdm_profiles_test.go b/server/service/windows_mdm_profiles_test.go index 378175e01a..9d34375d6b 100644 --- a/server/service/windows_mdm_profiles_test.go +++ b/server/service/windows_mdm_profiles_test.go @@ -297,6 +297,28 @@ func TestAdditionalNDESValidationForWindowsProfiles(t *testing.T) { name: "nil ndes vars returns nil", contents: validProfile, }, + { + name: "subject name with trailing whitespace in LocURI is still validated for renewal id", + contents: addItem("./Device/Vendor/MSFT/ClientCertificateInstall/SCEP/cert1/Install/Challenge", "$FLEET_VAR_NDES_SCEP_CHALLENGE") + + addItem("./Device/Vendor/MSFT/ClientCertificateInstall/SCEP/cert1/Install/ServerURL", "$FLEET_VAR_NDES_SCEP_PROXY_URL") + + addItem("./Device/Vendor/MSFT/ClientCertificateInstall/SCEP/cert1/Install/SubjectName ", "CN=test"), + wantErr: true, + errContains: "SubjectName item must contain the $FLEET_VAR_CERTIFICATE_RENEWAL_ID variable in the OU field", + }, + { + name: "challenge with trailing whitespace in LocURI still validates correctly", + contents: addItem("./Device/Vendor/MSFT/ClientCertificateInstall/SCEP/cert1/Install/Challenge ", "hardcoded-password") + + addItem("./Device/Vendor/MSFT/ClientCertificateInstall/SCEP/cert1/Install/ServerURL", "$FLEET_VAR_NDES_SCEP_PROXY_URL"), + wantErr: true, + errContains: `must be in the SCEP certificate's "Challenge" field`, + }, + { + name: "server url with trailing whitespace in LocURI still validates correctly", + contents: addItem("./Device/Vendor/MSFT/ClientCertificateInstall/SCEP/cert1/Install/Challenge", "$FLEET_VAR_NDES_SCEP_CHALLENGE") + + addItem("./Device/Vendor/MSFT/ClientCertificateInstall/SCEP/cert1/Install/ServerURL ", "https://hardcoded.example.com"), + wantErr: true, + errContains: `must be in the SCEP certificate's "ServerURL" field`, + }, } for _, tt := range tests { @@ -318,6 +340,67 @@ func TestAdditionalNDESValidationForWindowsProfiles(t *testing.T) { } } +func TestAdditionalCustomSCEPValidationForWindowsProfiles(t *testing.T) { + t.Parallel() + + addItem := func(locURI, data string) string { + return fmt.Sprintf( + `%s%s`, + locURI, data, + ) + } + + customSCEPVars := &CustomSCEPVarsFound{} + customSCEPVars, _ = customSCEPVars.SetURL("ca1") + customSCEPVars, _ = customSCEPVars.SetChallenge("ca1") + customSCEPVars, _ = customSCEPVars.SetRenewalID() + + tests := []struct { + name string + contents string + wantErr bool + errContains string + }{ + { + name: "valid custom SCEP profile", + contents: addItem( + "./Device/Vendor/MSFT/ClientCertificateInstall/SCEP/cert1/Install/SubjectName", + "CN=test,OU=$FLEET_VAR_CERTIFICATE_RENEWAL_ID", + ), + }, + { + name: "subject name missing renewal id", + contents: addItem( + "./Device/Vendor/MSFT/ClientCertificateInstall/SCEP/cert1/Install/SubjectName", + "CN=test", + ), + wantErr: true, + errContains: "SubjectName item must contain the $FLEET_VAR_CERTIFICATE_RENEWAL_ID variable in the OU field", + }, + { + name: "subject name with trailing whitespace in LocURI is still validated for renewal id", + contents: addItem( + "./Device/Vendor/MSFT/ClientCertificateInstall/SCEP/cert1/Install/SubjectName ", + "CN=test", + ), + wantErr: true, + errContains: "SubjectName item must contain the $FLEET_VAR_CERTIFICATE_RENEWAL_ID variable in the OU field", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := additionalCustomSCEPValidationForWindowsProfiles(tt.contents, customSCEPVars) + if tt.wantErr { + require.Error(t, err) + require.Contains(t, err.Error(), tt.errContains) + } else { + require.NoError(t, err) + } + }) + } +} + func TestNewMDMWindowsConfigProfileSoftwareUpdate(t *testing.T) { // osUpdateSyncML contains the Windows Update install policy LocURI, marking it // as a software update profile. otherSyncML is an unrelated policy.