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
This commit is contained in:
@@ -0,0 +1 @@
|
||||
Normalized LocURI values before validation in Windows profile handling.
|
||||
@@ -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")
|
||||
|
||||
@@ -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(
|
||||
`<Add><Item><Target><LocURI>%s</LocURI></Target><Data>%s</Data></Item></Add>`,
|
||||
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.
|
||||
|
||||
Reference in New Issue
Block a user